Add --warn-error to Rewatch (#7916)

* Add --warn-error to Rewatch

* Add Rewatch notes for agents.md

* Also pass warnings/errors to parse arguments

* Changelog

* Don't return warning args for non local deps

* fmt

* Clean up

* extract module_name_package_pairs

* Remove additional clone

Author: nojaf

AGENTS.md

@@ -254,4 +254,173 @@ The compiler is designed for fast feedback loops and scales to large codebases:
 - Remember Lambda IR is the core optimization layer
 - All `lam_*.ml` files process this representation
 - Use `lam_print.ml` for debugging lambda expressions
-- Test both with and without optimization passes
+- Test both with and without optimization passes
+
+## Working on the Build System
+
+### Rewatch Architecture
+
+Rewatch is the modern build system written in Rust that replaces the legacy bsb (BuckleScript) build system. It provides faster incremental builds, better error messages, and improved developer experience.
+
+#### Key Components
+
+```
+rewatch/src/
+├── build/              # Core build system logic
+│   ├── build_types.rs  # Core data structures (BuildState, Module, etc.)
+│   ├── compile.rs      # Compilation logic and bsc argument generation
+│   ├── parse.rs        # AST generation and parser argument handling
+│   ├── packages.rs     # Package discovery and dependency resolution
+│   ├── deps.rs         # Dependency analysis and module graph
+│   ├── clean.rs        # Build artifact cleanup
+│   └── logs.rs         # Build logging and error reporting
+├── cli.rs              # Command-line interface definitions
+├── config.rs           # rescript.json configuration parsing
+├── watcher.rs          # File watching and incremental builds
+└── main.rs             # Application entry point
+```
+
+#### Build System Flow
+
+1. **Initialization** (`build::initialize_build`)
+   - Parse `rescript.json` configuration
+   - Discover packages and dependencies
+   - Set up compiler information
+   - Create initial `BuildState`
+
+2. **AST Generation** (`build::parse`)
+   - Generate AST files using `bsc -bs-ast`
+   - Handle PPX transformations
+   - Process JSX
+
+3. **Dependency Analysis** (`build::deps`)
+   - Analyze module dependencies from AST files
+   - Build dependency graph
+   - Detect circular dependencies
+
+4. **Compilation** (`build::compile`)
+   - Generate `bsc` compiler arguments
+   - Compile modules in dependency order
+   - Handle warnings and errors
+   - Generate JavaScript output
+
+5. **Incremental Updates** (`watcher.rs`)
+   - Watch for file changes
+   - Determine dirty modules
+   - Recompile only affected modules
+
+### Development Guidelines
+
+#### Adding New Features
+
+1. **CLI Arguments**: Add to `cli.rs` in `BuildArgs` and `WatchArgs`
+2. **Configuration**: Extend `config.rs` for new `rescript.json` fields
+3. **Build Logic**: Modify appropriate `build/*.rs` modules
+4. **Thread Parameters**: Pass new parameters through the build system chain
+5. **Add Tests**: Include unit tests for new functionality
+
+#### Common Patterns
+
+- **Parameter Threading**: New CLI flags need to be passed through:
+  - `main.rs` → `build::build()` → `initialize_build()` → `BuildState`
+  - `main.rs` → `watcher::start()` → `async_watch()` → `initialize_build()`
+
+- **Configuration Precedence**: Command-line flags override `rescript.json` config
+- **Error Handling**: Use `anyhow::Result` for error propagation
+- **Logging**: Use `log::debug!` for development debugging
+
+#### Testing
+
+```bash
+# Run rewatch tests (from project root)
+cargo test --manifest-path rewatch/Cargo.toml
+
+# Test specific functionality
+cargo test --manifest-path rewatch/Cargo.toml config::tests::test_get_warning_args
+
+# Run clippy for code quality
+cargo clippy --manifest-path rewatch/Cargo.toml --all-targets --all-features
+
+# Check formatting
+cargo fmt --check --manifest-path rewatch/Cargo.toml
+
+# Build rewatch
+cargo build --manifest-path rewatch/Cargo.toml --release
+
+# Or use the Makefile shortcuts
+make rewatch          # Build rewatch
+make test-rewatch     # Run integration tests
+```
+
+**Note**: The rewatch project is located in the `rewatch/` directory with its own `Cargo.toml` file. All cargo commands should be run from the project root using the `--manifest-path rewatch/Cargo.toml` flag, as shown in the CI workflow.
+
+**Integration Tests**: The `make test-rewatch` command runs bash-based integration tests located in `rewatch/tests/suite-ci.sh`. These tests use the `rewatch/testrepo/` directory as a test workspace with various package configurations to verify rewatch's behavior across different scenarios.
+
+#### Debugging
+
+- **Build State**: Use `log::debug!` to inspect `BuildState` contents
+- **Compiler Args**: Check generated `bsc` arguments in `compile.rs`
+- **Dependencies**: Inspect module dependency graph in `deps.rs`
+- **File Watching**: Monitor file change events in `watcher.rs`
+
+#### Performance Considerations
+
+- **Incremental Builds**: Only recompile dirty modules
+- **Parallel Compilation**: Use `rayon` for parallel processing
+- **Memory Usage**: Be mindful of `BuildState` size in large projects
+- **File I/O**: Minimize file system operations
+
+#### Performance vs Code Quality Trade-offs
+
+When clippy suggests refactoring that could impact performance, consider the trade-offs:
+
+- **Parameter Structs vs Many Arguments**: While clippy prefers parameter structs for functions with many arguments, sometimes the added complexity isn't worth it. Use `#[allow(clippy::too_many_arguments)]` for functions that legitimately need many parameters and where a struct would add unnecessary complexity.
+
+- **Cloning vs Borrowing**: Sometimes cloning is necessary due to Rust's borrow checker rules. If the clone is:
+  - Small and one-time (e.g., `Vec<String>` with few elements)
+  - Necessary for correct ownership semantics
+  - Not in a hot path
+  
+  Then accept the clone rather than over-engineering the solution.
+
+- **When to Optimize**: Profile before optimizing. Most "performance concerns" in build systems are negligible compared to actual compilation time.
+
+- **Avoid Unnecessary Type Conversions**: When threading parameters through multiple function calls, use consistent types (e.g., `String` throughout) rather than converting between `String` and `&str` at each boundary. This eliminates unnecessary allocations and conversions.
+
+#### Compatibility with Legacy bsb
+
+- **Command-line Flags**: Maintain compatibility with bsb flags where possible
+- **Configuration**: Support both old (`bs-*`) and new field names
+- **Output Format**: Generate compatible build artifacts
+- **Error Messages**: Provide clear migration guidance
+
+### Common Tasks
+
+#### Adding New CLI Flags
+
+1. Add to `BuildArgs` and `WatchArgs` in `cli.rs`
+2. Update `From<BuildArgs> for WatchArgs` implementation
+3. Pass through `main.rs` to build functions
+4. Thread through build system to where it's needed
+5. Add unit tests for the new functionality
+
+#### Modifying Compiler Arguments
+
+1. Update `compiler_args()` in `build/compile.rs`
+2. Consider both parsing and compilation phases
+3. Handle precedence between CLI flags and config
+4. Test with various `rescript.json` configurations
+
+#### Working with Dependencies
+
+1. Use `packages.rs` for package discovery
+2. Update `deps.rs` for dependency analysis
+3. Handle both local and external dependencies
+4. Consider dev dependencies vs regular dependencies
+
+#### File Watching
+
+1. Modify `watcher.rs` for file change handling
+2. Update `AsyncWatchArgs` for new parameters
+3. Handle different file types (`.res`, `.resi`, etc.)
+4. Consider performance impact of watching many files

CHANGELOG.md

@@ -32,6 +32,7 @@
 - Option optimization: do not create redundant local vars. https://github.com/rescript-lang/rescript/pull/7915
 - Js output: remove superfluous newline after every `if`. https://github.com/rescript-lang/rescript/pull/7920
 - Rewatch: Traverse upwards for package resolution in single context projects. https://github.com/rescript-lang/rescript/pull/7896
+- Rewatch: Add `--warn-error` flag to `build`. https://github.com/rescript-lang/rescript/pull/7916
 
 #### :house: Internal
 

rewatch/src/build.rs

@@ -82,6 +82,8 @@ pub fn get_compiler_args(rescript_file_path: &Path) -> Result<String> {
         &project_context.current_config,
         relative_filename,
         &contents,
+        /* is_local_dep */ true,
+        /* warn_error_override */ None,
     )?;
     let is_interface = filename.to_string_lossy().ends_with('i');
     let has_interface = if is_interface {
@@ -101,6 +103,7 @@ pub fn get_compiler_args(rescript_file_path: &Path) -> Result<String> {
         &None,
         is_type_dev,
         true,
+        None, // No warn_error_override for compiler-args command
     )?;
 
     let result = serde_json::to_string_pretty(&CompilerArgs {
@@ -132,7 +135,8 @@ pub fn initialize_build(
     path: &Path,
     build_dev_deps: bool,
     snapshot_output: bool,
-) -> Result<BuildState> {
+    warn_error: Option<String>,
+) -> Result<BuildCommandState> {
     let project_context = ProjectContext::new(path)?;
     let compiler = get_compiler_info(&project_context)?;
 
@@ -189,7 +193,7 @@ pub fn initialize_build(
         let _ = stdout().flush();
     }
 
-    let mut build_state = BuildState::new(project_context, packages, compiler);
+    let mut build_state = BuildCommandState::new(project_context, packages, compiler, warn_error);
     packages::parse_packages(&mut build_state);
     let timing_source_files_elapsed = timing_source_files.elapsed();
 
@@ -300,7 +304,7 @@ impl fmt::Display for IncrementalBuildError {
 }
 
 pub fn incremental_build(
-    build_state: &mut BuildState,
+    build_state: &mut BuildCommandState,
     default_timing: Option<Duration>,
     initial_build: bool,
     show_progress: bool,
@@ -370,7 +374,8 @@ pub fn incremental_build(
         }
     }
     let timing_deps = Instant::now();
-    deps::get_deps(build_state, &build_state.deleted_modules.to_owned());
+    let deleted_modules = build_state.deleted_modules.clone();
+    deps::get_deps(build_state, &deleted_modules);
     let timing_deps_elapsed = timing_deps.elapsed();
     current_step += 1;
 
@@ -385,7 +390,7 @@ pub fn incremental_build(
     }
 
     mark_modules_with_expired_deps_dirty(build_state);
-    mark_modules_with_deleted_deps_dirty(build_state);
+    mark_modules_with_deleted_deps_dirty(&mut build_state.build_state);
     current_step += 1;
 
     //print all the compile_dirty modules
@@ -488,7 +493,7 @@ pub fn incremental_build(
     }
 }
 
-fn log_deprecations(build_state: &BuildState) {
+fn log_deprecations(build_state: &BuildCommandState) {
     build_state.packages.iter().for_each(|(_, package)| {
         // Only warn for local dependencies, not external packages
         if package.is_local_dep {
@@ -522,14 +527,15 @@ fn log_deprecated_config_field(package_name: &str, field_name: &str, new_field_n
 // is watching this file.
 // we don't need to do this in an incremental build because there are no file
 // changes (deletes / additions)
-pub fn write_build_ninja(build_state: &BuildState) {
+pub fn write_build_ninja(build_state: &BuildCommandState) {
     for package in build_state.packages.values() {
         // write empty file:
         let mut f = File::create(package.get_build_path().join("build.ninja")).expect("Unable to write file");
         f.write_all(b"").expect("unable to write to ninja file");
     }
 }
 
+#[allow(clippy::too_many_arguments)]
 pub fn build(
     filter: &Option<regex::Regex>,
     path: &Path,
@@ -538,7 +544,8 @@ pub fn build(
     create_sourcedirs: bool,
     build_dev_deps: bool,
     snapshot_output: bool,
-) -> Result<BuildState> {
+    warn_error: Option<String>,
+) -> Result<BuildCommandState> {
     let default_timing: Option<std::time::Duration> = if no_timing {
         Some(std::time::Duration::new(0.0 as u64, 0.0 as u32))
     } else {
@@ -552,6 +559,7 @@ pub fn build(
         path,
         build_dev_deps,
         snapshot_output,
+        warn_error,
     )
     .map_err(|e| anyhow!("Could not initialize build. Error: {e}"))?;
 

rewatch/src/build/build_types.rs

@@ -3,7 +3,7 @@ use crate::config::Config;
 use crate::project_context::ProjectContext;
 use ahash::{AHashMap, AHashSet};
 use blake3::Hash;
-use std::{fmt::Display, path::PathBuf, time::SystemTime};
+use std::{fmt::Display, ops::Deref, path::PathBuf, time::SystemTime};
 
 #[derive(Debug, Clone, PartialEq)]
 pub enum ParseState {
@@ -90,6 +90,9 @@ impl Module {
     }
 }
 
+/// Core build state containing all the essential data needed for compilation.
+/// This is the minimal state required for basic build operations like cleaning.
+/// Used by commands that don't need command-line specific overrides (e.g., `clean`).
 #[derive(Debug)]
 pub struct BuildState {
     pub project_context: ProjectContext,
@@ -101,6 +104,21 @@ pub struct BuildState {
     pub deps_initialized: bool,
 }
 
+/// Extended build state that includes command-line specific overrides.
+/// Wraps `BuildState` and adds command-specific data like warning overrides.
+/// Used by commands that need to respect CLI flags (e.g., `build`, `watch`).
+///
+/// The separation exists because:
+/// - `clean` command only needs core build data, no CLI overrides
+/// - `build`/`watch` commands need both core data AND CLI overrides
+/// - This prevents the "code smell" of optional fields that are None for some commands
+#[derive(Debug)]
+pub struct BuildCommandState {
+    pub build_state: BuildState,
+    // Command-line --warn-error flag override (takes precedence over rescript.json config)
+    pub warn_error_override: Option<String>,
+}
+
 #[derive(Debug, Clone)]
 pub struct CompilerInfo {
     pub bsc_path: PathBuf,
@@ -116,6 +134,7 @@ impl BuildState {
     pub fn get_module(&self, module_name: &str) -> Option<&Module> {
         self.modules.get(module_name)
     }
+
     pub fn new(
         project_context: ProjectContext,
         packages: AHashMap<String, Package>,
@@ -142,6 +161,48 @@ impl BuildState {
     }
 }
 
+impl BuildCommandState {
+    pub fn new(
+        project_context: ProjectContext,
+        packages: AHashMap<String, Package>,
+        compiler: CompilerInfo,
+        warn_error_override: Option<String>,
+    ) -> Self {
+        Self {
+            build_state: BuildState::new(project_context, packages, compiler),
+            warn_error_override,
+        }
+    }
+
+    pub fn get_warn_error_override(&self) -> Option<String> {
+        self.warn_error_override.clone()
+    }
+
+    pub fn module_name_package_pairs(&self) -> Vec<(String, String)> {
+        self.build_state
+            .modules
+            .iter()
+            .map(|(name, module)| (name.clone(), module.package_name.clone()))
+            .collect()
+    }
+}
+
+// Implement Deref to automatically delegate method calls to the inner BuildState
+impl Deref for BuildCommandState {
+    type Target = BuildState;
+
+    fn deref(&self) -> &Self::Target {
+        &self.build_state
+    }
+}
+
+// Implement DerefMut to allow mutable access to the inner BuildState
+impl std::ops::DerefMut for BuildCommandState {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.build_state
+    }
+}
+
 #[derive(Debug)]
 pub struct AstModule {
     pub module_name: String,