Fix type narrowing for class equality comparisons - #11607
Fix type narrowing for class equality comparisons#11607Henry Su (hsusul) wants to merge 3 commits into
Conversation
|
🔒 Automated review in progress — Stella Huang (@StellaHuang95) is auto-reviewing this PR. |
| } | ||
|
|
||
| if (isOrIsNotOperator) { | ||
| if (isOrIsNotOperator || equalsOrNotEqualsOperator) { |
There was a problem hiding this comment.
Issue · Please address or respond
Do not route all ==/!= comparisons through the is/is not block. That block continues into indexed/literal narrowing paths and returns before the dedicated equality block, duplicating and potentially diverging from existing equality behavior. Add the class-comparison branch directly to the existing equalsOrNotEqualsOperator block instead.
|
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:L313.
This narrowing is unsound when the class has a metaclass that overrides |
|
|
||
| def test_neq_concrete(cls: type[Sub1] | type[Sub2]): | ||
| if cls != Sub2: | ||
| assert_type(cls, type[Sub1]) |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Add a negative-boundary case for comparison with a non-@final class, where the negative branch must not narrow. The current coverage would still pass if the isFinal guard in narrowTypeForClassComparison were removed. [verified]
|
Thank you for the detailed review and feedback! I have updated the implementation to address all three points:
|
| return { | ||
| type: narrowTypeForUserDefinedTypeGuard( | ||
| evaluator, | ||
| type, |
There was a problem hiding this comment.
Issue · Please address or respond
This hunk implements a separate TypeGuard/TypeIs-union feature that is not described by this PR. Combining TypeIs[int] | TypeIs[str] into a single strict guard means the negative branch excludes int | str, but union-return TypeIs semantics are not defined and this can over-narrow. Please remove or split this work into a separately justified change with coverage for negative and mixed-union behavior.
| if (!isIsOperator && isInstantiableClass(concreteSubtype) && hasCustomEqualityMetaclass(concreteSubtype)) { | ||
| return subtype; | ||
| } | ||
|
|
There was a problem hiding this comment.
Issue · Please address or respond
For ==/!=, this guard still lets an instance-typed object reach the existing object special case below, which narrows x: object to type[int] after if x == int:. An arbitrary instance can implement __eq__ that returns true for int without being that class object, so this is an unsound false narrowing. Restrict equality-based class narrowing to instantiable-class reference subtypes, and add a regression sample for an instance whose __eq__ matches a class object.
…iable class reference subtypes and remove extraneous TypeGuard union hunk
b605ac0 to
7f45fe2
Compare
|
Thank you for the additional review and feedback! I have updated the PR to address both comments:
|
| def test_nonguard(x: object): | ||
| if check_nonguard(x): | ||
| # Non-guard members in the return type union cause the type guard to be rejected. | ||
| assert_type(x, object) |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This unrelated TypeIs/TypeGuard sample is not registered by any test and is therefore never exercised. Please remove it from this PR, or split it into a separately scoped and registered change.
[verified]
|
Please update the PR description to reflect the implemented approach. |
Stella Huang (StellaHuang95)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
| return mapSubtypes(referenceType, (subtype) => { | ||
| let concreteSubtype = evaluator.makeTopLevelTypeVarsConcrete(subtype); | ||
|
|
||
| if (!isIsOperator && isInstantiableClass(concreteSubtype) && hasCustomEqualityMetaclass(concreteSubtype)) { |
There was a problem hiding this comment.
Issue · Please address or respond
📍 packages/pyright-internal/src/analyzer/typeGuards.ts:2641
This narrowing is verified unsound for open types like type[Base]: a runtime subclass can introduce a metaclass whose __eq__ makes it compare equal to Sub1, even though it is unrelated to Sub1. Restrict equality narrowing to alternatives whose equality semantics are statically closed—such as exact/final classes—and add a regression using a subclass-defined custom metaclass.
[verified]
| adjIsPositiveTest, | ||
| /* isIsOperator */ false | ||
| ), | ||
| isIncomplete: !!rightTypeResult.isIncomplete, |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 packages/pyright-internal/src/analyzer/typeGuards.ts:373
Verified behavior is operand-order dependent: cls == Sub1 narrows, but Sub1 == cls does not. If equality narrowing remains supported, handle the reversed form or explicitly document and test why it must remain directional.
[verified]
|
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:L2600.
📍 packages/pyright-internal/src/analyzer/typeGuards.ts:2588 [verified] |
|
|
||
| def test_eq_instance_object(x: object): | ||
| if x == Sub1: | ||
| assert_type(x, object) |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 packages/pyright-internal/src/tests/samples/typeGuard4.py:34
The custom-metaclass test places custom equality on both operands, so the RHS early return masks coverage of the per-subtype LHS guard; add a case with an ordinary RHS and custom-metaclass LHS, plus a __ne__-only case. EqualityDummy currently participates in no assertion, so remove it or use it in a meaningful regression.
[verified]
| def test_nonguard(x: object): | ||
| if check_nonguard(x): | ||
| # Non-guard members in the return type union cause the type guard to be rejected. | ||
| assert_type(x, object) |
There was a problem hiding this comment.
Issue · Please address or respond
📍 packages/pyright-internal/src/tests/samples/typeIs5.py:1
This unrelated TypeIs/TypeGuard-union fixture is not registered by any test, so none of its assertions run. Remove it from this PR or move and register it as part of a separate, scoped change.
[verified]
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
| const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeGuard4.py']); | ||
| TestUtils.validateResults(analysisResults, 0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Issue · Please address or respond
Add equivalent regression coverage under packages/pylance-internal/src/tests/ so this user-visible narrowing change is exercised through the Pylance harness; the pyright-internal test can remain as supplemental coverage.
| def test_nonguard(x: object): | ||
| if check_nonguard(x): | ||
| # Non-guard members in the return type union cause the type guard to be rejected. | ||
| assert_type(x, object) |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This newly added TypeIs sample is not registered by this PR's test-runner change, so it is dormant and unrelated to the class-comparison fix. Remove it from this PR, or register and justify it as separately tested work.
This sample was not registered with any test and is unrelated to the class-comparison narrowing fix in this PR.
| MemberAccessFlags.SkipTypeBaseClass | MemberAccessFlags.SkipObjectBaseClass | ||
| ) | ||
| ) { | ||
| return true; |
There was a problem hiding this comment.
Issue · Please address or respond
Checking only the declared type[Base] metaclass is insufficient here. An open type[Base] can hold a runtime subclass with a custom metaclass whose __eq__ makes cls == Sub1 true without identity, but this path narrows it to type[Sub1]. Restrict equality narrowing to statically closed alternatives or conservatively retain open class hierarchies.
| const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeGuard4.py']); | ||
| TestUtils.validateResults(analysisResults, 0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Issue · Please address or respond
Please add equivalent regression coverage under packages/pylance-internal/src/tests/. This Pyright change currently has only Pyright-harness coverage, contrary to the required Pylance-first test-placement rule.
| def test_eq_custom_meta(cls: type[Custom1] | type[Custom2]): | ||
| if cls == Custom1: | ||
| assert_type(cls, type[Custom1] | type[Custom2]) | ||
|
|
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This test returns at the RHS custom-metaclass guard, since both operands use CustomMeta, so it cannot exercise the new per-subtype LHS guard. Add an ordinary-RHS/custom-metaclass-LHS case and a __ne__-only metaclass case.
Summary
Fixes a bug where Pyright fails to perform type narrowing when comparing a class type (
type[Base],type[T], or a union of class types) against a class object using equality operators (==or!=).Reproduction
Current vs. Corrected Behavior
Current Behavior:
if cls is Sub1:narrowsclstotype[Sub1].if cls == Sub1:does NOT narrowcls, leavingclsastype[Base]ortype[T], leading to false-positive diagnostic errors (reportReturnType).if cls != Sub2:does NOT narrowclswhenSub2is@final.Corrected Behavior:
is/is not) and equality (==/!=) comparisons against class objects narrow class types consistently.Typing Rule & Root Cause
In Python, class objects are unique singletons in memory. Comparing a class object
clsto a classSub1using equality (if cls == Sub1:) is semantically identical to identity comparison (if cls is Sub1:).In
packages/pyright-internal/src/analyzer/typeGuards.ts, line 256 previously checked onlyif (isOrIsNotOperator)before delegating tonarrowTypeForClassComparison. BecauseequalsOrNotEqualsOperator(==/!=) was excluded from this block, Pyright skipped callingnarrowTypeForClassComparisonwhenrightTypewas an instantiable class (isInstantiableClass(rightType)).Implementation Details
In
typeGuards.ts:if (isOrIsNotOperator || equalsOrNotEqualsOperator).narrowTypeForLiteralComparisonretainsisOrIsNotOperatorguard so literal comparisons are unaffected.narrowTypeForClassComparisonfor bothis/is notand==/!=class comparisons.Regression Coverage
Added
packages/pyright-internal/src/tests/samples/typeGuard4.pyand registeredTypeGuard4intypeEvaluator6.test.tsto test class equality comparison narrowing for concrete class types, TypeVar class types, and unions with final classes.Validation Results
npx jest typeEvaluator6.test.ts -t "TypeGuard4": PASS (1 test passed)npx jest typeEvaluator: PASS (all 8 test suites, 1185 tests passed)npm run check(syncpack, eslint, prettier): PASS (no issues found)npm run typecheck: PASS (executed command in 3 packages)git diff --check: PASS (clean diff, no whitespace errors)Compatibility Considerations
This change is strictly additive to type narrowing for class equality comparisons (
==and!=). Existing type evaluation and diagnostic behavior outside of class comparisons are unchanged.