Skip to content

Commit

Permalink
feat: add --skip-modifiers flag
Browse files Browse the repository at this point in the history
  • Loading branch information
alexfertel committed Jun 9, 2024
1 parent 3991e37 commit 1d83b51
Show file tree
Hide file tree
Showing 15 changed files with 162 additions and 32 deletions.
9 changes: 8 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
10 changes: 8 additions & 2 deletions src/check/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -35,14 +38,16 @@ 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 {
/// Creates a new `Context`.
///
/// This structure contains everything necessary to perform checks between
/// trees and Solidity files.
pub(crate) fn new(tree: PathBuf) -> Result<Self, Violation> {
pub(crate) fn new(tree: PathBuf, cfg: Config) -> Result<Self, Violation> {

Check warning on line 50 in src/check/context.rs

View workflow job for this annotation

GitHub Actions / clippy

this argument is passed by value, but not consumed in the function body

warning: this argument is passed by value, but not consumed in the function body --> src/check/context.rs:50:43 | 50 | pub(crate) fn new(tree: PathBuf, cfg: Config) -> Result<Self, Violation> { | ^^^^^^ help: consider taking a reference instead: `&Config` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value note: the lint level is defined here --> src/lib.rs:3:22 | 3 | #![warn(clippy::all, clippy::pedantic, clippy::cargo)] | ^^^^^^^^^^^^^^^^ = note: `#[warn(clippy::needless_pass_by_value)]` implied by `#[warn(clippy::pedantic)]`
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| {

Check warning on line 53 in src/check/context.rs

View workflow job for this annotation

GitHub Actions / clippy

calling `Config::default()` is more clear than this expression

warning: calling `Config::default()` is more clear than this expression --> src/check/context.rs:53:58 | 53 | let hir = crate::hir::translate(&tree_contents, &Default::default()).map_err(|e| { | ^^^^^^^^^^^^^^^^^^ help: try: `Config::default()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#default_trait_access = note: `#[warn(clippy::default_trait_access)]` implied by `#[warn(clippy::pedantic)]`
Expand Down Expand Up @@ -71,6 +76,7 @@ impl Context {
src,
pt,
comments,
cfg: cfg.clone(),
})
}

Expand Down
7 changes: 5 additions & 2 deletions src/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Check warning on line 45 in src/check/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/check/mod.rs#L45

Added line #L45 was not covered by tests
}

impl Default for Check {
Expand All @@ -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<Context> = 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()
})
Expand Down
18 changes: 11 additions & 7 deletions src/check/rules/structural_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
))

Check warning on line 140 in src/check/rules/structural_match.rs

View workflow job for this annotation

GitHub Actions / clippy

consider adding a `;` to the last statement for consistent formatting

warning: consider adding a `;` to the last statement for consistent formatting --> src/check/rules/structural_match.rs:134:25 | 134 | / violations.push(Violation::new( 135 | | ViolationKind::MatchingFunctionMissing(fn_hir.clone(), hir_idx), 136 | | Location::Code( 137 | | ctx.tree.to_string_lossy().into_owned(), 138 | | fn_hir.span.start.line, 139 | | ), 140 | | )) | |__________________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_if_nothing_returned = note: `#[warn(clippy::semicolon_if_nothing_returned)]` implied by `#[warn(clippy::pedantic)]` help: add a `;` here | 134 ~ violations.push(Violation::new( 135 + ViolationKind::MatchingFunctionMissing(fn_hir.clone(), hir_idx), 136 + Location::Code( 137 + ctx.tree.to_string_lossy().into_owned(), 138 + fn_hir.span.start.line, 139 + ), 140 + )); |
}
}
}
};
}
Expand Down
11 changes: 10 additions & 1 deletion src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),

Check warning on line 26 in src/config.rs

View workflow job for this annotation

GitHub Actions / clippy

calling `Check::default()` is more clear than this expression

warning: calling `Check::default()` is more clear than this expression --> src/config.rs:26:38 | 26 | Commands::Scaffold(_) => Default::default(), | ^^^^^^^^^^^^^^^^^^ help: try: `Check::default()` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#default_trait_access

Check warning on line 26 in src/config.rs

View check run for this annotation

Codecov / codecov/patch

src/config.rs#L26

Added line #L26 was not covered by tests
}
}

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

Check warning on line 40 in src/config.rs

View workflow job for this annotation

GitHub Actions / clippy

struct update has no effect, all the fields in the struct have already been specified

warning: struct update has no effect, all the fields in the struct have already been specified --> src/config.rs:40:19 | 40 | ..Config::default() | ^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_update = note: `#[warn(clippy::needless_update)]` implied by `#[warn(clippy::all)]`
};
}

Expand Down
1 change: 0 additions & 1 deletion src/hir/combiner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
30 changes: 16 additions & 14 deletions src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand All @@ -19,25 +21,25 @@ pub fn translate(text: &str, cfg: &Config) -> anyhow::Result<Hir> {
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<Hir> {
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<Hir> {
pub(crate) fn translate_and_combine_trees(text: &str, cfg: &Config) -> error::Result<Hir> {
let trees = split_trees(text);
let hirs = trees
.map(|tree| translate_tree_to_hir(tree, cfg))
.collect::<crate::error::Result<Vec<Hir>>>()?;
.collect::<error::Result<Vec<Hir>>>()?;
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<Hir> {

Check warning on line 40 in src/hir/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

item name ends with its containing module's name

warning: item name ends with its containing module's name --> src/hir/mod.rs:40:8 | 40 | pub fn translate_tree_to_hir(tree: &str, cfg: &Config) -> error::Result<Hir> { | ^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_name_repetitions = note: `#[warn(clippy::module_name_repetitions)]` implied by `#[warn(clippy::pedantic)]`

Check warning on line 40 in src/hir/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

docs for function returning `Result` missing `# Errors` section

warning: docs for function returning `Result` missing `# Errors` section --> src/hir/mod.rs:40:1 | 40 | pub fn translate_tree_to_hir(tree: &str, cfg: &Config) -> error::Result<Hir> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_errors_doc
let ast = syntax::parse(tree)?;
let mut discoverer = ModifierDiscoverer::new();
let modifiers = discoverer.discover(&ast);
Ok(translator::Translator::new().translate(&ast, modifiers, cfg))
}
3 changes: 3 additions & 0 deletions src/scaffold/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Check warning on line 50 in src/scaffold/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/scaffold/mod.rs#L50

Added line #L50 was not covered by tests
}

Check warning on line 51 in src/scaffold/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

more than 3 bools in a struct

warning: more than 3 bools in a struct --> src/scaffold/mod.rs:24:1 | 24 | / pub struct Scaffold { 25 | | /// The set of tree files to generate from. 26 | | /// 27 | | /// Each Solidity file will be named after its matching ... | 50 | | pub skip_modifiers: bool, 51 | | } | |_^ | = help: consider using a state machine or refactoring bools into two-variant enums = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#struct_excessive_bools = note: `#[warn(clippy::struct_excessive_bools)]` implied by `#[warn(clippy::pedantic)]`

impl Default for Scaffold {
Expand Down
13 changes: 10 additions & 3 deletions src/sol/translator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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)?);
}
}
Expand Down
15 changes: 14 additions & 1 deletion tests/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 20 additions & 0 deletions tests/check/skip_modifiers.t.sol
Original file line number Diff line number Diff line change
@@ -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))`.
}
}
8 changes: 8 additions & 0 deletions tests/check/skip_modifiers.tree
Original file line number Diff line number Diff line change
@@ -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))`.
21 changes: 21 additions & 0 deletions tests/scaffold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
20 changes: 20 additions & 0 deletions tests/scaffold/skip_modifiers.t.sol
Original file line number Diff line number Diff line change
@@ -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))`.
}
}
8 changes: 8 additions & 0 deletions tests/scaffold/skip_modifiers.tree
Original file line number Diff line number Diff line change
@@ -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))`.

0 comments on commit 1d83b51

Please sign in to comment.