[red-knot] Silence errors in unreachable type annotations / class bases #17342
Conversation
|
e612893 to
855aa75
Compare
|
I haven't followed all the reachability discussions and I'm sorry if we discussed this before. Is the explicit reachability check now required for all diagnostics? Or is it only a small subset? |
I hope this is the last one 😄. I could imagine that we might need to add it to a few more isolated diagnostics, but it will not be required to check reachability for all diagnostics. That's a feature! We will still report division-by-zero, if you do Whether that approach truly takes us all the way, I don't know. But for now, this is my last PR w.r.t. unreachable code. This solves the last remaining TODOs in that test suite (expect for some TODOs around a new feature that would highlight/report unreachable code). |
855aa75 to
8891ccd
Compare
|
I'm not opposed to this approach but I've two thoughts:
|
|
@MichaReiser Excellent questions.
I don't know the full answer yet. And I find it hard to formulate a general strategy. If a rule detects a problem with the code that would also be there if the code was not unreachable, then it should report the problem in all cases (not respect reachabililty). So the following two diagnostics should be emitted in unreachable code: def _():
return
x: str = 1 # Object of type `Literal[1]` is not assignable to `str`
1 / 0 # Cannot divide object of type `Literal[1]` by zeroThe cases where we need to consider reachability are all related to the fact that the rule would otherwise emit a false positive because it depends on loading a symbol from a place in the code that is "on the other side" of the "reachability boundary". For example: def _():
class SomeClass: ...
some_variable = 1
return
some_variable # no unreachable-reference error here, even though `some_variable` is not visible
x: SomeClass = SomeClass() # no invalid-type-form error here, even though `SomeClass` is not visible
Yes, probably. Where would you look for this information / where should I put it?
That is a completely reasonable concern. I don't know how this scales. For now, four (out of ~50) rules needed a special "reachability treatment". Three of these rules (unresolved-reference, unresolved-attribute, unresolved-import) are very obviously related to "looking up symbols". So I would argue that there is just one case (invalid-type-form) that was kind of unexpected for me. It is related to the fact that we infer a type of from typing import Never
def _(IsThisAValidTypeExpression: Never):
x: IsThisAValidTypeExpression # Variable of type `Never` is not allowed in a type expression (lint:invalid-type-form)So to summarize, I hope that this approach does scale. But I could also imagine that we find more problems as we go. And it might turn out that we need to silence all diagnostics in unreachable code eventually. |
|
Thanks. This explanation was very useful!
Hmm, that's a good question. Two ideas come to my mind:
But I don't think there's any very obvious place. |
* main: [red-knot] Specialize `str.startswith` for string literals (#17351) [syntax-errors] `yield`, `yield from`, and `await` outside functions (#17298) [red-knot] Refresh diagnostics when changing related files (#17350) Add `Checker::import_from_typing` (#17340) Don't add chaperone space after escaped quote in triple quote (#17216) [red-knot] Silence errors in unreachable type annotations / class bases (#17342)
|
After discovering some problems with our current approach which I documented here, I decided to postpone the documentation of any guidelines here, as it seems likely that we need to pivot to a different strategy of silencing diagnostics in unreachable code. For now, I would suggest to simply not care about unreachable code when introducing new diagnostics (unless there are obvious false positives in the ecosystem checks). I opened a new ticket so we don't forget about this: astral-sh/ty#127 |
Summary
For silencing
invalid-type-formdiagnostics in unreachable code, we use the same approach that we use before and check the reachability that we already record.For silencing
invalid-bases, we simply check if the type of the base isNever. If so, we silence the diagnostic with the argument that the class construction would never happen.Test Plan
Updated Markdown tests.