-
Notifications
You must be signed in to change notification settings - Fork 109
Fix classmethod inference for TypeVars with PEP 696 defaults #1735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2337,16 +2337,40 @@ export function createTypeEvaluator( | |
| } | ||
|
|
||
| // If this is an unspecialized generic class, specialize it using the default | ||
| // values for its type parameters. | ||
| // values for its type parameters. Skip this if we're suppressing the use | ||
| // of attribute access override, such as with dundered methods (like __call__). | ||
| if ( | ||
| isInstantiableClass(objectType) && | ||
| !objectType.priv.includeSubclasses && | ||
| objectType.shared.typeParams.length > 0 | ||
| ) { | ||
| // Skip this if we're suppressing the use of attribute access override, | ||
| // such as with dundered methods (like __call__). | ||
| if ((flags & MemberAccessFlags.SkipAttributeAccessOverride) === 0) { | ||
| objectType = specializeWithDefaultTypeArgs(objectType); | ||
| let skipDefaultSpecialization = false; | ||
|
|
||
| // For classmethods on classes with PEP 696 explicit defaults, | ||
| // defer specialization so TypeVars remain free for inference | ||
| // from arguments (defaults applied via constructorTypeVarScopeId). | ||
| if ( | ||
| !objectType.priv.typeArgs && | ||
| objectType.shared.typeParams.some((tp) => isTypeVar(tp) && tp.shared.isDefaultExplicit) | ||
|
Comment on lines
+2350
to
+2355
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
there's no since this change deliberately goes against the PEP, it should probably only be enabled with the
Comment on lines
+2350
to
+2355
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, while i'm more than happy to deviate from the PEPs when we disagree with them, i'm a bit reluctant to support this in particular because generics on classmethods are already unsafe. see #1088 i don't know if it's a good idea to introduce a change that makes it easier for users to rely on this potentially dangerous pattern, until that issue is resolved. Code sample in basedpyright playground from typing import reveal_type
class Foo[T = int]:
_value: T | None = None
@classmethod
def set_value(cls, value: T):
cls._value = value
@classmethod
def get_value(cls) -> T | None:
return cls._value
Foo.set_value("") # error. this change would prevent this error from being reported
reveal_type(Foo.get_value()) # basedpyright: `int | None`, runtime: `str` |
||
| ) { | ||
| const memberInfo = lookUpClassMember(objectType, memberName); | ||
| if (memberInfo) { | ||
| const memberType = getEffectiveTypeOfSymbol(memberInfo.symbol); | ||
| if (isFunction(memberType) && FunctionType.isClassMethod(memberType)) { | ||
| skipDefaultSpecialization = true; | ||
| } else if (isOverloaded(memberType)) { | ||
| const overloads = OverloadedType.getOverloads(memberType); | ||
| if (overloads.length > 0 && FunctionType.isClassMethod(overloads[0])) { | ||
| skipDefaultSpecialization = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!skipDefaultSpecialization) { | ||
| objectType = specializeWithDefaultTypeArgs(objectType); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # This sample tests support for PEP 696 -- default types for TypeVars. | ||
| # In particular, it tests that class-level TypeVars with defaults can | ||
| # still be inferred from classmethod arguments. | ||
|
|
||
| from typing import Generic, Protocol, Self, assert_type | ||
|
|
||
| from typing_extensions import TypeVar # pyright: ignore[reportMissingModuleSource] | ||
|
|
||
| T = TypeVar("T", default=int) | ||
| U = TypeVar("U") | ||
| T_co = TypeVar("T_co", default=int, covariant=True) | ||
|
|
||
|
|
||
| class Factory(Protocol[T_co]): | ||
| def __call__(self) -> T_co: ... | ||
|
|
||
|
|
||
| class ClassA(Generic[T]): | ||
| @classmethod | ||
| def create(cls, *, factory: Factory[T] | None = None) -> Self: ... | ||
|
|
||
| def method(self) -> T: ... | ||
|
|
||
|
|
||
| def str_factory() -> str: ... | ||
|
|
||
|
|
||
| # No factory - should use default. | ||
| v1 = ClassA.create() | ||
| assert_type(v1, ClassA[int]) | ||
|
|
||
| # With factory - should infer from argument, not use default. | ||
| v2 = ClassA.create(factory=str_factory) | ||
| assert_type(v2, ClassA[str]) | ||
|
|
||
|
|
||
| class SubA(ClassA[T]): | ||
| pass | ||
|
|
||
|
|
||
| # Subclass without factory - should use default. | ||
| v3 = SubA.create() | ||
| assert_type(v3, SubA[int]) | ||
|
|
||
| # Subclass with factory - should infer from argument. | ||
| v4 = SubA.create(factory=str_factory) | ||
| assert_type(v4, SubA[str]) | ||
|
|
||
|
|
||
| # Direct parameter pattern (not wrapped in Protocol). | ||
| class ClassB(Generic[T]): | ||
| @classmethod | ||
| def from_value(cls, value: T) -> Self: ... | ||
|
|
||
| v5 = ClassB.from_value("hello") | ||
| assert_type(v5, ClassB[str]) | ||
|
|
||
| v6 = ClassB.from_value(42) | ||
| assert_type(v6, ClassB[int]) | ||
|
|
||
|
|
||
| # Mixed TypeVars: one with default, one without. | ||
| class ClassC(Generic[U, T]): | ||
| @classmethod | ||
| def build(cls, key: U, *, factory: Factory[T] | None = None) -> Self: ... | ||
|
|
||
| v7 = ClassC.build("key", factory=str_factory) | ||
| assert_type(v7, ClassC[str, str]) | ||
|
|
||
| v8 = ClassC.build("key") | ||
| assert_type(v8, ClassC[str, int]) | ||
|
|
||
|
|
||
| # Instance method should still get defaults applied. | ||
| v9 = ClassA[int]() | ||
| assert_type(v9.method(), int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was the comment about
__all__moved? i think it makes more sense next to the condition it's talking about