[ty] Implement protocol property check#19029
Open
mtshiba wants to merge 20 commits intoastral-sh:mainfrom
Open
[ty] Implement protocol property check#19029mtshiba wants to merge 20 commits intoastral-sh:mainfrom
mtshiba wants to merge 20 commits intoastral-sh:mainfrom
Conversation
mtshiba
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Contributor
|
AlexWaygood
reviewed
Jun 29, 2025
crates/ty_python_semantic/resources/mdtest/annotations/callable.md
Outdated
Show resolved
Hide resolved
AlexWaygood
reviewed
Jun 29, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Wow, thank you! I'll probably take a full look tomorrow :-D
mtshiba
force-pushed
the
protocol-property-check
branch
from
5b5ea68 to
a823074
Compare
Contributor
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-08-25 10:20:11.122932016 +0000
+++ new-output.txt 2025-08-25 10:20:13.657941955 +0000
@@ -252,17 +252,17 @@
dataclasses_transform_converter.py:108:5: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 6
dataclasses_transform_converter.py:109:5: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 6
dataclasses_transform_converter.py:112:11: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 6
-dataclasses_transform_converter.py:114:1: error[invalid-assignment] Object of type `Literal["f1"]` is not assignable to attribute `field0` of type `int`
-dataclasses_transform_converter.py:115:1: error[invalid-assignment] Object of type `Literal["f6"]` is not assignable to attribute `field3` of type `ConverterClass`
-dataclasses_transform_converter.py:116:1: error[invalid-assignment] Object of type `Literal[b"f6"]` is not assignable to attribute `field3` of type `ConverterClass`
-dataclasses_transform_converter.py:119:1: error[invalid-assignment] Object of type `Literal[1]` is not assignable to attribute `field3` of type `ConverterClass`
+dataclasses_transform_converter.py:114:1: error[invalid-assignment] Object of type `Literal["f1"]` is not assignable to attribute `field0` on type `int`
+dataclasses_transform_converter.py:115:1: error[invalid-assignment] Object of type `Literal["f6"]` is not assignable to attribute `field3` on type `ConverterClass`
+dataclasses_transform_converter.py:116:1: error[invalid-assignment] Object of type `Literal[b"f6"]` is not assignable to attribute `field3` on type `ConverterClass`
+dataclasses_transform_converter.py:119:1: error[invalid-assignment] Object of type `Literal[1]` is not assignable to attribute `field3` on type `ConverterClass`
dataclasses_transform_converter.py:121:11: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 7
dataclasses_transform_converter.py:130:31: error[invalid-argument-type] Argument to function `model_field` is incorrect: Expected `(Literal[1], /) -> Unknown`, found `def converter_simple(s: str) -> int`
dataclasses_transform_field.py:49:43: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `(...) -> @Todo(unsupported type[X] special form)`
dataclasses_transform_field.py:75:16: error[too-many-positional-arguments] Too many positional arguments: expected 0, got 1
dataclasses_transform_func.py:53:8: error[missing-argument] No arguments provided for required parameters `id`, `name`
dataclasses_transform_func.py:53:18: error[too-many-positional-arguments] Too many positional arguments: expected 0, got 2
-dataclasses_transform_func.py:57:1: error[invalid-assignment] Object of type `Literal[3]` is not assignable to attribute `name` of type `str`
+dataclasses_transform_func.py:57:1: error[invalid-assignment] Object of type `Literal[3]` is not assignable to attribute `name` on type `str`
dataclasses_transform_func.py:61:6: error[unsupported-operator] Operator `<` is not supported for types `Customer1` and `Customer1`
dataclasses_transform_func.py:65:36: error[unknown-argument] Argument `salary` does not match any known parameter
dataclasses_transform_func.py:71:8: error[missing-argument] No arguments provided for required parameters `id`, `name`
@@ -428,8 +428,8 @@
generics_self_advanced.py:44:9: error[type-assertion-failure] Argument does not have asserted type `typing.Self`
generics_self_advanced.py:45:9: error[type-assertion-failure] Argument does not have asserted type `typing.Self`
generics_self_attributes.py:26:33: error[invalid-argument-type] Argument is incorrect: Expected `typing.Self | None`, found `LinkedList[int]`
-generics_self_attributes.py:29:5: error[invalid-assignment] Object of type `OrdinalLinkedList` is not assignable to attribute `next` of type `typing.Self | None`
-generics_self_attributes.py:32:5: error[invalid-assignment] Object of type `LinkedList[int]` is not assignable to attribute `next` of type `typing.Self | None`
+generics_self_attributes.py:29:5: error[invalid-assignment] Object of type `OrdinalLinkedList` is not assignable to attribute `next` on type `typing.Self | None`
+generics_self_attributes.py:32:5: error[invalid-assignment] Object of type `LinkedList[int]` is not assignable to attribute `next` on type `typing.Self | None`
generics_self_basic.py:14:9: error[type-assertion-failure] Argument does not have asserted type `Self@set_scale`
generics_self_basic.py:20:16: error[invalid-return-type] Return type does not match returned value: expected `Self@method2`, found `Shape`
generics_self_basic.py:33:16: error[invalid-return-type] Return type does not match returned value: expected `Self@cls_method2`, found `Shape`
@@ -728,13 +728,19 @@
overloads_evaluation.py:322:5: error[type-assertion-failure] Argument does not have asserted type `list[int]`
protocols_class_objects.py:58:1: error[invalid-assignment] Object of type `<class 'ConcreteA'>` is not assignable to `ProtoA1`
protocols_class_objects.py:59:1: error[invalid-assignment] Object of type `<class 'ConcreteA'>` is not assignable to `ProtoA2`
+protocols_class_objects.py:74:1: error[invalid-assignment] Object of type `<class 'ConcreteB'>` is not assignable to `ProtoB1`
protocols_definition.py:114:1: error[invalid-assignment] Object of type `Concrete2_Bad1` is not assignable to `Template2`
protocols_definition.py:115:1: error[invalid-assignment] Object of type `Concrete2_Bad2` is not assignable to `Template2`
protocols_definition.py:116:1: error[invalid-assignment] Object of type `Concrete2_Bad3` is not assignable to `Template2`
protocols_definition.py:156:1: error[invalid-assignment] Object of type `Concrete3_Bad1` is not assignable to `Template3`
protocols_definition.py:159:1: error[invalid-assignment] Object of type `Concrete3_Bad4` is not assignable to `Template3`
protocols_definition.py:160:1: error[invalid-assignment] Object of type `Concrete3_Bad5` is not assignable to `Template3`
+protocols_definition.py:218:1: error[invalid-assignment] Object of type `Concrete4_Bad1` is not assignable to `Template4`
protocols_definition.py:219:1: error[invalid-assignment] Object of type `Concrete4_Bad2` is not assignable to `Template4`
+protocols_definition.py:339:1: error[invalid-assignment] Object of type `Concrete6_Bad1` is not assignable to `Template6`
+protocols_definition.py:340:1: error[invalid-assignment] Object of type `Concrete6_Bad2` is not assignable to `Template6`
+protocols_definition.py:341:1: error[invalid-assignment] Object of type `Concrete6_Bad3` is not assignable to `Template6`
+protocols_generic.py:146:1: error[invalid-assignment] Object of type `ConcreteHasProperty3` is not assignable to `HasPropertyProto`
protocols_merging.py:52:1: error[invalid-assignment] Object of type `SCConcrete2` is not assignable to `SizedAndClosable1`
protocols_merging.py:53:1: error[invalid-assignment] Object of type `SCConcrete2` is not assignable to `SizedAndClosable2`
protocols_merging.py:54:1: error[invalid-assignment] Object of type `SCConcrete2` is not assignable to `SizedAndClosable3`
@@ -850,5 +856,5 @@
typeddicts_operations.py:60:1: error[type-assertion-failure] Argument does not have asserted type `str | None`
typeddicts_type_consistency.py:101:1: error[invalid-assignment] Object of type `Unknown | None` is not assignable to `str`
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
-Found 851 diagnostics
+Found 857 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details. |
AlexWaygood
approved these changes
Aug 15, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
I'm sorry this has taken me so long to review!
This mostly looks great to me. The only things I'd like changed before merging are:
- I do prefer "on" rather than "of" for the error message you're changing, I'm afraid -- sorry! Can you revert back to the error message we currently have?
- I suggested a few minor optimisations below.
I also outlined a different design that would mean you could simplify some code here, which would also fix some other issues we have with regards to normalisation of protocols with property members. That's not blocking for this PR, however; I'm fine to land this and then I or you can tackle that refactor in a followup.
crates/ty_python_semantic/resources/mdtest/annotations/callable.md
Outdated
Show resolved
Hide resolved
AlexWaygood
requested changes
Aug 15, 2025
Member
There was a problem hiding this comment.
Since approving this PR, I ran this on a large codebase that we have access to internally, and realised that it causes new stack overflows. We'll need to fix that before landing this PR, unfortunately :-(
I managed to reduce the code triggering the stack overflow to this, though you might be able to minimize it further:
from typing import Protocol
class Foo[T]: ...
class A(Protocol):
@property
def _(self: "A") -> Foo: ...
class B(Protocol):
@property
def b(self) -> Foo[A]: ...
class C(Undefined): ...
class D:
b: Foo[C]
class E[T: B](Protocol): ...
x: E[D]Two strategies that can often be used to avoid stack overflows with recursive protocols are:
- Adding more calls to
.normalized()before doinghas_relation_to()checks - Improving use of type visitors to guard against recursion: using
.has_relation_to_implmethods rather than.has_relation_tomethods
Contributor
I think we should prefer the latter option (and eventually avoid using the former at all? at least for recursion-avoidance, maybe there are other reasons we need to normalize), but if the former option unblocks the PR more easily, we can always follow up and switch from the former to the latter. |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
mtshiba
force-pushed
the
protocol-property-check
branch
from
9bf0951 to
b15509e
Compare
mtshiba
commented
Aug 16, 2025
| (Type::ProtocolInstance(protocol), other) | ||
| | (other, Type::ProtocolInstance(protocol)) => visitor.visit((self, other), || { | ||
| (Type::ProtocolInstance(protocol), other_ty) | ||
| | (other_ty, Type::ProtocolInstance(protocol)) => visitor.visit((self, other), || { |
Collaborator
Author
There was a problem hiding this comment.
Although it's not directly related to this PR, I fixed a subtle bug. other was incorrectly shadowed here.
mtshiba
force-pushed
the
protocol-property-check
branch
from
e8d5e91 to
ed4cc36
Compare
AlexWaygood
reviewed
Aug 26, 2025
Comment on lines
+411
to
+430
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, salsa::Update, get_size2::GetSize)] | ||
| struct PropertyMember<'db> { | ||
| get_type: Option<Type<'db>>, | ||
| set_type: Option<Type<'db>>, | ||
| } | ||
|
|
||
| impl<'db> PropertyMember<'db> { | ||
| fn from_property_instance(property: PropertyInstanceType<'db>, db: &'db dyn Db) -> Self { | ||
| PropertyMember { | ||
| // if the getter or setter is a function literal, we should | ||
| // upcast it to a `CallableType` so that two protocols with identical property | ||
| // members are recognized as equivalent. | ||
| get_type: property | ||
| .getter(db) | ||
| .map(|ty| ty.into_callable(db).unwrap_or(ty)), | ||
| set_type: property | ||
| .setter(db) | ||
| .map(|ty| ty.into_callable(db).unwrap_or(ty)), | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Thank you very much for this PR -- it was really helpful! This still doesn't go quite in the direction I was thinking of, however -- you can see #19936 for the kind of thing I had in mind.
I think what I'd like to do at this point is to first land #20095 -- which includes many of your test improvements, plus some of my own -- and then land a cleaned-up version of #19936 on top of that. I'll list you as a co-author of both PRs, since both build on work you've contributed. I hope that's okay!
Collaborator
Author
There was a problem hiding this comment.
OK, let's move on!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a follow-up to #18847 and implements checks on protocol property members.
Since the logic implemented in
TypeInferenceBuilder::validate_attribute_assignmentwas required to check setters, I extracted the judgment part as aTypemethod. As a side effect, the diagnostics emission part and the judgment part are now separated, which I think has improved readability a little. Additionally, some error messages have been consolidated to make them more consistent.Test Plan
Some of the TODOs in
protocols.mdnow work properly.