[ty] Disallow Self in metaclass and static methods#23231
Conversation
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 86.32% to 86.38%. The percentage of expected errors that received a diagnostic increased from 80.30% to 80.68%. The number of fully passing files held steady at 67/133. SummaryHow are test cases classified?Each test case represents one expected error annotation or a group of annotations sharing a tag. Counts are per test case, not per diagnostic — multiple diagnostics on the same line count as one. Required annotations (
Test file breakdown1 file altered
True positives added (4)4 diagnostics
|
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
charliermarsh
changed the title
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
|
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Thanks, this is surprisingly tricky! (As I'm sure you already know, given it looks like this was already the third try...)
| return false; | ||
| }; | ||
|
|
||
| infer_definition_types(db, binding_definition) |
There was a problem hiding this comment.
This means we will create a Salsa cycle if we ever have to evaluate annotations while still within infer_definition_types for the function itself, and one of those annotations is Self. That can be triggered by another stacked decorator that takes and returns Callable[P, R], because that requires evaluating the signature of the original function, which means evaluating its annotations.
It's hard to say right now if this cycle causes correctness problems or not, because the .as_function_literal() issue mentioned below would always mask it in those same stacked-decorator cases. But either way it's not great for perf to introduce more cycles.
But I'm also not sure what to do to avoid this cycle. I think probably the right answer is to factor out a separate query specifically for inferring decorator types on a function. Then we could call that query here with no cycle risk. That's very doable; could try it in this PR or as a follow-up if it seems like the cycle isn't causing any immediate breakage here.
charliermarsh
force-pushed
the
charlie/self3
branch
2 times, most recently
from
1c05450 to
609397d
Compare
charliermarsh
force-pushed
the
charlie/self3
branch
2 times, most recently
from
9e93ebe to
aaf46b3
Compare
charliermarsh
force-pushed
the
charlie/self3
branch
2 times, most recently
from
9533793 to
01c4a2e
Compare
charliermarsh
force-pushed
the
charlie/self3
branch
from
01c4a2e to
9c7b4dc
Compare
charliermarsh
force-pushed
the
charlie/self3
branch
from
9c7b4dc to
be06b01
Compare
| let InferenceRegion::Definition(definition) = self.region else { | ||
| panic!("Expected Definition region"); | ||
| }; |
There was a problem hiding this comment.
I think it will make the code easier to follow if we stick to the existing pattern of all the other inference kinds. This would imply a new InferenceRegion::FunctionDecorators(definition) to make it explicit that we are, in fact, inferring a more specific region than the entire definition. And then we'd just call infer_region method to do the actual inference here, with no need for the unreachable panic.
| let decorator_inference = function_known_decorators(db, definition); | ||
| self.context.extend(decorator_inference.diagnostics()); | ||
| self.expressions.extend( | ||
| decorator_inference | ||
| .expression_types() | ||
| .iter() | ||
| .map(|(expression, ty)| (*expression, *ty)), | ||
| ); |
There was a problem hiding this comment.
I'm not sure if it currently has any visible impact (it seems like, independently, our called_functions tracking for check_overloaded_function isn't quite working as I think it should, even on main), but I think we should also copy called_functions over, otherwise we will miss recording functions that are called as decorators.
Not sure if we might also need to copy over definitions, in case of walrus expressions inside a decorator expression?
charliermarsh
force-pushed
the
charlie/self3
branch
from
be06b01 to
c3e81ce
Compare
* main: [ty] make `test-case` a dev-dependency (#24187) [ty] implement cycle normalization for more types to prevent too-many-cycle panics (#24061) [ty] Silence all diagnostics in unreachable code (#24179) [ty] Intern `InferableTypeVars` (#24161) Implement unnecessary-if (RUF050) (#24114) Recognize `Self` annotation and `self` assignment in SLF001 (#24144) Bump the npm version before publish (#24178) [ty] Disallow Self in metaclass and static methods (#23231) Use trusted publishing for NPM packages (#24171) [ty] Respect non-explicitly defined dataclass params (#24170) Add RUF072: warn when using operator on an f-string (#24162) [ty] Check return type of generator functions (#24026) Implement useless-finally (RUF-072) (#24165) [ty] Add test for a dataclass with a default field converter (#24169) [ty] Dataclass field converters (#23088) [flake8-bandit] Treat sys.executable as trusted input in S603 (#24106) [ty] Add support for `typing.Concatenate` (#23689) `ASYNC115`: autofix to use full qualified `anyio.lowlevel` import (#24166) [ty] Disallow read-only fields in TypedDict updates (#24128) Speed up diagnostic rendering (#24146)
2 participants
Summary
Closes astral-sh/ty#1897.