[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.
https://github.com/astral-sh/ruff/pull/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.
This commit is contained in:
Carl Meyer 2025-10-07 18:38:24 -07:00 committed by GitHub
parent ff386b4797
commit 5d3a35e071
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 47 additions and 12 deletions

View file

@ -301,6 +301,30 @@ reveal_type(Container("a")) # revealed: Container[str]
reveal_type(Container(b"a")) # revealed: Container[bytes]
```
## Implicit self for classes with a default value for their generic parameter
```py
from typing import Self, TypeVar, Generic
class Container[T = bytes]:
def method(self) -> Self:
return self
def _(c: Container[str], d: Container):
reveal_type(c.method()) # revealed: Container[str]
reveal_type(d.method()) # revealed: Container[bytes]
T = TypeVar("T", default=bytes)
class LegacyContainer(Generic[T]):
def method(self) -> Self:
return self
def _(c: LegacyContainer[str], d: LegacyContainer):
reveal_type(c.method()) # revealed: LegacyContainer[str]
reveal_type(d.method()) # revealed: LegacyContainer[bytes]
```
## Invalid Usage
`Self` cannot be used in the signature of a function or variable.

View file

@ -7735,6 +7735,12 @@ pub enum BindingContext<'db> {
Synthetic,
}
impl<'db> From<Definition<'db>> for BindingContext<'db> {
fn from(definition: Definition<'db>) -> Self {
BindingContext::Definition(definition)
}
}
impl<'db> BindingContext<'db> {
fn name(self, db: &'db dyn Db) -> Option<String> {
match self {

View file

@ -26,10 +26,9 @@ use crate::types::function::FunctionType;
use crate::types::generics::{GenericContext, typing_self, walk_generic_context};
use crate::types::infer::nearest_enclosing_class;
use crate::types::{
ApplyTypeMappingVisitor, BindingContext, BoundTypeVarInstance, ClassLiteral,
FindLegacyTypeVarsVisitor, HasRelationToVisitor, IsEquivalentVisitor, KnownClass,
MaterializationKind, NormalizedVisitor, TypeMapping, TypeRelation, VarianceInferable,
todo_type,
ApplyTypeMappingVisitor, BoundTypeVarInstance, ClassLiteral, FindLegacyTypeVarsVisitor,
HasRelationToVisitor, IsEquivalentVisitor, KnownClass, MaterializationKind, NormalizedVisitor,
TypeMapping, TypeRelation, VarianceInferable, todo_type,
};
use crate::{Db, FxOrderSet};
use ruff_python_ast::{self as ast, name::Name};
@ -415,9 +414,7 @@ impl<'db> Signature<'db> {
let plain_return_ty = definition_expression_type(db, definition, returns.as_ref())
.apply_type_mapping(
db,
&TypeMapping::MarkTypeVarsInferable(Some(BindingContext::Definition(
definition,
))),
&TypeMapping::MarkTypeVarsInferable(Some(definition.into())),
);
if function_node.is_async && !is_generator {
KnownClass::CoroutineType
@ -1256,8 +1253,18 @@ impl<'db> Parameters<'db> {
let class = nearest_enclosing_class(db, index, scope_id).unwrap();
Some(
typing_self(db, scope_id, typevar_binding_context, class, &Type::TypeVar)
.expect("We should always find the surrounding class for an implicit self: Self annotation"),
// It looks like unnecessary work here that we create the implicit Self
// annotation using non-inferable typevars and then immediately apply
// `MarkTypeVarsInferable` to it. However, this is currently necessary to
// ensure that implicit-Self and explicit Self annotations are both treated
// the same. Marking type vars inferable will cause reification of lazy
// typevar defaults/bounds/constraints; this needs to happen for both
// implicit and explicit Self so they remain the "same" typevar.
typing_self(db, scope_id, typevar_binding_context, class, &Type::NonInferableTypeVar)
.expect("We should always find the surrounding class for an implicit self: Self annotation").apply_type_mapping(
db,
&TypeMapping::MarkTypeVarsInferable(None),
)
)
} else {
// For methods of non-generic classes that are not otherwise generic (e.g. return `Self` or
@ -1680,9 +1687,7 @@ impl<'db> Parameter<'db> {
annotated_type: parameter.annotation().map(|annotation| {
definition_expression_type(db, definition, annotation).apply_type_mapping(
db,
&TypeMapping::MarkTypeVarsInferable(Some(BindingContext::Definition(
definition,
))),
&TypeMapping::MarkTypeVarsInferable(Some(definition.into())),
)
}),
kind,