Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: major (reproduced by execution during review)
An imported file that exists but fails to read leaves no trace in FileAnalysis, so no later event ever invalidates the importing entry's stale analysis.
Repro sequence: main.inf has use lib;; lib.inf exists on disk but read_to_string fails (invalid UTF-8, or a permission/lock error). didOpen(main) → analysis computed: the walk enqueues lib (exists() true), read fails, WalkProblem::Io is recorded, no LoadedFile is pushed — and load_project_resilient silently discards Io problems (core/inference/src/project.rs:302: WalkProblem::Io { .. } | WalkProblem::InvalidSegment { .. } => {}). Result: closure_paths = {main} only, had_missing_import = false.
Now the user didOpens lib.inf with valid text and even didChanges it: invalidate(lib, true) matches nothing (lib not in main's closure; main has no missing import), so the stale analysis of main — missing all of lib's symbols — is served indefinitely until main itself is edited/closed. Confirmed by a runnable repro: generation stayed 1 across didOpen(lib) and didChange(lib); the arena kept 1 source file.
This falsifies the database.rs:33 claim that the widening rule is "simple and always correct", and is the non-entry twin of the entry-read-failure case the code explicitly hardens against (ide/ide-db/src/analysis.rs:116-124 handles only the entry).
Suggested fix: have load_project_resilient surface Io problems (the failed path is already in WalkProblem::Io), and in FileAnalysis::compute add read-failed paths to closure_paths and/or fold them into had_missing_import so a didOpen/didChange of the failed file invalidates the analysis. Add a test mirroring analysis_of_an_unreadable_entry_recovers_after_didopen for a non-entry closure file.
Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: major (reproduced by execution during review)
An imported file that exists but fails to read leaves no trace in
FileAnalysis, so no later event ever invalidates the importing entry's stale analysis.Repro sequence:
main.infhasuse lib;;lib.infexists on disk butread_to_stringfails (invalid UTF-8, or a permission/lock error). didOpen(main) → analysis computed: the walk enqueues lib (exists()true), read fails,WalkProblem::Iois recorded, noLoadedFileis pushed — andload_project_resilientsilently discards Io problems (core/inference/src/project.rs:302:WalkProblem::Io { .. } | WalkProblem::InvalidSegment { .. } => {}). Result:closure_paths = {main}only,had_missing_import = false.Now the user didOpens lib.inf with valid text and even didChanges it:
invalidate(lib, true)matches nothing (lib not in main's closure; main has no missing import), so the stale analysis of main — missing all of lib's symbols — is served indefinitely until main itself is edited/closed. Confirmed by a runnable repro: generation stayed 1 across didOpen(lib) and didChange(lib); the arena kept 1 source file.This falsifies the
database.rs:33claim that the widening rule is "simple and always correct", and is the non-entry twin of the entry-read-failure case the code explicitly hardens against (ide/ide-db/src/analysis.rs:116-124handles only the entry).Suggested fix: have
load_project_resilientsurface Io problems (the failed path is already inWalkProblem::Io), and inFileAnalysis::computeadd read-failed paths toclosure_pathsand/or fold them intohad_missing_importso a didOpen/didChange of the failed file invalidates the analysis. Add a test mirroringanalysis_of_an_unreadable_entry_recovers_after_didopenfor a non-entry closure file.