Conversation
`on_change` passes the editor's live buffer to solc — the comment above the call says so explicitly — but then converted the byte offsets solc returned into LSP positions using `tokio::fs::read_to_string(&file_path)`, a separate read of the file on disk. solc reports offsets into the text it was given, so resolving them against different text puts every diagnostic in the file at the wrong position. The obvious objection is that at didSave the editor has just flushed the buffer, so disk should match. Three things break that: - A client may omit the optional `text` on didSave. `run_did_save` (src/lsp.rs:1721) then falls back to the text_cache — the live buffer, unsaved edits included — while disk still holds the last-saved content. - Saves are collapsed. `did_save` (src/lsp.rs:3540) routes each URI through a watch channel and the worker picks up the *latest* params via `borrow_and_update` (src/lsp.rs:3581), so a save can be processed after further edits have landed. - The scaffold-recovery path compiles generated content that was delivered by applyEdit and never written, so disk is empty and `byte_offset_to_position` collapses every diagnostic onto line 0. Both call sites now pass `¶ms.text`, the same text handed to solc. Imports are unaffected and deliberately still resolved against disk: they reach solc as `"urls"` rather than inlined content, so solc reads them from disk itself. Adds two protocol-level regression tests, covering both ways the mapping goes wrong. With the buffer longer than disk the bad offset runs past end-of-file and clamps — main reports (10,0)-(10,0) instead of (12,16)-(12,30). With the buffer shorter, the offset still lands inside the on-disk text and yields a wrong-but-plausible line — main reports (6,5)-(7,0) instead of (7,16)-(7,30), which is the variant that survives a bug report. Both assert the full range, and both fail on main and pass here. Like tests/build.rs the tests need the Foundry toolchain on PATH, which CI already installs. They wait for two publishDiagnostics payloads before dirtying the buffer, because `on_change` emits an empty one up front to clear stale squiggles before solc runs; waiting for one would race the build. Lint is left enabled so the exercised branch is the one a real Foundry project takes. The test harness is duplicated from tests/lsp_file_ops_settings.rs. Happy to extract a shared tests/common/mod.rs in a follow-up if preferred — it is left out here to keep this diff to the fix.
rifuki
marked this pull request as draft
August 25, 2026 10:01
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
1 participant
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.
Closes #235.
Both call sites in
on_changenow pass¶ms.text— the same text handed to solc — instead of re-reading the file from disk to map offsets.I traced the three callers (
did_open,run_did_save,did_create_files) and in each one theStringpassed asparams.textis the onesolc_ast()inlines as the source unit's"content", so the two can't disagree. Imports are left on disk deliberately: they reach solc as"urls", so solc reads them itself andcross_file_error_diagnosticsis already right.Two regression tests, one per failure mode. With the buffer longer than disk the offset runs past EOF and clamps —
mainreports(10,0)-(10,0)where the error is at(12,16)-(12,30). With the buffer shorter it lands inside the disk text and gives a plausible wrong line —mainreports(6,5)-(7,0)instead of(7,16)-(7,30). Both assert the full range and both fail onmain.They wait for two
publishDiagnosticsbefore dirtying the buffer, sinceon_changesends an empty one up front to clear stale squiggles before solc runs — waiting for one races the build. Lint is left on so the branch exercised is the one a real Foundry project takes. Liketests/build.rsthey need forge on PATH, which CI installs.cargo test666 passed, clippy and fmt clean.The test harness is copied from
tests/lsp_file_ops_settings.rs. I can pull it out into a sharedtests/common/mod.rsif you'd rather, but that touches another test file so I left it out of this one.