[red-knot] Decorators and properties #17017
Merged
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
Contributor
|
sharkdp
force-pushed
the
david/decorators
branch
2 times, most recently
from
ffea673 to
c724bad
Compare
sharkdp
force-pushed
the
david/decorators
branch
from
c724bad to
82ae1e7
Compare
sharkdp
commented
Mar 31, 2025
sharkdp
force-pushed
the
david/decorators
branch
from
82ae1e7 to
4d6da0d
Compare
sharkdp
force-pushed
the
david/decorators
branch
5 times, most recently
from
69812a3 to
2eae328
Compare
sharkdp
commented
Apr 1, 2025
Comment on lines
+69
to
+70
| # TODO: this should not be an error | ||
| # error: [unresolved-reference] |
Contributor
Author
There was a problem hiding this comment.
These are regressions, but after talking about it, we consider them fairly low priority. I will open a ticket to address them as soon as this PR is merged.
Contributor
Author
There was a problem hiding this comment.
sharkdp
commented
Apr 1, 2025
sharkdp
force-pushed
the
david/decorators
branch
from
2eae328 to
48d66b9
Compare
sharkdp
commented
Apr 1, 2025
carljm
approved these changes
Apr 2, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Excellent work as always, thank you!
| ## Lambdas as decorators | ||
|
|
||
| ```py | ||
| @lambda f: f |
Contributor
There was a problem hiding this comment.
Wow, the GitHub Python syntax highlighter and I have something in common: neither of us knew that this was valid syntax.
| | Type::AlwaysFalsy | ||
| | Type::AlwaysTruthy => true, | ||
| | Type::AlwaysTruthy | ||
| | Type::PropertyInstance(_) => true, |
Contributor
There was a problem hiding this comment.
Is this true even if some setter argument or getter return type is not fully static?
Currently I think effectively so, since those don't affect subtyping or assignability of the property type? It's always treated as just an instance of builtins.property. But in future when we add Protocol support, that won't necessarily be the case anymore.
I think it would probably be best to only call it fully static if its internal types are also all fully static?
Contributor
Author
There was a problem hiding this comment.
Oh, interesting — I hadn't thought about non-fully-static types in the context of structural subtyping. Does a class with an attribute/method that has a non-fully static type somewhere in its signature automatically become non-fully static?
The logical conclusion of that would be that we would have to consider types like int non-fully static, because int.__pow__ is annotated as returning Any.
Contributor
Author
There was a problem hiding this comment.
I left things as they were for now, but happy to make adjustments once I understand the situation more clearly.
Contributor
There was a problem hiding this comment.
Oh, yeah, good points. This could get ugly... I think you made the right choice, we'll have to consider this more deeply in implementing protocols.
sharkdp
force-pushed
the
david/decorators
branch
from
b35c7dc to
24ea203
Compare
This was referenced May 7, 2025
maxmynter
pushed a commit
to maxmynter/ruff
that referenced
this pull request
Apr 3, 2025
## Summary Add support for decorators on function as well as support for properties by adding special handling for `@property` and `@<name of property>.setter`/`.getter` decorators. closes astral-sh#16987 ## Ecosystem results - ✔️ A lot of false positives are fixed by our new understanding of properties - 🔴 A bunch of new false positives (typically `possibly-unbound-attribute` or `invalid-argument-type`) occur because we currently do not perform type narrowing on attributes. And with the new understanding of properties, this becomes even more relevant. In many cases, the narrowing occurs through an assertion, so this is also something that we need to implement to get rid of these false positives. - 🔴 A few new false positives occur because we do not understand generics, and therefore some calls to custom setters fail. - 🔴 Similarly, some false positives occur because we do not understand protocols yet. - ✔️ Seems like a true positive to me. [The setter](https://github.com/pypa/packaging/blob/e624d8edfaa28865de7b5a7da8bd59fd410e5331/src/packaging/specifiers.py#L752-L754) only accepts `bools`, but `None` is assigned in [this line](https://github.com/pypa/packaging/blob/e624d8edfaa28865de7b5a7da8bd59fd410e5331/tests/test_specifiers.py#L688). ``` + error[lint:invalid-assignment] /tmp/mypy_primer/projects/packaging/tests/test_specifiers.py:688:9: Invalid assignment to data descriptor attribute `prereleases` on type `SpecifierSet` with custom `__set__` method ``` - :heavy_check_mark: This is arguable also a true positive. The setter [here](https://github.com/Textualize/rich/blob/0c6c75644f80530de219dae3e94f0aeb999f9b4c/rich/table.py#L359-L363) returns `Table`, but typeshed wants [setters to return `None`](https://github.com/python/typeshed/blob/bf8d2a99126dcb155692c0ba56c4866fcd618393/stdlib/builtins.pyi#L1298). ``` + error[lint:invalid-argument-type] /tmp/mypy_primer/projects/rich/rich/table.py:359:5: Object of type `Literal[padding]` cannot be assigned to parameter 2 (`fset`) of bound method `setter`; expected type `(Any, Any, /) -> None` ``` ## Follow ups - Fix the `@no_type_check` regression - Implement class decorators ## Test Plan New Markdown test suites for decorators and properties.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PR adds support for decorators on functions. It also adds support for properties by adding special handling for
@propertyand@<name of property>.setter/.getterdecorators.closes #16987
Ecosystem results
possibly-unbound-attributeorinvalid-argument-type) occur because we currently do not perform type narrowing on attributes. And with the new understanding of properties, this becomes even more relevant. In many cases, the narrowing occurs through an assertion, so this is also something that we need to implement to get rid of these false positives.bools, butNoneis assigned in this line.Table, but typeshed wants setters to returnNone.Follow ups
@no_type_checkregressionTest Plan
New Markdown test suites for decorators and properties.