From 1d83b51caaea62064f8b7fce908512d3bb550826 Mon Sep 17 00:00:00 2001 From: Alexander Gonzalez Date: Sun, 9 Jun 2024 16:45:11 +0200 Subject: [PATCH] feat: add --skip-modifiers flag --- README.md | 9 ++++++++- src/check/context.rs | 10 ++++++++-- src/check/mod.rs | 7 +++++-- src/check/rules/structural_match.rs | 18 ++++++++++------- src/config.rs | 11 ++++++++++- src/hir/combiner.rs | 1 - src/hir/mod.rs | 30 +++++++++++++++-------------- src/scaffold/mod.rs | 3 +++ src/sol/translator.rs | 13 ++++++++++--- tests/check.rs | 15 ++++++++++++++- tests/check/skip_modifiers.t.sol | 20 +++++++++++++++++++ tests/check/skip_modifiers.tree | 8 ++++++++ tests/scaffold.rs | 21 ++++++++++++++++++++ tests/scaffold/skip_modifiers.t.sol | 20 +++++++++++++++++++ tests/scaffold/skip_modifiers.tree | 8 ++++++++ 15 files changed, 162 insertions(+), 32 deletions(-) create mode 100644 tests/check/skip_modifiers.t.sol create mode 100644 tests/check/skip_modifiers.tree create mode 100644 tests/scaffold/skip_modifiers.t.sol create mode 100644 tests/scaffold/skip_modifiers.tree diff --git a/README.md b/README.md index 3678cd1..0112c47 100644 --- a/README.md +++ b/README.md @@ -114,9 +114,12 @@ $ bulloak scaffold -wf ./**/*.tree Note all tests are showing as passing when their body is empty. To prevent this, you can use the `-S` (or `--vm-skip`) option to add a `vm.skip(true);` at the -beginning of each test function. This option will also add an import for +beginning of each test function. This option will also add an import for forge-std's `Test.sol` and all test contracts will inherit from it. +You can skip emitting the modifiers by passing the `-m` (or `--skip--modifiers`) +option. This way, the generated files will only include the test functions. + ### Check That Your Code And Spec Match You can use `bulloak check` to make sure that your Solidity files match your @@ -194,6 +197,10 @@ automatically fixed, and bulloak's output will reflect that. warn: 13 checks failed (run `bulloak check --fix <.tree files>` to apply 11 fixes) ``` +You can skip checking that the modifiers are present by passing the `-m` +(or `--skip--modifiers`) option. This way, `bulloak` will not warn when a +modifier is missing from the generated file. + #### Rules The following rules are currently implemented: diff --git a/src/check/context.rs b/src/check/context.rs index c41c178..36bb8f1 100644 --- a/src/check/context.rs +++ b/src/check/context.rs @@ -8,8 +8,11 @@ use std::{ path::{Path, PathBuf}, }; -use crate::hir::{self, Hir}; use crate::{check::Violation, scaffold::emitter::Emitter}; +use crate::{ + config::Config, + hir::{self, Hir}, +}; use super::{location::Location, violation::ViolationKind}; @@ -35,6 +38,8 @@ pub(crate) struct Context { pub(crate) pt: SourceUnit, /// The comments present in the Solidity file. pub(crate) comments: Comments, + /// The config passed to `bulloak check`. + pub(crate) cfg: Config, } impl Context { @@ -42,7 +47,7 @@ impl Context { /// /// This structure contains everything necessary to perform checks between /// trees and Solidity files. - pub(crate) fn new(tree: PathBuf) -> Result { + pub(crate) fn new(tree: PathBuf, cfg: Config) -> Result { let tree_path_cow = tree.to_string_lossy(); let tree_contents = try_read_to_string(&tree)?; let hir = crate::hir::translate(&tree_contents, &Default::default()).map_err(|e| { @@ -71,6 +76,7 @@ impl Context { src, pt, comments, + cfg: cfg.clone(), }) } diff --git a/src/check/mod.rs b/src/check/mod.rs index a943266..93fbdac 100644 --- a/src/check/mod.rs +++ b/src/check/mod.rs @@ -40,6 +40,9 @@ pub struct Check { /// to standard output instead of writing to files. #[arg(long, requires = "fix-violations", default_value_t = false)] stdout: bool, + /// Whether to emit modifiers. + #[arg(short = 'm', long, default_value_t = false)] + pub skip_modifiers: bool, } impl Default for Check { @@ -52,13 +55,13 @@ impl Check { /// Entrypoint for `bulloak check`. /// /// Note that we don't deal with `solang_parser` errors at all. - pub(crate) fn run(&self, _cfg: &Config) -> anyhow::Result<()> { + pub(crate) fn run(&self, cfg: &Config) -> anyhow::Result<()> { let mut violations = Vec::new(); let ctxs: Vec = self .files .iter() .filter_map(|tree_path| { - Context::new(tree_path.clone()) + Context::new(tree_path.clone(), cfg.clone()) .map_err(|violation| violations.push(violation)) .ok() }) diff --git a/src/check/rules/structural_match.rs b/src/check/rules/structural_match.rs index eb35d0a..76d9069 100644 --- a/src/check/rules/structural_match.rs +++ b/src/check/rules/structural_match.rs @@ -129,13 +129,17 @@ fn check_fns_structure( Some((sol_idx, _)) => present_fn_indices.push((hir_idx, sol_idx)), // We didn't find a matching function, so this is a // violation. - None => violations.push(Violation::new( - ViolationKind::MatchingFunctionMissing(fn_hir.clone(), hir_idx), - Location::Code( - ctx.tree.to_string_lossy().into_owned(), - fn_hir.span.start.line, - ), - )), + None => { + if !ctx.cfg.check().skip_modifiers { + violations.push(Violation::new( + ViolationKind::MatchingFunctionMissing(fn_hir.clone(), hir_idx), + Location::Code( + ctx.tree.to_string_lossy().into_owned(), + fn_hir.span.start.line, + ), + )) + } + } } }; } diff --git a/src/config.rs b/src/config.rs index 3aefa0f..1331445 100644 --- a/src/config.rs +++ b/src/config.rs @@ -20,15 +20,24 @@ impl Config { } } + pub(crate) fn check(&self) -> crate::check::Check { + match self.command.clone() { + Commands::Check(c) => c, + Commands::Scaffold(_) => Default::default(), + } + } + /// Return a cloned `Config` instance with `with_vm_skip` set to the passed /// value. - #[must_use] pub fn with_vm_skip(&self, with_vm_skip: bool) -> Config { + #[must_use] + pub fn with_vm_skip(&self, with_vm_skip: bool) -> Config { if let Commands::Scaffold(ref s) = self.command { return Config { command: Commands::Scaffold(crate::scaffold::Scaffold { with_vm_skip, ..s.clone() }), + ..Config::default() }; } diff --git a/src/hir/combiner.rs b/src/hir/combiner.rs index a33eb5a..cb70b40 100644 --- a/src/hir/combiner.rs +++ b/src/hir/combiner.rs @@ -406,7 +406,6 @@ bulloak error: contract name missing at tree root #2"; // Append a comment HIR to the hirs. hirs.push(root(vec![comment("this is a random comment".to_owned())])); - println!("{:#?}", hirs); let text = trees.join("\n\n"); let children = match combine(&text, hirs).unwrap() { Hir::Root(root) => root.children, diff --git a/src/hir/mod.rs b/src/hir/mod.rs index fa897c0..dc0c15e 100644 --- a/src/hir/mod.rs +++ b/src/hir/mod.rs @@ -9,7 +9,9 @@ pub mod visitor; pub use hir::*; -use crate::{config::Config, scaffold::modifiers::ModifierDiscoverer, utils::split_trees}; +use crate::{ + config::Config, error, scaffold::modifiers::ModifierDiscoverer, syntax, utils::split_trees, +}; /// High-level function that returns a HIR given the contents of a `.tree` file. /// @@ -19,25 +21,25 @@ pub fn translate(text: &str, cfg: &Config) -> anyhow::Result { Ok(translate_and_combine_trees(text, cfg)?) } -/// Generates the HIR for a single tree. -/// -/// This function leverages [`crate::syntax::parse`] and [`crate::hir::translator::Translator::translate`] -/// to hide away most of the complexity of `bulloak`'s internal compiler. -pub fn translate_tree_to_hir(tree: &str, cfg: &Config) -> crate::error::Result { - let ast = crate::syntax::parse(tree)?; - let mut discoverer = ModifierDiscoverer::new(); - let modifiers = discoverer.discover(&ast); - Ok(translator::Translator::new().translate(&ast, modifiers, cfg)) -} - /// High-level function that returns a HIR given the contents of a `.tree` file. /// /// This function leverages [`translate_tree_to_hir`] to generate the HIR for each tree, /// and [`crate::hir::combiner::Combiner::combine`] to combine the HIRs into a single HIR. -pub(crate) fn translate_and_combine_trees(text: &str, cfg: &Config) -> crate::error::Result { +pub(crate) fn translate_and_combine_trees(text: &str, cfg: &Config) -> error::Result { let trees = split_trees(text); let hirs = trees .map(|tree| translate_tree_to_hir(tree, cfg)) - .collect::>>()?; + .collect::>>()?; Ok(combiner::Combiner::new().combine(text, hirs)?) } + +/// Generates the HIR for a single tree. +/// +/// This function leverages [`crate::syntax::parse`] and [`crate::hir::translator::Translator::translate`] +/// to hide away most of the complexity of `bulloak`'s internal compiler. +pub fn translate_tree_to_hir(tree: &str, cfg: &Config) -> error::Result { + let ast = syntax::parse(tree)?; + let mut discoverer = ModifierDiscoverer::new(); + let modifiers = discoverer.discover(&ast); + Ok(translator::Translator::new().translate(&ast, modifiers, cfg)) +} diff --git a/src/scaffold/mod.rs b/src/scaffold/mod.rs index 6a9646a..8b2b500 100644 --- a/src/scaffold/mod.rs +++ b/src/scaffold/mod.rs @@ -45,6 +45,9 @@ pub struct Scaffold { /// Whether to add vm.skip(true) at the begining of each test. #[arg(short = 'S', long = "vm-skip", default_value_t = false)] pub with_vm_skip: bool, + /// Whether to emit modifiers. + #[arg(short = 'm', long, default_value_t = false)] + pub skip_modifiers: bool, } impl Default for Scaffold { diff --git a/src/sol/translator.rs b/src/sol/translator.rs index 9b6dec8..8d3a7b5 100644 --- a/src/sol/translator.rs +++ b/src/sol/translator.rs @@ -38,17 +38,21 @@ pub(crate) struct Translator { sol_version: String, /// A flag indicating if there is a forge-std dependency. with_forge_std: bool, + /// Whether to emit modifiers. + skip_modifiers: bool, } impl Translator { /// Create a new translator. #[must_use] pub(crate) fn new(cfg: &Config) -> Self { - let cfg = cfg.scaffold(); - let with_forge_std = [cfg.with_vm_skip].into_iter().any(|f| f); + let scaffold_cfg = cfg.scaffold(); + let with_forge_std = [scaffold_cfg.with_vm_skip].into_iter().any(|f| f); + Self { - sol_version: cfg.solidity_version, + sol_version: scaffold_cfg.solidity_version, with_forge_std, + skip_modifiers: scaffold_cfg.skip_modifiers, } } @@ -441,6 +445,9 @@ impl Visitor for TranslatorI { let mut parts = Vec::with_capacity(contract.children.len()); for child in &contract.children { if let Hir::FunctionDefinition(function) = child { + if function.is_modifier() && self.translator.skip_modifiers { + continue; + } parts.push(self.visit_function(function)?); } } diff --git a/tests/check.rs b/tests/check.rs index 8a7baa0..8e3b0a8 100644 --- a/tests/check.rs +++ b/tests/check.rs @@ -51,6 +51,20 @@ fn checks_valid_structural_match() { assert!(stdout.contains("All checks completed successfully! No issues found.")); } +#[test] +fn checks_modifiers_skipped() { + let cwd = env::current_dir().unwrap(); + let binary_path = get_binary_path(); + let tree_path = cwd.join("tests").join("check").join("skip_modifiers.tree"); + + let output = cmd(&binary_path, "check", &tree_path, &["-m"]); + let stderr = String::from_utf8(output.stderr).unwrap(); + let stdout = String::from_utf8(output.stdout).unwrap(); + + assert_eq!("", stderr); + assert!(stdout.contains("All checks completed successfully! No issues found.")); +} + #[test] fn checks_missing_sol_file() { let cwd = env::current_dir().unwrap(); @@ -241,7 +255,6 @@ fn fixes_extra_fn_plus_wrong_order() { let output = cmd(&binary_path, "check", &tree_path, &["--fix", "--stdout"]); let actual = String::from_utf8(output.stdout).unwrap(); - println!("what {}", actual); let expected = r"// SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.0; diff --git a/tests/check/skip_modifiers.t.sol b/tests/check/skip_modifiers.t.sol new file mode 100644 index 0000000..201723f --- /dev/null +++ b/tests/check/skip_modifiers.t.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.0; + +contract HashPairTestSanitize { + function test_ShouldNeverRevert() external { + // It should never revert. + } + + function test_WhenFirstArgIsSmallerThanSecondArg() external whenFirstArgIsSmallerThanSecondArg { + // It should match the result of `keccak256(abi.encodePacked(a,b))`. + } + + function test_WhenFirstArgIsZero() external whenFirstArgIsSmallerThanSecondArg { + // It should do something. + } + + function test_WhenFirstArgIsBiggerThanSecondArg() external { + // It should match the result of `keccak256(abi.encodePacked(b,a))`. + } +} diff --git a/tests/check/skip_modifiers.tree b/tests/check/skip_modifiers.tree new file mode 100644 index 0000000..47bee27 --- /dev/null +++ b/tests/check/skip_modifiers.tree @@ -0,0 +1,8 @@ +HashPairTest.Sanitize +├── It should never revert. +├── When first arg is smaller than second arg +│ ├── When first arg is zero +│ │ └── It should do something. +│ └── It should match the result of `keccak256(abi.encodePacked(a,b))`. +└── When first arg is bigger than second arg + └── It should match the result of `keccak256(abi.encodePacked(b,a))`. diff --git a/tests/scaffold.rs b/tests/scaffold.rs index 893ab70..4620ddf 100644 --- a/tests/scaffold.rs +++ b/tests/scaffold.rs @@ -69,6 +69,27 @@ fn scaffolds_trees_with_vm_skip() { } } +#[test] +fn scaffolds_trees_with_skip_modifiers() { + let cwd = env::current_dir().unwrap(); + let binary_path = get_binary_path(); + let tests_path = cwd.join("tests").join("scaffold"); + let trees = ["skip_modifiers.tree"]; + + for tree_name in trees { + let tree_path = tests_path.join(tree_name); + let output = cmd(&binary_path, "scaffold", &tree_path, &["-m"]); + let actual = String::from_utf8(output.stdout).unwrap(); + + let mut output_file = tree_path.clone(); + output_file.set_extension("t.sol"); + let expected = fs::read_to_string(output_file).unwrap(); + + // We trim here because we don't care about ending newlines. + assert_eq!(expected.trim(), actual.trim()); + } +} + #[test] fn skips_trees_when_file_exists() { let cwd = env::current_dir().unwrap(); diff --git a/tests/scaffold/skip_modifiers.t.sol b/tests/scaffold/skip_modifiers.t.sol new file mode 100644 index 0000000..201723f --- /dev/null +++ b/tests/scaffold/skip_modifiers.t.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.0; + +contract HashPairTestSanitize { + function test_ShouldNeverRevert() external { + // It should never revert. + } + + function test_WhenFirstArgIsSmallerThanSecondArg() external whenFirstArgIsSmallerThanSecondArg { + // It should match the result of `keccak256(abi.encodePacked(a,b))`. + } + + function test_WhenFirstArgIsZero() external whenFirstArgIsSmallerThanSecondArg { + // It should do something. + } + + function test_WhenFirstArgIsBiggerThanSecondArg() external { + // It should match the result of `keccak256(abi.encodePacked(b,a))`. + } +} diff --git a/tests/scaffold/skip_modifiers.tree b/tests/scaffold/skip_modifiers.tree new file mode 100644 index 0000000..47bee27 --- /dev/null +++ b/tests/scaffold/skip_modifiers.tree @@ -0,0 +1,8 @@ +HashPairTest.Sanitize +├── It should never revert. +├── When first arg is smaller than second arg +│ ├── When first arg is zero +│ │ └── It should do something. +│ └── It should match the result of `keccak256(abi.encodePacked(a,b))`. +└── When first arg is bigger than second arg + └── It should match the result of `keccak256(abi.encodePacked(b,a))`.