Skip to content

[ty] Fix attribute access on TypedDicts#19758

Merged
sharkdp merged 1 commit intomainfrom
david/typeddict-attributes
Aug 5, 2025
Merged

[ty] Fix attribute access on TypedDicts#19758
sharkdp merged 1 commit intomainfrom
david/typeddict-attributes

Conversation

@sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Aug 5, 2025

Summary

This PR fixes a few inaccuracies in attribute access on TypedDicts. It also changes the return type of type(person) to type[dict[str, object]] if person: Person is an inhabitant of a TypedDict Person. We still use type[Person] as the meta type of Person, however (see reasoning here).

Test Plan

Updated Markdown tests.

@sharkdp sharkdp added the ty Multi-file analysis & type inference label Aug 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@sharkdp sharkdp force-pushed the david/typeddict-attributes branch 2 times, most recently from cf54b6c to 9ad1ecf Compare August 5, 2025 10:31
__setattr__ :: bound method object.__setattr__(name: str, value: Any, /) -> None
__sizeof__ :: bound method object.__sizeof__() -> int
__str__ :: bound method object.__str__() -> str
__subclasshook__ :: bound method type.__subclasshook__(subclass: type, /) -> bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were just (slightly) wrong before. FYI @BurntSushi.

@sharkdp sharkdp force-pushed the david/typeddict-attributes branch from 9ad1ecf to b2a1106 Compare August 5, 2025 10:34
// method, but `instance_of_SomeClass.__delattr__` is.
for Member { name, .. } in all_declarations_and_bindings(db, class_body_scope) {
let result = parent_instance.member(db, name.as_str());
let result = ty.member(db, name.as_str());
Copy link
Contributor Author

@sharkdp sharkdp Aug 5, 2025

Choose a reason for hiding this comment

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

This is a small bugfix. Instead of trying to access the member on instances of the parent class, just access them on the type that we're adding completions for — similar to how it's done in the method above. If we don't do this, the descriptor protocol will be invoked with the wrong instance type (which is why we see the changes in bound methods).

It was also possible to change this for TypedDict, because "instances"/inhabitants of TypedDict-based classes are not represented by Type::NominalInstance, but Type::TypedDict.

@sharkdp sharkdp marked this pull request as ready for review August 5, 2025 10:37
@sharkdp sharkdp force-pushed the david/typeddict-attributes branch from b2a1106 to 45ef715 Compare August 5, 2025 10:44
@sharkdp sharkdp force-pushed the david/typeddict-attributes branch from 45ef715 to b6b9352 Compare August 5, 2025 10:50
Comment on lines 5619 to 5623
return SubclassOfType::from(
db,
KnownClass::Dict
.to_specialized_class_type(
db,
[KnownClass::Str.to_instance(db), Type::object(db)],
)
.expect("dict[] takes two type parameters"),
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a class-literal type rather than a subclass-of type? Similar to the way we know that an int-literal must be an instance of exactly int (not a subclass of int), it's invalid for a TypedDict inhabitant to be an instance of a dict subclass -- it must be an instance of exactly dict, I think? This is different to NominalInstance types, which is why we use SubclassOf types as the meta-types for most instance types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't this be a class-literal type rather than a subclass-of type?

This is what I had at first. Then I noticed that type({}) is type[dict[Unknown, Unknown]] and I tried to model that similarly.

it's invalid for a TypedDict inhabitant to be an instance of a dict subclass -- it must be an instance of exactly dict

Yes, I think that's right. Will change back to <class 'dict[str, object]'>.

Copy link
Member

@AlexWaygood AlexWaygood Aug 5, 2025

Choose a reason for hiding this comment

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

This is what I had at first. Then I noticed that type({}) is type[dict[Unknown, Unknown]] and I tried to model that similarly.

yeah, the apparent inconsistency here is due to the fact that with x = {}, we infer the type of x as dict[Unknown, Unknown], which is "lossy" -- looking at the source code, we know that it is (for now) an instance of exactly dict, but this is not recorded in the type we infer: the type we infer allows for the possibility that x could be an instance of a subclass of dict. That lossiness in the type then means that the only safe type we can infer for type(x) here is type[dict[Unknown, Unknown]] rather than <class 'dict[Unknown, Unknown]'>

And the "lossiness" of the type we infer for x there is sort-of deliberate: you usually want to be able to substitute a subclass of dict wherever a dict is expected! But for TypedDict types, that's not allowed, so we can use a more precise type for type() calls on TypedDict inhabitants.

@sharkdp sharkdp force-pushed the david/typeddict-attributes branch 2 times, most recently from 98b1db7 to f5e3f3a Compare August 5, 2025 11:41
Copy link
Member

@AlexWaygood AlexWaygood 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!

@sharkdp sharkdp force-pushed the david/typeddict-attributes branch from f5e3f3a to eeb2d34 Compare August 5, 2025 11:48
@sharkdp sharkdp merged commit 948f3f8 into main Aug 5, 2025
38 checks passed
@sharkdp sharkdp deleted the david/typeddict-attributes branch August 5, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants