[ty] Add decorator check for implicit attribute assignments #18587
Conversation
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
med1844
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
med1844
changed the title
Contributor
|
CodSpeed Instrumentation Performance ReportMerging #18587 will degrade performances by 6.91%Comparing Summary
Benchmarks breakdown
|
Contributor
Author
|
mypy primer looks really bad... converting to draft for now. I think I might have messed up staticmethods... |
Contributor
Author
|
Removing the mapping |
Contributor
|
Thank you for working on this. I'm not doing a review for now since it's still a draft. Let us know when you need help with this. |
med1844
force-pushed
the
check_assignments_in_classmethods
branch
from
01ddbfa to
065cb83
Compare
Contributor
Author
|
It took me a good while to manually check why there's so many more diagnostics in I thought it's me that screwed up some As for So, the conclusion is that to my best knowledge, the |
MichaReiser
reviewed
Jun 11, 2025
med1844
force-pushed
the
check_assignments_in_classmethods
branch
from
065cb83 to
be588ca
Compare
sharkdp
pushed a commit
that referenced
this pull request
Jun 20, 2025
## Summary Add support for `@staticmethod`s. Overall, the changes are very similar to #16305. #18587 will be dependent on this PR for a potential fix of astral-sh/ty#207. mypy_primer will look bad since the new code allows ty to check more code. ## Test Plan Added new markdown tests. Please comment if there's any missing tests that I should add in, thank you.
med1844
force-pushed
the
check_assignments_in_classmethods
branch
from
be588ca to
c34582f
Compare
Contributor
|
Thank you very much for working on this! I am planning to review this tomorrow. |
sharkdp
approved these changes
Jun 24, 2025
Contributor
sharkdp
left a comment
sharkdp
left a comment
There was a problem hiding this comment.
Thank you very much — this is great!
In particular, thank you for going through the salsa debugging session and for adding a incremental-computation test.
While looking through the ecosystem changes, I noticed a behavior related to attributes that are implicitly defined in methods with an unknown decorator:
class C:
@unknown_decorator
def f(self):
self.x: int = 1
C.x # this was previously an errorWe now consider these attributes to be available on the class itself. I think this is actually a good example of the gradual guarantee: unknown_decorator could, in principle, be an alias to builtins.classmethod. And so it seems good to not show an error here. Because if unknown_decorator was annotated appropriately, we would not show an error as well.
I added a test to document this.
Contributor
I think it's fine to keep this for a follow-up changeset. I generally agree with your analysis, but maybe adding an implicit class C1:
x: ClassVar[int]
class C2:
@classmethod
def method(cls):
cls.x: intIf this is inconsistent for some reason (maybe when printing the type + qualifiers of |
Contributor
I acknowledged this performance regression. There are also a few smaller 1-2% regressions in other benchmarks. On this branch, we need to do a lot more work on every attribute access The |
Contributor
Author
|
Thank you for the review and the commits! Sorry I was not paying enough attention to code comments. Regarding the test case:
My main concern is that it would be surprising for users to see
I'm not a huge fan of adding something so similar to If we decides to go with |
dcreager
added a commit
that referenced
this pull request
Jun 24, 2025
* main: [ty] Fix false positives when subscripting an object inferred as having an `Intersection` type (#18920) [`flake8-use-pathlib`] Add autofix for `PTH202` (#18763) [ty] Add relative import completion tests [ty] Clarify what "cursor" means [ty] Add a cursor test builder [ty] Enforce sort order of completions (#18917) [formatter] Fix missing blank lines before decorated classes in .pyi files (#18888) Apply fix availability and applicability when adding to `DiagnosticGuard` and remove `NoqaCode::rule` (#18834) py-fuzzer: allow relative executable paths (#18915) [ty] Change `environment.root` to accept multiple paths (#18913) [ty] Rename `src.root` setting to `environment.root` (#18760) Use file path for detecting package root (#18914) Consider virtual path for various server actions (#18910) [ty] Introduce `UnionType::try_from_elements` and `UnionType::try_map` (#18911) [ty] Support narrowing on `isinstance()`/`issubclass()` if the second argument is a dynamic, intersection, union or typevar type (#18900) [ty] Add decorator check for implicit attribute assignments (#18587) [`ruff`] Trigger `RUF037` for empty string and byte strings (#18862) [ty] Avoid duplicate diagnostic in unpacking (#18897) [`pyupgrade`] Extend version detection to include `sys.version_info.major` (`UP036`) (#18633) [`ruff`] Frozen Dataclass default should be valid (`RUF009`) (#18735)
Contributor
👍
I think we'll eventually want to add a richer return type for methods such as
In this process, we could probably also add metadata for what is needed here.
I think we could probably try to build a nice abstraction? What I'm more concerned about is the fact that type qualifiers have a rather precise meaning, and they are only associated with declared / explicitly annotated symbols or attributes. With this change, we would also attach non-trivial type qualifiers to attributes that were defined without an annotation ( |
Contributor
Author
|
I'm not sure if I fully understands your points, here's my understanding:
The metadata approach seems to solve the root problem and is extensible, though I guess it needs a little bit more refactoring. |
Contributor
Yes, exactly. I'm afraid this would be a larger project, but the issue here is not that urgent, I think. So it might make sense to just wait for that metadata refactoring/extension. |
Contributor
Author
|
Sounds good! I will work on other issues while waiting for the refactor. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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
Previously, the checks for implicit attribute assignments didn't properly account for method decorators. This PR fixes that by:
implicit_instance_attribute. This allows it to filter out methods with mismatching decorators when analyzing attribute assignments.ClassLiteral::own_class_memberfunction will now search in classmethods.staticmethod: it has been added intoKnownClassand together with the new decorator check, it will no longer expose attributes when the assignment target name is the same as the first method name.If accepted, it should fix astral-sh/ty#205 and astral-sh/ty#207.
Test Plan
This is tested with existing mdtest suites and is able to get most of the TODO marks for implicit assignments in classmethods and staticmethods removed.
However, there's one specific test case I failed to figure out how to correctly resolve:
https://github.com/med1844/ruff/blob/b279508bdc63c1ed6fc4ccf9d43d3719fe7a202b/crates/ty_python_semantic/resources/mdtest/attributes.md?plain=1#L754-L755
I tried to add
instance_member().is_unbound()check in this else branch but it causes tests with class attributes defined in class body to fail. While it's possible to implicitly addClassVarto qualifiers to make this assignment fail and keep everything else passing, it doesn't feel like the right solution.Another problem is that this PR also kind of breaks
dependency_implicit_instance_attributeanddependency_own_instance_memberinsrc/types/infer.rs. This seems to be caused by the new decorator check, which uses the inferred method function type to get the inferred decorator type. Thus, any change inside function body causes re-inferring method type. As a result, both "no re-infer" assertions in these two tests fails. I'm not 100% sure, but that's my current theory.Thank you very much for taking time to review 🙏