Skip to content

Fix negative type narrowing for tuple membership checks with class objects - #11623

Open
Henry Su (hsusul) wants to merge 2 commits into
microsoft:mainfrom
hsusul:fix/tuple-membership-class-type-narrowing
Open

Fix negative type narrowing for tuple membership checks with class objects#11623
Henry Su (hsusul) wants to merge 2 commits into
microsoft:mainfrom
hsusul:fix/tuple-membership-class-type-narrowing

Conversation

@hsusul

Copy link
Copy Markdown
Contributor

Summary

Fixes negative type narrowing for tuple membership checks (in / not in) when the tuple contains instantiable class objects (type[T]).

Problem

Given a union of class types x: type[A] | type[B] | type[C], evaluating if x not in (A, B): (or if x in (A, B): ... else:) failed to eliminate type[A] and type[B] in the negative branch, leaving x improperly as type[A] | type[B] | type[C].

Minimal Reproduction

class ClassA: pass
class ClassB: pass
class ClassC: pass

def test(x: type[ClassA] | type[ClassB] | type[ClassC]):
    if x not in (ClassA, ClassB):
        # Before: type of "x" is "type[ClassA] | type[ClassB] | type[ClassC]"
        # After: type of "x" is "type[ClassC]"
        assert_type(x, type[ClassC])

Cause

narrowTypeForContainer in typeGuards.ts previously only collected None and literal class instances (isClassInstance(...) && isLiteralType(...)) into typesToEliminate. Instantiable class types (isInstantiableClass(...), e.g., class objects ClassA, ClassB) were omitted. As a result, typesToEliminate remained empty, returning referenceType unchanged in negative tests.

Solution

Include isInstantiableClass(tupleEntry.type) in typesToEliminate so mapSubtypes eliminates matching class types during negative container narrowing.

Validation

  • npx jest src/tests/checker.test.ts -t "TypeNarrowingContainer1" (PASSED)
  • npm run typecheck (PASSED with 0 errors)
  • npm run check (PASSED for eslint, prettier, syncpack)
  • git diff --check (PASSED)
  • Added sample test case typeNarrowingContainer1.py.

@StellaHuang95

Copy link
Copy Markdown
Collaborator

GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for packages/pyright-internal/src/analyzer/typeGuards.ts:L2219.

Issue · Please address or respond

This unconditionally adds class objects to the negative-branch elimination set, but type[A] annotations include subclasses. A SubA(A) value reaches x not in (A,) at runtime, yet isTypeSame ignores includeSubclasses and eliminates the entire type[A] subtype. Please restrict this to classes that cannot have distinct subclasses reaching the branch (for example, final classes), matching the existing class-comparison and positive-container narrowing guards.

[verified]

@StellaHuang95

Copy link
Copy Markdown
Collaborator

GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for packages/pyright-internal/src/analyzer/typeGuards.ts:L2211.

Warning · Non-blocking recommendation

The comment now contradicts the expanded handling. Update it to describe instantiable-class handling and its exactness/subclass constraint so it continues documenting the soundness invariant.

[verified]

if x not in (ClassA, ClassB):
assert_type(x, type[ClassC])
else:
assert_type(x, type[ClassA] | type[ClassB])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning · Non-blocking recommendation

This sample covers only leaf classes, so it misses the unsound subclass case. Add SubA(ClassA) and assert that the negative branch retains the type[ClassA] possibility when ClassA is not final.

[verified]

@StellaHuang95 Stella Huang (StellaHuang95) added the review-auto:changes-requested Automated review: posted blocking findings to address. label Aug 11, 2026
@hsusul

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review! I've updated the implementation and tests accordingly:

  1. Restricted to Final Classes: Negative container membership elimination for instantiable class objects (type[T]) now requires ClassType.isFinal(tupleEntry.type). For non-final classes, a subclass SubA(A) reaches the negative branch at runtime (since SubA != A), so type[A] is retained.
  2. Updated Documentation Comment: Updated the comment in narrowTypeForContainer to document the exactness/@final subclass constraint for instantiable classes.
  3. Expanded Test Coverage: Updated typeNarrowingContainer1.py to cover both @final classes and non-final class hierarchies (asserting that type[NonFinalClassA] is retained in negative branches).

@StellaHuang95

Copy link
Copy Markdown
Collaborator

The minimal reproduction and solution text should use @final classes and explain the subclass-safety restriction; otherwise they describe behavior that this change does not provide.

@rchiodo

Rich Chiodo (rchiodo) commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR.

containerType.priv.tupleTypeArgs.forEach((tupleEntry) => {
if (!tupleEntry.isUnbounded) {
if (isNoneInstance(tupleEntry.type)) {
typesToEliminate.push(tupleEntry.type);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning · Non-blocking recommendation

📍 packages/pyright-internal/src/analyzer/typeGuards.ts:2149

The implementation deliberately narrows only final class objects, but the PR title, summary, and reproduction describe non-final ClassA and ClassB as narrowing away. Update the PR text and example to use @final classes so the documented behavior matches the verified safety boundary.

[verified]

@rchiodo Rich Chiodo (rchiodo) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved via Review Center.

@rchiodo Rich Chiodo (rchiodo) added review-auto:approved Automated review: no blocking findings (approval posted). and removed review-auto:changes-requested Automated review: posted blocking findings to address. labels Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-auto:approved Automated review: no blocking findings (approval posted).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants