Skip to content

fix Reference Counting #2194 - #2907

Closed
asukaminato0721 wants to merge 4 commits into
meta:mainfrom
asukaminato0721:2194
Closed

asukaminato0721 wants to merge 4 commits into
meta:mainfrom
asukaminato0721:2194

Conversation

@asukaminato0721

@asukaminato0721 asukaminato0721 commented Mar 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes #2194

The server now advertises textDocument/codeLens when indexing is enabled and returns CodeLens entries for class/function/method definitions using the existing global references engine.

Each lens shows N reference(s) and wires to VS Code’s built-in editor.action.showReferences, so clicking the count opens the references UI.

Test Plan

add LSP interaction test

@meta-cla meta-cla Bot added the cla signed label Mar 26, 2026
@asukaminato0721
asukaminato0721 force-pushed the 2194 branch 2 times, most recently from 011421e to c711129 Compare March 26, 2026 12:02
@asukaminato0721
asukaminato0721 marked this pull request as ready for review March 26, 2026 12:04
Copilot AI review requested due to automatic review settings March 26, 2026 12:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds CodeLens-based reference counts for Python symbols in the Pyrefly LSP when indexing is enabled, backed by the existing global references engine, and introduces an LSP interaction test to validate the behavior.

Changes:

  • Advertise textDocument/codeLens capability when indexing is enabled and handle CodeLensRequest on the server.
  • Generate CodeLens entries for class/function/method definitions with editor.action.showReferences wired up.
  • Add LSP interaction tests and test fixtures validating CodeLens reference counts.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pyrefly/lib/lsp/non_wasm/server.rs Advertises CodeLens capability and implements textDocument/codeLens request handling to return reference-count lenses.
pyrefly/lib/lsp/non_wasm/workspace.rs Adds code_lens flag to disabled-language-services gating for textDocument/codeLens.
pyrefly/lib/test/lsp/lsp_interaction/object_model.rs Adds a test client helper to send CodeLensRequest.
pyrefly/lib/test/lsp/lsp_interaction/mod.rs Registers new code_lens LSP interaction test module.
pyrefly/lib/test/lsp/lsp_interaction/code_lens.rs Adds tests verifying capability advertisement and reference-count CodeLens results.
pyrefly/lib/test/lsp/lsp_interaction/test_files/code_lens_references/{symbols.py,usage.py} Adds small fixtures used by the new CodeLens tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pyrefly/lib/lsp/non_wasm/server.rs Resolved
Comment thread pyrefly/lib/lsp/non_wasm/server.rs Resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@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. ✅

@stroxler

Copy link
Copy Markdown
Contributor

I'm closing this PR, I think it's too old to review (for example, already attached to a stale diff).

If you decide to open a fresh PR, which I think we will be better at turning around, some suggestions from AI review:

  1. Clicking a lens fails in VS Code (reference_count.rs:88). The server sends plain JSON as the arguments to editor.action.showReferences: a URI string, a position, and a list of locations. The language client passes command arguments through unchanged, and the extension has no wrapper, while VS Code's handler checks assertType(URI.isUri(resource)), so the click throws. The tests only check the JSON shape, so they can't catch this. Fix: add a pyrefly.showReferences command in lsp/src that converts the arguments into real VS Code objects before calling the built-in command (rust-analyzer does the same).

  2. Other editors get lenses that can't be clicked. That command exists only in VS Code; Neovim, Helix and Zed would send workspace/executeCommand, which the server doesn't handle. Fix: only emit reference lenses when the client says it supports them, e.g. via an initialization option the extension sends.

  3. On by default, with no separate opt-out and no docs. The only gate is indexing being on, and background indexing is the default (commands/lsp.rs:44). Every user gets a lens above every class, function and method, including nested functions and each @overload. Turning off codeLens also removes Run/Test. Fix: add a referencesCodeLens setting that defaults to off, modeled on runnableCodeLens, and document it in IDE.mdx.

  4. Resolve is expensive, and cancelling doesn't stop it. Each resolve runs a full global references search on find_reference_queue, which is a single serial thread. Cancellation is only registered once a task starts (server.rs:5573), and queued tasks never check canceled_requests. So batches VS Code cancels while you scroll or type still run, and real Find References and rename requests wait behind them. Fix: check for cancellation before a queued task runs; consider a lower-priority queue or one search per document.

  5. Edits can make a lens count the wrong symbol. Adding CodeLensResolveRequest to ONLY_ONCE means resolves aren't cancelled when the file changes. The lens's stored data is just the URI, so an old position gets looked up in new text. Fix: drop it from ONLY_ONCE, or store the document version in the lens data.

  6. The count can differ from Find References. Resolve re-implements the search without external (Glean) references. Fix: reuse async_find_references_helper and filter out notebook locations in its map_result, which also removes the duplicated code.

  7. Smaller behavior gaps:

    • If the definition isn't found, the lens comes back with no command and VS Code shows a blank line.
    • Counts don't refresh when other files change or indexing finishes; a workspace/codeLens/refresh would fix that.
    • Resolves are logged as FindFromDefinition, which inflates the Find References metrics.

Nits (we should still address these, code quality is important!)

  • Layout. The Run/Test detection is pure AST code but moved under server/ and now reaches into Server internals. Keep the detection standalone and only the glue under server/. Also replace use super::super::Server and the inline std::path::Path with imports.
  • Error handling. resolve_data returns Result<_, String>, and resolve_code_lens sends an error response itself and then returns Ok(()). Return a typed error and let the dispatcher send the response.
  • Unreachable code. The notebook-cell filter in runnable.rs can no longer trigger because notebooks now return early; it should be removed per the AGENTS.md rule.
  • Cleanup. The rename_all on the single-field struct does nothing, and moving the CodeLensRequest dispatch block adds diff noise.
  • Tests.
    • Missing cases: definition not found, bad resolve data, indexing off, zero and one reference.
    • The notebook test should also show that plain textDocument/references does return the notebook cells, so it can't pass without the filter.
  • PR summary is out of date. It still describes eager lenses gated on indexing.

Just noting: some things are good in the existing work:

  • Every request path sends exactly one response.
  • With indexing off, the only extra cost is advertising the resolve capability.
  • Notebook sources are rejected and notebook references are filtered out, which addresses kinto0's earlier request.
  • The expected counts in the tests (3/2/2, declaration excluded) are right, and the paths in them work on any OS.

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.

Reference Counting

5 participants