From eb175addd4ebf25a6c0c5301b3f2cfae7d0c6618 Mon Sep 17 00:00:00 2001 From: Patrick Ribbsaeter Date: Tue, 6 Oct 2026 20:46:01 +0200 Subject: [PATCH] Fix inherited asymmetric descriptor detection (#11605) * Fix inherited asymmetric descriptor detection * Preserve conditional narrowing after asymmetric descriptor writes Classification B: Pyright limitation. Reset asymmetric assignments to the getter-derived read type, retaining incompleteness and allowing later guards without preserving setter-input or stale pre-write narrowing. Add precise positive/negative descriptor regression coverage and document the behavior. --------- --- docs/mypy-comparison.md | 2 + .../src/analyzer/codeFlowEngine.ts | 3 +- .../src/analyzer/typeEvaluator.ts | 34 ++- .../src/tests/samples/descriptor6.py | 226 ++++++++++++++++++ .../src/tests/samples/descriptor7.py | 134 +++++++++++ .../src/tests/typeEvaluator8.test.ts | 20 ++ 6 files changed, 413 insertions(+), 6 deletions(-) create mode 100644 packages/pyright-internal/src/tests/samples/descriptor6.py create mode 100644 packages/pyright-internal/src/tests/samples/descriptor7.py diff --git a/docs/mypy-comparison.md b/docs/mypy-comparison.md index 041bc82..2d02b5b 100644 --- a/docs/mypy-comparison.md +++ b/docs/mypy-comparison.md @@ -229,6 +229,8 @@ def func2(): When pyright evaluates a write to a class variable that contains a descriptor object (including properties), it normally applies assignment-based type narrowing. However, when the descriptor is asymmetric — that is, its “getter” type is different from its “setter” type, pyright refrains from applying assignment-based type narrowing. For a full discussion of this, refer to [this issue](https://github.com/python/mypy/issues/3004). Mypy has not yet implemented the agreed-upon behavior, so its type narrowing behavior may differ from pyright’s in this case. +After an asymmetric descriptor assignment, pyright resets the read type to the getter's return type. This invalidates earlier narrowing without treating the assigned value as the read result. Subsequent conditions, such as an `is not None` check, can still narrow the read type. + ### Parameter Type Inference diff --git a/packages/pyright-internal/src/analyzer/codeFlowEngine.ts b/packages/pyright-internal/src/analyzer/codeFlowEngine.ts index 702bab3..1d37ed1 100644 --- a/packages/pyright-internal/src/analyzer/codeFlowEngine.ts +++ b/packages/pyright-internal/src/analyzer/codeFlowEngine.ts @@ -579,7 +579,8 @@ export function getCodeFlowEngine( reference.nodeType === ParseNodeType.MemberAccess && evaluator.isAsymmetricAccessorAssignment(targetNode) ) { - flowTypeResult = undefined; + // Reset to the read type so subsequent conditions can narrow it. + flowTypeResult = options?.typeAtStart; } } diff --git a/packages/pyright-internal/src/analyzer/typeEvaluator.ts b/packages/pyright-internal/src/analyzer/typeEvaluator.ts index 87ec87d..87c7cd7 100644 --- a/packages/pyright-internal/src/analyzer/typeEvaluator.ts +++ b/packages/pyright-internal/src/analyzer/typeEvaluator.ts @@ -7246,8 +7246,8 @@ export function createTypeEvaluator( // Determine if we're calling __set__ on an asymmetric descriptor or property. let isAsymmetricAccessor = false; - if (usage.method === 'set' && isClass(methodClassType)) { - if (isAsymmetricDescriptorClass(methodClassType)) { + if (usage.method === 'set') { + if (isAsymmetricDescriptorClass(concreteMemberType)) { isAsymmetricAccessor = true; } } @@ -7506,14 +7506,38 @@ export function createTypeEvaluator( let isAsymmetric = false; - const getterSymbolResult = lookUpClassMember(classType, '__get__', MemberAccessFlags.SkipBaseClasses); - const setterSymbolResult = lookUpClassMember(classType, '__set__', MemberAccessFlags.SkipBaseClasses); + // Accessors can be defined at different levels of the descriptor's MRO. + // Ignore instance members so an instance attribute that happens to share + // a dunder name doesn't shadow the descriptor protocol during this check. + const accessorFlags = MemberAccessFlags.SkipInstanceMembers | MemberAccessFlags.SkipAttributeAccessOverride; + const getterSymbolResult = lookUpClassMember(classType, '__get__', accessorFlags); + const setterSymbolResult = lookUpClassMember(classType, '__set__', accessorFlags); if (!getterSymbolResult || !setterSymbolResult) { isAsymmetric = false; } else { let getterType = getTypeOfMember(getterSymbolResult); - const setterType = getTypeOfMember(setterSymbolResult); + let setterType = getTypeOfMember(setterSymbolResult); + + // The getter and setter can be declared on different classes. Specialize + // both against the concrete descriptor class so Self and inherited type + // variables are compared in the same context. + if (getterSymbolResult.classType && isInstantiableClass(getterSymbolResult.classType)) { + getterType = partiallySpecializeType( + getterType, + getterSymbolResult.classType, + getTypeClassType(), + classType + ); + } + if (setterSymbolResult.classType && isInstantiableClass(setterSymbolResult.classType)) { + setterType = partiallySpecializeType( + setterType, + setterSymbolResult.classType, + getTypeClassType(), + classType + ); + } // If this is an overload, find the appropriate overload. if (isOverloaded(getterType)) { diff --git a/packages/pyright-internal/src/tests/samples/descriptor6.py b/packages/pyright-internal/src/tests/samples/descriptor6.py new file mode 100644 index 0000000..f48655b --- /dev/null +++ b/packages/pyright-internal/src/tests/samples/descriptor6.py @@ -0,0 +1,226 @@ +# This sample tests that assignment through an inherited asymmetric descriptor +# does not narrow subsequent reads to the setter's input type. + +from typing import Any, Literal, Self, assert_type, cast, overload + + +class Getter[T]: + def __get__(self, instance: Any, owner: Any = None) -> T: + return cast(Any, None) + + +class Descriptor[T, U](Getter[T]): + def __set__(self, instance: Any, value: T | U) -> None: + pass + + +class IntFromStr(Descriptor[int, str]): + pass + + +class Container: + value: IntFromStr + + +container = Container() +container.value = "1" +assert_type(container.value, int) + + +class SetterBase: + def __get__(self, instance: Any, owner: Any = None) -> int | str: + return cast(Any, None) + + def __set__(self, instance: Any, value: int | str) -> None: + pass + + +class GetterOverride(SetterBase): + def __get__(self, instance: Any, owner: Any = None) -> int: + return 0 + + +class OverrideContainer: + value: GetterOverride + + +override_container = OverrideContainer() +override_container.value = "1" +assert_type(override_container.value, int) + + +class AsymmetricProperty: + @property + def value(self) -> int: + return 0 + + @value.setter + def value(self, new_value: str) -> None: + pass + + +prop = AsymmetricProperty() +prop.value = "1" +assert_type(prop.value, int) + + +# Regression test for an instance attribute named __get__ not shadowing the +# class-level descriptor accessor during asymmetry detection. +class IntGetterStrSetter: + def __get__(self, obj: object, owner: object = None) -> int: + return 42 + + def __set__(self, obj: object, value: str) -> None: + pass + + +class ShadowedGetter(IntGetterStrSetter): + def __init__(self) -> None: + self.__get__: Any = None + + +class ShadowedGetterContainer: + value: ShadowedGetter = ShadowedGetter() + + +shadowed = ShadowedGetterContainer() +shadowed.value = "text" +assert_type(shadowed.value, int) + + +# Regression test for inherited generic accessors that use Self. +class SelfDescriptor: + stored: Self + + def __get__(self, obj: object, owner: object = None) -> Self: + return self.stored + + def __set__(self, obj: object, value: Self) -> None: + self.stored = value + + +class SelfSetterOverride(SelfDescriptor): + def __set__(self, obj: object, value: Self) -> None: # pyright: ignore[reportIncompatibleMethodOverride] + super().__set__(obj, value) + + +class SelfValue(SelfSetterOverride): + def extra(self) -> int: + return 42 + + +class SelfContainer: + value: SelfSetterOverride = SelfSetterOverride() + + +self_container = SelfContainer() +self_container.value = SelfValue() +assert_type(self_container.value, SelfValue) +self_container.value.extra() + + +# Sibling-mixin / diamond inheritance where accessors come from different branches. +class DiamondGetterMixin: + def __get__(self, instance: Any, owner: Any = None) -> int: + return 42 + + +class DiamondSetterMixin: + def __set__(self, instance: Any, value: str) -> None: + pass + + +class DiamondDescriptor(DiamondGetterMixin, DiamondSetterMixin): + pass + + +class DiamondContainer: + value: DiamondDescriptor = DiamondDescriptor() + + +diamond = DiamondContainer() +diamond.value = "text" +assert_type(diamond.value, int) + + +# Symmetric inherited descriptor preserves assignment narrowing. +class SymmetricGetterBase: + def __get__(self, instance: Any, owner: Any = None) -> int | str: + return cast(Any, None) + + +class SymmetricInheritedDescriptor(SymmetricGetterBase): + def __set__(self, instance: Any, value: int | str) -> None: + pass + + +class SymmetricContainer: + value: SymmetricInheritedDescriptor = SymmetricInheritedDescriptor() + + +sym = SymmetricContainer() +sym.value = "narrowed" +assert_type(sym.value, Literal["narrowed"]) + + +# Multiple specializations of the same generic descriptor without cross-instance leakage. +class GenericAsymmetricDesc[G, S]: + def __get__(self, instance: Any, owner: Any = None) -> G: + return cast(Any, None) + + def __set__(self, instance: Any, value: S) -> None: + pass + + +class DualSpecializationContainer: + first: GenericAsymmetricDesc[int, str] = GenericAsymmetricDesc() + second: GenericAsymmetricDesc[float, bytes] = GenericAsymmetricDesc() + + +dual = DualSpecializationContainer() +dual.first = "first" +assert_type(dual.first, int) + +dual.second = b"second" +assert_type(dual.second, float) + + +# Generic descriptor used through a generic container. +class GenericContainer[T]: + item: Descriptor[T, str] + + +gen_container: GenericContainer[int] = GenericContainer() +gen_container.item = "item" +assert_type(gen_container.item, int) + + +# Inherited overloaded accessors. +class OverloadedGetterBase: + @overload + def __get__(self, instance: None, owner: Any) -> Self: ... + @overload + def __get__(self, instance: object, owner: Any) -> int: ... + def __get__(self, instance: object | None, owner: Any = None) -> Any: + return cast(Any, None) + + +class OverloadedDescriptor(OverloadedGetterBase): + def __set__(self, instance: object, value: str) -> None: + pass + + +class OverloadedContainer: + value: OverloadedDescriptor = OverloadedDescriptor() + + +overloaded = OverloadedContainer() +overloaded.value = "test" +assert_type(overloaded.value, int) +assert_type(OverloadedContainer.value, OverloadedDescriptor) + + +# Rejection of invalid setter inputs. +container.value = 1.23 # pyright: ignore[reportAttributeAccessIssue] +diamond.value = 123 # pyright: ignore[reportAttributeAccessIssue] +overloaded.value = 123 # pyright: ignore[reportAttributeAccessIssue] diff --git a/packages/pyright-internal/src/tests/samples/descriptor7.py b/packages/pyright-internal/src/tests/samples/descriptor7.py new file mode 100644 index 0000000..2510b7e --- /dev/null +++ b/packages/pyright-internal/src/tests/samples/descriptor7.py @@ -0,0 +1,134 @@ +# This sample tests conditional narrowing after asymmetric descriptor assignments. + +from typing import assert_type + + +class Getter: + def __get__(self, instance: object, owner: object = None) -> int | None: + return None + + +class InheritedDescriptor(Getter): + def __set__(self, instance: object, value: int | str | None) -> None: + pass + + +class DirectDescriptor: + def __get__(self, instance: object, owner: object = None) -> int | None: + return None + + def __set__(self, instance: object, value: int | str | None) -> None: + pass + + +class InheritedContainer: + value = InheritedDescriptor() + + +class DirectContainer: + value = DirectDescriptor() + + +class PropertyContainer: + @property + def value(self) -> int | None: + return None + + @value.setter + def value(self, new_value: int | str | None) -> None: + pass + + +def inherited_guard(container: InheritedContainer, value: int | None) -> int: + container.value = value + assert_type(container.value, int | None) + if container.value is not None: + assert_type(container.value, int) + return container.value + 1 + else: + assert_type(container.value, None) + return 0 + + +def direct_guard(container: DirectContainer, value: int | None) -> int: + container.value = value + assert_type(container.value, int | None) + if container.value is not None: + assert_type(container.value, int) + return container.value + 1 + else: + assert_type(container.value, None) + return 0 + + +def property_guard(container: PropertyContainer, value: int | None) -> int: + container.value = value + assert_type(container.value, int | None) + if container.value is not None: + assert_type(container.value, int) + return container.value + 1 + else: + assert_type(container.value, None) + return 0 + + +def reset_guard(container: InheritedContainer) -> None: + if container.value is not None: + assert_type(container.value, int) + container.value = "reset" + assert_type(container.value, int | None) + + # This should generate an error because the write invalidates the earlier guard. + _ = container.value + 1 + + if container.value is not None: + assert_type(container.value, int) + _ = container.value + 1 + + +def branch_guard(container: InheritedContainer, condition: bool) -> None: + if condition: + container.value = "parsed" + else: + container.value = 1 + + assert_type(container.value, int | None) + if container.value is not None: + assert_type(container.value, int) + + +def loop_guard(container: InheritedContainer) -> None: + while container.value is not None: + assert_type(container.value, int) + container.value = "next" + assert_type(container.value, int | None) + if container.value is not None: + assert_type(container.value, int) + + +class GenericGetter[T]: + def __get__(self, instance: object, owner: object = None) -> T | None: + return None + + +class GenericDescriptor[T](GenericGetter[T]): + def __set__(self, instance: object, value: T | str | None) -> None: + pass + + +class GenericContainer[T]: + value: GenericDescriptor[T] = GenericDescriptor() + + +def generic_guard[T](container: GenericContainer[T]) -> T | None: + container.value = "parsed" + assert_type(container.value, T | None) + if container.value is not None: + assert_type(container.value, T) + return container.value + return None + + +def invalid_write(container: InheritedContainer) -> None: + # This should generate an error because the setter does not accept bytes. + container.value = b"invalid" diff --git a/packages/pyright-internal/src/tests/typeEvaluator8.test.ts b/packages/pyright-internal/src/tests/typeEvaluator8.test.ts index 92ba4cc..d78a710 100644 --- a/packages/pyright-internal/src/tests/typeEvaluator8.test.ts +++ b/packages/pyright-internal/src/tests/typeEvaluator8.test.ts @@ -13,6 +13,7 @@ import * as assert from 'assert'; import { EvalFlags } from '../analyzer/typeEvaluatorTypes'; import { ClassType, isClassInstance, isInstantiableClass, UnknownType } from '../analyzer/types'; import { ConfigOptions } from '../common/configOptions'; +import { DiagnosticRule } from '../common/diagnosticRules'; import { pythonVersion3_10, pythonVersion3_11, pythonVersion3_8, pythonVersion3_12 } from '../common/pythonVersion'; import { Uri } from '../common/uri/uri'; import { ParseNodeType } from '../parser/parseNodes'; @@ -863,6 +864,25 @@ test('Descriptor5', () => { TestUtils.validateResults(analysisResults, 0); }); +test('Descriptor6', () => { + const analysisResults = TestUtils.typeAnalyzeSampleFiles(['descriptor6.py']); + + TestUtils.validateResults(analysisResults, 0); +}); + +test('Descriptor7', () => { + const analysisResults = TestUtils.typeAnalyzeSampleFiles(['descriptor7.py']); + + TestUtils.validateResults(analysisResults, 2); + assert.deepStrictEqual( + analysisResults[0].errors.map((diagnostic) => [diagnostic.range.start.line + 1, diagnostic.getRule()]), + [ + [82, DiagnosticRule.reportOptionalOperand], + [134, DiagnosticRule.reportAttributeAccessIssue], + ] + ); +}); + test('Partial1', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['partial1.py']); -- 2.55.0.windows.3