Skip to content

Commit 0a81f6b

Browse files
committed
fix: preserve ParseError stage in mark_modules_with_expired_deps
mark_modules_with_expired_deps_for_recompile was downgrading ParseError modules to SourceDirty, allowing them to enter the compile universe with a stale AST from a previous successful build. When bsc compiled the module using this stale AST it "succeeded", then the post-loop code attempted the invalid transition SourceDirty → TypeChecked, which panicked via debug_assert in set_compilation_stage. Inside spawn_blocking, this panic prevented BuildCommandState from being re-inserted into ProjectMap.states, permanently losing the project. The fix adds a ParseError guard alongside the existing CompileError and Parsed guards. Also adds a Skipped variant to CompileFileOutcome so modules not sent to bsc are never added to successfully_compiled.
1 parent 33ae364 commit 0a81f6b

5 files changed

Lines changed: 169 additions & 13 deletions

File tree

rewatch/src/build/build_types.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,12 @@ impl SourceFileModule {
525525
///
526526
/// Panics in debug builds if the transition is invalid (see
527527
/// [`CompilationStage::can_transition_to`]).
528+
///
529+
/// **WARNING**: In the LSP, `file_build` uses a take-remove-reinsert
530+
/// pattern for `BuildCommandState`. If this assertion fires inside
531+
/// `spawn_blocking`, the panic prevents the state from being
532+
/// re-inserted into `ProjectMap.states`, permanently losing the
533+
/// project and breaking all subsequent builds for that project.
528534
pub fn set_compilation_stage(&mut self, new_stage: CompilationStage) {
529535
debug_assert!(
530536
self.compilation_stage.can_transition_to(&new_stage),

rewatch/src/build/compile.rs

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,13 @@ enum CompileFileOutcome {
3030
Warning(String),
3131
/// Compilation failed.
3232
Error(String),
33+
/// Module was not sent to the compiler (already at target stage with clean deps).
34+
Skipped,
3335
}
3436

3537
impl CompileFileOutcome {
36-
fn is_error(&self) -> bool {
37-
matches!(self, CompileFileOutcome::Error(_))
38+
fn is_success(&self) -> bool {
39+
matches!(self, CompileFileOutcome::Success | CompileFileOutcome::Warning(_))
3840
}
3941
}
4042

@@ -237,8 +239,8 @@ pub fn process_in_waves(
237239
// dependencies had unchanged output (.cmi). Skip it.
238240
return Some(CompileModuleResult {
239241
module_name: module_name.to_string(),
240-
implementation: CompileFileOutcome::Success,
241-
interface: Some(CompileFileOutcome::Success),
242+
implementation: CompileFileOutcome::Skipped,
243+
interface: Some(CompileFileOutcome::Skipped),
242244
is_clean_cmi: true,
243245
was_compiled: false,
244246
});
@@ -401,19 +403,20 @@ pub fn process_in_waves(
401403
}
402404
Module::SourceFile(_) => match &entry.implementation {
403405
CompileFileOutcome::Warning(warning) => (Some(warning.clone()), None),
404-
CompileFileOutcome::Success => (None, None),
406+
CompileFileOutcome::Success | CompileFileOutcome::Skipped => (None, None),
405407
CompileFileOutcome::Error(error) => (None, Some(error.clone())),
406408
},
407409
};
408410

409411
let (interface_warning, interface_error) = match &entry.interface {
410412
Some(CompileFileOutcome::Warning(warning)) => (Some(warning.clone()), None),
411-
Some(CompileFileOutcome::Success) => (None, None),
413+
Some(CompileFileOutcome::Success) | Some(CompileFileOutcome::Skipped) => (None, None),
412414
Some(CompileFileOutcome::Error(error)) => (None, Some(error.clone())),
413415
None => (None, None),
414416
};
415417

416-
if !entry.implementation.is_error() && entry.interface.as_ref().is_none_or(|r| !r.is_error())
418+
if entry.implementation.is_success()
419+
&& entry.interface.as_ref().is_none_or(|r| r.is_success())
417420
{
418421
successfully_compiled.insert(entry.module_name.clone());
419422
} else if entry.was_compiled {
@@ -1321,13 +1324,16 @@ pub fn mark_modules_with_expired_deps_for_recompile(build_state: &mut BuildComma
13211324
build_state.modules.iter_mut().for_each(|(module_name, module)| {
13221325
if let Module::SourceFile(sf) = module
13231326
&& modules_with_expired_deps.contains(module_name)
1324-
// Preserve CompileError and Parsed — both already signal "needs
1325-
// compilation". CompileError carries semantic meaning for the LSP
1326-
// flow (step 3 in file_build::build_batch uses it to detect
1327-
// modules that need full compilation after a dependency fix).
1328-
// Parsed modules were just parsed in this cycle and resetting them
1329-
// loses their source/AST hashes.
1327+
// Preserve CompileError, ParseError, and Parsed — all already
1328+
// signal "needs work". CompileError carries semantic meaning for
1329+
// the LSP flow (step 3 in file_build::build_batch uses it to
1330+
// detect modules that need full compilation after a dependency
1331+
// fix). Parsed modules were just parsed in this cycle and
1332+
// resetting them loses their source/AST hashes. ParseError
1333+
// modules need reparsing, not recompilation — downgrading them
1334+
// to SourceDirty would let them be compiled with a stale AST.
13301335
&& !sf.compilation_stage().is_compile_error()
1336+
&& !sf.compilation_stage().is_parse_error()
13311337
&& !matches!(sf.compilation_stage(), CompilationStage::Parsed { .. })
13321338
{
13331339
// Extract parse hashes from the current stage when available.

rewatch/src/lsp/queue/file_build.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ pub(super) async fn run(
139139
guard.uri_cache.retain(|_, r| *r != root);
140140
guard.states.insert(root, state);
141141
}
142+
} else {
143+
tracing::error!("projects mutex poisoned in build flush phase 3");
142144
}
143145

144146
(results, remaining_intent)
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
2+
3+
exports[`lsp parse error + new file creation > preserves ParseError stage when creating a file triggers mark_modules_with_expired_deps 1`] = `
4+
[
5+
"rewatch.lsp",
6+
" lsp.initialized",
7+
" lsp.register_watchers[watcher_count=3]",
8+
" lsp.discover_package[name=@rewatch-test/dep-chain]",
9+
" lsp.source_dir[dir=packages/dep-chain/src, recursive=true]",
10+
" lsp.initial_build[project=@rewatch-test/dep-chain]",
11+
" build.load_package_sources[package=@rewatch-test/dep-chain]",
12+
" packages.parse_packages",
13+
" clean.cleanup_previous_build",
14+
" build.parse[dirty_modules=4]",
15+
" build.parse_file[module=App, package=@rewatch-test/dep-chain]",
16+
" build.parse_file[module=Button, package=@rewatch-test/dep-chain]",
17+
" build.parse_file[module=Leaf, package=@rewatch-test/dep-chain]",
18+
" build.parse_file[module=Main, package=@rewatch-test/dep-chain]",
19+
" full_typecheck[module_count=4, output=lsp]",
20+
" build.typecheck",
21+
" build.typecheck_wave[file_count=2]",
22+
" build.typecheck_file[module=Button, package=@rewatch-test/dep-chain]",
23+
" build.typecheck_file[module=Leaf, package=@rewatch-test/dep-chain]",
24+
" build.typecheck_wave[file_count=1]",
25+
" build.typecheck_file[module=App, package=@rewatch-test/dep-chain]",
26+
" build.typecheck_wave[file_count=1]",
27+
" build.typecheck_file[module=Main, package=@rewatch-test/dep-chain]",
28+
" lsp.did_save[file=packages/dep-chain/src/Leaf.res]",
29+
" lsp.flush[incremental_builds=packages/dep-chain/src/Leaf.res]",
30+
" lsp.flush.file_build.batch[modules=["Leaf"], error_count=1]",
31+
" lsp.flush.file_build.compile_dependencies",
32+
" build.parse[dirty_modules=1]",
33+
" build.parse_file[module=Leaf, package=@rewatch-test/dep-chain]",
34+
" build.parse_error",
35+
" lsp.flush.file_build.typecheck_dependents[dependent_count=2]",
36+
" typecheck_dependents[module_count=4, output=lsp]",
37+
" build.typecheck",
38+
" build.typecheck_wave[file_count=1]",
39+
" build.typecheck_file[module=App, package=@rewatch-test/dep-chain]",
40+
" build.typecheck_wave[file_count=1]",
41+
" build.typecheck_file[module=Main, package=@rewatch-test/dep-chain]",
42+
" lsp.flush.file_build.compile_resolved",
43+
" lsp.did_change_watched_files[file_count=1]",
44+
" lsp.flush[project_builds=created: packages/dep-chain/src/NewModule.res, incremental_builds=packages/dep-chain/src/NewModule.res]",
45+
" lsp.flush.project_build[project=@rewatch-test/dep-chain]",
46+
" build.load_package_sources[package=@rewatch-test/dep-chain]",
47+
" packages.parse_packages",
48+
" clean.cleanup_previous_build",
49+
" build.parse[dirty_modules=2]",
50+
" build.parse_file[module=Leaf, package=@rewatch-test/dep-chain]",
51+
" build.parse_file[module=NewModule, package=@rewatch-test/dep-chain]",
52+
" build.parse_error",
53+
" lsp.flush.file_build.batch[modules=["NewModule"]]",
54+
" lsp.flush.file_build.compile_dependencies",
55+
" build.parse[dirty_modules=1]",
56+
" build.parse_file[module=NewModule, package=@rewatch-test/dep-chain]",
57+
" compile_dependencies[module_count=5, output=lsp]",
58+
" build.compile",
59+
" build.compile_wave[file_count=1]",
60+
" build.compile_wave[file_count=1]",
61+
" build.compile_file[module=NewModule, package=@rewatch-test/dep-chain, suffix=.mjs, module_system=esmodule]",
62+
" build.compile_error",
63+
" lsp.flush.file_build.typecheck_dependents[dependent_count=0]",
64+
" typecheck_dependents[module_count=5, output=lsp]",
65+
" build.typecheck",
66+
" build.typecheck_wave[file_count=0]",
67+
" lsp.flush.file_build.compile_resolved",
68+
" lsp.did_save[file=packages/dep-chain/src/Leaf.res]",
69+
" lsp.flush[incremental_builds=packages/dep-chain/src/Leaf.res]",
70+
" lsp.flush.file_build.batch[modules=["Leaf"]]",
71+
" lsp.flush.file_build.compile_dependencies",
72+
" build.parse[dirty_modules=1]",
73+
" build.parse_file[module=Leaf, package=@rewatch-test/dep-chain]",
74+
" compile_dependencies[module_count=5, output=lsp]",
75+
" build.compile",
76+
" build.compile_wave[file_count=1]",
77+
" build.compile_file[module=Leaf, package=@rewatch-test/dep-chain, suffix=.mjs, module_system=esmodule]",
78+
" lsp.flush.file_build.typecheck_dependents[dependent_count=3]",
79+
" typecheck_dependents[module_count=5, output=lsp]",
80+
" build.typecheck",
81+
" build.typecheck_wave[file_count=2]",
82+
" build.typecheck_file[module=App, package=@rewatch-test/dep-chain]",
83+
" build.typecheck_file[module=NewModule, package=@rewatch-test/dep-chain]",
84+
" build.typecheck_wave[file_count=1]",
85+
" build.typecheck_file[module=Main, package=@rewatch-test/dep-chain]",
86+
" lsp.flush.file_build.compile_resolved[modules=["NewModule"]]",
87+
" compile_dependencies[module_count=5, output=lsp]",
88+
" build.compile",
89+
" build.compile_wave[file_count=1]",
90+
" build.compile_wave[file_count=1]",
91+
" build.compile_file[module=NewModule, package=@rewatch-test/dep-chain, suffix=.mjs, module_system=esmodule]",
92+
]
93+
`;
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import { writeFileSync } from "node:fs";
2+
import path from "node:path";
3+
import { pathToFileURL } from "node:url";
4+
import { describe, expect, it } from "vitest";
5+
import { runLspTest } from "../../helpers/test-context.mjs";
6+
7+
describe("lsp parse error + new file creation", { timeout: 60_000 }, () => {
8+
it("preserves ParseError stage when creating a file triggers mark_modules_with_expired_deps", () =>
9+
runLspTest(
10+
async ({ lsp, lspCwd, writeFile }) => {
11+
const rootUri = pathToFileURL(lspCwd).href;
12+
await lsp.initialize(rootUri);
13+
await lsp.waitForNotification("rescript/buildFinished", 30000);
14+
15+
// Step 1: Break Leaf.res with a syntax error (parse error, not type error).
16+
await writeFile("src/Leaf.res", "let value = {\n");
17+
await lsp.waitForNotification("rescript/buildFinished", 30000);
18+
19+
// Leaf.res should have parse error diagnostics
20+
let diagnostics = lsp.getDiagnostics();
21+
const leafDiag = diagnostics.find(d => d.file === "src/Leaf.res");
22+
expect(leafDiag, "Expected diagnostics for Leaf.res").toBeDefined();
23+
expect(leafDiag.diagnostics.length).toBeGreaterThan(0);
24+
25+
// Step 2: Create a new file that depends on Leaf (which has a parse error).
26+
const newFilePath = path.join(lspCwd, "src", "NewModule.res");
27+
writeFileSync(newFilePath, 'let greeting = "hello " ++ Leaf.value\n');
28+
lsp.notifyWatchedFilesChanged([
29+
{ relativePath: "src/NewModule.res", type: 1 },
30+
]);
31+
await lsp.waitForNotification("rescript/buildFinished", 30000);
32+
33+
// Step 3: Fix Leaf.res
34+
await writeFile("src/Leaf.res", 'let value = "fixed"\n');
35+
await lsp.waitForNotification("rescript/buildFinished", 30000);
36+
37+
// Leaf.res diagnostics should be cleared
38+
diagnostics = lsp.getDiagnostics();
39+
const leafDiagAfter = diagnostics.find(d => d.file === "src/Leaf.res");
40+
if (leafDiagAfter) {
41+
expect(
42+
leafDiagAfter.diagnostics,
43+
`Expected no diagnostics for src/Leaf.res after fix, got: ${JSON.stringify(leafDiagAfter.diagnostics)}`,
44+
).toEqual([]);
45+
}
46+
},
47+
{ cwd: "packages/dep-chain" },
48+
));
49+
});

0 commit comments

Comments
 (0)