diff --git a/CHANGELOG.md b/CHANGELOG.md index f7615554e0..c31fbe9d27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ #### :bug: Bug fix - Fix partial application generalization for `...`. https://github.com/rescript-lang/rescript/pull/8343 +- Rewatch: preserve warnings after atomic-save full rebuilds. https://github.com/rescript-lang/rescript/pull/8358 #### :memo: Documentation diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index 8a37cdaf61..3eba1f1d0b 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -140,6 +140,44 @@ fn unregister_watches(watcher: &mut RecommendedWatcher, watch_paths: &[(PathBuf, } } +fn carry_forward_compile_warnings(previous: &BuildCommandState, next: &mut BuildCommandState) { + for (module_name, next_module) in next.build_state.modules.iter_mut() { + let Some(previous_module) = previous.build_state.modules.get(module_name) else { + continue; + }; + if previous_module.package_name != next_module.package_name { + continue; + } + + match (&previous_module.source_type, &mut next_module.source_type) { + (SourceType::SourceFile(previous_source), SourceType::SourceFile(next_source)) => { + if previous_source.implementation.path == next_source.implementation.path { + next_source.implementation.compile_warnings = + previous_source.implementation.compile_warnings.clone(); + + if next_source.implementation.compile_warnings.is_some() { + next_source.implementation.compile_state = + previous_source.implementation.compile_state.clone(); + } + } + + if let (Some(previous_interface), Some(next_interface)) = + (&previous_source.interface, &mut next_source.interface) + && previous_interface.path == next_interface.path + { + next_interface.compile_warnings = previous_interface.compile_warnings.clone(); + + if next_interface.compile_warnings.is_some() { + next_interface.compile_state = previous_interface.compile_state.clone(); + } + } + } + (SourceType::MlMap(_), SourceType::MlMap(_)) => (), + _ => (), + } + } +} + struct AsyncWatchArgs<'a> { watcher: &'a mut RecommendedWatcher, current_watch_paths: Vec<(PathBuf, RecursiveMode)>, @@ -374,7 +412,7 @@ async fn async_watch( } CompileType::Full => { let timing_total = Instant::now(); - build_state = build::initialize_build( + let mut next_build_state = build::initialize_build( None, filter, show_progress, @@ -384,6 +422,12 @@ async fn async_watch( ) .expect("Could not initialize build"); + // Full rebuilds can be triggered by editor atomic saves that surface as rename events. + // Preserve warning state for unchanged modules so their warnings are re-emitted after the + // fresh build state replaces the previous one. + carry_forward_compile_warnings(&build_state, &mut next_build_state); + build_state = next_build_state; + // Re-register watches based on the new build state unregister_watches(watcher, ¤t_watch_paths); current_watch_paths = compute_watch_paths(&build_state, path); @@ -475,3 +519,197 @@ pub fn start( .await }) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::build::build_types::{ + CompileState, CompilerInfo, Implementation, Interface, Module, ParseState, SourceFile, SourceType, + }; + use crate::build::packages::{Namespace, Package}; + use crate::config; + use crate::project_context::ProjectContext; + use ahash::{AHashMap, AHashSet}; + use std::path::PathBuf; + use std::sync::RwLock; + use std::time::SystemTime; + + fn test_project_context(root: &str) -> ProjectContext { + let root_path = PathBuf::from(root); + let config = config::tests::create_config(config::tests::CreateConfigArgs { + name: "test-root".to_string(), + bs_deps: vec![], + build_dev_deps: vec![], + allowed_dependents: None, + path: root_path.clone(), + }); + + ProjectContext { + current_config: config, + monorepo_context: None, + node_modules_exist_cache: RwLock::new(AHashMap::new()), + packages_cache: RwLock::new(AHashMap::new()), + } + } + + fn test_package(name: &str, path: &str) -> Package { + let package_path = PathBuf::from(path); + Package { + name: name.to_string(), + config: config::tests::create_config(config::tests::CreateConfigArgs { + name: name.to_string(), + bs_deps: vec![], + build_dev_deps: vec![], + allowed_dependents: None, + path: package_path.clone(), + }), + source_folders: AHashSet::new(), + source_files: None, + namespace: Namespace::NoNamespace, + modules: None, + path: package_path, + dirs: None, + is_local_dep: true, + is_root: true, + } + } + + fn test_build_state(module_name: &str, module: Module) -> BuildCommandState { + let root = "/tmp/rewatch-warning-carry-forward"; + let package = test_package("test-package", root); + let mut packages = AHashMap::new(); + packages.insert(package.name.clone(), package); + + let compiler = CompilerInfo { + bsc_path: PathBuf::from("/tmp/bsc"), + bsc_hash: blake3::hash(b"test-bsc"), + runtime_path: PathBuf::from("/tmp/runtime"), + }; + + let mut build_state = BuildCommandState::new( + PathBuf::from(root), + test_project_context(root), + packages, + compiler, + None, + ); + build_state.insert_module(module_name, module); + build_state + } + + fn test_module( + implementation_path: &str, + implementation_warning: Option<&str>, + interface_path: Option<&str>, + interface_warning: Option<&str>, + ) -> Module { + let implementation_compile_state = if implementation_warning.is_some() { + CompileState::Warning + } else { + CompileState::Success + }; + let interface_compile_state = if interface_warning.is_some() { + CompileState::Warning + } else { + CompileState::Success + }; + + Module { + source_type: SourceType::SourceFile(SourceFile { + implementation: Implementation { + path: PathBuf::from(implementation_path), + parse_state: ParseState::Success, + compile_state: implementation_compile_state, + last_modified: SystemTime::UNIX_EPOCH, + parse_dirty: false, + compile_warnings: implementation_warning.map(str::to_string), + }, + interface: interface_path.map(|interface_path| Interface { + path: PathBuf::from(interface_path), + parse_state: ParseState::Success, + compile_state: interface_compile_state, + last_modified: SystemTime::UNIX_EPOCH, + parse_dirty: false, + compile_warnings: interface_warning.map(str::to_string), + }), + }), + deps: AHashSet::new(), + dependents: AHashSet::new(), + package_name: "test-package".to_string(), + compile_dirty: false, + last_compiled_cmi: None, + last_compiled_cmt: None, + deps_dirty: false, + is_type_dev: false, + } + } + + #[test] + fn carries_forward_implementation_warnings_for_matching_module_paths() { + let previous = test_build_state( + "ModuleA", + test_module("src/ModuleA.res", Some("warning: impl"), None, None), + ); + let mut next = test_build_state("ModuleA", test_module("src/ModuleA.res", None, None, None)); + + carry_forward_compile_warnings(&previous, &mut next); + + let module = next.get_module("ModuleA").expect("module should exist"); + let SourceType::SourceFile(source_file) = &module.source_type else { + panic!("expected source file module"); + }; + + assert_eq!( + source_file.implementation.compile_warnings.as_deref(), + Some("warning: impl") + ); + assert_eq!(source_file.implementation.compile_state, CompileState::Warning); + } + + #[test] + fn does_not_carry_forward_warnings_when_module_paths_change() { + let previous = test_build_state( + "ModuleA", + test_module("src/ModuleA.res", Some("warning: impl"), None, None), + ); + let mut next = test_build_state("ModuleA", test_module("src/ModuleARenamed.res", None, None, None)); + + carry_forward_compile_warnings(&previous, &mut next); + + let module = next.get_module("ModuleA").expect("module should exist"); + let SourceType::SourceFile(source_file) = &module.source_type else { + panic!("expected source file module"); + }; + + assert_eq!(source_file.implementation.compile_warnings, None); + assert_eq!(source_file.implementation.compile_state, CompileState::Success); + } + + #[test] + fn carries_forward_interface_warnings_for_matching_interface_paths() { + let previous = test_build_state( + "ModuleA", + test_module( + "src/ModuleA.res", + None, + Some("src/ModuleA.resi"), + Some("warning: interface"), + ), + ); + let mut next = test_build_state( + "ModuleA", + test_module("src/ModuleA.res", None, Some("src/ModuleA.resi"), None), + ); + + carry_forward_compile_warnings(&previous, &mut next); + + let module = next.get_module("ModuleA").expect("module should exist"); + let SourceType::SourceFile(source_file) = &module.source_type else { + panic!("expected source file module"); + }; + let interface = source_file.interface.as_ref().expect("interface should exist"); + + assert_eq!(interface.compile_warnings.as_deref(), Some("warning: interface")); + assert_eq!(interface.compile_state, CompileState::Warning); + } +} diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 14f9330b88..3f1313b6c6 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -86,6 +86,7 @@ fi # Watch tests ./watch/01-watch-recompile.sh && ./watch/02-watch-warnings-persist.sh && +./watch/02-watch-warnings-persist-atomic-save.sh && ./watch/03-watch-new-file.sh && ./watch/04-watch-config-change.sh && ./watch/05-watch-ignores-non-source.sh && diff --git a/rewatch/tests/watch/02-watch-warnings-persist-atomic-save.sh b/rewatch/tests/watch/02-watch-warnings-persist-atomic-save.sh new file mode 100755 index 0000000000..63adb0fd2b --- /dev/null +++ b/rewatch/tests/watch/02-watch-warnings-persist-atomic-save.sh @@ -0,0 +1,129 @@ +#!/bin/bash +cd $(dirname $0) +source "../utils.sh" +cd ../../testrepo + +bold "Test: Warnings persist after atomic-save rename of unrelated module" + +error_output=$(rewatch clean 2>&1) +if [ $? -eq 0 ]; +then + success "Repo Cleaned" +else + error "Error Cleaning Repo" + printf "%s\n" "$error_output" >&2 + exit 1 +fi + +wait_for_pattern() { + local file="$1"; local pattern="$2"; local timeout="${3:-30}" + while [ "$timeout" -gt 0 ]; do + grep -q "$pattern" "$file" 2>/dev/null && return 0 + sleep 1 + timeout=$((timeout - 1)) + done + return 1 +} + +wait_for_changed_completed_log() { + local file="$1"; local baseline="$2"; local timeout="${3:-30}" + while [ "$timeout" -gt 0 ]; do + if [ -f "$file" ] && ! cmp -s "$file" "$baseline" && grep -q "#Done(" "$file" 2>/dev/null; then + return 0 + fi + sleep 1 + timeout=$((timeout - 1)) + done + return 1 +} + +COMPILER_LOG="./packages/watch-warnings/lib/bs/.compiler.log" +B_FILE="./packages/watch-warnings/src/B.res" +WATCH_STDOUT="rewatch-stdout.log" +WATCH_STDERR="rewatch-stderr.log" +TMP_DIR="$(mktemp -d "${TMPDIR:-/tmp}/rewatch-watch-warning-atomic-save.XXXXXX")" +INITIAL_LOG="$TMP_DIR/initial.compiler.log" +BACKUP_B="$TMP_DIR/B.res.bak" + +cp "$B_FILE" "$BACKUP_B" + +cleanup() { + set +e + if [ -f "$BACKUP_B" ]; then + cp "$BACKUP_B" "$B_FILE" + fi + exit_watcher + sleep 1 + rm -rf "$TMP_DIR" + rm -f "$WATCH_STDOUT" "$WATCH_STDERR" +} +trap cleanup EXIT + +rewatch_bg watch > "$WATCH_STDOUT" 2> "$WATCH_STDERR" & + +if ! wait_for_pattern "$COMPILER_LOG" "unused value unusedValue" 30 || ! wait_for_pattern "$COMPILER_LOG" "#Done(" 30; then + error "Initial build did not finish with warning from ModuleA.res in $COMPILER_LOG" + echo + echo "=== compiler log ===" + cat "$COMPILER_LOG" 2>/dev/null || true + echo + echo "=== watch stderr ===" + cat "$WATCH_STDERR" 2>/dev/null || true + echo + echo "=== watch stdout ===" + cat "$WATCH_STDOUT" 2>/dev/null || true + exit 1 +fi +success "Initial build shows warning from ModuleA.res in $COMPILER_LOG" + +cp "$COMPILER_LOG" "$INITIAL_LOG" + +# Simulate an editor atomic save by writing a temp file and renaming it into place. +tmp_b_save="$(mktemp "${TMPDIR:-/tmp}/B.res.XXXXXX")" +printf 'let world = () => Console.log("world")\n// trigger atomic save\n' > "$tmp_b_save" +mv "$tmp_b_save" "$B_FILE" + +if ! wait_for_changed_completed_log "$COMPILER_LOG" "$INITIAL_LOG" 30; then + error "Compiler log did not complete a new cycle after atomic save of B.res" + echo + echo "=== compiler log ===" + cat "$COMPILER_LOG" 2>/dev/null || true + echo + echo "=== watch stderr ===" + cat "$WATCH_STDERR" 2>/dev/null || true + echo + echo "=== watch stdout ===" + cat "$WATCH_STDOUT" 2>/dev/null || true + exit 1 +fi + +if grep -q "unused value unusedValue" "$COMPILER_LOG"; then + success "Warning from ModuleA.res persists after atomic save of B.res" +else + error "Warning from ModuleA.res was lost after atomic save of B.res" + echo + echo "=== compiler log ===" + cat "$COMPILER_LOG" + echo + echo "=== watch stderr ===" + cat "$WATCH_STDERR" 2>/dev/null || true + echo + echo "=== watch stdout ===" + cat "$WATCH_STDOUT" 2>/dev/null || true + exit 1 +fi + +cp "$BACKUP_B" "$B_FILE" +sleep 1 +exit_watcher +sleep 2 +rm -f "$WATCH_STDOUT" "$WATCH_STDERR" + +if git diff --exit-code ./packages/watch-warnings > /dev/null 2>&1; +then + success "No leftover changes in watch-warnings package" +else + error "Leftover changes detected in watch-warnings package" + git diff ./packages/watch-warnings + exit 1 +fi