Skip to content

Conversation

@AlexWaygood
Copy link
Member

Summary

This PR fixes two bugs in our inference of protocol interfaces.

Firstly, we currently correctly record whether a protocol member is marked as a ClassVar... but only if the protocol originates from a .py file! If the protocol originated from a .pyi file, we were accidentally throwing this information away -- this is due to the fact that we iterate over both declarations and bindings to infer a protocol interface, and in a stub file x: ClassVar[int] is considered both a declaration and a binding, but the bindings iterator doesn't return any information about the qualifiers, and the information from the bindings iterator was overwriting any qualifiers information we'd obtained from the declarations iterator.

Secondly, we currently get subprotocols very wrong. For a situation like this, we infer SubProto as having a method member method that returns int, not bool, because we're incorrectly overwriting the information regarding the subprotocol with information about the parent protocol:

from typing import Protocol

class BaseProto(Protocol):
    def method(self) -> int: ...

class SubProto(BaseProto, Protocol):
    def method(self) -> bool: ...

Test Plan

Mdtests

@AlexWaygood AlexWaygood added bug Something isn't working ty Multi-file analysis & type inference labels Aug 17, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2025

Diagnostic diff on typing conformance tests

Changes were detected when running ty on typing conformance tests
--- old-output.txt	2025-08-19 10:29:21.750235491 +0000
+++ new-output.txt	2025-08-19 10:29:24.220254034 +0000
@@ -737,7 +737,6 @@
 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_definition.py:79:1: error[invalid-assignment] Object of type `Concrete` is not assignable to `Template`
 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`
@@ -860,5 +859,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 861 diagnostics
+Found 860 diagnostics
 WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2025

mypy_primer results

Changes were detected when running on open source projects
rich (https://github.com/Textualize/rich)
- rich/layout.py:113:46: error[invalid-argument-type] Argument to function `ratio_resolve` is incorrect: Expected `Sequence[Edge]`, found `Sequence[Layout]`
- rich/layout.py:133:48: error[invalid-argument-type] Argument to function `ratio_resolve` is incorrect: Expected `Sequence[Edge]`, found `Sequence[Layout]`
- Found 312 diagnostics
+ Found 310 diagnostics
No memory usage changes detected ✅

@AlexWaygood AlexWaygood force-pushed the alex/proto-meta-todo branch from fe877cd to 87144ba Compare August 17, 2025 18:08
@AlexWaygood AlexWaygood force-pushed the alex/proto-meta-todo branch from 46c1907 to f09afdd Compare August 18, 2025 20:34
Base automatically changed from alex/proto-meta-todo to main August 18, 2025 20:38
Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Comment on lines +646 to +670
static_assert(is_subtype_of(Foo, HasXWithDefault))
static_assert(is_assignable_to(Foo, HasXWithDefault))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The polarity of these tests has changed. They previously asserted that Foo was not a a subtype of HasXWithDefault. Indeed, Foo does not have an x attribute with a default, so if the current tests reflect the intended behavior, can we add a comment why this is correct (is the fact that the protocol member has a default value irrelevant?)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I keep on changing my mind about this, and need to make a decision 😆 thanks for calling it out.

As the conformance-suite diff shows, the conformance suite tests include an example where a class with implicit instance attributes is considered a subtype of a protocol where the member is bound in the class body. I think you can make that sound, but only if you say that the instance attribute is not present on the meta-protocol (type[HasXWithDefault]), which would obviously imply that we'd do attribute lookup differently for type[] Protocol types than we would for type[] nominal types.

I was actually confused about why the conformance suite diff changed, though, and you made me look again and realise that this PR was fixing another bug. We were inferring this protocol as having an x: Literal[0] member rather than an x: int member:

class P(Protocol):
    x: int = 0

I'll add a comment by this test, and add another test that explicitly asserts that we infer the correct interface for members that have default values.

}
_ => ProtocolMemberKind::Other(ty),
};
for (symbol_id, declarations) in use_def_map.all_end_of_scope_symbol_declarations() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do this sort of thing a lot in ty's code base: iterating over both bindings and declarations separately, hoping that there is a 1:1 relationship. This is not always the case. For example:

class D: # maybe a protocol, maybe a dataclass with fields, …
    if condition:
        attr: int
    else:
        attr = ""

This is unlikely to be a problem for protocols in particular, I think. And it's certainly not something specific to this PR. But I think it would be very beneficial in a lot of places if we could somehow iterate over all definitions and then get the declared type (for attr: int), the inferred type (for attr = ""), or both (for attr: int = 0).

Here, it would eliminate the mutation of the entry. And the bug would probably not have happened in the first place if the iterator would yield either a DeclaredType { ty }, an InferredType { ty }, or Both { declared_ty, inferred_ty }. See #19756 for a somewhat related problem that I fixed a few weeks ago.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created astral-sh/ty#1051 from this comment

@AlexWaygood AlexWaygood enabled auto-merge (squash) August 19, 2025 10:28
@AlexWaygood AlexWaygood merged commit e5c091b into main Aug 19, 2025
37 checks passed
@AlexWaygood AlexWaygood deleted the alex/subproto branch August 19, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants