From 03fadd584e8264428552e7c6f31307b577bce7a2 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 30 Jan 2024 12:15:00 +0100 Subject: [PATCH] chore: only use a compiler filter if not empty --- crates/common/src/compile.rs | 80 +++++++++++++++++------------ crates/common/src/glob.rs | 18 ++++--- crates/forge/bin/cmd/build.rs | 13 +++-- crates/forge/bin/cmd/test/filter.rs | 75 ++++++++++++--------------- crates/forge/bin/cmd/test/mod.rs | 2 +- 5 files changed, 101 insertions(+), 87 deletions(-) diff --git a/crates/common/src/compile.rs b/crates/common/src/compile.rs index 86dd4edf6c27..eccea208cd04 100644 --- a/crates/common/src/compile.rs +++ b/crates/common/src/compile.rs @@ -378,7 +378,10 @@ pub fn compile_target_with_filter( let graph = Graph::resolve(&project.paths)?; // Checking if it's a standalone script, or part of a project. - let mut compiler = ProjectCompiler::new().filter(Box::new(SkipBuildFilters(skip))).quiet(quiet); + let mut compiler = ProjectCompiler::new().quiet(quiet); + if !skip.is_empty() { + compiler = compiler.filter(Box::new(SkipBuildFilters::new(skip)?)); + } if !graph.files().contains_key(target_path) { if verify { eyre::bail!("You can only verify deployments from inside a project! Make sure it exists with `forge tree`."); @@ -469,13 +472,20 @@ pub fn etherscan_project(metadata: &Metadata, target_path: impl AsRef) -> } /// Bundles multiple `SkipBuildFilter` into a single `FileFilter` -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct SkipBuildFilters(pub Vec); +#[derive(Clone, Debug)] +pub struct SkipBuildFilters(Vec); impl FileFilter for SkipBuildFilters { /// Only returns a match if _no_ exclusion filter matches fn is_match(&self, file: &Path) -> bool { - self.0.iter().all(|filter| filter.is_match(file)) + self.0.iter().all(|matcher| is_match_exclude(matcher, file)) + } +} + +impl SkipBuildFilters { + /// Creates a new `SkipBuildFilters` from multiple `SkipBuildFilter`. + pub fn new(matchers: impl IntoIterator) -> Result { + matchers.into_iter().map(|m| m.compile()).collect::>().map(Self) } } @@ -491,6 +501,14 @@ pub enum SkipBuildFilter { } impl SkipBuildFilter { + fn new(s: &str) -> Self { + match s { + "test" | "tests" => SkipBuildFilter::Tests, + "script" | "scripts" => SkipBuildFilter::Scripts, + s => SkipBuildFilter::Custom(s.to_string()), + } + } + /// Returns the pattern to match against a file fn file_pattern(&self) -> &str { match self { @@ -499,15 +517,9 @@ impl SkipBuildFilter { SkipBuildFilter::Custom(s) => s.as_str(), } } -} -impl> From for SkipBuildFilter { - fn from(s: T) -> Self { - match s.as_ref() { - "test" | "tests" => SkipBuildFilter::Tests, - "script" | "scripts" => SkipBuildFilter::Scripts, - s => SkipBuildFilter::Custom(s.to_string()), - } + fn compile(&self) -> Result { + self.file_pattern().parse().map_err(Into::into) } } @@ -515,23 +527,20 @@ impl FromStr for SkipBuildFilter { type Err = Infallible; fn from_str(s: &str) -> result::Result { - Ok(s.into()) + Ok(Self::new(s)) } } -impl FileFilter for SkipBuildFilter { - /// Matches file only if the filter does not apply - /// - /// This is returns the inverse of `file.name.contains(pattern) || matcher.is_match(file)` - fn is_match(&self, file: &Path) -> bool { - fn exclude(file: &Path, pattern: &str) -> Option { - let matcher: GlobMatcher = pattern.parse().unwrap(); - let file_name = file.file_name()?.to_str()?; - Some(file_name.contains(pattern) || matcher.is_match(file.as_os_str().to_str()?)) - } - - !exclude(file, self.file_pattern()).unwrap_or_default() +/// Matches file only if the filter does not apply. +/// +/// This returns the inverse of `file.name.contains(pattern) || matcher.is_match(file)`. +fn is_match_exclude(matcher: &GlobMatcher, path: &Path) -> bool { + fn is_match(matcher: &GlobMatcher, path: &Path) -> Option { + let file_name = path.file_name()?.to_str()?; + Some(file_name.contains(matcher.as_str()) || matcher.is_match(path)) } + + !is_match(matcher, path).unwrap_or_default() } #[cfg(test)] @@ -540,19 +549,24 @@ mod tests { #[test] fn test_build_filter() { + let tests = SkipBuildFilter::Tests.compile().unwrap(); + let scripts = SkipBuildFilter::Scripts.compile().unwrap(); + let custom = |s: &str| SkipBuildFilter::Custom(s.to_string()).compile().unwrap(); + let file = Path::new("A.t.sol"); - assert!(!SkipBuildFilter::Tests.is_match(file)); - assert!(SkipBuildFilter::Scripts.is_match(file)); - assert!(!SkipBuildFilter::Custom("A.t".to_string()).is_match(file)); + assert!(!is_match_exclude(&tests, file)); + assert!(is_match_exclude(&scripts, file)); + assert!(!is_match_exclude(&custom("A.t"), file)); let file = Path::new("A.s.sol"); - assert!(SkipBuildFilter::Tests.is_match(file)); - assert!(!SkipBuildFilter::Scripts.is_match(file)); - assert!(!SkipBuildFilter::Custom("A.s".to_string()).is_match(file)); + assert!(is_match_exclude(&tests, file)); + assert!(!is_match_exclude(&scripts, file)); + assert!(!is_match_exclude(&custom("A.s"), file)); let file = Path::new("/home/test/Foo.sol"); - assert!(!SkipBuildFilter::Custom("*/test/**".to_string()).is_match(file)); + assert!(!is_match_exclude(&custom("*/test/**"), file)); + let file = Path::new("/home/script/Contract.sol"); - assert!(!SkipBuildFilter::Custom("*/script/**".to_string()).is_match(file)); + assert!(!is_match_exclude(&custom("*/script/**"), file)); } } diff --git a/crates/common/src/glob.rs b/crates/common/src/glob.rs index e573104f67a5..104dd4f5ea50 100644 --- a/crates/common/src/glob.rs +++ b/crates/common/src/glob.rs @@ -35,12 +35,16 @@ impl GlobMatcher { /// /// The glob `./test/*` won't match absolute paths like `test/Contract.sol`, which is common /// format here, so we also handle this case here - pub fn is_match(&self, path: &str) -> bool { - let mut matches = self.matcher.is_match(path); - if !matches && !path.starts_with("./") && self.as_str().starts_with("./") { - matches = self.matcher.is_match(format!("./{path}")); + pub fn is_match(&self, path: &Path) -> bool { + if self.matcher.is_match(path) { + return true; } - matches + + if !path.starts_with("./") && self.as_str().starts_with("./") { + return self.matcher.is_match(format!("./{}", path.display())); + } + + false } /// Returns the `Glob` string used to compile this matcher. @@ -76,7 +80,7 @@ mod tests { #[test] fn can_match_glob_paths() { let matcher: GlobMatcher = "./test/*".parse().unwrap(); - assert!(matcher.is_match("test/Contract.sol")); - assert!(matcher.is_match("./test/Contract.sol")); + assert!(matcher.is_match(Path::new("test/Contract.sol"))); + assert!(matcher.is_match(Path::new("./test/Contract.sol"))); } } diff --git a/crates/forge/bin/cmd/build.rs b/crates/forge/bin/cmd/build.rs index 839f5a504f19..239f16f5fe13 100644 --- a/crates/forge/bin/cmd/build.rs +++ b/crates/forge/bin/cmd/build.rs @@ -87,13 +87,18 @@ impl BuildArgs { project = config.project()?; } - let output = ProjectCompiler::new() + let mut compiler = ProjectCompiler::new() .print_names(self.names) .print_sizes(self.sizes) .quiet(self.format_json) - .bail(!self.format_json) - .filter(Box::new(SkipBuildFilters(self.skip.unwrap_or_default()))) - .compile(&project)?; + .bail(!self.format_json); + if let Some(skip) = self.skip { + if !skip.is_empty() { + compiler = compiler.filter(Box::new(SkipBuildFilters::new(skip)?)); + } + } + let output = compiler.compile(&project)?; + if self.format_json { println!("{}", serde_json::to_string_pretty(&output.clone().output())?); } diff --git a/crates/forge/bin/cmd/test/filter.rs b/crates/forge/bin/cmd/test/filter.rs index 71abb41b0039..1c8751c07094 100644 --- a/crates/forge/bin/cmd/test/filter.rs +++ b/crates/forge/bin/cmd/test/filter.rs @@ -44,28 +44,26 @@ pub struct FilterArgs { impl FilterArgs { /// Merges the set filter globs with the config's values - pub fn merge_with_config(&self, config: &Config) -> ProjectPathsAwareFilter { - let mut filter = self.clone(); - if filter.test_pattern.is_none() { - filter.test_pattern = config.test_pattern.clone().map(|p| p.into()); + pub fn merge_with_config(mut self, config: &Config) -> ProjectPathsAwareFilter { + if self.test_pattern.is_none() { + self.test_pattern = config.test_pattern.clone().map(Into::into); } - if filter.test_pattern_inverse.is_none() { - filter.test_pattern_inverse = config.test_pattern_inverse.clone().map(|p| p.into()); + if self.test_pattern_inverse.is_none() { + self.test_pattern_inverse = config.test_pattern_inverse.clone().map(Into::into); } - if filter.contract_pattern.is_none() { - filter.contract_pattern = config.contract_pattern.clone().map(|p| p.into()); + if self.contract_pattern.is_none() { + self.contract_pattern = config.contract_pattern.clone().map(Into::into); } - if filter.contract_pattern_inverse.is_none() { - filter.contract_pattern_inverse = - config.contract_pattern_inverse.clone().map(|p| p.into()); + if self.contract_pattern_inverse.is_none() { + self.contract_pattern_inverse = config.contract_pattern_inverse.clone().map(Into::into); } - if filter.path_pattern.is_none() { - filter.path_pattern = config.path_pattern.clone().map(Into::into); + if self.path_pattern.is_none() { + self.path_pattern = config.path_pattern.clone().map(Into::into); } - if filter.path_pattern_inverse.is_none() { - filter.path_pattern_inverse = config.path_pattern_inverse.clone().map(Into::into); + if self.path_pattern_inverse.is_none() { + self.path_pattern_inverse = config.path_pattern_inverse.clone().map(Into::into); } - ProjectPathsAwareFilter { args_filter: filter, paths: config.project_paths() } + ProjectPathsAwareFilter { args_filter: self, paths: config.project_paths() } } } @@ -86,15 +84,13 @@ impl FileFilter for FilterArgs { /// Returns true if the file regex pattern match the `file` /// /// If no file regex is set this returns true if the file ends with `.t.sol`, see - /// [FoundryPathExr::is_sol_test()] + /// [`FoundryPathExt::is_sol_test()`]. fn is_match(&self, file: &Path) -> bool { - if let Some(file) = file.as_os_str().to_str() { - if let Some(ref glob) = self.path_pattern { - return glob.is_match(file) - } - if let Some(ref glob) = self.path_pattern_inverse { - return !glob.is_match(file) - } + if let Some(glob) = &self.path_pattern { + return glob.is_match(file) + } + if let Some(glob) = &self.path_pattern_inverse { + return !glob.is_match(file) } file.is_sol_test() } @@ -124,10 +120,6 @@ impl TestFilter for FilterArgs { } fn matches_path(&self, path: &Path) -> bool { - let Some(path) = path.to_str() else { - return false; - }; - let mut ok = true; if let Some(re) = &self.path_pattern { ok = ok && re.is_match(path); @@ -141,26 +133,25 @@ impl TestFilter for FilterArgs { impl fmt::Display for FilterArgs { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let mut patterns = Vec::new(); - if let Some(ref p) = self.test_pattern { - patterns.push(format!("\tmatch-test: `{}`", p.as_str())); + if let Some(p) = &self.test_pattern { + writeln!(f, "\tmatch-test: `{}`", p.as_str())?; } - if let Some(ref p) = self.test_pattern_inverse { - patterns.push(format!("\tno-match-test: `{}`", p.as_str())); + if let Some(p) = &self.test_pattern_inverse { + writeln!(f, "\tno-match-test: `{}`", p.as_str())?; } - if let Some(ref p) = self.contract_pattern { - patterns.push(format!("\tmatch-contract: `{}`", p.as_str())); + if let Some(p) = &self.contract_pattern { + writeln!(f, "\tmatch-contract: `{}`", p.as_str())?; } - if let Some(ref p) = self.contract_pattern_inverse { - patterns.push(format!("\tno-match-contract: `{}`", p.as_str())); + if let Some(p) = &self.contract_pattern_inverse { + writeln!(f, "\tno-match-contract: `{}`", p.as_str())?; } - if let Some(ref p) = self.path_pattern { - patterns.push(format!("\tmatch-path: `{}`", p.as_str())); + if let Some(p) = &self.path_pattern { + writeln!(f, "\tmatch-path: `{}`", p.as_str())?; } - if let Some(ref p) = self.path_pattern_inverse { - patterns.push(format!("\tno-match-path: `{}`", p.as_str())); + if let Some(p) = &self.path_pattern_inverse { + writeln!(f, "\tno-match-path: `{}`", p.as_str())?; } - write!(f, "{}", patterns.join("\n")) + Ok(()) } } diff --git a/crates/forge/bin/cmd/test/mod.rs b/crates/forge/bin/cmd/test/mod.rs index bd8787bcd49e..fa32bec93bf7 100644 --- a/crates/forge/bin/cmd/test/mod.rs +++ b/crates/forge/bin/cmd/test/mod.rs @@ -507,7 +507,7 @@ impl TestArgs { /// Returns the flattened [`FilterArgs`] arguments merged with [`Config`]. pub fn filter(&self, config: &Config) -> ProjectPathsAwareFilter { - self.filter.merge_with_config(config) + self.filter.clone().merge_with_config(config) } /// Returns whether `BuildArgs` was configured with `--watch`