-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[red-knot] Reachability analysis #17199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[red-knot] Reachability analysis #17199
Conversation
|
751b05e
to
05a808b
Compare
/// use(y) | ||
/// ``` | ||
/// Depending on the value of `test`, the `y = 1`, `y = 2`, or both bindings may be visible. | ||
/// The use of `x` is recorded with a reachability constraint of `[test]`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm completely fine with keeping this more concise.
### Attributes | ||
|
||
When attribute expressions appear in unreachable code, we should not emit `unresolved-attribute` | ||
diagnostics: | ||
|
||
```py | ||
import sys | ||
import builtins | ||
|
||
if sys.version_info >= (3, 11): | ||
# TODO | ||
# error: [unresolved-attribute] | ||
builtins.ExceptionGroup | ||
``` | ||
|
||
### Imports | ||
|
||
When import statements appear in unreachable code, we should not emit `unresolved-import` | ||
diagnostics: | ||
|
||
```py | ||
import sys | ||
|
||
if sys.version_info >= (3, 11): | ||
# TODO | ||
# error: [unresolved-import] | ||
from builtins import ExceptionGroup | ||
|
||
# TODO | ||
# error: [unresolved-import] | ||
import builtins.ExceptionGroup | ||
|
||
# See https://docs.python.org/3/whatsnew/3.11.html#new-modules | ||
|
||
# TODO | ||
# error: [unresolved-import] | ||
import tomllib | ||
|
||
# TODO | ||
# error: [unresolved-import] | ||
import wsgiref.types | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave attributes and imports for a follow up. I want to get some feedback on this first.
05a808b
to
7ae5d31
Compare
7ae5d31
to
100d88b
Compare
@@ -218,26 +255,24 @@ def f(): | |||
|
|||
In the example below, since we use `x` in the `inner` function, we use the "public" type of `x`, | |||
which currently refers to the end-of-scope type of `x`. Since the end of the `outer` scope is | |||
unreachable, we treat `x` as if it was not defined. This behavior can certainly be improved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing changed in this test, but this explanation was outdated. And the return x # …
below as confusing.
|
||
```py | ||
def outer(): | ||
x = 1 | ||
|
||
def inner(): | ||
return x # Name `x` used when not defined | ||
reveal_type(x) # revealed: Unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that we reveal Unknown
here is tracked in https://github.com/astral-sh/ruff/issues/15777
FEATURE_X_ACTIVATED = False | ||
from typing import Literal | ||
|
||
FEATURE_X_ACTIVATED: Literal[False] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support Final
yet, so I needed to declare this as Literal[False]
. Otherwise, the public type (which we use below) would be Unknown | Literal[False]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
100d88b
to
03a2dcf
Compare
* origin/main: [red-knot] Default `python-platform` to current platform (#17183) [red-knot] Add new 'unreachable code' test case (#17306) [red-knot] mypy_primer: Run on `async-utils` (#17303) [red-knot] Add custom `__setattr__` support (#16748) [red-knot] Add `__init__` arguments check when doing `try_call` on a class literal (#16512) [`flake8-pie`] Avoid false positive for multiple assignment with `auto()` (`PIE796`) (#17274) [syntax-errors] Async comprehension in sync comprehension (#17177) [`airflow`] Expand module path check to individual symbols (`AIR302`) (#17278) [syntax-errors] Check annotations in annotated assignments (#17283) [syntax-errors] Extend annotation checks to `await` (#17282) [red-knot] Add support for `assert_never` (#17287) [`flake8-pytest-style`] Avoid false positive for legacy form of `pytest.raises` (`PT011`) (#17231) [red-knot] Do not show types for literal expressions on hover (#17290) [red-knot] Fix dead-code clippy warning (#17291) [red-knot] Reachability analysis (#17199) [red-knot] Don't use latency-sensitive for handlers (#17227)
Summary
This PR proposes a new approach to silencing
unresolved-reference
diagnostics by keeping track of the reachability of each use of a symbol. The changes merged in #17169 are still needed for the "Use of variable in nested function" test case, but that could also be solved in another way eventually (see astral-sh/ty#210). We can use the same technique to silenceunresolved-import
andunresolved-attribute
false-positives, but I think this could be merged in isolation.Test Plan
New Markdown tests, ecosystem tests