[ty] Synthesize a _replace method for NamedTuples#22153
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
9ddf0c0 to
11e89b6
Compare
_replace method for NamedTuples_replace method for NamedTuples
_replace method for NamedTuples_replace method for NamedTuples
11e89b6 to
36b4ab8
Compare
| // For callable members that use `Self` type variables (e.g., synthesized methods like | ||
| // `_replace` on NamedTuples), replace `Self` with the instance type of this class. | ||
| // This ensures that when accessing `Person._replace`, we get `(self: Person, ...) -> Person` | ||
| // instead of `(self: Self, ...) -> Self`. | ||
| let self_instance = Type::instance(db, ClassType::NonGeneric(self)); | ||
| member = member.map_type(|ty| apply_self_to_callable(db, ty, self_instance)); |
There was a problem hiding this comment.
What's not clear to me is why we are accessing person._replace on the class to begin with, when we are calling it on an instance. That's something we do for dunder methods (to mimic the runtime, which also does that), but it's not something we should do for a normal instance method call; _replace is not a dunder method. That seems to be at the root of the need for this.
There was a problem hiding this comment.
Or to put the same question in a more specific way: the previous version of the code had this special case in the branch where instance was None in the descriptor protocol. But that should not be the case for a call like person._replace(...), where we clearly have an instance. So it seems like that's what we need to root-cause in order to fix this properly?
There was a problem hiding this comment.
With #22155, we now get the expected behavior for instances.
We still get Self for classes:
reveal_type(Person._replace) # revealed: (self: Self, *, name: str = ..., age: int | None = ...) -> SelfBut perhaps that's expected?
| // For callable members that use `Self` type variables (e.g., synthesized methods like | ||
| // `_replace` on NamedTuples), replace `Self` with the instance type of this class. | ||
| // This ensures that when accessing `Person._replace`, we get `(self: Person, ...) -> Person` | ||
| // instead of `(self: Self, ...) -> Self`. | ||
| let self_instance = Type::instance(db, ClassType::NonGeneric(self)); | ||
| member = member.map_type(|ty| apply_self_to_callable(db, ty, self_instance)); |
There was a problem hiding this comment.
Or to put the same question in a more specific way: the previous version of the code had this special case in the branch where instance was None in the descriptor protocol. But that should not be the case for a call like person._replace(...), where we clearly have an instance. So it seems like that's what we need to root-cause in order to fix this properly?
94837f2 to
0bfcbfc
Compare
|
I'd be inclined to revert the changes to diff --git a/crates/ty_python_semantic/resources/mdtest/named_tuple.md b/crates/ty_python_semantic/resources/mdtest/named_tuple.md
index bf5b1e4d9b..812ff930c0 100644
--- a/crates/ty_python_semantic/resources/mdtest/named_tuple.md
+++ b/crates/ty_python_semantic/resources/mdtest/named_tuple.md
@@ -347,7 +347,7 @@ satisfy:
def expects_named_tuple(x: typing.NamedTuple):
reveal_type(x) # revealed: tuple[object, ...] & NamedTupleLike
reveal_type(x._make) # revealed: bound method type[NamedTupleLike]._make(iterable: Iterable[Any]) -> NamedTupleLike
- reveal_type(x._replace) # revealed: bound method NamedTupleLike._replace(**kwargs) -> NamedTupleLike
+ reveal_type(x._replace) # revealed: bound method NamedTupleLike._replace(...) -> NamedTupleLike
# revealed: Overload[(value: tuple[object, ...], /) -> tuple[object, ...], (value: tuple[_T@__add__, ...], /) -> tuple[object, ...]]
reveal_type(x.__add__)
reveal_type(x.__iter__) # revealed: bound method tuple[object, ...].__iter__() -> Iterator[object]
diff --git a/crates/ty_python_semantic/src/types/instance.rs b/crates/ty_python_semantic/src/types/instance.rs
index 11780e6fc6..9e674065b9 100644
--- a/crates/ty_python_semantic/src/types/instance.rs
+++ b/crates/ty_python_semantic/src/types/instance.rs
@@ -3,7 +3,6 @@
use std::borrow::Cow;
use std::marker::PhantomData;
-use super::class::CodeGeneratorKind;
use super::protocol_class::ProtocolInterface;
use super::{BoundTypeVarInstance, ClassType, KnownClass, SubclassOfType, Type, TypeVarVariance};
use crate::place::PlaceAndQualifiers;
@@ -133,16 +132,6 @@ impl<'db> Type<'db> {
relation_visitor: &HasRelationToVisitor<'db>,
disjointness_visitor: &IsDisjointVisitor<'db>,
) -> ConstraintSet<'db> {
- // Iff we're checking a NamedTuple against NamedTupleLike, skip the `_replace` method
- // signature. NamedTuples synthesize `_replace` methods with specific keyword-only
- // parameters (to detect invalid arguments), which are not strictly subtypes of the
- // protocol's `(**kwargs)` signature, but are intended to be considered as satisfying it.
- let is_namedtuple_protocol_check = matches!(&protocol.inner, Protocol::FromClass(class) if class.is_known(db, KnownClass::NamedTupleLike))
- && self.as_nominal_instance().is_some_and(|instance| {
- let (class_literal, specialization) = instance.class(db).class_literal(db);
- CodeGeneratorKind::NamedTuple.matches(db, class_literal, specialization)
- });
-
let structurally_satisfied = if let Type::ProtocolInstance(self_protocol) = self {
self_protocol.interface(db).has_relation_to_impl(
db,
@@ -157,16 +146,6 @@ impl<'db> Type<'db> {
.inner
.interface(db)
.members(db)
- .filter(|member| {
- // Skip `_replace` check for NamedTuple vs NamedTupleLike. NamedTuples
- // synthesize `_replace` with specific keyword-only parameters to detect
- // invalid arguments, but this signature is not a strict subtype of the
- // protocol's `(**kwargs)` signature.
- if is_namedtuple_protocol_check && member.name() == "_replace" {
- return false;
- }
- true
- })
.when_all(db, |member| {
member.is_satisfied_by(
db,
diff --git a/crates/ty_vendored/ty_extensions/ty_extensions.pyi b/crates/ty_vendored/ty_extensions/ty_extensions.pyi
index 347b6b4b34..ed0bf16186 100644
--- a/crates/ty_vendored/ty_extensions/ty_extensions.pyi
+++ b/crates/ty_vendored/ty_extensions/ty_extensions.pyi
@@ -190,6 +190,16 @@ class NamedTupleLike(Protocol):
@classmethod
def _make(cls: type[Self], iterable: Iterable[Any]) -> Self: ...
def _asdict(self, /) -> dict[str, Any]: ...
- def _replace(self, /, **kwargs) -> Self: ...
+
+ # Positional arguments aren't actually accepted by these methods at runtime,
+ # but adding the `*args` parameters means that all `NamedTuple` classes
+ # are understood as assignable to this protocol due to the special case
+ # outlined in https://typing.python.org/en/latest/spec/callables.html#meaning-of-in-callable:
+ #
+ # > If the input signature in a function definition includes both a
+ # > `*args` and `**kwargs` parameter and both are typed as `Any`
+ # > (explicitly or implicitly because it has no annotation), a type
+ # > checker should treat this as the equivalent of `...`.
+ def _replace(self, *args, **kwargs) -> Self: ...
if sys.version_info >= (3, 13):
- def __replace__(self, **kwargs) -> Self: ...
+ def __replace__(self, *args, **kwargs) -> Self: ... |
|
On Python 3.13+, it looks like we should also be synthesising precise |
|
(Depends on #22155.) |
Summary
Closes astral-sh/ty#2170.