Respect skip-interpreter-query for client-provided pythonPath in the LSP - #4615
Open
rootkiller6788 wants to merge 1 commit into
Open
Respect skip-interpreter-query for client-provided pythonPath in the LSP#4615rootkiller6788 wants to merge 1 commit into
rootkiller6788 wants to merge 1 commit into
Conversation
When a workspace config sets skip-interpreter-query = true, the LSP should not query the interpreter. But a client-provided pythonPath was queried eagerly in PythonInfo::new, before any config was read, and then applied on top of the config, so the opt-out had no effect and a deleted interpreter produced a spurious "Failed to query interpreter" error. Defer the query to the point where the workspace config is applied, and skip applying the client interpreter entirely when the config opted out of queries. A config with skip-interpreter-query = true now never performs (or pays for) an interpreter query. Adds a regression test for the LSP case and a cross-platform unit test for the workspace-side logic.
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D116931728. (Because this pull request was imported automatically, there will not be any future comments.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4445: with
skip-interpreter-query = trueinpyrefly.toml, the LSP still queried the interpreter the client supplied viapythonPath— logging e.g.ERROR Failed to query interpreter at <deleted venv>...and then applying that interpreter on top of the config, defeating the opt-out.Root cause
PythonInfo::newqueried the interpreter eagerly inupdate_pythonpath, i.e. as soon as the client sentpythonPath, before any config file had been resolved. Theskip-interpreter-queryflag only lives on theConfigFile, so it was never consulted. The configurer then applied the pre-queried environment unconditionally (when the config hadn't picked an interpreter).Fix
PythonInfonow stores only the interpreter path; the query is deferred to the point where the workspace config is applied (WorkspaceConfigConfigurer::configure).skip-interpreter-query).skip-interpreter-query = truenow never performs (or pays for) an interpreter query, and the clientpythonPathis ignored in favor of the config's own environment settings.Tests
test_skip_interpreter_query_ignores_lsp_pythonpath(unix-gated LSP interaction test): opens a project whose config setsskip-interpreter-query = true, verifies an unresolved import stays an error even after the client sends apythonPaththat would resolve it.test_skip_interpreter_query_blocks_client_pythonpath(cross-platform unit test inworkspace.rs): pins that askip-interpreter-queryconfig ignores a client interpreter while a config silent about interpreters still applies it.Verified with
cargo +1.96.1 test: all LSP interaction tests (1081) and theworkspace/configurationmodules pass.