Skip to content

Fix type narrowing for class equality comparisons - #11607

Open
Henry Su (hsusul) wants to merge 3 commits into
microsoft:mainfrom
hsusul:fix/type-equality-comparison-narrowing
Open

Fix type narrowing for class equality comparisons#11607
Henry Su (hsusul) wants to merge 3 commits into
microsoft:mainfrom
hsusul:fix/type-equality-comparison-narrowing

Conversation

@hsusul

Copy link
Copy Markdown
Contributor

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

from typing import TypeVar, assert_type, final

class Base: pass
class Sub1(Base): pass

@final
class Sub2(Base): pass

T = TypeVar("T", bound=Base)

def test_eq_concrete(cls: type[Base]) -> type[Sub1]:
    if cls == Sub1:
        assert_type(cls, type[Sub1])  # Currently fails: expected "type[Sub1]" but received "type[Base]"
        return cls  # Currently fails with false-positive reportReturnType
    raise ValueError()

def test_neq_concrete(cls: type[Sub1] | type[Sub2]):
    if cls != Sub2:
        assert_type(cls, type[Sub1])  # Currently fails: expected "type[Sub1]" but received "type[Sub1] | type[Sub2]"

def test_eq_typevar(cls: type[T]) -> type[Sub1]:
    if cls == Sub1:
        assert_type(cls, type[Sub1])  # Currently fails: expected "type[Sub1]" but received "type[T]"
        return cls
    raise ValueError()

Current vs. Corrected Behavior

  • Current Behavior:

    • if cls is Sub1: narrows cls to type[Sub1].
    • if cls == Sub1: does NOT narrow cls, leaving cls as type[Base] or type[T], leading to false-positive diagnostic errors (reportReturnType).
    • if cls != Sub2: does NOT narrow cls when Sub2 is @final.
  • Corrected Behavior:

    • Both identity (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 cls to a class Sub1 using 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 only if (isOrIsNotOperator) before delegating to narrowTypeForClassComparison. Because equalsOrNotEqualsOperator (== / !=) was excluded from this block, Pyright skipped calling narrowTypeForClassComparison when rightType was an instantiable class (isInstantiableClass(rightType)).

Implementation Details

In typeGuards.ts:

  1. Updated the check at line 256 to if (isOrIsNotOperator || equalsOrNotEqualsOperator).
  2. Ensured narrowTypeForLiteralComparison retains isOrIsNotOperator guard so literal comparisons are unaffected.
  3. Enabled narrowTypeForClassComparison for both is/is not and ==/!= class comparisons.

Regression Coverage

Added packages/pyright-internal/src/tests/samples/typeGuard4.py and registered TypeGuard4 in typeEvaluator6.test.ts to 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.

@StellaHuang95

Stella Huang (StellaHuang95) commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

🔒 Automated review in progress — Stella Huang (@StellaHuang95) is auto-reviewing this PR.

}

if (isOrIsNotOperator) {
if (isOrIsNotOperator || equalsOrNotEqualsOperator) {

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.

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.

@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:L313.

Issue · Please address or respond

This narrowing is unsound when the class has a metaclass that overrides __eq__ or __ne__: cls == Sub1 can be true while cls is not Sub1, and cls != Sub2 can be true while cls is Sub2. Narrowing here can therefore suppress a legitimate diagnostic. Restrict this behavior to a sound case or avoid equality-based class narrowing. [verified]


def test_neq_concrete(cls: type[Sub1] | type[Sub2]):
if cls != Sub2:
assert_type(cls, type[Sub1])

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

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]

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

Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review and feedback! I have updated the implementation to address all three points:

  1. Control Flow Structure:

    • Reverted the isOrIsNotOperator block check at line 256.
    • Added class comparison narrowing directly inside the dedicated if (equalsOrNotEqualsOperator) block (around line 384) passing /* isIsOperator */ false.
  2. Soundness & Metaclasses:

    • Added hasCustomEqualityMetaclass to check whether the effective metaclass of either operand overrides __eq__ or __ne__ (looking up members with MemberAccessFlags.SkipTypeBaseClass | MemberAccessFlags.SkipObjectBaseClass).
    • Equality-based class narrowing (== / !=) is restricted to sound cases where default identity-based type equality applies, and is bypassed whenever a custom metaclass defines its own equality operators.
  3. Test Coverage:

    • Added test_neq_non_final to typeGuard4.py verifying that negative equality comparison against non-@final classes does not narrow.
    • Added test_eq_custom_meta to typeGuard4.py verifying that equality comparison against classes with custom metaclass equality does not narrow.

return {
type: narrowTypeForUserDefinedTypeGuard(
evaluator,
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.

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;
}

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.

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.

@hsusul
Henry Su (hsusul) force-pushed the fix/type-equality-comparison-narrowing branch from b605ac0 to 7f45fe2 Compare August 10, 2026 16:48
@hsusul

Copy link
Copy Markdown
Contributor Author

Thank you for the additional review and feedback! I have updated the PR to address both comments:

  1. Removed Extraneous TypeGuard/TypeIs Hunk:

    • Reverted the unrelated TypeGuard/TypeIs union return type hunk in typeGuards.ts.
  2. Restricted Equality Class Narrowing & Added Regression Coverage:

    • Updated narrowTypeForClassComparison to return subtype without narrowing when TypeBase.isInstance(concreteSubtype) and isIsOperator is false. Equality-based class narrowing (== / !=) is now strictly restricted to instantiable-class reference subtypes, preventing x: object from being falsely narrowed under if x == Sub1:.
    • Added regression test test_eq_instance_object in typeGuard4.py verifying that if x == Sub1: does not narrow x: object when x is an instance whose __eq__ matches a class object.

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)

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 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]

@StellaHuang95

Copy link
Copy Markdown
Collaborator

Please update the PR description to reflect the implemented approach.

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.

@StellaHuang95 Stella Huang (StellaHuang95) 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 10, 2026

@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) 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.

return mapSubtypes(referenceType, (subtype) => {
let concreteSubtype = evaluator.makeTopLevelTypeVarsConcrete(subtype);

if (!isIsOperator && isInstantiableClass(concreteSubtype) && hasCustomEqualityMetaclass(concreteSubtype)) {

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.

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,

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: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]

@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:L2600.

Warning · Non-blocking recommendation

📍 packages/pyright-internal/src/analyzer/typeGuards.ts:2588
hasCustomEqualityMetaclass duplicates metaclass-method detection already implemented by customMetaclassSupportsMethod in operations.ts, with different lookup flags. Extract or reuse a shared primitive so equality semantics do not drift between analyzers.

[verified]


def test_eq_instance_object(x: object):
if x == Sub1:
assert_type(x, object)

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/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)

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.

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]

@StellaHuang95 Stella Huang (StellaHuang95) added review-auto:changes-requested Automated review: posted blocking findings to address. and removed review-auto:approved Automated review: no blocking findings (approval posted). labels Aug 13, 2026
@rchiodo

Rich Chiodo (rchiodo) commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

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

const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeGuard4.py']);
TestUtils.validateResults(analysisResults, 0);
});

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.

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)

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 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;

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.

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);
});

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.

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])

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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-auto:changes-requested Automated review: posted blocking findings to address.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants