Repository navigation
fix Command-line entry-point for LSP features #3148 - #4183
asukaminato0721 wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
|
This pull request has been imported. If you are a Meta employee, you can view this in D114356563. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@kinto0 I think you'd be the best reviewer for this one, since you opened the original feature request and have been doing a lot of pyrefly+AI experiments. Do we still want a command-line LSP entrypoint? |
stroxler
left a comment
There was a problem hiding this comment.
Thanks!
I think this is potentially exciting. I'll leave @kinto0 as the assigned reviewer because he is the one who filed this while thinking about agent interactions.
That said, an initial round of AI review highlighted some issues with how this works:
Confirmed findings
1. target file does not select its project (most critical issue)
FilesArgs::get(Vec::new(), ...) enters CWD project mode, installs that project's constant configuration, and only afterward appends the target file. Target-local and nested configurations are bypassed.
Executed repro: querying the same absolute core.py for references from its project returned four locations. Running from its parent returned only the declaration, with exit status 0. Supplying the target project's --config restored four locations. An agent can therefore mistake a project-selection failure for “no callers.”
Minimum fix: absent explicit --config, discover the project from the target and populate that project's candidate files. Merely checking the target alone will still miss reverse dependencies. Preserve appropriate per-file configuration. This also needs to avoid project mode's explicit rejection of build-system configurations; sourcedb projects currently cannot enter this path.
2. bundled stubs make ordinary semantic queries fail
The adapter directly converts ModulePath::as_path() to a file URI. Bundled typeshed paths are relative/internal paths; the LSP's existing module-to-URI helper materializes them first.
Executed repro: at json.dumps(...), find-references --include-declaration exited 1 with “reference ... does not have a file URL,” dropping the whole response. call-hierarchy exited 1 with “definition does not have a file URL.”
Minimum fix: reuse bundled-aware URI conversion. Do not absolutize a bundled path against CWD and manufacture a nonexistent location. Cover builtin and standard-library symbols in tests.
3. missing target is indistinguishable from valid empty hover
The target is appended without checking load success. The loader can represent a failed read as an empty module plus a diagnostic, which this adapter does not inspect.
Executed repro: pyrefly query hover missing.py 1 1 printed null, exited 0, and emitted no missing-file error. Valid whitespace hover produced the same result.
Minimum fix: propagate target load failure as a nonzero exit with an actionable error, while permitting useful queries in projects with ordinary typing errors. Reject invalid/stale positions rather than silently clamping them if practical.
Cost and output contract
Every query currently fully checks and retains the selected project and its dependencies at Require::Everything (171–175). Existing LSP project indexing is lighter. A target-only hover and cheaper project indexing are bounded improvements worth considering.
On this development build/high-core FaaS node, Black queries took roughly 2.9–3.1 seconds and 602–622 MiB peak RSS; project checking took 2.25 seconds and 542 MiB. Single-threaded Black queries took about 17 seconds. These are exploratory measurements, not release-build benchmarks or production latency predictions.
Inputs are one-based UTF-16; outputs use zero-based LSP ranges and file URIs. That is workable, but help/examples must make the asymmetry explicit so an agent can round-trip returned locations. Keep the output contract experimental until validated. No new work runs on existing non-query command paths.
Minimum next step
Fix the three adapter bugs, then add end-to-end CLI tests asserting exact locations/order, clean JSON stdout, configuration discovery, bundled stubs, and failure behavior. Document coordinates and known coverage limits. Run a small real-repo agent comparison: identify callers, inspect a signature, and traverse one call edge, checking correctness plus token/latency cost against grep/file reads.
Also interesting: I asked "does it help an agent?"
(@kinto0 : I like to ask an agent whether it would help an agent, it's interesting to try to interview them although I'm not sure how reliable the opinion is)
Yes, materially. Querying Black's format_str returned:
- Its typed signature and documentation, in about 1.0 KB of JSON.
- Three reference locations, including the declaration, in about 0.4 KB.
- Callers
format_cellandformat_file_contents, and callees including_format_str_onceand the line-range helpers, in about 2.6 KB.
This supports a real navigation/impact-analysis workflow without reading every file or implementing the LSP handshake. JSON-wrapped Markdown hover is acceptable; the issue does not require a bespoke semantic summary schema. Symbol-name discovery, a daemon, batching, and MCP can remain follow-ups.
But do not promise exhaustive impact analysis. In the tested imported engine, an aliased import's call was missing from references/incoming calls, and querying a non-callable local could identify its enclosing function. These are limitations of reused engine/helpers, not new adapter regressions; they should inform the effectiveness experiment. They were not re-tested against the newer GitHub base.
Kyle's issue explicitly says to experiment with real-world tools and test effectiveness before merging. The Black smoke test demonstrates potential, but is not that controlled effectiveness test. The right conclusion is “keep and refine this small CLI prototype,” not “replace it with a larger agent platform,” and not “the issue is fully solved.”
Summary
Fixes #3148
New commands:
Positions are one-based UTF-16. Call hierarchy returns the resolved item plus incoming and outgoing calls. Results are deterministically ordered for agent consumption.
Test Plan
add test