-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Discover site-packages from the environment that ty is installed in #21286
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
c97e603 to
bb73359
Compare
Gankra
left a comment
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.
Approach seems reasonable!
In terms of testing I would probably add a new test to crates/ty/tests/cli/python_environment.rs by adding an API to CliTest that lets you specify "copy the ty binary to this other dir before executing it".
Probably not a blocker, but as a brief note, I think this can be slow, e.g., in uv my experience was that the uv binary is pretty big and moving it around during tests is expensive. |
bb73359 to
1bdf24b
Compare
Is the mitigation as simple as hardlinking or maybe even symlinking here, or are we liable to run into temp-is-a-different-fs issues? |
|
It doesn't seem horribly slow so let's just not worry about it. What's the best practice for filtering here...? Of course, this snapshot is broken on Windows. I see there's some |
|
1dd2848 seems sufficient / fine |
Gankra
left a comment
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.
rad!
* main: (188 commits) [ty] Discover site-packages from the environment that ty is installed in (astral-sh#21286) [ty] Make special cases for `UnionType` slightly narrower (astral-sh#21276) Require ignore 0.4.24 in `Cargo.toml` (astral-sh#21292) [ty] Favour imported symbols over builtin symbols (astral-sh#21285) docs: revise Ruff setup instructions for Zed editor (astral-sh#20935) [ty] Update salsa (astral-sh#21281) [syntax-error]: no binding for nonlocal PLE0117 as a semantic syntax error (astral-sh#21032) [ty] Constraining a typevar with itself (possibly via union or intersection) (astral-sh#21273) [`ruff`] Fix false positives on starred arguments (`RUF057`) (astral-sh#21256) [ty] Simplify unions containing multiple type variables during inference (astral-sh#21275) [ty] Add `ty_server::Db` trait (astral-sh#21241) [ty] Refactor `Range` to/from `TextRange` conversion as prep for notebook support (astral-sh#21230) [ty] Fix playground crash when file name includes path separator (astral-sh#21151) [`refurb`] Fix false negative for underscores before sign in `Decimal` constructor (`FURB157`) (astral-sh#21190) [ty] Allow values of type `None` in type expressions (astral-sh#21263) Run codspeed benchmarks with `profiling` profile (astral-sh#21261) [ty] Update expected diagnostic count in benchmarks (astral-sh#21269) Avoid extra parentheses for long `match` patterns with `as` captures (astral-sh#21176) [ty] Update salsa (astral-sh#21265) [ty] `dict` is not assignable to `TypedDict` (astral-sh#21238) ...
* origin/main: Remove duplicate preview tests for `FURB101` and `FURB103` (#21303) [ty] Add support for `Literal`s in implicit type aliases (#21296) [ty] Add missing `heap_size` to `variance_of` queries (#21318) [`pyupgrade`] Fix false positive on relative imports from local `.builtins` module (`UP029`) (#21309) [ty] Make range/position conversions fallible (#21297) Bump 0.14.4 (#21306) Fix main by using `infer_expression` (#21299) [ty] Understand legacy and PEP 695 `ParamSpec` (#21139) [ty] Discover site-packages from the environment that ty is installed in (#21286) [ty] Make special cases for `UnionType` slightly narrower (#21276) Require ignore 0.4.24 in `Cargo.toml` (#21292) [ty] Favour imported symbols over builtin symbols (#21285) docs: revise Ruff setup instructions for Zed editor (#20935) [ty] Update salsa (#21281) [syntax-error]: no binding for nonlocal PLE0117 as a semantic syntax error (#21032)
Summary
Closes astral-sh/ty#989
There are various situations where users expect the Python packages installed in the same environment as ty itself to be considered during type checking. A minimal example would look like:
or
While these are a bit contrived, there are real-world situations where a user would expect a similar behavior to work. Notably, all of the other type checkers consider their own environment when determining search paths (though I'll admit that I have not verified when they choose not to do this).
One common situation where users are encountering this today is with
uvx --with-requirements script.py ty check script.py— which is currently our "best" recommendation for type checking a PEP 723 script, but it doesn't work.Of the options discussed in astral-sh/ty#989 (comment), I've chosen (2) as our criteria for including ty's environment in the search paths.
.venvis discovered in the working directory, we will prepend ty's environment to the search paths. The dependencies in ty's environment (e.g., fromuvx --with) will take precedence.VIRTUAL_ENV(i.e., including conda prefixes) is set, we will not include ty's environment.The reason we need to special case the
.venvcase is that we bothuvx tytoday as a way to check your projectuvx --with <...> tyAnd I don't want (2) to break when you happen to be in a project (i.e., if we only included ty's environment when no environment is found) and don't want to remove support for (1).
I think long-term, I want to make
uvx <cmd>layer the environment on top of the project environment (in uv), which would obviate the need for this change when you're using uv. However, that change is breaking and I think users will expect this behavior in contexts where they're not using uv, so I think we should handle it in ty regardless.I've opted not to include the environment if it's non-virtual (i.e., a system environment) for now. It seems better to start by being more restrictive. I left a comment in the code.
Test Plan
I did some manual testing with the initial commit, then subsequently added some unit tests.
Notice we do not include ty's environment if
VIRTUAL_ENVis set