Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Ignore SPDX and contract size for tests #775

Merged
merged 5 commits into from
Jan 8, 2022
Merged

Ignore SPDX and contract size for tests #775

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0870288
Filter for unignored warnings
onbjerg Jan 8, 2022
650dbe0
Pass compiler output to diagnostics
onbjerg Jan 8, 2022
74e4d5a
Ignore SPDX and contract size warnings for tests
onbjerg Jan 8, 2022
371afb9
Fix lint
onbjerg Jan 8, 2022
b8c06db
Skip lookups if error is not a warning
onbjerg Jan 8, 2022
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
47 changes: 35 additions & 12 deletions ethers-solc/src/artifacts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,12 +537,17 @@ impl CompilerOutput {
}

/// Whether the output contains a compiler warning
pub fn has_warning(&self) -> bool {
self.errors.iter().any(|err| err.severity.is_warning())
pub fn has_warning(&self, ignored_error_codes: &[u64]) -> bool {
self.errors.iter().any(|err| {
let is_ignored =
err.error_code.as_ref().map_or(false, |code| !ignored_error_codes.contains(code));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be more efficient to compute this only if err.severity.is_warning() == true


err.severity.is_warning() && !is_ignored
})
}

pub fn diagnostics<'a>(&'a self, ignored_error_codes: &'a [u64]) -> OutputDiagnostics {
OutputDiagnostics { errors: &self.errors, ignored_error_codes }
OutputDiagnostics { compiler_output: self, ignored_error_codes }
}

/// Finds the _first_ contract with the given name
Expand Down Expand Up @@ -683,19 +688,29 @@ impl OutputContracts {
/// Helper type to implement display for solc errors
#[derive(Clone, Debug)]
pub struct OutputDiagnostics<'a> {
errors: &'a [Error],
compiler_output: &'a CompilerOutput,
ignored_error_codes: &'a [u64],
}

impl<'a> OutputDiagnostics<'a> {
/// Returns true if there is at least one error of high severity
pub fn has_error(&self) -> bool {
self.errors.iter().any(|err| err.severity.is_error())
self.compiler_output.has_error()
}

/// Returns true if there is at least one warning
pub fn has_warning(&self) -> bool {
self.errors.iter().any(|err| err.severity.is_warning())
self.compiler_output.has_warning(self.ignored_error_codes)
}

fn is_test<T: AsRef<str>>(&self, contract_path: T) -> bool {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how robust this is and I couldn't really figure out a good way to do it for deps, should the test be "if it sits in node_modules or lib then it is a dependency"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, one idea I have is to leave this completely up to the user,

rn we're using it like this:

ethers-rs/ethers-solc/src/lib.rs

Line 864 in 371afb9

output.diagnostics(&self.ignored_error_codes).fmt(f)

perhaps we can add something like

 filter: dyn ContractFilter

trait ContractFilter {
   fn matches(&self, contract: &Contract) -> bool;
}

impl ContractFilter for () {
  fn matches(&self) -> boo {true}
}

then implement this for Fn(&str) -> bool

so we can add a CompilerOutput::diagnostics_with_filter(error_codes, dyn ContractFilter)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also do that for filtering of warnings in general, so instead of a contract filter etc. we just give the user

  • has_warnings
  • has_errors
  • a way to iterate the errors and filter them themselves

unless there is a specific reason we have this in ethers-solc as opposed to foundry?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg, so we should map each error to its contract and ignore an error if:

  • ignored error code
  • contract should be ignored

if contract_path.as_ref().ends_with(".t.sol") {
return true
}

self.compiler_output.find(&contract_path).map_or(false, |contract| {
contract.abi.map_or(false, |abi| abi.functions.contains_key("IS_TEST"))
})
}
}

Expand All @@ -708,13 +723,21 @@ impl<'a> fmt::Display for OutputDiagnostics<'a> {
} else {
f.write_str("Compiler run successful")?;
}
for err in self.errors {
// Do not log any ignored error codes
if let Some(error_code) = err.error_code {
if !self.ignored_error_codes.contains(&error_code) {
writeln!(f, "\n{}", err)?;
for err in &self.compiler_output.errors {
let is_ignored = err.error_code.as_ref().map_or(false, |code| {
if let Some(source_location) = &err.source_location {
// we ignore spdx and contract size warnings in test
// files. if we are looking at one of these warnings
// from a test file we skip
if self.is_test(&source_location.file) && (*code == 1878 || *code == 5574) {
return true
}
}
} else {

self.ignored_error_codes.contains(code)
});

if !is_ignored {
writeln!(f, "\n{}", err)?;
}
}
Expand Down
5 changes: 4 additions & 1 deletion ethers-solc/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,10 @@ impl<T: ArtifactOutput> ProjectCompileOutput<T> {

/// Whether there were warnings
pub fn has_compiler_warnings(&self) -> bool {
self.compiler_output.as_ref().map(|o| o.has_warning()).unwrap_or_default()
self.compiler_output
.as_ref()
.map(|o| o.has_warning(&self.ignored_error_codes))
.unwrap_or_default()
}

/// Finds the first contract with the given name and removes it from the set
Expand Down