Skip to content

fix Command-line entry-point for LSP features #3148 - #4183

Open
asukaminato0721 wants to merge 1 commit into
meta:mainfrom
asukaminato0721:3148
Open

asukaminato0721 wants to merge 1 commit into
meta:mainfrom
asukaminato0721:3148

Conversation

@asukaminato0721

Copy link
Copy Markdown
Collaborator

Summary

Fixes #3148

New commands:

pyrefly query hover FILE LINE COLUMN
pyrefly query find-references --include-declaration FILE LINE COLUMN
pyrefly query call-hierarchy FILE LINE COLUMN

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

@github-actions

This comment has been minimized.

@meta-codesync

meta-codesync Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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.)

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@rchen152

rchen152 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

@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?

@rchen152 rchen152 assigned kinto0 and unassigned connernilsen Nov 2, 2026
@rchen152
rchen152 requested a review from kinto0 November 2, 2026 01:41

@stroxler stroxler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

query.rs:150–155

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

query.rs:223–225 and 264–265

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

query.rs:155–191

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_cell and format_file_contents, and callees including _format_str_once and 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.”

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Command-line entry-point for LSP features

5 participants