Skip to content

Commit 5d3a35e

Browse files
authored
[ty] fix implicit Self on generic class with typevar default (#20754)
## Summary Typevar attributes (bound/constraints/default) can be either lazily evaluated or eagerly evaluated. Currently they are lazily evaluated for PEP 695 typevars, and eager for legacy and synthetic typevars. #20598 will make them lazy also for legacy typevars, and the ecosystem report on that PR surfaced the issue fixed here (because legacy typevars are much more common in the ecosystem than PEP 695 typevars.) Applying a transform to a typevar (normalization, materialization, or mark-inferable) will reify all lazy attributes and create a new typevar with eager attributes. In terms of Salsa identity, this transformed typevar will be considered different from the original typevar, whether or not the attributes were actually transformed. In general, this is not a problem, since all typevars in a given generic context will be transformed, or not, together. The exception to this was implicit-self vs explicit Self annotations. The typevar we created for implicit self was created initially using inferable typevars, whereas an explicit Self annotation is initially non-inferable, then transformed via mark-inferable when accessed as part of a function signature. If the containing class (which becomes the upper bound of `Self`) is generic, and has e.g. a lazily-evaluated default, then the explicit-Self annotation will reify that default in the upper bound, and the implicit-self would not, leading them to be treated as different typevars, and causing us to fail to solve a call to a method such as `def method(self) -> Self` correctly. The fix here is to treat implicit-self more like explicit-Self, initially creating it as non-inferable and then using the mark-inferable transform on it. This is less efficient, but restores the invariant that all typevars in a given generic context are transformed together, or not, fixing the bug. In the improved-constraint-solver work, the separation of typevars into "inferable" and "non-inferable" is expected to disappear, along with the mark-inferable transform, which would render both this bug and the fix moot. So this fix is really just temporary until that lands. There is a performance regression, but not a huge one: 1-2% on most projects, 5% on one outlier. This seems acceptable, given that it should be fully recovered by removing the mark-inferable transform. ## Test Plan Added mdtests that failed before this change.
1 parent ff386b4 commit 5d3a35e

File tree

3 files changed

+47
-12
lines changed

3 files changed

+47
-12
lines changed

crates/ty_python_semantic/resources/mdtest/annotations/self.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,30 @@ reveal_type(Container("a")) # revealed: Container[str]
301301
reveal_type(Container(b"a")) # revealed: Container[bytes]
302302
```
303303

304+
## Implicit self for classes with a default value for their generic parameter
305+
306+
```py
307+
from typing import Self, TypeVar, Generic
308+
309+
class Container[T = bytes]:
310+
def method(self) -> Self:
311+
return self
312+
313+
def _(c: Container[str], d: Container):
314+
reveal_type(c.method()) # revealed: Container[str]
315+
reveal_type(d.method()) # revealed: Container[bytes]
316+
317+
T = TypeVar("T", default=bytes)
318+
319+
class LegacyContainer(Generic[T]):
320+
def method(self) -> Self:
321+
return self
322+
323+
def _(c: LegacyContainer[str], d: LegacyContainer):
324+
reveal_type(c.method()) # revealed: LegacyContainer[str]
325+
reveal_type(d.method()) # revealed: LegacyContainer[bytes]
326+
```
327+
304328
## Invalid Usage
305329

306330
`Self` cannot be used in the signature of a function or variable.

crates/ty_python_semantic/src/types.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7735,6 +7735,12 @@ pub enum BindingContext<'db> {
77357735
Synthetic,
77367736
}
77377737

7738+
impl<'db> From<Definition<'db>> for BindingContext<'db> {
7739+
fn from(definition: Definition<'db>) -> Self {
7740+
BindingContext::Definition(definition)
7741+
}
7742+
}
7743+
77387744
impl<'db> BindingContext<'db> {
77397745
fn name(self, db: &'db dyn Db) -> Option<String> {
77407746
match self {

crates/ty_python_semantic/src/types/signatures.rs

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,9 @@ use crate::types::function::FunctionType;
2626
use crate::types::generics::{GenericContext, typing_self, walk_generic_context};
2727
use crate::types::infer::nearest_enclosing_class;
2828
use crate::types::{
29-
ApplyTypeMappingVisitor, BindingContext, BoundTypeVarInstance, ClassLiteral,
30-
FindLegacyTypeVarsVisitor, HasRelationToVisitor, IsEquivalentVisitor, KnownClass,
31-
MaterializationKind, NormalizedVisitor, TypeMapping, TypeRelation, VarianceInferable,
32-
todo_type,
29+
ApplyTypeMappingVisitor, BoundTypeVarInstance, ClassLiteral, FindLegacyTypeVarsVisitor,
30+
HasRelationToVisitor, IsEquivalentVisitor, KnownClass, MaterializationKind, NormalizedVisitor,
31+
TypeMapping, TypeRelation, VarianceInferable, todo_type,
3332
};
3433
use crate::{Db, FxOrderSet};
3534
use ruff_python_ast::{self as ast, name::Name};
@@ -415,9 +414,7 @@ impl<'db> Signature<'db> {
415414
let plain_return_ty = definition_expression_type(db, definition, returns.as_ref())
416415
.apply_type_mapping(
417416
db,
418-
&TypeMapping::MarkTypeVarsInferable(Some(BindingContext::Definition(
419-
definition,
420-
))),
417+
&TypeMapping::MarkTypeVarsInferable(Some(definition.into())),
421418
);
422419
if function_node.is_async && !is_generator {
423420
KnownClass::CoroutineType
@@ -1256,8 +1253,18 @@ impl<'db> Parameters<'db> {
12561253
let class = nearest_enclosing_class(db, index, scope_id).unwrap();
12571254

12581255
Some(
1259-
typing_self(db, scope_id, typevar_binding_context, class, &Type::TypeVar)
1260-
.expect("We should always find the surrounding class for an implicit self: Self annotation"),
1256+
// It looks like unnecessary work here that we create the implicit Self
1257+
// annotation using non-inferable typevars and then immediately apply
1258+
// `MarkTypeVarsInferable` to it. However, this is currently necessary to
1259+
// ensure that implicit-Self and explicit Self annotations are both treated
1260+
// the same. Marking type vars inferable will cause reification of lazy
1261+
// typevar defaults/bounds/constraints; this needs to happen for both
1262+
// implicit and explicit Self so they remain the "same" typevar.
1263+
typing_self(db, scope_id, typevar_binding_context, class, &Type::NonInferableTypeVar)
1264+
.expect("We should always find the surrounding class for an implicit self: Self annotation").apply_type_mapping(
1265+
db,
1266+
&TypeMapping::MarkTypeVarsInferable(None),
1267+
)
12611268
)
12621269
} else {
12631270
// For methods of non-generic classes that are not otherwise generic (e.g. return `Self` or
@@ -1680,9 +1687,7 @@ impl<'db> Parameter<'db> {
16801687
annotated_type: parameter.annotation().map(|annotation| {
16811688
definition_expression_type(db, definition, annotation).apply_type_mapping(
16821689
db,
1683-
&TypeMapping::MarkTypeVarsInferable(Some(BindingContext::Definition(
1684-
definition,
1685-
))),
1690+
&TypeMapping::MarkTypeVarsInferable(Some(definition.into())),
16861691
)
16871692
}),
16881693
kind,

0 commit comments

Comments
 (0)