[asukaminato0721]

Summary

Fixes #1332

stale on-disk source files can no longer leave textDocument/definition pointing at old locations or returning null.

the definition path now detects when the request document has drifted from disk, invalidates all known filesystem-backed modules, reruns the handle, and, if the first lookup is still empty, retries once with that source file refreshed from disk.

Test Plan

add test

[Copilot]

Pull request overview

This PR fixes an LSP textDocument/definition edge case where the language server’s view of a file can drift from the on-disk contents (e.g., after rebases/external edits), leading to missing or incorrect go-to-definition locations. The server now detects request-file drift from disk, invalidates filesystem-backed modules, re-runs analysis, and (when needed) retries definition with refreshed contents.

Changes:

• Add stale-on-disk detection and recovery logic in the non-WASM LSP server’s go-to-definition flow (including module invalidation + retry).
• Refactor go-to-definition logic to separate “compute response” from the higher-level retry/refresh behavior.
• Add a regression test covering recovery when both the requesting file and the target definition file moved on disk without LSP change notifications.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pyrefly/lib/lsp/non_wasm/server.rs	Detects stale on-disk contents during go-to-definition, invalidates filesystem modules, re-runs analysis, and retries with refreshed source when necessary.
pyrefly/lib/test/lsp/lsp_interaction/definition.rs	Adds a regression test ensuring go-to-definition recovers after on-disk edits without corresponding LSP updates.

---

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

[meta-codesync]

This pull request has been imported. If you are a Meta employee, you can view this in D120742808. (Because this pull request was imported automatically, there will not be any future comments.)

[codspeed]

Merging this PR will not alter performance

✅ 35 untouched benchmarks
⏩ 9 skipped benchmarks¹

---

_{Comparing asukaminato0721:1332 (bc0e5d6) with main (1af3067)}

Footnotes

1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[stroxler]

Thanks! Some issues that I think are worth a look based on AI review before we merge this:

1. Unsaved edits can produce definitions for the wrong symbol.
   [server.rs:5022–5038](https://github.com/meta/pyrefly/blob/9d78ec598ee6efc5461df900aaf6e73eef32eea5/pyrefly/lib/lsp/non_wasm/server.rs#L5022-L5038)
   A buffer/disk difference is normal during unsaved editing. If the user changes value = target to value = missing and correctly sends didChange, the initial definition lookup should fail. Instead, the retry replaces the transaction’s buffer with disk text and reuses the client position, returning target’s definition. Open-document contents must remain authoritative; an unsuccessful lookup is not evidence of missed synchronization.

2. Ordinary unsaved edits repeatedly reload every known filesystem module.
   [server.rs:5000–5009](https://github.com/meta/pyrefly/blob/9d78ec598ee6efc5461df900aaf6e73eef32eea5/pyrefly/lib/lsp/non_wasm/server.rs#L5000-L5009)
   Even appending an unsaved comment triggers global invalidation on every definition request. Passing only the requesting handle to run does not limit the work: run_step queues every dirty module. Equality cutoff avoids downstream recomputation, not disk reads. The buffer/disk mismatch persists, so this repeats until saving. Track actual disk changes independently of unsaved-buffer differences. The reload mechanism is confirmed; latency was not measured.

3. Changes confined to the destination file remain undetected.
   [server.rs:4999–5010](https://github.com/meta/pyrefly/blob/9d78ec598ee6efc5461df900aaf6e73eef32eea5/pyrefly/lib/lsp/non_wasm/server.rs#L4999-L5010)
   Cache a jump from unchanged foo.py into closed bar.py, then externally prepend a comment to bar.py without a watcher notification. Since only foo.py is checked for staleness, nothing is invalidated and the old destination location remains. This misses the core moved-definition case in #1332. Add a regression where only the destination changes, and check destination freshness.

4. A successful lookup can still resolve stale coordinates to the wrong symbol.
   [server.rs:5011–5015](https://github.com/meta/pyrefly/blob/9d78ec598ee6efc5461df900aaf6e73eef32eea5/pyrefly/lib/lsp/non_wasm/server.rs#L5011-L5015)
   In the missed-didChange scenario this PR supports, prepend a line above consecutive uses of first and second. The new position for first can land on second in the old buffer. That produces a nonempty response, so the early return skips recovery. The added test forces an empty first lookup and misses this wrong-but-valid result.

Suggested direction: preserve editor-buffer authority and make disk freshness a separate synchronization concern, refreshing and re-resolving destination modules rather than interpreting client positions against disk text.

[github-actions]

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

[stroxler]

Review automatically exported from Phabricator review in Meta.

[meta-codesync]

This pull request has been merged in 0ab1f74.