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.

@rchiodo

Copy link
Copy Markdown
Collaborator

Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified.

@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 24, 2026

@bschnurr Bill Schnurr (bschnurr) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved via Review Center.

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.

4 participants