From e418e91a0dcf2771fb11ac130d3b40374d52412a Mon Sep 17 00:00:00 2001 From: Michael Rogenmoser Date: Wed, 15 Jul 2026 13:52:11 +0200 Subject: [PATCH] script: apply override_files before validation and slang Override files were resolved late, in the emit_template stage, so they did not apply to file-existence validation nor to the slang file-list reduction. This meant a shadowed original still had to exist on disk, and slang parsed both the original and its override as duplicate module definitions. Resolve override_files right after flatten() instead: drop the override groups and rewrite like-named files in the regular groups to the overriding path (preserving SourceType). Only the overriding files are then validated, and slang analyzes/reduces them instead of the files they shadow. The OVERRIDDEN annotation is carried through a side map so --source-annotations output is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 + src/cmd/script.rs | 166 +++++++++++++++++++--------------- tests/pickle/Bender.yml | 5 + tests/pickle/override/leaf.sv | 4 + tests/script.rs | 86 ++++++++++++++++++ 5 files changed, 191 insertions(+), 72 deletions(-) create mode 100644 tests/pickle/override/leaf.sv diff --git a/CHANGELOG.md b/CHANGELOG.md index da509645..dad7c116 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## Unreleased +### Fixed +- script: apply `override_files` before validation and the slang pass, so overriding files replace their targets in file-existence checks and in `--top`/`--trim-incdirs` reduction (previously slang saw both the original and the override as duplicate modules); the overridden-file annotation is preserved. ## 0.32.1 - 2026-07-07 ### Added diff --git a/src/cmd/script.rs b/src/cmd/script.rs index c88b0ab4..e5a0b178 100644 --- a/src/cmd/script.rs +++ b/src/cmd/script.rs @@ -369,9 +369,15 @@ pub fn run(sess: &Session, args: &ScriptArgs) -> Result<()> { .filter_packages(&packages, args.keep_excluded_incdirs) .unwrap_or_default(); - // Flatten and validate the sources. + // Flatten the sources, then resolve `override_files` groups before anything else inspects + // the file set. Doing this here — rather than at template-emit time — means the replaced + // files never reach validation or the slang pass: only the overriding files have their + // existence checked, and slang analyzes and reduces the overriding files instead of the + // ones they shadow (previously it saw both, colliding as duplicate module definitions). + let (srcs, override_comments) = apply_file_overrides(srcs.flatten()); + + // Validate the sources. let srcs = srcs - .flatten() .into_iter() .map(|f| f.validate(&ValidationContext::default())) .collect::>>()?; @@ -503,6 +509,7 @@ pub fn run(sess: &Session, args: &ScriptArgs) -> Result<()> { only_args, srcs, &unparseable_paths, + &override_comments, ) } @@ -775,6 +782,68 @@ fn add_defines(defines: &mut IndexMap>, define_args: &[St static JSON: &str = "json"; +/// Apply `override_files` source groups to a flattened source list. +/// +/// A group flagged `override_files` contributes no files of its own; instead each of its files +/// replaces — by basename — the like-named file in the regular groups. Resolving this here, +/// before validation and the slang pass, lets every later stage see the final file set: the +/// shadowed originals are gone (so they need not exist on disk, and slang never parses both the +/// original and its replacement as duplicate modules) and slang reachability/trimming operates +/// on the overriding files. +/// +/// All `override_files` groups are dropped from the returned list. The returned map is keyed by +/// each overriding path and holds the annotation describing what it replaced, so +/// `--source-annotations` can still surface the substitution downstream. +fn apply_file_overrides<'ctx>( + srcs: Vec>, +) -> (Vec>, IndexMap) { + // Collect the overriding files, keyed by basename. On a basename clash the last group wins, + // matching the previous emit-time behavior. + let mut overrides: IndexMap<&'ctx str, (&'ctx Path, &'ctx str)> = IndexMap::new(); + for src in &srcs { + if !src.override_files { + continue; + } + for file in &src.files { + if let SourceFile::File(p, _) = file + && let Some(name) = p.file_name().and_then(std::ffi::OsStr::to_str) + { + overrides.insert(name, (*p, src.package.unwrap_or("None"))); + } + } + } + + // Drop the override groups and rewrite matching files in the remaining groups to point at + // the overriding path, recording the annotation. The file's `SourceType` is preserved, so + // downstream categorization treats the replacement exactly like the file it shadowed. + let mut comments: IndexMap = IndexMap::new(); + let srcs = srcs + .into_iter() + .filter(|src| !src.override_files) + .map(|mut src| { + for file in &mut src.files { + if let SourceFile::File(p, _) = file { + let old = *p; + if let Some(name) = old.file_name().and_then(std::ffi::OsStr::to_str) + && let Some((new_path, pkg)) = overrides.get(name).copied() + && new_path != old + { + comments.insert( + new_path.to_path_buf(), + format!("OVERRIDDEN from {}: {}", pkg, old.to_string_lossy()), + ); + *p = new_path; + } + } + } + src + }) + .collect(); + + (srcs, comments) +} + +#[allow(clippy::too_many_arguments)] fn emit_template( sess: &Session, mut tera_context: Context, @@ -783,6 +852,7 @@ fn emit_template( only: OnlyArgs, srcs: Vec, unparseable_paths: &std::collections::HashSet, + override_comments: &IndexMap, ) -> Result<()> { // Helper for annotating FileEntry.comment on files that survived filtering despite slang // failing to parse them; visible to users with `--source-annotations`. @@ -793,6 +863,14 @@ fn emit_template( None } }; + // The `--source-annotations` comment for a file: the override annotation if this path + // replaced another (see `apply_file_overrides`), otherwise an unparseable-file note. + let file_comment = |p: &Path| -> Option { + override_comments + .get(p) + .cloned() + .or_else(|| unparseable_comment(p)) + }; tera_context.insert("HEADER_AUTOGEN", HEADER_AUTOGEN); tera_context.insert("root", sess.root); // tera_context.insert("srcs", &srcs); @@ -808,11 +886,12 @@ fn emit_template( let mut all_defines = IndexMap::new(); let mut all_incdirs = IndexSet::new(); - let mut all_files = IndexSet::new(); + let mut all_files: IndexSet<&Path> = IndexSet::new(); let mut all_verilog = vec![]; let mut all_vhdl = vec![]; let mut unknown_files = vec![]; - let mut all_override_files: IndexSet<(&Path, &str)> = IndexSet::new(); + // `override_files` groups were already resolved in `apply_file_overrides`, so every group + // here contributes its files directly. for src in &srcs { all_defines.extend( src.defines @@ -820,19 +899,10 @@ fn emit_template( .map(|(k, (_, v))| (k.to_string(), v.map(String::from))), ); all_incdirs.extend(src.get_incdirs()); - - // If override_files is set, source files are not automatically included, only to replace files with matching basenames. - if src.override_files { - all_override_files.extend(src.files.iter().filter_map(|file| match file { - SourceFile::File(p, _) => Some((*p, src.package.unwrap_or("None"))), - SourceFile::Group(_) => None, - })); - } else { - all_files.extend(src.files.iter().filter_map(|file| match file { - SourceFile::File(p, _) => Some((*p, None::)), - SourceFile::Group(_) => None, - })); - } + all_files.extend(src.files.iter().filter_map(|file| match file { + SourceFile::File(p, _) => Some(*p), + SourceFile::Group(_) => None, + })); } add_defines(&mut all_defines, &args.define); @@ -852,40 +922,11 @@ fn emit_template( }; tera_context.insert("all_incdirs", &all_incdirs); - // replace files in all_files with override files - let override_map = all_override_files - .iter() - .map(|(f, pkg)| { - ( - f.file_name() - .and_then(std::ffi::OsStr::to_str) - .unwrap_or(""), - (*f, pkg), - ) - }) - .collect::>(); let all_files = all_files .into_iter() - .map(|file| { - let basename = file - .0 - .file_name() - .and_then(std::ffi::OsStr::to_str) - .unwrap_or(""); - match override_map.get(&basename) { - Some((new_path, pkg)) => FileEntry { - file: new_path.to_path_buf(), - comment: Some(format!( - "OVERRIDDEN from {}: {}", - pkg, - file.0.to_string_lossy() - )), - }, - None => FileEntry { - file: file.0.to_path_buf(), - comment: file.1.or_else(|| unparseable_comment(file.0)), - }, - } + .map(|p| FileEntry { + file: p.to_path_buf(), + comment: file_comment(p), }) .collect::>(); @@ -895,9 +936,6 @@ fn emit_template( let mut split_srcs = vec![]; for src in srcs { - if src.override_files { - continue; - } separate_files_in_group( src, // Categorize by file type. The extra `Some(..)` wrapper means untyped files @@ -938,26 +976,10 @@ fn emit_template( files: files .iter() .map(|f| match f { - SourceFile::File(p, _) => { - let basename = p - .file_name() - .and_then(std::ffi::OsStr::to_str) - .unwrap_or(""); - match override_map.get(&basename) { - Some((new_path, pkg)) => FileEntry { - file: new_path.to_path_buf(), - comment: Some(format!( - "OVERRIDDEN from {}: {}", - pkg, - p.to_string_lossy() - )), - }, - None => FileEntry { - file: p.to_path_buf(), - comment: unparseable_comment(p), - }, - } - } + SourceFile::File(p, _) => FileEntry { + file: p.to_path_buf(), + comment: file_comment(p), + }, SourceFile::Group(_) => unreachable!(), }) .collect(), diff --git a/tests/pickle/Bender.yml b/tests/pickle/Bender.yml index 2f8c3dcd..11190ea6 100644 --- a/tests/pickle/Bender.yml +++ b/tests/pickle/Bender.yml @@ -36,6 +36,11 @@ sources: files: - src/broken.sv + - target: override + override_files: true + files: + - override/leaf.sv + - target: untyped files: - src/misc_rtl.sv diff --git a/tests/pickle/override/leaf.sv b/tests/pickle/override/leaf.sv new file mode 100644 index 00000000..eee0bcd8 --- /dev/null +++ b/tests/pickle/override/leaf.sv @@ -0,0 +1,4 @@ +// Overriding version of leaf, used only when the `override` target is active. +module leaf; + // OVERRIDE MARKER +endmodule diff --git a/tests/script.rs b/tests/script.rs index c8ca8500..a5b42b69 100644 --- a/tests/script.rs +++ b/tests/script.rs @@ -435,4 +435,90 @@ mod tests { ); } } + + /// The raw source-path lines of a flist-plus output (dropping `+incdir+` / `+define+` + /// directives and `//` annotation lines). + fn source_paths(output: &str) -> Vec<&str> { + output + .lines() + .map(str::trim) + .filter(|l| { + !l.is_empty() + && !l.starts_with("+incdir+") + && !l.starts_with("+define+") + && !l.starts_with("//") + }) + .collect() + } + + /// An `override_files` group replaces the like-named file by basename. The overriding path + /// (`override/leaf.sv`) must appear in place of the original (`src/leaf.sv`), exactly once, + /// and `--source-annotations` must record the substitution. + #[test] + fn script_override_files_replaces_by_basename() { + let out = run_script(&[ + "--target", + "top", + "--target", + "override", + "flist-plus", + "--source-annotations", + ]); + + let leaf_paths: Vec<&str> = source_paths(&out) + .into_iter() + .filter(|p| basename(p) == "leaf.sv") + .collect(); + assert_eq!( + leaf_paths.len(), + 1, + "leaf.sv must appear exactly once (overriding, not duplicated):\n{out}" + ); + assert!( + leaf_paths[0] + .replace('\\', "/") + .ends_with("override/leaf.sv"), + "the overriding path must be emitted, not the original: {:?}\n{out}", + leaf_paths[0] + ); + assert!( + out.lines() + .any(|l| l.contains("OVERRIDDEN") && l.contains("leaf.sv")), + "expected an OVERRIDDEN annotation for leaf.sv:\n{out}" + ); + } + + /// Overrides are resolved before the slang pass, so slang analyzes the overriding file + /// (never both it and the original, which share `module leaf` and would otherwise collide as + /// a duplicate module). With `--top top`, leaf is reachable via top → core → leaf, so the + /// overriding file survives the reachability trim. + #[test] + fn script_override_files_applies_before_slang_top() { + let out = run_script(&[ + "--target", + "top", + "--target", + "override", + "--top", + "top", + "flist-plus", + ]); + + let leaf_paths: Vec<&str> = source_paths(&out) + .into_iter() + .filter(|p| basename(p) == "leaf.sv") + .collect(); + assert_eq!( + leaf_paths.len(), + 1, + "overriding leaf.sv must survive the --top trim exactly once:\n{out}" + ); + assert!( + leaf_paths[0] + .replace('\\', "/") + .ends_with("override/leaf.sv"), + "the overriding path must be the one kept by slang: {:?}\n{out}", + leaf_paths[0] + ); + } }