[ty] Disallow Self in metaclass and static methods#22755
[ty] Disallow Self in metaclass and static methods#22755charliermarsh wants to merge 1 commit intomainfrom
Conversation
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 83.14% to 83.23%. The percentage of expected errors that received a diagnostic increased from 74.24% to 74.63%. Summary
True positives addedDetails
|
|
|
(Needs refinement, not ready for review!) |
ac2f05d to
e785fba
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
5d54ca3 to
3612413
Compare
fd312f3 to
1cdece4
Compare
1cdece4 to
08ed698
Compare
| /// | ||
| /// This is a cheap check that doesn't trigger type inference. A class with no | ||
| /// explicit bases only inherits from `object`. | ||
| pub(super) fn has_explicit_bases(self, db: &'db dyn Db) -> bool { |
There was a problem hiding this comment.
Because this method directly accessed the AST, it needs to be Salsa-tracked to avoid overly eager cache invalidation if it's accessed while inferring types for a different module to the one the class is defined in. But we try not to have too many Salsa-tracked functions/methods, because they increase our memory-usage, so it may be better to use the existing explicit_bases method unless this buys us a lot in terms of performance
| pass | ||
| ``` | ||
|
|
||
| ## `__new__` is not a static method |
There was a problem hiding this comment.
The behaviour demonstrated in this test is correct, but the prose describing it seems pretty confused. __new__ is a staticmethod, even if isn't decorated with @staticmethod. But using Self in __new__ is allowed, partly because of the fact that at runtime it is heavily special-cased by the interpreter and behaves in many ways more like you'd usually expect a classmethod to behave. It always receives a cls argument with type type[Self] first, and it often returns an object of type Self.
| } | ||
|
|
||
| /// Check if a function definition is decorated with `@staticmethod`. | ||
| pub(crate) fn is_function_staticmethod<'db>(db: &'db dyn Db, definition: Definition<'db>) -> bool { |
There was a problem hiding this comment.
Uff, I think we already have 3 methods for determining if a function is a staticmethod — I'd prefer not to add a fourth 😄
65ed42e to
443853e
Compare
c374d90 to
da36c0b
Compare
bfcdb75 to
fbadee6
Compare
4e293ce to
f806a7b
Compare
72fbc2e to
fea3526
Compare
|
I re-did this to track state during traversal rather than re-detecting static methods et al after the fact, because I had a lot of trouble shaking the regression on Altair. I suspect people will have qualms with the approach, open to feedback 😭 |
6867e2c to
f7d022d
Compare
a718377 to
50fb8e4
Compare
1a3c53f to
12947d2
Compare
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
5c27a4c to
eb2ff03
Compare
eb2ff03 to
0fd45c6
Compare
0fd45c6 to
37d02cb
Compare
|
Closing in favor of #23231 which is simpler. |
Summary
Closes astral-sh/ty#1897.