diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c959cd4..64e4d8a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,6 +29,7 @@ jobs: profile: minimal toolchain: stable override: true + - uses: foundry-rs/foundry-toolchain@v1 - run: cargo test --locked --all-features lint: diff --git a/Cargo.lock b/Cargo.lock index cba228c..8d89cfe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -44,7 +44,7 @@ version = "0.2.14" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d9b39be18770d11421cdb1b9947a45dd3f37e93092cbf377614828a319d5fee8" dependencies = [ - "hermit-abi", + "hermit-abi 0.1.19", "libc", "winapi", ] @@ -91,12 +91,55 @@ dependencies = [ "memchr", ] +[[package]] +name = "cc" +version = "1.0.79" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50d30906286121d95be3d479533b458f87493b30a4b5f79a607db8f5d11aa91f" + [[package]] name = "cfg-if" version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" +[[package]] +name = "clap" +version = "4.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec0b0588d44d4d63a87dbd75c136c166bbfd9a86a31cb89e09906521c7d3f5e3" +dependencies = [ + "bitflags", + "clap_derive", + "clap_lex", + "is-terminal", + "once_cell", + "strsim", + "termcolor", +] + +[[package]] +name = "clap_derive" +version = "4.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "684a277d672e91966334af371f1a7b5833f9aa00b07c84e92fbce95e00208ce8" +dependencies = [ + "heck", + "proc-macro-error", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "clap_lex" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "783fe232adfca04f90f56201b26d79682d4cd2625e0bc7290b95123afe558ade" +dependencies = [ + "os_str_bytes", +] + [[package]] name = "colored" version = "2.0.0" @@ -162,6 +205,27 @@ dependencies = [ "log", ] +[[package]] +name = "errno" +version = "0.2.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f639046355ee4f37944e44f60642c6f3a7efa3cf6b78c78a0d989a8ce6c396a1" +dependencies = [ + "errno-dragonfly", + "libc", + "winapi", +] + +[[package]] +name = "errno-dragonfly" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aa68f1b12764fab894d2755d2518754e71b4fd80ecfb822714a1206c2aab39bf" +dependencies = [ + "cc", + "libc", +] + [[package]] name = "fixedbitset" version = "0.4.2" @@ -204,6 +268,12 @@ version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" +[[package]] +name = "heck" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "95505c38b4572b2d910cecb0281560f54b440a19336cbbcb27bf6ce6adc6f5a8" + [[package]] name = "hermit-abi" version = "0.1.19" @@ -213,6 +283,12 @@ dependencies = [ "libc", ] +[[package]] +name = "hermit-abi" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fed44880c466736ef9a5c5b5facefb5ed0785676d0c02d612db14e54f0d84286" + [[package]] name = "indexmap" version = "1.9.1" @@ -223,6 +299,28 @@ dependencies = [ "hashbrown", ] +[[package]] +name = "io-lifetimes" +version = "1.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1abeb7a0dd0f8181267ff8adc397075586500b81b28a73e8a0208b00fc170fb3" +dependencies = [ + "libc", + "windows-sys 0.45.0", +] + +[[package]] +name = "is-terminal" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22e18b0a45d56fe973d6db23972bf5bc46f988a4a2385deac9cc29572f09daef" +dependencies = [ + "hermit-abi 0.3.1", + "io-lifetimes", + "rustix", + "windows-sys 0.45.0", +] + [[package]] name = "itertools" version = "0.10.5" @@ -282,6 +380,12 @@ version = "0.2.134" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "329c933548736bc49fd575ee68c89e8be4d260064184389a5b77517cddd99ffb" +[[package]] +name = "linux-raw-sys" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f051f77a7c8e6957c0696eac88f26b0117e54f52d3fc682ab19397a8812846a4" + [[package]] name = "lock_api" version = "0.4.9" @@ -360,6 +464,12 @@ version = "1.16.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "86f0b0d4bf799edbc74508c1e8bf170ff5f41238e5f8225603ca7caaae2b7860" +[[package]] +name = "os_str_bytes" +version = "6.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b7820b9daea5457c9f21c69448905d723fbd21136ccf521748f23fd49e723ee" + [[package]] name = "parking_lot" version = "0.12.1" @@ -380,7 +490,7 @@ dependencies = [ "libc", "redox_syscall", "smallvec", - "windows-sys", + "windows-sys 0.42.0", ] [[package]] @@ -461,6 +571,30 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "925383efa346730478fb4838dbe9137d2a47675ad789c546d150a6e1dd4ab31c" +[[package]] +name = "proc-macro-error" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" +dependencies = [ + "proc-macro-error-attr", + "proc-macro2", + "quote", + "syn", + "version_check", +] + +[[package]] +name = "proc-macro-error-attr" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" +dependencies = [ + "proc-macro2", + "quote", + "version_check", +] + [[package]] name = "proc-macro-hack" version = "0.5.19" @@ -571,6 +705,20 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" +[[package]] +name = "rustix" +version = "0.36.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f43abb88211988493c1abb44a70efa56ff0ce98f233b7b276146f1f3f7ba9644" +dependencies = [ + "bitflags", + "errno", + "io-lifetimes", + "libc", + "linux-raw-sys", + "windows-sys 0.45.0", +] + [[package]] name = "rustversion" version = "1.0.9" @@ -602,6 +750,7 @@ checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd" name = "scopelint" version = "0.0.14" dependencies = [ + "clap", "colored", "once_cell", "regex", @@ -679,6 +828,12 @@ dependencies = [ "precomputed-hash", ] +[[package]] +name = "strsim" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" + [[package]] name = "syn" version = "1.0.101" @@ -722,6 +877,15 @@ dependencies = [ "winapi", ] +[[package]] +name = "termcolor" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "be55cf8942feac5c765c2c993422806843c9a9a45d4d5c407ad6dd2ea95eb9b6" +dependencies = [ + "winapi-util", +] + [[package]] name = "text-size" version = "1.1.0" @@ -888,44 +1052,68 @@ dependencies = [ "windows_x86_64_msvc", ] +[[package]] +name = "windows-sys" +version = "0.45.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75283be5efb2831d37ea142365f009c02ec203cd29a3ebecbc093d52315b66d0" +dependencies = [ + "windows-targets", +] + +[[package]] +name = "windows-targets" +version = "0.42.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e2522491fbfcd58cc84d47aeb2958948c4b8982e9a2d8a2a35bbaed431390e7" +dependencies = [ + "windows_aarch64_gnullvm", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_gnullvm", + "windows_x86_64_msvc", +] + [[package]] name = "windows_aarch64_gnullvm" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41d2aa71f6f0cbe00ae5167d90ef3cfe66527d6f613ca78ac8024c3ccab9a19e" +checksum = "8c9864e83243fdec7fc9c5444389dcbbfd258f745e7853198f365e3c4968a608" [[package]] name = "windows_aarch64_msvc" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dd0f252f5a35cac83d6311b2e795981f5ee6e67eb1f9a7f64eb4500fbc4dcdb4" +checksum = "4c8b1b673ffc16c47a9ff48570a9d85e25d265735c503681332589af6253c6c7" [[package]] name = "windows_i686_gnu" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fbeae19f6716841636c28d695375df17562ca208b2b7d0dc47635a50ae6c5de7" +checksum = "de3887528ad530ba7bdbb1faa8275ec7a1155a45ffa57c37993960277145d640" [[package]] name = "windows_i686_msvc" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "84c12f65daa39dd2babe6e442988fc329d6243fdce47d7d2d155b8d874862246" +checksum = "bf4d1122317eddd6ff351aa852118a2418ad4214e6613a50e0191f7004372605" [[package]] name = "windows_x86_64_gnu" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf7b1b21b5362cbc318f686150e5bcea75ecedc74dd157d874d754a2ca44b0ed" +checksum = "c1040f221285e17ebccbc2591ffdc2d44ee1f9186324dd3e84e99ac68d699c45" [[package]] name = "windows_x86_64_gnullvm" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "09d525d2ba30eeb3297665bd434a54297e4170c7f1a44cad4ef58095b4cd2028" +checksum = "628bfdf232daa22b0d64fdb62b09fcc36bb01f05a3939e20ab73aaf9470d0463" [[package]] name = "windows_x86_64_msvc" -version = "0.42.0" +version = "0.42.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f40009d85759725a34da6d89a94e63d7bdc50a862acf0dbc7c8e488f1edcb6f5" +checksum = "447660ad36a13288b1db4d4248e857b510e8c3a225c822ba4fb748c0aafecffd" diff --git a/Cargo.toml b/Cargo.toml index 4bbe66d..2efde4e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ version = "0.0.14" [dependencies] + clap = { version = "4.1.6", features = ["derive"] } colored = "2.0.0" once_cell = "1.16.0" regex = "1.6.0" diff --git a/README.md b/README.md index b50c75d..ea37320 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # ScopeLint -A simple and opinionated tool designed to for basic formatting/linting of Solidity and TOML code in foundry projects. +A simple and opinionated tool designed for basic formatting/linting of Solidity and TOML code in foundry projects. ## Overview diff --git a/src/check/mod.rs b/src/check/mod.rs new file mode 100644 index 0000000..c5d13d7 --- /dev/null +++ b/src/check/mod.rs @@ -0,0 +1,79 @@ +use colored::Colorize; +use std::{error::Error, ffi::OsStr, fs}; +use walkdir::WalkDir; + +/// Contains all the types and methods to generate a report of all the invalid items found. +pub mod report; + +/// Contains helper methods, traits, etc. used by the validators and report generation. +pub mod utils; + +/// Contains all the validators to ensure Solidity files follow conventions and best practices. +pub mod validators; + +/// Validates the code formatting, and print details on any conventions that are not being followed. +/// # Errors +/// Returns an error if the formatting or convention validations fail. +pub fn run(taplo_opts: taplo::formatter::Options) -> Result<(), Box> { + // We run the formatting check separate to just indicate whether or not the user needs to format + // the codebase, whereas the other validators return granular information about what to fix + // since they currently can't be fixed automatically. + let valid_names = validate_conventions(); + let valid_fmt = validators::formatting::validate(taplo_opts); + + if valid_names.is_ok() && valid_fmt.is_ok() { + Ok(()) + } else { + Err("One or more checks failed, review above output".into()) + } +} + +// ============================= +// ======== Validations ======== +// ============================= + +fn validate_conventions() -> Result<(), Box> { + let paths = ["./src", "./script", "./test"]; + let results = validate(paths)?; + + if !results.is_valid() { + eprint!("{results}"); + eprintln!("{}: Convention checks failed, see details above", "error".bold().red()); + return Err("Invalid names found".into()) + } + Ok(()) +} + +// Core validation method that walks the directory and validates all Solidity files. +fn validate(paths: [&str; 3]) -> Result> { + let mut results = report::Report::default(); + + for path in paths { + for result in WalkDir::new(path) { + let dent = match result { + Ok(dent) => dent, + Err(err) => { + eprintln!("{err}"); + continue + } + }; + + if !dent.file_type().is_file() || dent.path().extension() != Some(OsStr::new("sol")) { + continue + } + + // Get the parse tree (pt) of the file. + let file = dent.path(); + let content = fs::read_to_string(file)?; + let (pt, _comments) = solang_parser::parse(&content, 0).expect("Parsing failed"); + + // Run all checks. + results.add_items(validators::test_names::validate(file, &content, &pt)); + results.add_items(validators::src_names_internal::validate(file, &content, &pt)); + results + .add_items(validators::script_one_pubic_run_method::validate(file, &content, &pt)); + results.add_items(validators::constant_names::validate(file, &content, &pt)); + } + } + Ok(results) +} diff --git a/src/check/report.rs b/src/check/report.rs new file mode 100644 index 0000000..9ea8d88 --- /dev/null +++ b/src/check/report.rs @@ -0,0 +1,34 @@ +use super::utils::InvalidItem; +use std::fmt; + +/// A collection of invalid items to generate a report from. +#[derive(Default)] +pub struct Report { + /// A list of invalid items. + invalid_items: Vec, +} + +impl fmt::Display for Report { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result { + let mut sorted_items = self.invalid_items.clone(); + sorted_items.sort(); + + for item in sorted_items { + writeln!(f, "{}", item.description())?; + } + Ok(()) + } +} + +impl Report { + /// Extends the report with a list of invalid items. + pub fn add_items(&mut self, items: Vec) { + self.invalid_items.extend(items); + } + + /// Returns true if no issues were found. + #[must_use] + pub fn is_valid(&self) -> bool { + self.invalid_items.is_empty() + } +} diff --git a/src/check/utils.rs b/src/check/utils.rs new file mode 100644 index 0000000..7c88692 --- /dev/null +++ b/src/check/utils.rs @@ -0,0 +1,222 @@ +// The `.extension()` method only looks after the last dot in the file name, so it will return +// Some("sol") for both "Foo.sol" and "Foo.t.sol". This is not what we want here, so we just check +// extensions manually with `ends_with`. +#![allow(clippy::case_sensitive_file_extension_comparisons)] + +use solang_parser::pt::{ + FunctionAttribute, FunctionDefinition, FunctionTy, SourceUnit, Visibility, +}; +use std::path::Path; + +// ======================================= +// ======== For validator methods ======== +// ===============================-======= + +/// The type of validator that found the invalid item. +#[derive(PartialEq, Eq, PartialOrd, Ord, Clone)] +pub enum ValidatorKind { + /// A constant or immutable variable. + Constant, + /// A script file. + Script, + /// A source contract. + Src, + /// A test contract. + Test, +} + +/// A single invalid item found by a validator. +#[derive(PartialEq, Eq, PartialOrd, Ord, Clone)] +pub struct InvalidItem { + kind: ValidatorKind, + file: String, // File name. + text: String, // Details to show about the invalid item. + line: usize, // Line number. +} + +impl InvalidItem { + #[must_use] + /// Creates a new `InvalidItem`. + pub const fn new(kind: ValidatorKind, file: String, text: String, line: usize) -> Self { + Self { kind, file, text, line } + } + + #[must_use] + /// Returns a string describing the invalid item, which is shown to the user so they can triage + /// findings. + pub fn description(&self) -> String { + match self.kind { + ValidatorKind::Test => { + format!("Invalid test name in {} on line {}: {}", self.file, self.line, self.text) + } + ValidatorKind::Constant => { + format!( + "Invalid constant or immutable name in {} on line {}: {}", + self.file, self.line, self.text + ) + } + ValidatorKind::Script => { + format!("Invalid script interface in {}: {}", self.file, self.text) + } + ValidatorKind::Src => { + format!( + "Invalid src method name in {} on line {}: {}", + self.file, self.line, self.text + ) + } + } + } +} + +/// Categories of file kinds found in forge projects. +/// Two additional file kinds are not included here: `ScriptHelpers` and `TestHelpers`. These are +/// not currently used in any checks so they are excluded for now. +pub enum FileKind { + /// Executable script files live in the `scripts` directory and end with `.s.sol`. + Script, + /// Core contracts live in the `src` directory and end with `.sol`. + Src, + /// Contracts with test methods live in the `test` directory and end with `.t.sol`. + Test, +} + +/// Provides a method to check if a file is of a given kind. +pub trait IsFileKind { + /// Returns `true` if the file is of the given kind, `false` otherwise. + fn is_file_kind(&self, kind: FileKind) -> bool; +} + +impl IsFileKind for Path { + fn is_file_kind(&self, kind: FileKind) -> bool { + let path = self.to_str().unwrap(); + match kind { + FileKind::Script => path.starts_with("./script") && path.ends_with(".s.sol"), + FileKind::Src => path.starts_with("./src") && path.ends_with(".sol"), + FileKind::Test => path.starts_with("./test") && path.ends_with(".t.sol"), + } + } +} + +/// Provides a method to return the name of a function. +pub trait Name { + /// Returns the name of the function for standard functions, or `constructor`, `fallback` or + /// `receive` for other function types. + fn name(&self) -> String; +} + +/// Provides methods to return visibility information about a function. +pub trait VisibilitySummary { + /// Returns `true` if the function is internal or private, `false` otherwise. + fn is_internal_or_private(&self) -> bool; + /// Returns `true` if the function is public or external, `false` otherwise. + fn is_public_or_external(&self) -> bool; +} + +impl Name for FunctionDefinition { + fn name(&self) -> String { + match self.ty { + FunctionTy::Constructor => "constructor".to_string(), + FunctionTy::Fallback => "fallback".to_string(), + FunctionTy::Receive => "receive".to_string(), + FunctionTy::Function | FunctionTy::Modifier => self.name.as_ref().unwrap().name.clone(), + } + } +} + +impl VisibilitySummary for FunctionDefinition { + fn is_internal_or_private(&self) -> bool { + self.attributes.iter().any(|a| match a { + FunctionAttribute::Visibility(v) => { + matches!(v, Visibility::Private(_) | Visibility::Internal(_)) + } + _ => false, + }) + } + + fn is_public_or_external(&self) -> bool { + self.attributes.iter().any(|a| match a { + FunctionAttribute::Visibility(v) => { + matches!(v, Visibility::Public(_) | Visibility::External(_)) + } + _ => false, + }) + } +} + +#[must_use] +/// Converts the start offset of a `Loc` to `(line, col)`. Modified from +pub fn offset_to_line(content: &str, start: usize) -> usize { + debug_assert!(content.len() > start); + + let mut line_counter = 1; // First line is `1`. + for (offset, c) in content.chars().enumerate() { + if c == '\n' { + line_counter += 1; + } + if offset > start { + return line_counter + } + } + + unreachable!("content.len() > start") +} + +// =========================== +// ======== For tests ======== +// =========================== + +#[derive(Default)] +/// Given the number of expected findings for each file kind, this struct makes it easy to assert +/// the true number of findings for each file kind by calling it's `assert_eq` method. +pub struct ExpectedFindings { + /// The number of expected findings for script helper contracts. + pub script_helper: usize, + /// The number of expected findings for script contracts. + pub script: usize, + /// The number of expected findings for source contracts. + pub src: usize, + /// The number of expected findings for test helper contracts. + pub test_helper: usize, + /// The number of expected findings for test contracts. + pub test: usize, +} + +type ValidatorFn = dyn Fn(&Path, &str, &SourceUnit) -> Vec; + +impl ExpectedFindings { + #[must_use] + /// Creates a new `ExpectedFindings` with the given number of expected findings for each file + /// kind. Use this when a validator applies to all file kinds. If a validator only applies to + /// certain file kinds, you should initialize it using the form + /// `ExpectedFindings { test: 3, ..ExpectedFindings::default() }`. + pub const fn new(expected_findings: usize) -> Self { + Self { + script_helper: expected_findings, + script: expected_findings, + src: expected_findings, + test_helper: expected_findings, + test: expected_findings, + } + } + + /// Asserts that the number of invalid items found by the validator is equal to the expected + /// number for the given content, for each file kind. + /// # Panics + /// In practice this should not panic unless one of validations fails. + pub fn assert_eq(&self, content: &str, validate: &ValidatorFn) { + let (pt, _comments) = solang_parser::parse(content, 0).expect("Parsing failed"); + + let invalid_items_script_helper = + validate(Path::new("./script/MyContract.sol"), content, &pt); + let invalid_items_script = validate(Path::new("./script/MyContract.s.sol"), content, &pt); + let invalid_items_src = validate(Path::new("./src/MyContract.sol"), content, &pt); + let invalid_items_test_helper = validate(Path::new("./test/MyContract.sol"), content, &pt); + let invalid_items_test = validate(Path::new("./test/MyContract.t.sol"), content, &pt); + + assert_eq!(invalid_items_script_helper.len(), self.script_helper); + assert_eq!(invalid_items_script.len(), self.script); + assert_eq!(invalid_items_src.len(), self.src); + assert_eq!(invalid_items_test_helper.len(), self.test_helper); + assert_eq!(invalid_items_test.len(), self.test); + } +} diff --git a/src/check/validators/_template.rs b/src/check/validators/_template.rs new file mode 100644 index 0000000..68587be --- /dev/null +++ b/src/check/validators/_template.rs @@ -0,0 +1,50 @@ +use crate::check::utils::{offset_to_line, InvalidItem, Validator}; +use solang_parser::pt::{ + ContractPart, SourceUnit, SourceUnitPart, VariableAttribute, VariableDefinition, +}; +use std::path::Path; + +const fn is_matching_file(_file: &Path) -> bool { + true // Update the matching condition, see helpers in `src/check/utils.rs`. +} + +#[must_use] +/// Validates that . +pub fn validate(file: &Path, content: &str, pt: &SourceUnit) -> Vec { + if !is_matching_file(file) { + return Vec::new() + } + + let mut invalid_items: Vec = Vec::new(); + // Only edit below here to add your own validation logic. + for element in &pt.0 { + match element { + _ => (), + } + } + invalid_items +} + +// Add any helper methods here. The `validate` method should be the only public method. + +#[cfg(test)] +mod tests { + use super::*; + use crate::check::utils::ExpectedFindings; + + #[test] + fn test_validate() { + let content = r#" + contract MyContract { + // Fill in one or more sample contracts. See `script_one_pubic_run_method.rs` for + // an example of testing more contracts. + } + "#; + + let expected_findings = ExpectedFindings::new(0); + expected_findings.assert_eq(content, &validate); + } + + // Add any other unit tests here. Tests are named `test_`. See + // `constant_names.rs` for an example of additional unit tests. +} diff --git a/src/check/validators/constant_names.rs b/src/check/validators/constant_names.rs new file mode 100644 index 0000000..f4517e6 --- /dev/null +++ b/src/check/validators/constant_names.rs @@ -0,0 +1,143 @@ +use crate::check::utils::{offset_to_line, InvalidItem, ValidatorKind}; +use once_cell::sync::Lazy; +use regex::Regex; +use solang_parser::pt::{ + ContractPart, SourceUnit, SourceUnitPart, VariableAttribute, VariableDefinition, +}; +use std::path::Path; + +// A regex matching valid constant names, see the `validate_constant_names_regex` test for examples. +static RE_VALID_CONSTANT_NAME: Lazy = + Lazy::new(|| Regex::new(r"^(?:[$_]*[A-Z0-9][$_]*){1,}$").unwrap()); + +const fn is_matching_file(_file: &Path) -> bool { + true +} + +#[must_use] +/// Validates that constant and immutable variable names are in `ALL_CAPS`. +pub fn validate(file: &Path, content: &str, pt: &SourceUnit) -> Vec { + if !is_matching_file(file) { + return Vec::new() + } + + let mut invalid_items: Vec = Vec::new(); + for element in &pt.0 { + match element { + SourceUnitPart::VariableDefinition(v) => { + if let Some(invalid_item) = validate_name(file, content, v) { + invalid_items.push(invalid_item); + } + } + SourceUnitPart::ContractDefinition(c) => { + for el in &c.parts { + if let ContractPart::VariableDefinition(v) = el { + if let Some(invalid_item) = validate_name(file, content, v) { + invalid_items.push(invalid_item); + } + } + } + } + _ => (), + } + } + invalid_items +} + +fn is_valid_constant_name(name: &str) -> bool { + RE_VALID_CONSTANT_NAME.is_match(name) +} + +fn validate_name(file: &Path, content: &str, v: &VariableDefinition) -> Option { + let is_constant = v + .attrs + .iter() + .any(|a| matches!(a, VariableAttribute::Constant(_) | VariableAttribute::Immutable(_))); + let name = &v.name.name; + + if is_constant && !is_valid_constant_name(name) { + Some(InvalidItem::new( + ValidatorKind::Constant, + file.display().to_string(), + name.clone(), + offset_to_line(content, v.loc.start()), + )) + } else { + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::check::utils::ExpectedFindings; + + #[test] + fn test_validate() { + let content = r#" + contract MyContract { + // These have the constant or immutable keyword and should be valid. + uint256 constant MAX_UINT256 = type(uint256).max; + address constant ETH_ADDRESS = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; + + // These have the constant/immutable keyword and should be invalid. + bytes32 immutable zeroBytes = 0; + int256 immutable minInt256 = type(int256).min; + + // These should all be valid since they are not constant or immutable. + address alice = address(123); + uint256 aliceBalance = 500; + } + "#; + + let expected_findings = ExpectedFindings::new(2); + expected_findings.assert_eq(content, &validate); + } + + #[test] + fn test_is_valid_constant_name() { + let allowed_names = vec![ + "MAX_UINT256", + "256_MAXUINT", + "256_MAX_11_UINT", + "VARIABLE", + "VARIABLE_NAME", + "VARIABLE_NAME_", + "VARIABLE___NAME", + "VARIABLE_NAME_WOW", + "VARIABLE_NAME_WOW_AS_MANY_UNDERSCORES_AS_YOU_WANT", + "__VARIABLE", + "_VARIABLE__NAME", + "_VARIABLE_NAME__", + "_VARIABLE_NAME_WOW", + "_VARIABLE_NAME_WOW_AS_MANY_UNDERSCORES_AS_YOU_WANT", + "$VARIABLE_NAME", + "_$VARIABLE_NAME_", + "$_VARIABLE_NAME$", + "_$VARIABLE_NAME$_", + "$_VARIABLE_NAME_$", + "$_VARIABLE__NAME_", + ]; + + let disallowed_names = [ + "variable", + "variableName", + "_variable", + "_variable_Name", + "VARIABLe", + "VARIABLE_name", + "_VARIABLe", + "_VARIABLE_name", + "$VARIABLe", + "$VARIABLE_name", + ]; + + for name in allowed_names { + assert_eq!(is_valid_constant_name(name), true, "{name}"); + } + + for name in disallowed_names { + assert_eq!(is_valid_constant_name(name), false, "{name}"); + } + } +} diff --git a/src/check/validators/formatting.rs b/src/check/validators/formatting.rs new file mode 100644 index 0000000..97c8ea9 --- /dev/null +++ b/src/check/validators/formatting.rs @@ -0,0 +1,29 @@ +use colored::Colorize; +use std::{error::Error, fs, process}; + +/// Validates that Solidity and TOML files are formatted correctly. +/// # Errors +/// Returns an error if formatting is invalid or parsing fails. +pub fn validate(taplo_opts: taplo::formatter::Options) -> Result<(), Box> { + // Check Solidity with `forge fmt`. + let forge_status = process::Command::new("forge").arg("fmt").arg("--check").output()?; + + // Print any warnings/errors from `forge fmt`. + let stderr = String::from_utf8(forge_status.stderr)?; + let forge_ok = forge_status.status.success() && stderr.is_empty(); + print!("{stderr}"); // Prints nothing if stderr is empty. + + // Check TOML with `taplo fmt` + let config_orig = fs::read_to_string("./foundry.toml")?; + let config_fmt = taplo::formatter::format(&config_orig, taplo_opts); + let taplo_ok = config_orig == config_fmt; + + if !forge_ok || !taplo_ok { + eprintln!( + "{}: Formatting validation failed, run `scopelint fmt` to fix", + "error".bold().red() + ); + return Err("Invalid fmt found".into()) + } + Ok(()) +} diff --git a/src/check/validators/mod.rs b/src/check/validators/mod.rs new file mode 100644 index 0000000..4fed59d --- /dev/null +++ b/src/check/validators/mod.rs @@ -0,0 +1,14 @@ +/// Validates that Solidity and TOML files are formatted correctly. +pub mod formatting; + +/// Validates that constant and immutable variable names are in `ALL_CAPS`. +pub mod constant_names; + +/// Validates that a script has a single public method named `run`. +pub mod script_one_pubic_run_method; + +/// Validates that internal and private function names are prefixed with an underscore. +pub mod src_names_internal; + +/// Validates that test names are in the correct format. +pub mod test_names; diff --git a/src/check/validators/script_one_pubic_run_method.rs b/src/check/validators/script_one_pubic_run_method.rs new file mode 100644 index 0000000..6e5c1a8 --- /dev/null +++ b/src/check/validators/script_one_pubic_run_method.rs @@ -0,0 +1,122 @@ +use crate::check::utils::{ + FileKind, InvalidItem, IsFileKind, Name, ValidatorKind, VisibilitySummary, +}; +use solang_parser::pt::{ContractPart, SourceUnit, SourceUnitPart}; +use std::path::Path; + +fn is_matching_file(file: &Path) -> bool { + file.is_file_kind(FileKind::Script) +} + +#[must_use] +/// Validates that a script has a single public method named `run`. +pub fn validate(file: &Path, _content: &str, pt: &SourceUnit) -> Vec { + if !is_matching_file(file) { + return Vec::new() + } + + let mut public_methods: Vec = Vec::new(); + for element in &pt.0 { + if let SourceUnitPart::ContractDefinition(c) = element { + for el in &c.parts { + if let ContractPart::FunctionDefinition(f) = el { + let name = f.name(); + if f.is_public_or_external() && name != "setUp" && name != "constructor" { + public_methods.push(name); + } + } + } + } + } + + // Parse the public methods found to return a vec that's either empty if valid, or has a single + // invalid item otherwise. + match public_methods.len() { + 0 => { + vec![InvalidItem::new( + ValidatorKind::Script, + file.display().to_string(), + "No `run` method found".to_string(), + 0, // This spans multiple lines, so we don't have a line number. + )] + } + 1 => { + if public_methods[0] == "run" { + Vec::new() + } else { + vec![InvalidItem::new( + ValidatorKind::Script, + file.display().to_string(), + "The only public method must be named `run`".to_string(), + 0, + )] + } + } + _ => { + vec![InvalidItem::new( + ValidatorKind::Script, + file.display().to_string(), + format!("Scripts must have a single public method named `run` (excluding `setUp`), but the following methods were found: {public_methods:?}"), + 0, + )] + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::check::utils::ExpectedFindings; + + #[test] + fn test_validate() { + // TODO add another test for the third match arm + let content_good = r#" + contract MyContract { + function run() public {} + } + "#; + + // The number after `bad` on the variable name indicates the match arm covered. + let content_bad0 = r#" + contract MyContract {} + "#; + + let content_bad1 = r#" + contract MyContract { + function notRun() public {} + } + "#; + + let content_bad2_variant0 = r#" + contract MyContract { + function run() public {} + function run(string memory config) public {} + } + "#; + + let content_bad2_variant1 = r#" + contract MyContract { + function run() public {} + function foo() public {} + } + "#; + + let content_bad2_variant2 = r#" + contract MyContract { + function foo() public {} + function bar() public {} + } + "#; + + let expected_findings_good = ExpectedFindings::new(0); + expected_findings_good.assert_eq(content_good, &validate); + + let expected_findings_bad = ExpectedFindings { script: 1, ..Default::default() }; + expected_findings_bad.assert_eq(content_bad0, &validate); + expected_findings_bad.assert_eq(content_bad1, &validate); + expected_findings_bad.assert_eq(content_bad2_variant0, &validate); + expected_findings_bad.assert_eq(content_bad2_variant1, &validate); + expected_findings_bad.assert_eq(content_bad2_variant2, &validate); + } +} diff --git a/src/check/validators/src_names_internal.rs b/src/check/validators/src_names_internal.rs new file mode 100644 index 0000000..4e36fea --- /dev/null +++ b/src/check/validators/src_names_internal.rs @@ -0,0 +1,85 @@ +use crate::check::utils::{ + offset_to_line, FileKind, InvalidItem, IsFileKind, Name, ValidatorKind, VisibilitySummary, +}; +use solang_parser::pt::{ContractPart, FunctionDefinition, SourceUnit, SourceUnitPart}; +use std::path::Path; + +fn is_matching_file(file: &Path) -> bool { + file.is_file_kind(FileKind::Src) +} + +#[must_use] +/// Validates that internal and private function names are prefixed with an underscore. +pub fn validate(file: &Path, content: &str, pt: &SourceUnit) -> Vec { + if !is_matching_file(file) { + return Vec::new() + } + + let mut invalid_items: Vec = Vec::new(); + for element in &pt.0 { + match element { + SourceUnitPart::FunctionDefinition(f) => { + if let Some(invalid_item) = validate_name(file, content, f) { + invalid_items.push(invalid_item); + } + } + SourceUnitPart::ContractDefinition(c) => { + for el in &c.parts { + if let ContractPart::FunctionDefinition(f) = el { + if let Some(invalid_item) = validate_name(file, content, f) { + invalid_items.push(invalid_item); + } + } + } + } + _ => (), + } + } + invalid_items +} + +fn is_valid_internal_or_private_name(name: &str) -> bool { + name.starts_with('_') +} + +fn validate_name(file: &Path, content: &str, f: &FunctionDefinition) -> Option { + let name = f.name(); + if f.is_internal_or_private() && !is_valid_internal_or_private_name(&name) { + Some(InvalidItem::new( + ValidatorKind::Src, + file.display().to_string(), + name, + offset_to_line(content, f.loc.start()), + )) + } else { + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::check::utils::ExpectedFindings; + + #[test] + fn test_validate() { + let content = r#" + contract MyContract { + // Valid names for internal or private src methods. + function _myInternalMethod() internal {} + function _myPrivateMethod() private {} + + // Invalid names for internal or private src methods. + function myInternalMethod() internal {} + function myPrivateMethod() private {} + + // These should be ignored since they are public and external. + function myPublicMethod() public {} + function myExternalMethod() external {} + } + "#; + + let expected_findings = ExpectedFindings { src: 2, ..ExpectedFindings::default() }; + expected_findings.assert_eq(content, &validate); + } +} diff --git a/src/check/validators/test_names.rs b/src/check/validators/test_names.rs new file mode 100644 index 0000000..0eff128 --- /dev/null +++ b/src/check/validators/test_names.rs @@ -0,0 +1,153 @@ +use crate::check::utils::{ + offset_to_line, FileKind, InvalidItem, IsFileKind, Name, ValidatorKind, VisibilitySummary, +}; +use once_cell::sync::Lazy; +use regex::Regex; +use solang_parser::pt::{ContractPart, FunctionDefinition, SourceUnit, SourceUnitPart}; +use std::path::Path; + +// A regex matching valid test names, see the `validate_test_names_regex` test for examples. +static RE_VALID_TEST_NAME: Lazy = + Lazy::new(|| Regex::new(r"^test(Fork)?(Fuzz)?(_Revert(If|When|On))?_(\w+)*$").unwrap()); + +fn is_matching_file(file: &Path) -> bool { + file.is_file_kind(FileKind::Test) +} + +#[must_use] +/// Validates that test names are in the correct format. +pub fn validate(file: &Path, content: &str, pt: &SourceUnit) -> Vec { + if !is_matching_file(file) { + return Vec::new() + } + + let mut invalid_items: Vec = Vec::new(); + for element in &pt.0 { + match element { + SourceUnitPart::FunctionDefinition(f) => { + if let Some(invalid_item) = validate_name(file, content, f) { + invalid_items.push(invalid_item); + } + } + SourceUnitPart::ContractDefinition(c) => { + for el in &c.parts { + if let ContractPart::FunctionDefinition(f) = el { + if let Some(invalid_item) = validate_name(file, content, f) { + invalid_items.push(invalid_item); + } + } + } + } + _ => (), + } + } + invalid_items +} + +fn is_valid_test_name(name: &str) -> bool { + name.starts_with("test") && RE_VALID_TEST_NAME.is_match(name) +} + +fn is_test_function(f: &FunctionDefinition) -> bool { + f.is_public_or_external() && f.name().starts_with("test") +} + +fn validate_name(file: &Path, content: &str, f: &FunctionDefinition) -> Option { + let name = f.name(); + if is_test_function(f) && !is_valid_test_name(&name) { + Some(InvalidItem::new( + ValidatorKind::Test, + file.display().to_string(), + name, + offset_to_line(content, f.loc.start()), + )) + } else { + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::check::utils::ExpectedFindings; + + #[test] + fn test_validate() { + let content = r#" + contract MyContract { + // Good test names. + function test_Description() public {} + function test_Increment() public {} + function testFuzz_Description() external {} + function testFork_Description() external {} + + // Bad test names. + function test() public {} + function testDescription() public {} + function testDescriptionMoreInfo() external {} + + // Things that are not tests and should be ignored. + function test() internal {} + function testDescription() internal {} + function testDescriptionMoreInfo() private {} + + function _test() public {} + function _testDescription() public {} + function _testDescriptionMoreInfo() public {} + } + "#; + + let expected_findings = ExpectedFindings { test: 3, ..ExpectedFindings::default() }; + expected_findings.assert_eq(content, &validate); + } + + #[test] + fn test_is_valid_test_name() { + let allowed_names = vec![ + "test_Description", + "test_Increment", + "testFuzz_Description", + "testFork_Description", + "testForkFuzz_Description", + "testForkFuzz_Description_MoreInfo", + "test_RevertIf_Condition", + "test_RevertWhen_Condition", + "test_RevertOn_Condition", + "test_RevertOn_Condition_MoreInfo", + "testFuzz_RevertIf_Condition", + "testFuzz_RevertWhen_Condition", + "testFuzz_RevertOn_Condition", + "testFuzz_RevertOn_Condition_MoreInfo", + "testForkFuzz_RevertIf_Condition", + "testForkFuzz_RevertWhen_Condition", + "testForkFuzz_RevertOn_Condition", + "testForkFuzz_RevertOn_Condition_MoreInfo", + "testForkFuzz_RevertOn_Condition_MoreInfo_Wow", + "testForkFuzz_RevertOn_Condition_MoreInfo_Wow_As_Many_Underscores_As_You_Want", + ]; + + let disallowed_names = [ + "test", + "testDescription", + "testDescriptionMoreInfo", + // TODO The below are tough to prevent without regex look-ahead support. + // "test_RevertIfCondition", + // "test_RevertWhenCondition", + // "test_RevertOnCondition", + // "testFuzz_RevertIfDescription", + // "testFuzz_RevertWhenDescription", + // "testFuzz_RevertOnDescription", + // "testForkFuzz_RevertIfCondition", + // "testForkFuzz_RevertWhenCondition", + // "testForkFuzz_RevertOnCondition", + ]; + + for name in allowed_names { + assert_eq!(is_valid_test_name(name), true, "{name}"); + } + + for name in disallowed_names { + assert_eq!(is_valid_test_name(name), false, "{name}"); + } + } +} diff --git a/src/config.rs b/src/config.rs index 048767c..733dd7e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,33 +1,21 @@ -/// Available modes to run the program in. -pub enum Mode { - /// Formats the code. - Format, - /// Checks for conventions not being follows. - Check, - /// Prints the version. - Version, -} +use clap::{Parser, Subcommand}; -/// Program configuration. Valid modes are `fmt`, `check`, and `--version`. -pub struct Config { - /// The mode to run the program in. - pub mode: Mode, +#[derive(Debug, Parser)] +#[clap(version, about, after_help = "Learn more: https://github.com/ScopeLift/scopelint")] +/// Options for running scopelint. +pub struct Opts { + #[clap(subcommand)] + /// The mode to run scopelint in. + pub subcommand: Subcommands, } -impl Config { - /// Create a new configuration from the command line arguments. - /// # Errors - /// Errors if too many arguments are provided, or an invalid mode is provided. - pub fn build(args: &[String]) -> Result { - match args.len() { - 1 => Ok(Self { mode: Mode::Format }), // Default to format if no args provided. - 2 => match args[1].as_str() { - "fmt" => Ok(Self { mode: Mode::Format }), - "check" => Ok(Self { mode: Mode::Check }), - "--version" | "-v" => Ok(Self { mode: Mode::Version }), - _ => Err("Unrecognized mode: Must be 'fmt', 'check', or '--version'"), - }, - _ => Err("Too many arguments"), - } - } +#[derive(Debug, Subcommand)] +/// The mode to run scopelint in. +pub enum Subcommands { + #[clap(about = "Checks code to verify all conventions are being followed.")] + /// Checks code to verify all conventions are being followed. + Check, + #[clap(about = "Formats Solidity and TOML files in the codebase.")] + /// Formats Solidity and TOML files in the codebase. + Fmt, } diff --git a/src/fmt/mod.rs b/src/fmt/mod.rs new file mode 100644 index 0000000..06fdb9b --- /dev/null +++ b/src/fmt/mod.rs @@ -0,0 +1,20 @@ +use std::{error::Error, fs, process}; + +/// Format the code. +/// # Errors +/// Errors if `forge fmt` fails, or if `taplo` fails to format `foundry.toml`. +pub fn run(taplo_opts: taplo::formatter::Options) -> Result<(), Box> { + // Format Solidity with forge + let forge_status = process::Command::new("forge").arg("fmt").output()?; + + // Print any warnings/errors from `forge fmt`. + if !forge_status.stderr.is_empty() { + print!("{}", String::from_utf8(forge_status.stderr)?); + } + + // Format `foundry.toml` with taplo. + let config_orig = fs::read_to_string("./foundry.toml")?; + let config_fmt = taplo::formatter::format(&config_orig, taplo_opts); + fs::write("./foundry.toml", config_fmt)?; + Ok(()) +} diff --git a/src/lib.rs b/src/lib.rs index 7da2a8b..5f4688b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,40 +1,26 @@ #![doc = include_str!("../README.md")] -#![warn(missing_docs, unreachable_pub, unused, rust_2021_compatibility)] +#![warn(unreachable_pub, unused, rust_2021_compatibility)] #![warn(clippy::all, clippy::pedantic, clippy::cargo, clippy::nursery)] +#![allow(clippy::multiple_crate_versions)] +use std::error::Error; -use colored::Colorize; -use once_cell::sync::Lazy; +/// Runs validators on Solidity files. +pub mod check; -use regex::Regex; -use solang_parser::pt::{ - ContractPart, FunctionAttribute, FunctionDefinition, FunctionTy, SourceUnitPart, - VariableAttribute, VariableDefinition, Visibility, -}; -use std::{error::Error, ffi::OsStr, fs, path::Path, process}; -use walkdir::WalkDir; - -/// Program configuration. Valid modes are `fmt`, `check`, and `--version`. +/// Parses library configuration. pub mod config; -/// Utilities for formatting and printing a report of results. -pub mod report; - -// A regex matching valid test names, see the `validate_test_names_regex` test for examples. -static RE_VALID_TEST_NAME: Lazy = - Lazy::new(|| Regex::new(r"^test(Fork)?(Fuzz)?(_Revert(If|When|On))?_(\w+)*$").unwrap()); - -// A regex matching valid constant names, see the `validate_constant_names_regex` test for examples. -static RE_VALID_CONSTANT_NAME: Lazy = - Lazy::new(|| Regex::new(r"^(?:[$_]*[A-Z0-9][$_]*){1,}$").unwrap()); +/// Formats Solidity and TOML files. +pub mod fmt; // =========================== // ======== Execution ======== // =========================== -/// Takes the provided `config` and runs the program. +/// Takes the provided `opts` and runs the program. /// # Errors /// Errors if the provided mode fails to run. -pub fn run(config: &config::Config) -> Result<(), Box> { +pub fn run(opts: &config::Opts) -> Result<(), Box> { // Configure formatting options, https://taplo.tamasfe.dev/. let taplo_opts = taplo::formatter::Options { allowed_blank_lines: 1, @@ -44,414 +30,8 @@ pub fn run(config: &config::Config) -> Result<(), Box> { }; // Execute commands. - match config.mode { - config::Mode::Format => fmt(taplo_opts), - config::Mode::Check => check(taplo_opts), - config::Mode::Version => { - version(); - Ok(()) - } - } -} - -// Print the package version. -fn version() { - println!("{} v{}", env!("CARGO_PKG_NAME"), env!("CARGO_PKG_VERSION")); -} - -// Format the code, and print details on any invalid items. -fn fmt(taplo_opts: taplo::formatter::Options) -> Result<(), Box> { - // Format Solidity with forge - let forge_status = process::Command::new("forge").arg("fmt").output()?; - - // Print any warnings/errors from `forge fmt`. - if !forge_status.stderr.is_empty() { - print!("{}", String::from_utf8(forge_status.stderr)?); - } - - // Format `foundry.toml` with taplo. - let config_orig = fs::read_to_string("./foundry.toml")?; - let config_fmt = taplo::formatter::format(&config_orig, taplo_opts); - fs::write("./foundry.toml", config_fmt)?; - - // Check naming conventions. - validate_conventions() -} - -// Validate the code formatting, and print details on any invalid items. -fn check(taplo_opts: taplo::formatter::Options) -> Result<(), Box> { - let valid_names = validate_conventions(); - let valid_fmt = validate_fmt(taplo_opts); - - if valid_names.is_ok() && valid_fmt.is_ok() { - Ok(()) - } else { - Err("One or more checks failed, review above output".into()) - } -} - -// ============================= -// ======== Validations ======== -// ============================= - -// -------- Top level validation methods -------- - -fn validate_fmt(taplo_opts: taplo::formatter::Options) -> Result<(), Box> { - // Check Solidity with `forge fmt` - let forge_status = process::Command::new("forge").arg("fmt").arg("--check").output()?; - - // Print any warnings/errors from `forge fmt`. - let stderr = String::from_utf8(forge_status.stderr)?; - let forge_ok = forge_status.status.success() && stderr.is_empty(); - print!("{stderr}"); // Prints nothing if stderr is empty. - - // Check TOML with `taplo fmt` - let config_orig = fs::read_to_string("./foundry.toml")?; - let config_fmt = taplo::formatter::format(&config_orig, taplo_opts); - let taplo_ok = config_orig == config_fmt; - - if !forge_ok || !taplo_ok { - eprintln!( - "{}: Formatting validation failed, run `scopelint fmt` to fix", - "error".bold().red() - ); - return Err("Invalid fmt found".into()) - } - Ok(()) -} - -fn validate_conventions() -> Result<(), Box> { - let paths = ["./src", "./script", "./test"]; - let results = validate(paths)?; - - if !results.is_valid() { - eprint!("{results}"); - eprintln!("{}: Convention checks failed, see details above", "error".bold().red()); - return Err("Invalid names found".into()) - } - Ok(()) -} - -// -------- Validation implementation -------- - -trait Validate { - fn validate(&self, content: &str, file: &Path) -> Vec; -} - -trait Name { - fn name(&self) -> String; -} - -impl Validate for VariableDefinition { - fn validate(&self, content: &str, file: &Path) -> Vec { - let mut invalid_items = Vec::new(); - let name = &self.name.name; - - // Validate constants and immutables are in ALL_CAPS. - let is_constant = self - .attrs - .iter() - .any(|a| matches!(a, VariableAttribute::Constant(_) | VariableAttribute::Immutable(_))); - - if is_constant && !is_valid_constant_name(name) { - invalid_items.push(report::InvalidItem::new( - report::Validator::Constant, - file.display().to_string(), - name.clone(), - offset_to_line(content, self.loc.start()), - )); - } - - invalid_items - } -} - -impl Name for FunctionDefinition { - fn name(&self) -> String { - match self.ty { - FunctionTy::Constructor => "constructor".to_string(), - FunctionTy::Fallback => "fallback".to_string(), - FunctionTy::Receive => "receive".to_string(), - FunctionTy::Function | FunctionTy::Modifier => self.name.as_ref().unwrap().name.clone(), - } - } -} - -impl Validate for FunctionDefinition { - fn validate(&self, content: &str, file: &Path) -> Vec { - let mut invalid_items = Vec::new(); - let name = &self.name(); - - // Validate test names match the required pattern. - if file.starts_with("./test") && !is_valid_test_name(name) { - invalid_items.push(report::InvalidItem::new( - report::Validator::Test, - file.display().to_string(), - name.to_string(), - offset_to_line(content, self.loc.start()), - )); - } - - // Validate internal and private src methods start with an underscore. - let is_private = self.attributes.iter().any(|a| match a { - FunctionAttribute::Visibility(v) => { - matches!(v, Visibility::Private(_) | Visibility::Internal(_)) - } - _ => false, - }); - - if file.starts_with("./src") && is_private && !name.starts_with('_') { - invalid_items.push(report::InvalidItem::new( - report::Validator::Src, - file.display().to_string(), - name.to_string(), - offset_to_line(content, self.loc.start()), - )); - } - - invalid_items - } -} - -// Core validation method that walks the directory and validates all Solidity files. -fn validate(paths: [&str; 3]) -> Result> { - let mut results = report::Report::default(); - - for path in paths { - for result in WalkDir::new(path) { - let dent = match result { - Ok(dent) => dent, - Err(err) => { - eprintln!("{err}"); - continue - } - }; - - if !dent.file_type().is_file() || dent.path().extension() != Some(OsStr::new("sol")) { - continue - } - - // Executable script files are expected to end with `.s.sol`, whereas non-executable - // helper contracts in the scripts dir just end with `.sol`. - let is_script = - path == "./script" && dent.path().to_str().expect("Bad path").ends_with(".s.sol"); - - // Get the parse tree (pt) of the file. - let content = fs::read_to_string(dent.path())?; - let (pt, _comments) = solang_parser::parse(&content, 0).expect("Parsing failed"); - - // Variables used to track status of checks that are file-wide. - let mut public_methods: Vec = Vec::new(); - - // Run checks. - for element in pt.0 { - match element { - SourceUnitPart::FunctionDefinition(f) => { - results.add_items(f.validate(&content, dent.path())); - } - SourceUnitPart::VariableDefinition(v) => { - results.add_items(v.validate(&content, dent.path())); - } - SourceUnitPart::ContractDefinition(c) => { - for el in c.parts { - match el { - ContractPart::VariableDefinition(v) => { - results.add_items(v.validate(&content, dent.path())); - } - ContractPart::FunctionDefinition(f) => { - results.add_items(f.validate(&content, dent.path())); - - let name = f.name(); - let is_private = f.attributes.iter().any(|a| match a { - FunctionAttribute::Visibility(v) => { - matches!( - v, - Visibility::Private(_) | Visibility::Internal(_) - ) - } - _ => false, - }); - - if is_script && - !is_private && - name != "setUp" && - name != "constructor" - { - public_methods.push(name); - } - } - _ => (), - } - } - } - _ => (), - } - } - - // Validate scripts only have a single public run method, or no public methods (i.e. - // it's a helper contract not a script). - if is_script { - // If we have no public methods, the `run` method is missing. - match public_methods.len() { - 0 => { - results.add_item(report::InvalidItem::new( - report::Validator::Script, - dent.path().display().to_string(), - "No `run` method found".to_string(), - 0, // This spans multiple lines, so we don't have a line number. - )); - } - 1 => { - if public_methods[0] != "run" { - results.add_item(report::InvalidItem::new( - report::Validator::Script, - dent.path().display().to_string(), - "The only public method must be named `run`".to_string(), - 0, - )); - } - } - _ => { - results.add_item(report::InvalidItem::new( - report::Validator::Script, - dent.path().display().to_string(), - format!("Scripts must have a single public method named `run` (excluding `setUp`), but the following methods were found: {public_methods:?}"), - 0, - )); - } - } - } - } - } - Ok(results) -} - -fn is_valid_test_name(name: &str) -> bool { - if !name.starts_with("test") { - return true // Not a test function, so return. - } - RE_VALID_TEST_NAME.is_match(name) -} - -fn is_valid_constant_name(name: &str) -> bool { - RE_VALID_CONSTANT_NAME.is_match(name) -} - -// Converts the start offset of a `Loc` to `(line, col)`. Modified from https://github.com/foundry-rs/foundry/blob/45b9dccdc8584fb5fbf55eb190a880d4e3b0753f/fmt/src/helpers.rs#L54-L70 -fn offset_to_line(content: &str, start: usize) -> usize { - debug_assert!(content.len() > start); - - let mut line_counter = 1; // First line is `1`. - for (offset, c) in content.chars().enumerate() { - if c == '\n' { - line_counter += 1; - } - if offset > start { - return line_counter - } - } - - unreachable!("content.len() > start") -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn validate_test_names_regex() { - let allowed_names = vec![ - "test_Description", - "test_Increment", - "testFuzz_Description", - "testFork_Description", - "testForkFuzz_Description", - "testForkFuzz_Description_MoreInfo", - "test_RevertIf_Condition", - "test_RevertWhen_Condition", - "test_RevertOn_Condition", - "test_RevertOn_Condition_MoreInfo", - "testFuzz_RevertIf_Condition", - "testFuzz_RevertWhen_Condition", - "testFuzz_RevertOn_Condition", - "testFuzz_RevertOn_Condition_MoreInfo", - "testForkFuzz_RevertIf_Condition", - "testForkFuzz_RevertWhen_Condition", - "testForkFuzz_RevertOn_Condition", - "testForkFuzz_RevertOn_Condition_MoreInfo", - "testForkFuzz_RevertOn_Condition_MoreInfo_Wow", - "testForkFuzz_RevertOn_Condition_MoreInfo_Wow_As_Many_Underscores_As_You_Want", - ]; - - let disallowed_names = [ - "test", - "testDescription", - "testDescriptionMoreInfo", - // TODO The below are tough to prevent without regex look-ahead support. - // "test_RevertIfCondition", - // "test_RevertWhenCondition", - // "test_RevertOnCondition", - // "testFuzz_RevertIfDescription", - // "testFuzz_RevertWhenDescription", - // "testFuzz_RevertOnDescription", - // "testForkFuzz_RevertIfCondition", - // "testForkFuzz_RevertWhenCondition", - // "testForkFuzz_RevertOnCondition", - ]; - - for name in allowed_names { - assert_eq!(is_valid_test_name(name), true, "{name}"); - } - - for name in disallowed_names { - assert_eq!(is_valid_test_name(name), false, "{name}"); - } - } - - #[test] - fn validate_constant_names_regex() { - let allowed_names = vec![ - "MAX_UINT256", - "256_MAXUINT", - "256_MAX_11_UINT", - "VARIABLE", - "VARIABLE_NAME", - "VARIABLE_NAME_", - "VARIABLE___NAME", - "VARIABLE_NAME_WOW", - "VARIABLE_NAME_WOW_AS_MANY_UNDERSCORES_AS_YOU_WANT", - "__VARIABLE", - "_VARIABLE__NAME", - "_VARIABLE_NAME__", - "_VARIABLE_NAME_WOW", - "_VARIABLE_NAME_WOW_AS_MANY_UNDERSCORES_AS_YOU_WANT", - "$VARIABLE_NAME", - "_$VARIABLE_NAME_", - "$_VARIABLE_NAME$", - "_$VARIABLE_NAME$_", - "$_VARIABLE_NAME_$", - "$_VARIABLE__NAME_", - ]; - - let disallowed_names = [ - "variable", - "variableName", - "_variable", - "_variable_Name", - "VARIABLe", - "VARIABLE_name", - "_VARIABLe", - "_VARIABLE_name", - "$VARIABLe", - "$VARIABLE_name", - ]; - - for name in allowed_names { - assert_eq!(is_valid_constant_name(name), true, "{name}"); - } - - for name in disallowed_names { - assert_eq!(is_valid_constant_name(name), false, "{name}"); - } + match opts.subcommand { + config::Subcommands::Fmt => fmt::run(taplo_opts), + config::Subcommands::Check => check::run(taplo_opts), } } diff --git a/src/main.rs b/src/main.rs index e2e704b..07e9995 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,18 +1,15 @@ #![doc = include_str!("../README.md")] #![warn(missing_docs, unreachable_pub, unused, rust_2021_compatibility)] #![warn(clippy::all, clippy::pedantic, clippy::cargo, clippy::nursery)] -use colored::Colorize; -use std::{env, process}; +#![allow(clippy::multiple_crate_versions)] +use clap::Parser; +use scopelint::config::Opts; +use std::process; fn main() { - let args: Vec = env::args().collect(); + let opts = Opts::parse(); - let config = scopelint::config::Config::build(&args).unwrap_or_else(|err| { - eprintln!("{}: Argument parsing failed with '{err}'", "error".bold().red()); - process::exit(1); - }); - - if let Err(_err) = scopelint::run(&config) { + if let Err(_err) = scopelint::run(&opts) { // All warnings/errors have already been logged. process::exit(1); } diff --git a/src/report.rs b/src/report.rs deleted file mode 100644 index 633071e..0000000 --- a/src/report.rs +++ /dev/null @@ -1,91 +0,0 @@ -use std::fmt; - -/// The type of validator that found the invalid item. -#[derive(PartialEq, Eq, PartialOrd, Ord, Clone)] -pub enum Validator { - /// A constant or immutable variable. - Constant, - /// A script file. - Script, - /// A source contract. - Src, - /// A test contract. - Test, -} - -/// A single invalid item found by a validator. -#[derive(PartialEq, Eq, PartialOrd, Ord, Clone)] -pub struct InvalidItem { - kind: Validator, - file: String, // File name. - text: String, // Details to show about the invalid item. - line: usize, // Line number. -} - -impl InvalidItem { - /// Initializes a new `InvalidItem`. - #[must_use] - pub const fn new(kind: Validator, file: String, text: String, line: usize) -> Self { - Self { kind, file, text, line } - } - - fn description(&self) -> String { - match self.kind { - Validator::Test => { - format!("Invalid test name in {} on line {}: {}", self.file, self.line, self.text) - } - Validator::Constant => { - format!( - "Invalid constant or immutable name in {} on line {}: {}", - self.file, self.line, self.text - ) - } - Validator::Script => { - format!("Invalid script interface in {}: {}", self.file, self.text) - } - Validator::Src => { - format!( - "Invalid src method name in {} on line {}: {}", - self.file, self.line, self.text - ) - } - } - } -} - -/// A collection of invalid items to generate a report from. -#[derive(Default)] -pub struct Report { - /// A list of invalid items. - invalid_items: Vec, -} - -impl fmt::Display for Report { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result { - let mut sorted_items = self.invalid_items.clone(); - sorted_items.sort(); - - for item in sorted_items { - writeln!(f, "{}", item.description())?; - } - Ok(()) - } -} - -impl Report { - /// Extends the report with a new invalid item. - pub fn add_item(&mut self, item: InvalidItem) { - self.invalid_items.push(item); - } - - /// Extends the report with a list of invalid items. - pub fn add_items(&mut self, items: Vec) { - self.invalid_items.extend(items); - } - - /// Returns true if no issues were found. - #[must_use] - pub fn is_valid(&self) -> bool { - self.invalid_items.is_empty() - } -} diff --git a/tests/cli.rs b/tests/cli.rs index a2aecb4..7f414d6 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -38,6 +38,7 @@ fn test_check_proj1_all_findings() { "Invalid src method name in ./src/Counter.sol on line 25: privateShouldHaveLeadingUnderscore", "Invalid test name in ./test/Counter.t.sol on line 16: testIncrementBadName", "error: Convention checks failed, see details above", + "error: Formatting validation failed, run `scopelint fmt` to fix", "", ]; diff --git a/tests/proj1-AllFindings/src/Counter.sol b/tests/proj1-AllFindings/src/Counter.sol index 9500022..1d4428b 100644 --- a/tests/proj1-AllFindings/src/Counter.sol +++ b/tests/proj1-AllFindings/src/Counter.sol @@ -28,3 +28,4 @@ contract Counter { number += 1000; } } +