Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: only use a compiler filter if not empty #6954

Merged
merged 1 commit into from
Jan 30, 2024
Merged

chore: only use a compiler filter if not empty #6954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 47 additions & 33 deletions crates/common/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.");
Expand Down Expand Up @@ -469,13 +472,20 @@ pub fn etherscan_project(metadata: &Metadata, target_path: impl AsRef<Path>) ->
}

/// Bundles multiple `SkipBuildFilter` into a single `FileFilter`
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct SkipBuildFilters(pub Vec<SkipBuildFilter>);
#[derive(Clone, Debug)]
pub struct SkipBuildFilters(Vec<GlobMatcher>);

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<Item = SkipBuildFilter>) -> Result<Self> {
matchers.into_iter().map(|m| m.compile()).collect::<Result<_>>().map(Self)
}
}

Expand All @@ -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 {
Expand All @@ -499,39 +517,30 @@ impl SkipBuildFilter {
SkipBuildFilter::Custom(s) => s.as_str(),
}
}
}

impl<T: AsRef<str>> From<T> 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<GlobMatcher> {
self.file_pattern().parse().map_err(Into::into)
}
}

impl FromStr for SkipBuildFilter {
type Err = Infallible;

fn from_str(s: &str) -> result::Result<Self, Self::Err> {
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<bool> {
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<bool> {
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)]
Expand All @@ -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));
}
}
18 changes: 11 additions & 7 deletions crates/common/src/glob.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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")));
}
}
14 changes: 10 additions & 4 deletions crates/forge/bin/cmd/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,16 +87,22 @@ 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())?);
}

Ok(output)
}

Expand Down
94 changes: 50 additions & 44 deletions crates/forge/bin/cmd/test/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,29 +43,37 @@ pub struct FilterArgs {
}

impl FilterArgs {
/// Returns true if the filter is empty.
pub fn is_empty(&self) -> bool {
self.test_pattern.is_none() &&
self.test_pattern_inverse.is_none() &&
self.contract_pattern.is_none() &&
self.contract_pattern_inverse.is_none() &&
self.path_pattern.is_none() &&
self.path_pattern_inverse.is_none()
}

/// 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() }
}
}

Expand All @@ -86,15 +94,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()
}
Expand Down Expand Up @@ -124,10 +130,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);
Expand All @@ -141,26 +143,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(())
}
}

Expand All @@ -174,12 +175,17 @@ pub struct ProjectPathsAwareFilter {
// === impl ProjectPathsAwareFilter ===

impl ProjectPathsAwareFilter {
/// Returns the CLI arguments
/// Returns true if the filter is empty.
pub fn is_empty(&self) -> bool {
self.args_filter.is_empty()
}

/// Returns the CLI arguments.
pub fn args(&self) -> &FilterArgs {
&self.args_filter
}

/// Returns the CLI arguments mutably
/// Returns the CLI arguments mutably.
pub fn args_mut(&mut self) -> &mut FilterArgs {
&mut self.args_filter
}
Expand Down
12 changes: 7 additions & 5 deletions crates/forge/bin/cmd/test/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,14 +319,16 @@ impl TestArgs {
trace!(target: "forge::test", "running all tests");

if runner.matching_test_function_count(filter) == 0 {
let filter_str = filter.to_string();
if filter_str.is_empty() {
println!();
if filter.is_empty() {
println!(
"\nNo tests found in project! \
"No tests found in project! \
Forge looks for functions that starts with `test`."
);
} else {
println!("\nNo tests match the provided pattern:\n{filter_str}");
println!("No tests match the provided pattern:");
print!("{filter}");

// Try to suggest a test when there's no match
if let Some(test_pattern) = &filter.args().test_pattern {
let test_name = test_pattern.as_str();
Expand Down Expand Up @@ -507,7 +509,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`
Expand Down
Loading