diff --git a/Cargo.lock b/Cargo.lock index 44f207d6f082e..221be209fc728 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2232,6 +2232,12 @@ dependencies = [ "syn 2.0.104", ] +[[package]] +name = "boxcar" +version = "0.2.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26c4925bc979b677330a8c7fe7a8c94af2dbb4a2d37b4a20a80d884400f46baa" + [[package]] name = "bs58" version = "0.5.1" @@ -3924,6 +3930,7 @@ dependencies = [ "solar-data-structures", "solar-interface", "solar-parse", + "solar-sema", "thiserror 2.0.12", ] @@ -4070,9 +4077,9 @@ dependencies = [ [[package]] name = "foundry-block-explorers" -version = "0.19.1" +version = "0.20.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4955628c39a4f0571b25b71da149fedbde13a3501cd522018f242522ae26fd51" +checksum = "fc107bbc3b4480995fdf337ca0ddedc631728175f418d3136ead9df8f4dc465e" dependencies = [ "alloy-chains", "alloy-json-abi", @@ -4273,9 +4280,9 @@ dependencies = [ [[package]] name = "foundry-compilers" -version = "0.17.4" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "97aa785b2e4888720a07579d1b947286ab22ba8f774f7919bb4b9b332aabc073" +checksum = "f163b01cdad921f139776084391db2f1f5fb206ce2395f1847f0c1e992a89a4f" dependencies = [ "alloy-json-abi", "alloy-primitives", @@ -4310,9 +4317,9 @@ dependencies = [ [[package]] name = "foundry-compilers-artifacts" -version = "0.17.4" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c752b33d827730886038741dae810aaf068cabb92d5caab706ea3e9ef25c8910" +checksum = "c2676d70082ed23680fe2d08c0b750d5f7f2438c6d946f1cb140a76c5e5e0392" dependencies = [ "foundry-compilers-artifacts-solc", "foundry-compilers-artifacts-vyper", @@ -4320,9 +4327,9 @@ dependencies = [ [[package]] name = "foundry-compilers-artifacts-solc" -version = "0.17.4" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "53f7e0d4402323dce4f74bab71159bc5a9af3d6fbf74ba547f5305a2b0821299" +checksum = "a3ada94dc5946334bb08df574855ba345ab03ba8c6f233560c72c8d61fa9db80" dependencies = [ "alloy-json-abi", "alloy-primitives", @@ -4343,9 +4350,9 @@ dependencies = [ [[package]] name = "foundry-compilers-artifacts-vyper" -version = "0.17.4" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "802908ff50171b7b6dc014cbc391d27fe11aff59405e74b3dc41859364dbf44b" +checksum = "372052af72652e375a6e7eed22179bd8935114e25e1c5a8cca7f00e8f20bd94c" dependencies = [ "alloy-json-abi", "alloy-primitives", @@ -4358,9 +4365,9 @@ dependencies = [ [[package]] name = "foundry-compilers-core" -version = "0.17.4" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "495c8b7739f7807921e8538182b2fc7335c8a49bd71bd7f3e7982d14ba8594dc" +checksum = "bf0962c46855979300f6526ed57f987ccf6a025c2b92ce574b281d9cb2ef666b" dependencies = [ "alloy-primitives", "cfg-if", @@ -5024,10 +5031,6 @@ name = "hashbrown" version = "0.14.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" -dependencies = [ - "ahash", - "allocator-api2", -] [[package]] name = "hashbrown" @@ -5503,6 +5506,19 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "inturn" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "62f17d4bce58d4380de6432e6b1a0ebb561dfbbe21fc123204870b7006189677" +dependencies = [ + "boxcar", + "bumpalo", + "dashmap", + "hashbrown 0.14.5", + "thread_local", +] + [[package]] name = "io-uring" version = "0.7.8" @@ -5761,16 +5777,6 @@ dependencies = [ "rustversion", ] -[[package]] -name = "lasso" -version = "0.7.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e14eda50a3494b3bf7b9ce51c52434a761e383d7238ce1dd5dcec2fbc13e9fb" -dependencies = [ - "dashmap", - "hashbrown 0.14.5", -] - [[package]] name = "lazy_static" version = "1.5.0" @@ -8755,9 +8761,9 @@ dependencies = [ [[package]] name = "solar-ast" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "acb0d9abd88c1325e5c0f9bb153ebfb02ed40bc9639067d0ad120f5d29ee8777" +checksum = "1f7f30449c304fd09db4637209dc73bde7ec203e6e5c691fc9eed26b68cd105a" dependencies = [ "alloy-primitives", "bumpalo", @@ -8774,18 +8780,18 @@ dependencies = [ [[package]] name = "solar-config" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "908d455bb7c04acd783bd157b63791bf010cf6a495a845e48f7aee334aad319f" +checksum = "643ddf85ab917f5643ec47eb79ee6db6c6158cfaf7415e39840e154eecf7176f" dependencies = [ "strum 0.27.1", ] [[package]] name = "solar-data-structures" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a8b5a697cab81241c4623b4546c69e57182202847a75d7c0047c0dd10b923d8c" +checksum = "3cc21b4df6061e1c825c16faf8e1f16c2341f4c46a2b2a60e03069c4453fc5ac" dependencies = [ "bumpalo", "index_vec", @@ -8798,20 +8804,19 @@ dependencies = [ [[package]] name = "solar-interface" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "33e49e5a714ec80d7f9c8942584e5e86add1c5e824aa175d0681e9ac7a10e5cc" +checksum = "bb2571bfb2f54c5a24688afe6876682117e96f6fd35393398dbac43e5f6f7144" dependencies = [ "annotate-snippets", "anstream", "anstyle", "const-hex", - "derive_builder", "derive_more 2.0.1", "dunce", + "inturn", "itertools 0.14.0", "itoa", - "lasso", "match_cfg", "normalize-path", "rayon", @@ -8828,9 +8833,9 @@ dependencies = [ [[package]] name = "solar-macros" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e17f8b99f28f358b41acb6efe4d7b8f326541b3c58a15dd1931d6fe581feefef" +checksum = "4ca57257a3b6ef16bd7995d23da3e661e89cebdae6fb9e42e4331ba0f033bd1d" dependencies = [ "proc-macro2", "quote", @@ -8839,9 +8844,9 @@ dependencies = [ [[package]] name = "solar-parse" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c6982ef2ecfb1338c774c743ab7166ce8c3dcc92089ab77fad58eeb632b201e9" +checksum = "081b4d9e2ddd3c7bb90079b3eb253b957ef9eb5c860ed6d6a4068884ec3a8b85" dependencies = [ "alloy-primitives", "bitflags 2.9.1", @@ -8860,9 +8865,9 @@ dependencies = [ [[package]] name = "solar-sema" -version = "0.1.4" +version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fe8e0a3a6dfc7f4987bfb93f9b56079150407ef55c459964a1541c669554b74d" +checksum = "93b017d4017ee8324e669c6e5e6fe9a282ed07eb6171e5384ddd17b8a854766f" dependencies = [ "alloy-json-abi", "alloy-primitives", diff --git a/Cargo.toml b/Cargo.toml index eafc216c38224..848376fcd052a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -204,15 +204,15 @@ foundry-wallets = { path = "crates/wallets" } foundry-linking = { path = "crates/linking" } # solc & compilation utilities -foundry-block-explorers = { version = "0.19.1", default-features = false } -foundry-compilers = { version = "0.17.4", default-features = false } +foundry-block-explorers = { version = "0.20.0", default-features = false } +foundry-compilers = { version = "0.18.0", default-features = false } foundry-fork-db = "0.16" solang-parser = { version = "=0.3.9", package = "foundry-solang-parser" } -solar-ast = { version = "=0.1.4", default-features = false } -solar-parse = { version = "=0.1.4", default-features = false } -solar-interface = { version = "=0.1.4", default-features = false } -solar-sema = { version = "=0.1.4", default-features = false } -solar-data-structures = { version = "=0.1.4", default-features = false } +solar-ast = { version = "=0.1.5", default-features = false } +solar-parse = { version = "=0.1.5", default-features = false } +solar-interface = { version = "=0.1.5", default-features = false } +solar-sema = { version = "=0.1.5", default-features = false } +solar-data-structures = { version = "=0.1.5", default-features = false } ## alloy alloy-consensus = { version = "1.0.22", default-features = false } diff --git a/crates/chisel/src/solidity_helper.rs b/crates/chisel/src/solidity_helper.rs index b324705214d17..94173a482ccb6 100644 --- a/crates/chisel/src/solidity_helper.rs +++ b/crates/chisel/src/solidity_helper.rs @@ -53,7 +53,7 @@ impl SolidityHelper { errored: false, do_paint: yansi::is_enabled(), sess: Session::builder().with_silent_emitter(None).build(), - globals: SessionGlobals::new(), + globals: SessionGlobals::default(), } } diff --git a/crates/cli/src/opts/build/utils.rs b/crates/cli/src/opts/build/utils.rs index 0dedb38e2db61..a85d7866df4be 100644 --- a/crates/cli/src/opts/build/utils.rs +++ b/crates/cli/src/opts/build/utils.rs @@ -14,22 +14,27 @@ use std::path::PathBuf; /// /// * Configures include paths, remappings and registers all in-memory sources so that solar can /// operate without touching disk. +/// * If no `project` is provided, it will spin up a new ephemeral project. /// * If no `target_paths` are provided, all project files are processed. /// * Only processes the subset of sources with the most up-to-date Solitidy version. pub fn solar_pcx_from_build_opts<'sess>( sess: &'sess Session, - build: BuildOpts, - target_paths: Option>, + build: &BuildOpts, + project: Option<&Project>, + target_paths: Option<&[PathBuf]>, ) -> Result> { // Process build options let config = build.load_config()?; - let project = config.ephemeral_project()?; + let project = match project { + Some(project) => project, + None => &config.ephemeral_project()?, + }; let sources = match target_paths { // If target files are provided, only process those sources Some(targets) => { let mut sources = Sources::new(); - for t in targets.into_iter() { + for t in targets { let path = dunce::canonicalize(t)?; let source = Source::read(&path)?; sources.insert(path, source); @@ -44,7 +49,7 @@ pub fn solar_pcx_from_build_opts<'sess>( let graph = Graph::::resolve_sources(&project.paths, sources)?; let (version, sources, _) = graph // resolve graph into mapping language -> version -> sources - .into_sources_by_version(&project)? + .into_sources_by_version(project)? .sources .into_iter() // only interested in Solidity sources @@ -63,7 +68,7 @@ pub fn solar_pcx_from_build_opts<'sess>( version, ); - Ok(solar_pcx_from_solc_project(sess, &project, &solc, true)) + Ok(solar_pcx_from_solc_project(sess, project, &solc, true)) } /// Builds a Solar [`solar_sema::ParsingContext`] from a [`foundry_compilers::Project`] and a diff --git a/crates/forge/src/cmd/build.rs b/crates/forge/src/cmd/build.rs index 272cae93badaa..1dbb334b6227f 100644 --- a/crates/forge/src/cmd/build.rs +++ b/crates/forge/src/cmd/build.rs @@ -3,18 +3,18 @@ use clap::Parser; use eyre::Result; use forge_lint::{linter::Linter, sol::SolidityLinter}; use foundry_cli::{ - opts::BuildOpts, + opts::{BuildOpts, solar_pcx_from_build_opts}, utils::{LoadConfig, cache_local_signatures}, }; use foundry_common::{compile::ProjectCompiler, shell}; use foundry_compilers::{ - Project, ProjectCompileOutput, + CompilationError, FileFilter, Project, ProjectCompileOutput, compilers::{Language, multi::MultiCompilerLanguage}, solc::SolcLanguage, utils::source_files_iter, }; use foundry_config::{ - Config, + Config, SkipBuildFilters, figment::{ self, Metadata, Profile, Provider, error::Kind::InvalidType, @@ -103,8 +103,6 @@ impl BuildArgs { .ignore_eip_3860(self.ignore_eip_3860) .bail(!format_json); - // Runs the SolidityLinter before compilation. - self.lint(&project, &config)?; let output = compiler.compile(&project)?; // Cache project selectors. @@ -114,6 +112,11 @@ impl BuildArgs { sh_println!("{}", serde_json::to_string_pretty(&output.output())?)?; } + // Only run the `SolidityLinter` if there are no compilation errors + if output.output().errors.iter().all(|e| !e.is_error()) { + self.lint(&project, &config)?; + } + Ok(output) } @@ -147,15 +150,35 @@ impl BuildArgs { .flat_map(foundry_common::fs::canonicalize_path) .collect::>(); + let skip = SkipBuildFilters::new(config.skip.clone(), config.root.clone()); let curr_dir = std::env::current_dir()?; let input_files = config .project_paths::() .input_files_iter() - .filter(|p| !(ignored.contains(p) || ignored.contains(&curr_dir.join(p)))) + .filter(|p| { + skip.is_match(p) + && !(ignored.contains(p) || ignored.contains(&curr_dir.join(p))) + }) .collect::>(); if !input_files.is_empty() { - linter.lint(&input_files); + let sess = linter.init(); + + let pcx = solar_pcx_from_build_opts( + &sess, + &self.build, + Some(project), + Some(&input_files), + )?; + linter.early_lint(&input_files, pcx); + + let pcx = solar_pcx_from_build_opts( + &sess, + &self.build, + Some(project), + Some(&input_files), + )?; + linter.late_lint(&input_files, pcx); } } diff --git a/crates/forge/src/cmd/eip712.rs b/crates/forge/src/cmd/eip712.rs index 89f90f4cc37dc..3c72e0416f512 100644 --- a/crates/forge/src/cmd/eip712.rs +++ b/crates/forge/src/cmd/eip712.rs @@ -14,6 +14,7 @@ use std::{ collections::BTreeMap, fmt::{Display, Formatter, Result as FmtResult, Write}, path::{Path, PathBuf}, + slice, }; foundry_config::impl_figment_convert!(Eip712Args, build); @@ -56,8 +57,12 @@ impl Eip712Args { sess.enter_parallel(|| -> Result<()> { // Set up the parsing context with the project paths and sources. - let parsing_context = - solar_pcx_from_build_opts(&sess, self.build, Some(vec![self.target_path]))?; + let parsing_context = solar_pcx_from_build_opts( + &sess, + &self.build, + None, + Some(slice::from_ref(&self.target_path)), + )?; // Parse and resolve let hir_arena = ThreadLocal::new(); diff --git a/crates/forge/src/cmd/lint.rs b/crates/forge/src/cmd/lint.rs index dd485715aadca..4b3bf58893a69 100644 --- a/crates/forge/src/cmd/lint.rs +++ b/crates/forge/src/cmd/lint.rs @@ -4,9 +4,12 @@ use forge_lint::{ linter::Linter, sol::{SolLint, SolLintError, SolidityLinter}, }; -use foundry_cli::utils::{FoundryPathExt, LoadConfig}; +use foundry_cli::{ + opts::{BuildOpts, solar_pcx_from_build_opts}, + utils::{FoundryPathExt, LoadConfig}, +}; use foundry_compilers::{solc::SolcLanguage, utils::SOLC_EXTENSIONS}; -use foundry_config::{filter::expand_globs, impl_figment_convert_basic, lint::Severity}; +use foundry_config::{filter::expand_globs, lint::Severity}; use std::path::PathBuf; /// CLI arguments for `forge lint`. @@ -16,13 +19,6 @@ pub struct LintArgs { #[arg(value_hint = ValueHint::FilePath, value_name = "PATH", num_args(1..))] paths: Vec, - /// The project's root path. - /// - /// By default root of the Git repository, if in one, - /// or the current working directory. - #[arg(long, value_hint = ValueHint::DirPath, value_name = "PATH")] - root: Option, - /// Specifies which lints to run based on severity. Overrides the `severity` project config. /// /// Supported values: `high`, `med`, `low`, `info`, `gas`. @@ -37,9 +33,12 @@ pub struct LintArgs { /// Activates the linter's JSON formatter (rustc-compatible). #[arg(long)] json: bool, + + #[command(flatten)] + build: BuildOpts, } -impl_figment_convert_basic!(LintArgs); +foundry_config::impl_figment_convert!(LintArgs, build); impl LintArgs { pub fn run(self) -> Result<()> { @@ -112,7 +111,13 @@ impl LintArgs { .without_lints(exclude) .with_severity(if severity.is_empty() { None } else { Some(severity) }); - linter.lint(&input); + let sess = linter.init(); + + let pcx = solar_pcx_from_build_opts(&sess, &self.build, Some(&project), Some(&input))?; + linter.early_lint(&input, pcx); + + let pcx = solar_pcx_from_build_opts(&sess, &self.build, Some(&project), Some(&input))?; + linter.late_lint(&input, pcx); Ok(()) } diff --git a/crates/forge/tests/cli/build.rs b/crates/forge/tests/cli/build.rs index 26db462134e1d..c0dd0517551d8 100644 --- a/crates/forge/tests/cli/build.rs +++ b/crates/forge/tests/cli/build.rs @@ -46,7 +46,7 @@ contract Dummy { .unwrap(); // set up command - cmd.args(["compile", "--format-json"]).assert_success().stdout_eq(str![[r#" + cmd.args(["compile", "--format-json"]).assert_success().stderr_eq("").stdout_eq(str![[r#" { "errors": [ { diff --git a/crates/forge/tests/cli/cmd.rs b/crates/forge/tests/cli/cmd.rs index 1d5b6f9de3d72..fcf10023cdf6e 100644 --- a/crates/forge/tests/cli/cmd.rs +++ b/crates/forge/tests/cli/cmd.rs @@ -3022,9 +3022,9 @@ forgetest_init!(can_use_absolute_imports, |prj, cmd| { prj.add_lib( "myDependency/src/Config.sol", r#" - import "src/interfaces/IConfig.sol"; + import {IConfig} from "myDependency/src/interfaces/IConfig.sol"; - contract Config {} + contract Config is IConfig {} "#, ) .unwrap(); @@ -3032,9 +3032,11 @@ forgetest_init!(can_use_absolute_imports, |prj, cmd| { prj.add_source( "Greeter", r#" - import "myDependency/src/Config.sol"; + import {Config} from "myDependency/src/Config.sol"; - contract Greeter {} + contract Greeter { + Config config; + } "#, ) .unwrap(); diff --git a/crates/forge/tests/cli/lint.rs b/crates/forge/tests/cli/lint.rs index 1e115f7ae9eba..f11450bcd63f1 100644 --- a/crates/forge/tests/cli/lint.rs +++ b/crates/forge/tests/cli/lint.rs @@ -30,7 +30,7 @@ const OTHER_CONTRACT: &str = r#" pragma solidity ^0.8.0; // forge-lint: disable-next-line - import { ContractWithLints } from "ContractWithLints.sol"; + import { ContractWithLints } from "./ContractWithLints.sol"; contract OtherContractWithLints { uint256 VARIABLE_MIXED_CASE_INFO; diff --git a/crates/lint/Cargo.toml b/crates/lint/Cargo.toml index 5e78a55abbc3a..c48dcfb85e0ed 100644 --- a/crates/lint/Cargo.toml +++ b/crates/lint/Cargo.toml @@ -23,6 +23,7 @@ solar-parse.workspace = true solar-ast.workspace = true solar-interface = { workspace = true, features = ["json"] } solar-data-structures.workspace = true +solar-sema.workspace = true heck.workspace = true rayon.workspace = true diff --git a/crates/lint/src/inline_config.rs b/crates/lint/src/inline_config.rs index 05144dd798336..8ac513a2bfcf1 100644 --- a/crates/lint/src/inline_config.rs +++ b/crates/lint/src/inline_config.rs @@ -1,6 +1,7 @@ -use solar_ast::{Item, SourceUnit, visit::Visit}; +use solar_ast::{Item, SourceUnit, visit::Visit as VisitAst}; use solar_interface::SourceMap; use solar_parse::ast::Span; +use solar_sema::hir::{self, Visit as VisitHir}; use std::{collections::HashMap, fmt, marker::PhantomData, ops::ControlFlow}; /// An inline config item @@ -20,7 +21,7 @@ pub enum InlineConfigItem { impl InlineConfigItem { /// Parse an inline config item from a string. Validates lint IDs against available lints. - pub fn parse(s: &str, available_lints: &[&str]) -> Result { + pub fn parse(s: &str, lint_ids: &[&str]) -> Result { let (disable, relevant) = s.split_once('(').unwrap_or((s, "")); let lints = if relevant.is_empty() || relevant == "all)" { vec!["all".to_string()] @@ -37,7 +38,7 @@ impl InlineConfigItem { if id == "all" { continue; } - for lint in available_lints { + for lint in lint_ids { if *lint == id { continue 'ids; } @@ -108,10 +109,33 @@ impl InlineConfig { /// # Panics /// /// Panics if `items` is not sorted in ascending order of [`Span`]s. - pub fn new<'ast>( + pub fn from_ast<'ast>( items: impl IntoIterator, ast: &'ast SourceUnit<'ast>, source_map: &SourceMap, + ) -> Self { + Self::build(items, source_map, |offset| NextItemFinderAst::new(offset).find(ast)) + } + + /// Build a new inline config with an iterator of inline config items and their locations in a + /// source file. + /// + /// # Panics + /// + /// Panics if `items` is not sorted in ascending order of [`Span`]s. + pub fn from_hir<'hir>( + items: impl IntoIterator, + hir: &'hir hir::Hir<'hir>, + source_id: hir::SourceId, + source_map: &SourceMap, + ) -> Self { + Self::build(items, source_map, |offset| NextItemFinderHir::new(offset, hir).find(source_id)) + } + + fn build( + items: impl IntoIterator, + source_map: &SourceMap, + mut find_next_item: impl FnMut(usize) -> Option, ) -> Self { let mut disabled_ranges: HashMap> = HashMap::new(); let mut disabled_blocks: HashMap = HashMap::new(); @@ -127,7 +151,7 @@ impl InlineConfig { let src = file.src.as_str(); match item { InlineConfigItem::DisableNextItem(lints) => { - if let Some(next_item) = NextItemFinder::new(sp.hi().to_usize()).find(ast) { + if let Some(next_item) = find_next_item(sp.hi().to_usize()) { for lint in lints { disabled_ranges.entry(lint).or_default().push(DisabledRange { start: next_item.lo().to_usize(), @@ -135,7 +159,7 @@ impl InlineConfig { loose: false, }); } - }; + } } InlineConfigItem::DisableLine(lints) => { let start = src[..comment_range.start].rfind('\n').map_or(0, |i| i); @@ -228,13 +252,13 @@ impl InlineConfig { /// An AST visitor that finds the first `Item` that starts after a given offset. #[derive(Debug, Default)] -struct NextItemFinder<'ast> { +struct NextItemFinderAst<'ast> { /// The offset to search after. offset: usize, _pd: PhantomData<&'ast ()>, } -impl<'ast> NextItemFinder<'ast> { +impl<'ast> NextItemFinderAst<'ast> { fn new(offset: usize) -> Self { Self { offset, _pd: PhantomData } } @@ -248,7 +272,7 @@ impl<'ast> NextItemFinder<'ast> { } } -impl<'ast> Visit<'ast> for NextItemFinder<'ast> { +impl<'ast> VisitAst<'ast> for NextItemFinderAst<'ast> { type BreakValue = Span; fn visit_item(&mut self, item: &'ast Item<'ast>) -> ControlFlow { @@ -261,3 +285,48 @@ impl<'ast> Visit<'ast> for NextItemFinder<'ast> { self.walk_item(item) } } + +/// A HIR visitor that finds the first `Item` that starts after a given offset. +#[derive(Debug)] +struct NextItemFinderHir<'hir> { + hir: &'hir hir::Hir<'hir>, + /// The offset to search after. + offset: usize, +} + +impl<'hir> NextItemFinderHir<'hir> { + fn new(offset: usize, hir: &'hir hir::Hir<'hir>) -> Self { + Self { offset, hir } + } + + /// Finds the next HIR item which a span that begins after the `offset`. + fn find(&mut self, id: hir::SourceId) -> Option { + match self.visit_nested_source(id) { + ControlFlow::Break(span) => Some(span), + ControlFlow::Continue(()) => None, + } + } +} + +impl<'hir> VisitHir<'hir> for NextItemFinderHir<'hir> { + type BreakValue = Span; + + fn hir(&self) -> &'hir hir::Hir<'hir> { + self.hir + } + + fn visit_item(&mut self, item: hir::Item<'hir, 'hir>) -> ControlFlow { + // Check if this item starts after the offset. + if item.span().lo().to_usize() > self.offset { + return ControlFlow::Break(item.span()); + } + + // If the item is before the offset, skip traverse. + if item.span().hi().to_usize() < self.offset { + return ControlFlow::Continue(()); + } + + // Otherwise, continue traversing inside this item. + self.walk_item(item) + } +} diff --git a/crates/lint/src/linter.rs b/crates/lint/src/linter/early.rs similarity index 50% rename from crates/lint/src/linter.rs rename to crates/lint/src/linter/early.rs index 3167d7ca4a867..ada64ee853eb3 100644 --- a/crates/lint/src/linter.rs +++ b/crates/lint/src/linter/early.rs @@ -1,110 +1,55 @@ -use foundry_compilers::Language; -use foundry_config::lint::Severity; -use solar_ast::{ - self as ast, Expr, ImportDirective, ItemContract, ItemFunction, ItemStruct, SourceUnit, - UsingDirective, VariableDefinition, visit::Visit, -}; -use solar_interface::{ - Session, Span, - data_structures::Never, - diagnostics::{DiagBuilder, DiagId, MultiSpan}, -}; -use std::{ops::ControlFlow, path::PathBuf}; - -use crate::inline_config::InlineConfig; - -/// Trait representing a generic linter for analyzing and reporting issues in smart contract source -/// code files. A linter can be implemented for any smart contract language supported by Foundry. -/// -/// # Type Parameters -/// -/// - `Language`: Represents the target programming language. Must implement the [`Language`] trait. -/// - `Lint`: Represents the types of lints performed by the linter. Must implement the [`Lint`] -/// trait. -/// -/// # Required Methods -/// -/// - `lint`: Scans the provided source files emitting a daignostic for lints found. -pub trait Linter: Send + Sync + Clone { - type Language: Language; - type Lint: Lint; - - fn lint(&self, input: &[PathBuf]); -} - -pub trait Lint { - fn id(&self) -> &'static str; - fn severity(&self) -> Severity; - fn description(&self) -> &'static str; - fn help(&self) -> &'static str; -} - -pub struct LintContext<'s> { - sess: &'s Session, - with_description: bool, - pub inline_config: InlineConfig, -} - -impl<'s> LintContext<'s> { - pub fn new(sess: &'s Session, with_description: bool, config: InlineConfig) -> Self { - Self { sess, with_description, inline_config: config } - } +use solar_ast::{self as ast, visit::Visit}; +use solar_interface::data_structures::Never; +use std::ops::ControlFlow; - pub fn session(&self) -> &'s Session { - self.sess - } - - /// Helper method to emit diagnostics easily from passes - pub fn emit(&self, lint: &'static L, span: Span) { - if self.inline_config.is_disabled(span, lint.id()) { - return; - } - - let desc = if self.with_description { lint.description() } else { "" }; - let diag: DiagBuilder<'_, ()> = self - .sess - .dcx - .diag(lint.severity().into(), desc) - .code(DiagId::new_str(lint.id())) - .span(MultiSpan::from_span(span)) - .help(lint.help()); - - diag.emit(); - } -} +use super::LintContext; /// Trait for lints that operate directly on the AST. /// Its methods mirror `ast::visit::Visit`, with the addition of `LintCotext`. pub trait EarlyLintPass<'ast>: Send + Sync { - fn check_expr(&mut self, _ctx: &LintContext<'_>, _expr: &'ast Expr<'ast>) {} - fn check_item_struct(&mut self, _ctx: &LintContext<'_>, _struct: &'ast ItemStruct<'ast>) {} - fn check_item_function(&mut self, _ctx: &LintContext<'_>, _func: &'ast ItemFunction<'ast>) {} + fn check_expr(&mut self, _ctx: &LintContext<'_>, _expr: &'ast ast::Expr<'ast>) {} + fn check_item_struct(&mut self, _ctx: &LintContext<'_>, _struct: &'ast ast::ItemStruct<'ast>) {} + fn check_item_function( + &mut self, + _ctx: &LintContext<'_>, + _func: &'ast ast::ItemFunction<'ast>, + ) { + } fn check_variable_definition( &mut self, _ctx: &LintContext<'_>, - _var: &'ast VariableDefinition<'ast>, + _var: &'ast ast::VariableDefinition<'ast>, ) { } fn check_import_directive( &mut self, _ctx: &LintContext<'_>, - _import: &'ast ImportDirective<'ast>, + _import: &'ast ast::ImportDirective<'ast>, ) { } fn check_using_directive( &mut self, _ctx: &LintContext<'_>, - _using: &'ast UsingDirective<'ast>, + _using: &'ast ast::UsingDirective<'ast>, ) { } - fn check_item_contract(&mut self, _ctx: &LintContext<'_>, _contract: &'ast ItemContract<'ast>) { + fn check_item_contract( + &mut self, + _ctx: &LintContext<'_>, + _contract: &'ast ast::ItemContract<'ast>, + ) { } fn check_doc_comment(&mut self, _ctx: &LintContext<'_>, _cmnt: &'ast ast::DocComment) {} // TODO: Add methods for each required AST node type /// Should be called after the source unit has been visited. Enables lints that require /// knowledge of the entire AST to perform their analysis. - fn check_full_source_unit(&mut self, _ctx: &LintContext<'ast>, _ast: &'ast SourceUnit<'ast>) {} + fn check_full_source_unit( + &mut self, + _ctx: &LintContext<'ast>, + _ast: &'ast ast::SourceUnit<'ast>, + ) { + } } /// Visitor struct for `EarlyLintPass`es @@ -113,12 +58,20 @@ pub struct EarlyLintVisitor<'a, 's, 'ast> { pub passes: &'a mut [Box + 's>], } -/// Extends the [`Visit`] trait functionality with a hook that can run after the initial traversal. -impl<'s, 'ast> EarlyLintVisitor<'_, 's, 'ast> +impl<'a, 's, 'ast> EarlyLintVisitor<'a, 's, 'ast> where 's: 'ast, { - pub fn post_source_unit(&mut self, ast: &'ast SourceUnit<'ast>) { + pub fn new( + ctx: &'a LintContext<'s>, + passes: &'a mut [Box + 's>], + ) -> Self { + Self { ctx, passes } + } + + /// Extends the [`Visit`] trait functionality with a hook that can run after the initial + /// traversal. + pub fn post_source_unit(&mut self, ast: &'ast ast::SourceUnit<'ast>) { for pass in self.passes.iter_mut() { pass.check_full_source_unit(self.ctx, ast); } @@ -138,7 +91,7 @@ where self.walk_doc_comment(cmnt) } - fn visit_expr(&mut self, expr: &'ast Expr<'ast>) -> ControlFlow { + fn visit_expr(&mut self, expr: &'ast ast::Expr<'ast>) -> ControlFlow { for pass in self.passes.iter_mut() { pass.check_expr(self.ctx, expr) } @@ -147,7 +100,7 @@ where fn visit_variable_definition( &mut self, - var: &'ast VariableDefinition<'ast>, + var: &'ast ast::VariableDefinition<'ast>, ) -> ControlFlow { for pass in self.passes.iter_mut() { pass.check_variable_definition(self.ctx, var) @@ -157,7 +110,7 @@ where fn visit_item_struct( &mut self, - strukt: &'ast ItemStruct<'ast>, + strukt: &'ast ast::ItemStruct<'ast>, ) -> ControlFlow { for pass in self.passes.iter_mut() { pass.check_item_struct(self.ctx, strukt) @@ -167,7 +120,7 @@ where fn visit_item_function( &mut self, - func: &'ast ItemFunction<'ast>, + func: &'ast ast::ItemFunction<'ast>, ) -> ControlFlow { for pass in self.passes.iter_mut() { pass.check_item_function(self.ctx, func) @@ -177,7 +130,7 @@ where fn visit_import_directive( &mut self, - import: &'ast ImportDirective<'ast>, + import: &'ast ast::ImportDirective<'ast>, ) -> ControlFlow { for pass in self.passes.iter_mut() { pass.check_import_directive(self.ctx, import); @@ -187,7 +140,7 @@ where fn visit_using_directive( &mut self, - using: &'ast UsingDirective<'ast>, + using: &'ast ast::UsingDirective<'ast>, ) -> ControlFlow { for pass in self.passes.iter_mut() { pass.check_using_directive(self.ctx, using); @@ -197,7 +150,7 @@ where fn visit_item_contract( &mut self, - contract: &'ast ItemContract<'ast>, + contract: &'ast ast::ItemContract<'ast>, ) -> ControlFlow { for pass in self.passes.iter_mut() { pass.check_item_contract(self.ctx, contract); diff --git a/crates/lint/src/linter/late.rs b/crates/lint/src/linter/late.rs new file mode 100644 index 0000000000000..597f228b92e34 --- /dev/null +++ b/crates/lint/src/linter/late.rs @@ -0,0 +1,198 @@ +use solar_interface::data_structures::Never; +use solar_sema::hir; +use std::ops::ControlFlow; + +use super::LintContext; + +/// Trait for lints that operate on the HIR (High-level Intermediate Representation). +/// Its methods mirror `hir::visit::Visit`, with the addition of `LintCotext`. +pub trait LateLintPass<'hir>: Send + Sync { + fn check_nested_source( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _id: hir::SourceId, + ) { + } + fn check_nested_item( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _id: &'hir hir::ItemId, + ) { + } + fn check_nested_contract( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _id: &'hir hir::ContractId, + ) { + } + fn check_nested_function( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _id: &'hir hir::FunctionId, + ) { + } + fn check_nested_var( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _id: &'hir hir::VariableId, + ) { + } + fn check_item( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _item: hir::Item<'hir, 'hir>, + ) { + } + fn check_contract( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _contract: &'hir hir::Contract<'hir>, + ) { + } + fn check_function( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _func: &'hir hir::Function<'hir>, + ) { + } + fn check_modifier( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _mod: &'hir hir::Modifier<'hir>, + ) { + } + fn check_var( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _var: &'hir hir::Variable<'hir>, + ) { + } + fn check_expr( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _expr: &'hir hir::Expr<'hir>, + ) { + } + fn check_call_args( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _args: &'hir hir::CallArgs<'hir>, + ) { + } + fn check_stmt( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _stmt: &'hir hir::Stmt<'hir>, + ) { + } + fn check_ty( + &mut self, + _ctx: &LintContext<'_>, + _hir: &'hir hir::Hir<'hir>, + _ty: &'hir hir::Type<'hir>, + ) { + } +} + +/// Visitor struct for `LateLintPass`es +pub struct LateLintVisitor<'a, 's, 'hir> { + ctx: &'a LintContext<'s>, + passes: &'a mut [Box + 's>], + hir: &'hir hir::Hir<'hir>, +} + +impl<'a, 's, 'hir> LateLintVisitor<'a, 's, 'hir> +where + 's: 'hir, +{ + pub fn new( + ctx: &'a LintContext<'s>, + passes: &'a mut [Box + 's>], + hir: &'hir hir::Hir<'hir>, + ) -> Self { + Self { ctx, passes, hir } + } +} + +impl<'s, 'hir> hir::Visit<'hir> for LateLintVisitor<'_, 's, 'hir> +where + 's: 'hir, +{ + type BreakValue = Never; + + fn hir(&self) -> &'hir hir::Hir<'hir> { + self.hir + } + + fn visit_nested_source(&mut self, id: hir::SourceId) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_nested_source(self.ctx, self.hir, id); + } + self.walk_nested_source(id) + } + + fn visit_contract( + &mut self, + contract: &'hir hir::Contract<'hir>, + ) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_contract(self.ctx, self.hir, contract); + } + self.walk_contract(contract) + } + + fn visit_function(&mut self, func: &'hir hir::Function<'hir>) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_function(self.ctx, self.hir, func); + } + self.walk_function(func) + } + + fn visit_item(&mut self, item: hir::Item<'hir, 'hir>) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_item(self.ctx, self.hir, item); + } + self.walk_item(item) + } + + fn visit_var(&mut self, var: &'hir hir::Variable<'hir>) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_var(self.ctx, self.hir, var); + } + self.walk_var(var) + } + + fn visit_expr(&mut self, expr: &'hir hir::Expr<'hir>) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_expr(self.ctx, self.hir, expr); + } + self.walk_expr(expr) + } + + fn visit_stmt(&mut self, stmt: &'hir hir::Stmt<'hir>) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_stmt(self.ctx, self.hir, stmt); + } + self.walk_stmt(stmt) + } + + fn visit_ty(&mut self, ty: &'hir hir::Type<'hir>) -> ControlFlow { + for pass in self.passes.iter_mut() { + pass.check_ty(self.ctx, self.hir, ty); + } + self.walk_ty(ty) + } +} diff --git a/crates/lint/src/linter/mod.rs b/crates/lint/src/linter/mod.rs new file mode 100644 index 0000000000000..f6770038eecf3 --- /dev/null +++ b/crates/lint/src/linter/mod.rs @@ -0,0 +1,174 @@ +mod early; +mod late; + +pub use early::{EarlyLintPass, EarlyLintVisitor}; +pub use late::{LateLintPass, LateLintVisitor}; + +use foundry_compilers::Language; +use foundry_config::lint::Severity; +use solar_interface::{ + Session, Span, + diagnostics::{DiagBuilder, DiagId, DiagMsg, MultiSpan, Style}, +}; +use solar_sema::ParsingContext; +use std::path::PathBuf; + +use crate::inline_config::InlineConfig; + +/// Trait representing a generic linter for analyzing and reporting issues in smart contract source +/// code files. A linter can be implemented for any smart contract language supported by Foundry. +/// +/// # Type Parameters +/// +/// - `Language`: Represents the target programming language. Must implement the [`Language`] trait. +/// - `Lint`: Represents the types of lints performed by the linter. Must implement the [`Lint`] +/// trait. +/// +/// # Required Methods +/// +/// - `init`: Creates a new solar `Session` with the appropiate linter configuration. +/// - `early_lint`: Scans the source files (using the AST) emitting a diagnostic for lints found. +/// - `late_lint`: Scans the source files (using the HIR) emitting a diagnostic for lints found. +pub trait Linter: Send + Sync + Clone { + type Language: Language; + type Lint: Lint; + + fn init(&self) -> Session; + fn early_lint<'sess>(&self, input: &[PathBuf], pcx: ParsingContext<'sess>); + fn late_lint<'sess>(&self, input: &[PathBuf], pcx: ParsingContext<'sess>); +} + +pub trait Lint { + fn id(&self) -> &'static str; + fn severity(&self) -> Severity; + fn description(&self) -> &'static str; + fn help(&self) -> &'static str; +} + +pub struct LintContext<'s> { + sess: &'s Session, + with_description: bool, + pub inline_config: InlineConfig, +} + +impl<'s> LintContext<'s> { + pub fn new(sess: &'s Session, with_description: bool, config: InlineConfig) -> Self { + Self { sess, with_description, inline_config: config } + } + + pub fn session(&self) -> &'s Session { + self.sess + } + + /// Helper method to emit diagnostics easily from passes + pub fn emit(&self, lint: &'static L, span: Span) { + if self.inline_config.is_disabled(span, lint.id()) { + return; + } + + let desc = if self.with_description { lint.description() } else { "" }; + let diag: DiagBuilder<'_, ()> = self + .sess + .dcx + .diag(lint.severity().into(), desc) + .code(DiagId::new_str(lint.id())) + .span(MultiSpan::from_span(span)) + .help(lint.help()); + + diag.emit(); + } + + /// Emit a diagnostic with a code fix proposal. + /// + /// For Diff snippets, if no span is provided, it will use the lint's span. + /// If unable to get code from the span, it will fall back to a Block snippet. + pub fn emit_with_fix(&self, lint: &'static L, span: Span, snippet: Snippet) { + if self.inline_config.is_disabled(span, lint.id()) { + return; + } + + // Convert the snippet to ensure we have the appropriate type + let snippet = match snippet { + Snippet::Diff { desc, span: diff_span, add } => { + // Use the provided span or fall back to the lint span + let target_span = diff_span.unwrap_or(span); + + // Check if we can get the original code + if self.span_to_snippet(target_span).is_some() { + Snippet::Diff { desc, span: Some(target_span), add } + } else { + // Fall back to Block if we can't get the original code + Snippet::Block { desc, code: add } + } + } + // Block snippets remain unchanged + block => block, + }; + + let desc = if self.with_description { lint.description() } else { "" }; + let diag: DiagBuilder<'_, ()> = self + .sess + .dcx + .diag(lint.severity().into(), desc) + .code(DiagId::new_str(lint.id())) + .span(MultiSpan::from_span(span)) + .highlighted_note(snippet.to_note(self)) + .help(lint.help()); + + diag.emit(); + } + + pub fn span_to_snippet(&self, span: Span) -> Option { + self.sess.source_map().span_to_snippet(span).ok() + } +} + +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum Snippet { + /// Represents a code block. Can have an optional description. + Block { desc: Option<&'static str>, code: String }, + /// Represents a code diff. Can have an optional description and a span for the code to remove. + Diff { desc: Option<&'static str>, span: Option, add: String }, +} + +impl Snippet { + pub fn to_note(self, ctx: &LintContext<'_>) -> Vec<(DiagMsg, Style)> { + let mut output = Vec::new(); + match self.desc() { + Some(desc) => { + output.push((DiagMsg::from(desc), Style::NoStyle)); + output.push((DiagMsg::from("\n\n"), Style::NoStyle)); + } + None => output.push((DiagMsg::from(" \n"), Style::NoStyle)), + } + match self { + Self::Diff { span, add, .. } => { + // Get the original code from the span if provided + if let Some(span) = span + && let Some(rmv) = ctx.span_to_snippet(span) + { + for line in rmv.lines() { + output.push((DiagMsg::from(format!("- {line}\n")), Style::Removal)); + } + } + for line in add.lines() { + output.push((DiagMsg::from(format!("+ {line}\n")), Style::Addition)); + } + } + Self::Block { code, .. } => { + for line in code.lines() { + output.push((DiagMsg::from(format!("- {line}\n")), Style::NoStyle)); + } + } + } + output.push((DiagMsg::from("\n"), Style::NoStyle)); + output + } + + pub fn desc(&self) -> Option<&'static str> { + match self { + Self::Diff { desc, .. } => *desc, + Self::Block { desc, .. } => *desc, + } + } +} diff --git a/crates/lint/src/sol/gas/keccak.rs b/crates/lint/src/sol/gas/keccak.rs index 0bebe5ea66558..0bb129aef50aa 100644 --- a/crates/lint/src/sol/gas/keccak.rs +++ b/crates/lint/src/sol/gas/keccak.rs @@ -1,32 +1,101 @@ use super::AsmKeccak256; use crate::{ - linter::{EarlyLintPass, LintContext}, + linter::{LateLintPass, LintContext}, sol::{Severity, SolLint}, }; -use solar_ast::{CallArgsKind, Expr, ExprKind}; +use solar_ast::{self as ast, Span}; use solar_interface::kw; +use solar_sema::hir::{self}; declare_forge_lint!( ASM_KECCAK256, Severity::Gas, "asm-keccak256", - "hash using inline assembly to save gas" + "use of inefficient hashing mechanism; consider using inline assembly" ); -impl<'ast> EarlyLintPass<'ast> for AsmKeccak256 { - fn check_expr(&mut self, ctx: &LintContext<'_>, expr: &'ast Expr<'ast>) { - if let ExprKind::Call(expr, args) = &expr.kind - && let ExprKind::Ident(ident) = &expr.kind - && ident.name == kw::Keccak256 - { - // Do not flag when hashing a single literal, as the compiler should optimize it - if let CallArgsKind::Unnamed(ref exprs) = args.kind - && exprs.len() == 1 - && let ExprKind::Lit(_, _) = exprs[0].kind - { - return; +impl<'hir> LateLintPass<'hir> for AsmKeccak256 { + fn check_stmt( + &mut self, + ctx: &LintContext<'_>, + hir: &'hir hir::Hir<'hir>, + stmt: &'hir hir::Stmt<'hir>, + ) { + let check_expr_and_emit_lint = + |expr: &'hir hir::Expr<'hir>, assign: Option, is_return: bool| { + if let Some(hash_arg) = extract_keccak256_arg(expr) { + self.emit_lint( + ctx, + hir, + stmt.span, + expr, + hash_arg, + AsmContext { _assign: assign, _is_return: is_return }, + ); + } + }; + + match stmt.kind { + hir::StmtKind::DeclSingle(var_id) => { + let var = hir.variable(var_id); + if let Some(init) = var.initializer { + // Constants should be optimized by the compiler, so no gas savings apply. + if !matches!(var.mutability, Some(hir::VarMut::Constant)) { + check_expr_and_emit_lint(init, var.name, false); + } + } } - ctx.emit(&ASM_KECCAK256, expr.span); + // Expressions that don't (directly) assign to a variable + hir::StmtKind::Expr(expr) + | hir::StmtKind::Emit(expr) + | hir::StmtKind::Revert(expr) + | hir::StmtKind::DeclMulti(_, expr) + | hir::StmtKind::If(expr, ..) => check_expr_and_emit_lint(expr, None, false), + hir::StmtKind::Return(Some(expr)) => check_expr_and_emit_lint(expr, None, true), + _ => (), } } } + +impl AsmKeccak256 { + /// Emits lints (when possible with fix suggestions) for inefficient `keccak256` calls. + fn emit_lint( + &self, + ctx: &LintContext<'_>, + _hir: &hir::Hir<'_>, + _stmt_span: Span, + call: &hir::Expr<'_>, + _hash: &hir::Expr<'_>, + _asm_ctx: AsmContext, + ) { + ctx.emit(&ASM_KECCAK256, call.span); + } +} + +/// If the expression is a call to `keccak256` with one argument, returns that argument. +fn extract_keccak256_arg<'hir>(expr: &'hir hir::Expr<'hir>) -> Option<&'hir hir::Expr<'hir>> { + let hir::ExprKind::Call( + callee, + hir::CallArgs { kind: hir::CallArgsKind::Unnamed(args), .. }, + .., + ) = &expr.kind + else { + return None; + }; + + let is_keccak = if let hir::ExprKind::Ident([hir::Res::Builtin(builtin)]) = callee.kind { + matches!(builtin.name(), kw::Keccak256) + } else { + return None; + }; + + if is_keccak && args.len() == 1 { Some(&args[0]) } else { None } +} + +// -- HELPER FUNCTIONS AND STRUCTS ---------------------------------------------------------------- + +#[derive(Debug, Clone, Copy)] +struct AsmContext { + _assign: Option, + _is_return: bool, +} diff --git a/crates/lint/src/sol/gas/mod.rs b/crates/lint/src/sol/gas/mod.rs index 69bc9422f57f3..9664f3baf6dcc 100644 --- a/crates/lint/src/sol/gas/mod.rs +++ b/crates/lint/src/sol/gas/mod.rs @@ -1,6 +1,6 @@ -use crate::sol::{EarlyLintPass, SolLint}; +use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod keccak; use keccak::ASM_KECCAK256; -register_lints!((AsmKeccak256, (ASM_KECCAK256))); +register_lints!((AsmKeccak256, late, (ASM_KECCAK256))); diff --git a/crates/lint/src/sol/high/mod.rs b/crates/lint/src/sol/high/mod.rs index 110b60bfe6ae7..b27907dd1e4a9 100644 --- a/crates/lint/src/sol/high/mod.rs +++ b/crates/lint/src/sol/high/mod.rs @@ -1,4 +1,4 @@ -use crate::sol::{EarlyLintPass, SolLint}; +use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod incorrect_shift; mod unchecked_calls; @@ -7,7 +7,7 @@ use incorrect_shift::INCORRECT_SHIFT; use unchecked_calls::{ERC20_UNCHECKED_TRANSFER, UNCHECKED_CALL}; register_lints!( - (IncorrectShift, (INCORRECT_SHIFT)), - (UncheckedCall, (UNCHECKED_CALL)), - (UncheckedTransferERC20, (ERC20_UNCHECKED_TRANSFER)) + (IncorrectShift, early, (INCORRECT_SHIFT)), + (UncheckedCall, early, (UNCHECKED_CALL)), + (UncheckedTransferERC20, early, (ERC20_UNCHECKED_TRANSFER)) ); diff --git a/crates/lint/src/sol/high/unchecked_calls.rs b/crates/lint/src/sol/high/unchecked_calls.rs index 59db6fbe1f429..65eef5fee6cb2 100644 --- a/crates/lint/src/sol/high/unchecked_calls.rs +++ b/crates/lint/src/sol/high/unchecked_calls.rs @@ -24,6 +24,7 @@ declare_forge_lint!( // -- ERC20 UNCKECKED TRANSFERS ------------------------------------------------------------------- /// WARN: can issue false positives. It does not check that the contract being called is an ERC20. +/// TODO: re-implement using `LateLintPass` so that it can't issue false positives. impl<'ast> EarlyLintPass<'ast> for UncheckedTransferERC20 { fn check_item_function(&mut self, ctx: &LintContext<'_>, func: &'ast ItemFunction<'ast>) { if let Some(body) = &func.body { diff --git a/crates/lint/src/sol/info/mod.rs b/crates/lint/src/sol/info/mod.rs index bf1e18af2fa0e..50f6122325427 100644 --- a/crates/lint/src/sol/info/mod.rs +++ b/crates/lint/src/sol/info/mod.rs @@ -1,4 +1,4 @@ -use crate::sol::{EarlyLintPass, SolLint}; +use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod mixed_case; use mixed_case::{MIXED_CASE_FUNCTION, MIXED_CASE_VARIABLE}; @@ -13,9 +13,9 @@ mod imports; use imports::{UNALIASED_PLAIN_IMPORT, UNUSED_IMPORT}; register_lints!( - (PascalCaseStruct, (PASCAL_CASE_STRUCT)), - (MixedCaseVariable, (MIXED_CASE_VARIABLE)), - (MixedCaseFunction, (MIXED_CASE_FUNCTION)), - (ScreamingSnakeCase, (SCREAMING_SNAKE_CASE_CONSTANT, SCREAMING_SNAKE_CASE_IMMUTABLE)), - (Imports, (UNALIASED_PLAIN_IMPORT, UNUSED_IMPORT)) + (PascalCaseStruct, early, (PASCAL_CASE_STRUCT)), + (MixedCaseVariable, early, (MIXED_CASE_VARIABLE)), + (MixedCaseFunction, early, (MIXED_CASE_FUNCTION)), + (ScreamingSnakeCase, early, (SCREAMING_SNAKE_CASE_CONSTANT, SCREAMING_SNAKE_CASE_IMMUTABLE)), + (Imports, early, (UNALIASED_PLAIN_IMPORT, UNUSED_IMPORT)) ); diff --git a/crates/lint/src/sol/macros.rs b/crates/lint/src/sol/macros.rs index 2606ddc048ec9..d1e393e4c4017 100644 --- a/crates/lint/src/sol/macros.rs +++ b/crates/lint/src/sol/macros.rs @@ -36,43 +36,98 @@ macro_rules! declare_forge_lint { }; } -/// Registers Solidity linter passes with their corresponding `SolLint`. +/// Registers Solidity linter passes that can have both early and late variants. /// /// # Parameters /// -/// - `$pass_id`: Identifier of the generated struct that will implement the pass trait. -/// - (`$lint`): tuple with `SolLint` constants that should be evaluated on every input that pass. +/// Each pass is declared with: +/// - `$pass_id`: Identifier of the generated struct that will implement the pass trait(s). +/// - `$pass_type`: Either `early`, `late`, or `both` to indicate which traits to implement. +/// - `$lints`: A parenthesized, comma-separated list of `SolLint` constants. /// /// # Outputs /// -/// - Structs for each linting pass (which should manually implement `EarlyLintPass`) +/// - Structs for each linting pass +/// - Helper methods to create early and late passes with required lifetimes /// - `const REGISTERED_LINTS` containing all registered lint objects -/// - `const LINT_PASSES` mapping each lint to its corresponding pass #[macro_export] macro_rules! register_lints { - ( $( ($pass_id:ident, ($($lint:expr),+ $(,)?)) ),* $(,)? ) => { - // Declare the structs that will implement the pass trait + // 1. Internal rule for declaring structs. + ( @declare_structs $( ($pass_id:ident, $pass_type:ident, $lints:tt) ),* $(,)? ) => { $( #[derive(Debug, Default, Clone, Copy, Eq, PartialEq)] pub struct $pass_id; impl $pass_id { - pub fn as_lint_pass<'a>() -> Box> { - Box::new(Self::default()) - } + register_lints!(@early_impl $pass_id, $pass_type); + register_lints!(@late_impl $pass_id, $pass_type); } )* + }; - // Expose array constants - pub const REGISTERED_LINTS: &[SolLint] = &[$( $($lint,) + )*]; - pub const LINT_PASSES: &[(SolLint, fn() -> Box>)] = &[ - $( $( ($lint, || Box::new($pass_id::default())), )+ )* + // 2. Internal rule for declaring the const array. + ( @declare_consts $( ($pass_id:ident, $pass_type:ident, ($($lint:expr),* $(,)?)) ),* $(,)? ) => { + pub const REGISTERED_LINTS: &[SolLint] = &[ + $( + $($lint,)* + )* ]; + }; + + // 3. Internal rule for declaring the helper functions. + ( @declare_funcs $( ($pass_id:ident, $pass_type:ident, $lints:tt) ),* $(,)? ) => { + pub fn create_early_lint_passes<'a>() -> Vec<(Box>, SolLint)> { + vec![ + $( + register_lints!(@early_create $pass_id, $pass_type, $lints), + )* + ] + .into_iter() + .flatten() + .collect() + } + + pub fn create_late_lint_passes<'hir>() -> Vec<(Box>, SolLint)> { + vec![ + $( + register_lints!(@late_create $pass_id, $pass_type, $lints), + )* + ] + .into_iter() + .flatten() + .collect() + } + }; + + // --- HELPERS ------------------------------------------------------------ + (@early_impl $_pass_id:ident, late) => {}; + (@early_impl $pass_id:ident, $other:ident) => { + pub fn as_early_lint_pass<'a>() -> Box> { + Box::new(Self::default()) + } + }; - // Helper function to create lint passes with the required lifetime - pub fn create_lint_passes<'a>() -> Vec<(Box>, SolLint)> - { - vec![ $( $(($pass_id::as_lint_pass(), $lint), )+ )* ] + (@late_impl $_pass_id:ident, early) => {}; + (@late_impl $pass_id:ident, $other:ident) => { + pub fn as_late_lint_pass<'hir>() -> Box> { + Box::new(Self::default()) } }; + + (@early_create $_pass_id:ident, late, $_lints:tt) => { vec![] }; + (@early_create $pass_id:ident, $other:ident, ($($lint:expr),*)) => { + vec![ $(($pass_id::as_early_lint_pass(), $lint)),* ] + }; + + (@late_create $_pass_id:ident, early, $_lints:tt) => { vec![] }; + (@late_create $pass_id:ident, $other:ident, ($($lint:expr),*)) => { + vec![ $(($pass_id::as_late_lint_pass(), $lint)),* ] + }; + + // --- ENTRY POINT --------------------------------------------------------- + ( $($tokens:tt)* ) => { + register_lints! { @declare_structs $($tokens)* } + register_lints! { @declare_consts $($tokens)* } + register_lints! { @declare_funcs $($tokens)* } + }; } diff --git a/crates/lint/src/sol/med/mod.rs b/crates/lint/src/sol/med/mod.rs index 5afc21772ca06..35e55f594fa8f 100644 --- a/crates/lint/src/sol/med/mod.rs +++ b/crates/lint/src/sol/med/mod.rs @@ -1,6 +1,6 @@ -use crate::sol::{EarlyLintPass, SolLint}; +use crate::sol::{EarlyLintPass, LateLintPass, SolLint}; mod div_mul; use div_mul::DIVIDE_BEFORE_MULTIPLY; -register_lints!((DivideBeforeMultiply, (DIVIDE_BEFORE_MULTIPLY))); +register_lints!((DivideBeforeMultiply, early, (DIVIDE_BEFORE_MULTIPLY))); diff --git a/crates/lint/src/sol/mod.rs b/crates/lint/src/sol/mod.rs index 75d4999b2caa8..0d357a3e9404b 100644 --- a/crates/lint/src/sol/mod.rs +++ b/crates/lint/src/sol/mod.rs @@ -1,19 +1,26 @@ use crate::{ inline_config::{InlineConfig, InlineConfigItem}, - linter::{EarlyLintPass, EarlyLintVisitor, Lint, LintContext, Linter}, + linter::{ + EarlyLintPass, EarlyLintVisitor, LateLintPass, LateLintVisitor, Lint, LintContext, Linter, + }, }; use foundry_common::comments::Comments; use foundry_compilers::{ProjectPathsConfig, solc::SolcLanguage}; use foundry_config::lint::Severity; -use rayon::iter::{IntoParallelIterator, ParallelIterator}; -use solar_ast::{Arena, SourceUnit, visit::Visit}; +use rayon::iter::{ParallelBridge, ParallelIterator}; +use solar_ast::{self as ast, visit::Visit as VisitAST}; use solar_interface::{ Session, SourceMap, diagnostics::{self, DiagCtxt, JsonEmitter}, + source_map::{FileName, SourceFile}, +}; +use solar_sema::{ + ParsingContext, + hir::{self, Visit as VisitHIR}, }; use std::{ path::{Path, PathBuf}, - sync::Arc, + sync::{Arc, LazyLock}, }; use thiserror::Error; @@ -25,6 +32,15 @@ pub mod high; pub mod info; pub mod med; +static ALL_REGISTERED_LINTS: LazyLock> = LazyLock::new(|| { + let mut lints = Vec::new(); + lints.extend_from_slice(high::REGISTERED_LINTS); + lints.extend_from_slice(med::REGISTERED_LINTS); + lints.extend_from_slice(info::REGISTERED_LINTS); + lints.extend_from_slice(gas::REGISTERED_LINTS); + lints.into_iter().map(|lint| lint.id()).collect() +}); + /// Linter implementation to analyze Solidity source code responsible for identifying /// vulnerabilities gas optimizations, and best practices. #[derive(Debug, Clone)] @@ -74,67 +90,88 @@ impl SolidityLinter { self } - fn process_file(&self, sess: &Session, path: &Path) { - let arena = Arena::new(); - - let _ = sess.enter(|| -> Result<(), diagnostics::ErrorGuaranteed> { - // Initialize the parser, get the AST, and process the inline-config - let mut parser = solar_parse::Parser::from_file(sess, &arena, path)?; - let file = sess - .source_map() - .load_file(path) - .map_err(|e| sess.dcx.err(e.to_string()).emit())?; - let ast = parser.parse_file().map_err(|e| e.emit())?; - - // Declare all available passes and lints - let mut passes_and_lints = Vec::new(); - passes_and_lints.extend(high::create_lint_passes()); - passes_and_lints.extend(med::create_lint_passes()); - passes_and_lints.extend(info::create_lint_passes()); - - // Do not apply gas-severity rules on tests and scripts - if !self.path_config.is_test_or_script(path) { - passes_and_lints.extend(gas::create_lint_passes()); - } + fn include_lint(&self, lint: SolLint) -> bool { + self.severity.as_ref().is_none_or(|sev| sev.contains(&lint.severity())) + && self.lints_included.as_ref().is_none_or(|incl| incl.contains(&lint)) + && !self.lints_excluded.as_ref().is_some_and(|excl| excl.contains(&lint)) + } - // Filter based on linter config - let (lints, mut passes): (Vec<&str>, Vec>>) = - passes_and_lints - .into_iter() - .filter_map(|(pass, lint)| { - let matches_severity = match self.severity { - Some(ref target) => target.contains(&lint.severity()), - None => true, - }; - let matches_lints_inc = match self.lints_included { - Some(ref target) => target.contains(&lint), - None => true, - }; - let matches_lints_exc = match self.lints_excluded { - Some(ref target) => target.contains(&lint), - None => false, - }; - - if matches_severity && matches_lints_inc && !matches_lints_exc { - Some((lint.id(), pass)) - } else { - None - } - }) - .unzip(); - - // Process the inline-config - let comments = Comments::new(&file); - let inline_config = parse_inline_config(sess, &comments, &lints, &ast); - - // Initialize and run the visitor - let ctx = LintContext::new(sess, self.with_description, inline_config); - let mut visitor = EarlyLintVisitor { ctx: &ctx, passes: &mut passes }; - _ = visitor.visit_source_unit(&ast); - visitor.post_source_unit(&ast); + fn process_source_ast<'ast>( + &self, + sess: &'ast Session, + ast: &'ast ast::SourceUnit<'ast>, + file: &SourceFile, + path: &Path, + ) -> Result<(), diagnostics::ErrorGuaranteed> { + // Declare all available passes and lints + let mut passes_and_lints = Vec::new(); + passes_and_lints.extend(high::create_early_lint_passes()); + passes_and_lints.extend(med::create_early_lint_passes()); + passes_and_lints.extend(info::create_early_lint_passes()); + + // Do not apply gas-severity rules on tests and scripts + if !self.path_config.is_test_or_script(path) { + passes_and_lints.extend(gas::create_early_lint_passes()); + } - Ok(()) - }); + // Filter passes based on linter config + let mut passes: Vec>> = passes_and_lints + .into_iter() + .filter_map(|(pass, lint)| if self.include_lint(lint) { Some(pass) } else { None }) + .collect(); + + // Process the inline-config + let comments = Comments::new(file); + let inline_config = parse_inline_config(sess, &comments, InlineConfigSource::Ast(ast)); + + // Initialize and run the early lint visitor + let ctx = LintContext::new(sess, self.with_description, inline_config); + let mut early_visitor = EarlyLintVisitor::new(&ctx, &mut passes); + _ = early_visitor.visit_source_unit(ast); + early_visitor.post_source_unit(ast); + + Ok(()) + } + + fn process_source_hir<'hir>( + &self, + sess: &Session, + gcx: &solar_sema::ty::Gcx<'hir>, + source_id: hir::SourceId, + file: &'hir SourceFile, + ) -> Result<(), diagnostics::ErrorGuaranteed> { + // Declare all available passes and lints + let mut passes_and_lints = Vec::new(); + passes_and_lints.extend(high::create_late_lint_passes()); + passes_and_lints.extend(med::create_late_lint_passes()); + passes_and_lints.extend(info::create_late_lint_passes()); + + // Do not apply gas-severity rules on tests and scripts + if let FileName::Real(ref path) = file.name + && !self.path_config.is_test_or_script(path) + { + passes_and_lints.extend(gas::create_late_lint_passes()); + } + + // Filter passes based on config + let mut passes: Vec>> = passes_and_lints + .into_iter() + .filter_map(|(pass, lint)| if self.include_lint(lint) { Some(pass) } else { None }) + .collect(); + + // Process the inline-config + let comments = Comments::new(file); + let inline_config = + parse_inline_config(sess, &comments, InlineConfigSource::Hir((&gcx.hir, source_id))); + + // Run late lint visitor + let ctx = LintContext::new(sess, self.with_description, inline_config); + let mut late_visitor = LateLintVisitor::new(&ctx, &mut passes, &gcx.hir); + + // Visit this specific source + _ = late_visitor.visit_nested_source(source_id); + + Ok(()) } } @@ -142,10 +179,9 @@ impl Linter for SolidityLinter { type Language = SolcLanguage; type Lint = SolLint; - fn lint(&self, input: &[PathBuf]) { + /// Build solar session based on the linter config + fn init(&self) -> Session { let mut builder = Session::builder(); - - // Build session based on the linter config if self.with_json_emitter { let map = Arc::::default(); let json_emitter = JsonEmitter::new(Box::new(std::io::stderr()), map.clone()) @@ -160,21 +196,71 @@ impl Linter for SolidityLinter { // Create a single session for all files let mut sess = builder.build(); sess.dcx = sess.dcx.set_flags(|flags| flags.track_diagnostics = false); + sess + } - // Process the files in parallel - sess.enter_parallel(|| { - input.into_par_iter().for_each(|file| { - self.process_file(&sess, file); + /// Run AST-based lints + fn early_lint<'sess>(&self, input: &[PathBuf], mut pcx: ParsingContext<'sess>) { + let sess = pcx.sess; + _ = sess.enter_parallel(|| -> Result<(), diagnostics::ErrorGuaranteed> { + // Load all files into the parsing ctx + pcx.load_files(input)?; + + // Parse the sources + let ast_arena = solar_sema::thread_local::ThreadLocal::new(); + let ast_result = pcx.parse(&ast_arena); + + // Process each source in parallel + ast_result.sources.iter().par_bridge().for_each(|source| { + if let (FileName::Real(path), Some(ast)) = (&source.file.name, &source.ast) + && input.iter().any(|input_path| path.ends_with(input_path)) + { + _ = self.process_source_ast(sess, ast, &source.file, path) + } }); + + Ok(()) + }); + } + + /// Run HIR-based lints + fn late_lint<'sess>(&self, input: &[PathBuf], mut pcx: ParsingContext<'sess>) { + let sess = pcx.sess; + _ = sess.enter_parallel(|| -> Result<(), diagnostics::ErrorGuaranteed> { + // Load all files into the parsing ctx + pcx.load_files(input)?; + + // Parse and lower to HIR + let hir_arena = solar_sema::thread_local::ThreadLocal::new(); + let hir_result = pcx.parse_and_lower(&hir_arena); + + if let Ok(Some(gcx_wrapper)) = hir_result { + let gcx = gcx_wrapper.get(); + + // Process each source in parallel + gcx.hir.sources_enumerated().par_bridge().for_each(|(source_id, source)| { + if let FileName::Real(ref path) = source.file.name + && input.iter().any(|input_path| path.ends_with(input_path)) + { + _ = self.process_source_hir(sess, &gcx, source_id, &source.file); + } + }); + } + + Ok(()) }); } } -fn parse_inline_config<'ast>( +enum InlineConfigSource<'ast, 'hir> { + Ast(&'ast ast::SourceUnit<'ast>), + Hir((&'hir hir::Hir<'hir>, hir::SourceId)), +} + +fn parse_inline_config<'ast, 'hir>( sess: &Session, comments: &Comments, - lints: &[&'static str], - ast: &'ast SourceUnit<'ast>, + source: InlineConfigSource<'ast, 'hir>, ) -> InlineConfig { let items = comments.iter().filter_map(|comment| { let mut item = comment.lines.first()?.as_str(); @@ -186,7 +272,7 @@ fn parse_inline_config<'ast>( } let item = item.trim_start().strip_prefix("forge-lint:")?.trim(); let span = comment.span; - match InlineConfigItem::parse(item, lints) { + match InlineConfigItem::parse(item, &ALL_REGISTERED_LINTS) { Ok(item) => Some((span, item)), Err(e) => { sess.dcx.warn(e.to_string()).span(span).emit(); @@ -195,7 +281,12 @@ fn parse_inline_config<'ast>( } }); - InlineConfig::new(items, ast, sess.source_map()) + match source { + InlineConfigSource::Ast(ast) => InlineConfig::from_ast(items, ast, sess.source_map()), + InlineConfigSource::Hir((hir, id)) => { + InlineConfig::from_hir(items, hir, id, sess.source_map()) + } + } } #[derive(Error, Debug)] diff --git a/crates/lint/testdata/Imports.sol b/crates/lint/testdata/Imports.sol index 5b91c9c459265..c017b3ba8bfdc 100644 --- a/crates/lint/testdata/Imports.sol +++ b/crates/lint/testdata/Imports.sol @@ -9,45 +9,45 @@ import { docSymbol2, docSymbolWrongTag, //~NOTE: unused imports should be removed eventSymbol, - overrideSymbol, + BaseContract, symbolNotUsed, //~NOTE: unused imports should be removed IContract, IContractNotUsed //~NOTE: unused imports should be removed -} from "File.sol"; +} from "./auxiliary/ImportsFile.sol"; // forge-lint: disable-next-item import { symbolNotUsed2 -} from "File.sol"; +} from "./auxiliary/ImportsFile.sol"; // in this case, disabling the following line doesn't do anything // forge-lint: disable-next-line import { symbolNotUsed3 //~NOTE: unused imports should be removed -} from "File.sol"; +} from "./auxiliary/ImportsFile.sol"; import { CONSTANT_0, CONSTANT_1 //~NOTE: unused imports should be removed -} from "Constants.sol"; +} from "./auxiliary/ImportsConstants.sol"; import { - MyTpe, + MyType, MyOtherType, YetAnotherType //~NOTE: unused imports should be removed -} from "Types.sol"; +} from "./auxiliary/ImportsTypes.sol"; -import "SomeFile.sol"; //~NOTE: use named imports '{A, B}' or alias 'import ".." as X' -import "AnotherFile.sol"; //~NOTE: use named imports '{A, B}' or alias 'import ".." as X' +import "./auxiliary/ImportsSomeFile.sol"; //~NOTE: use named imports '{A, B}' or alias 'import ".." as X' +import "./auxiliary/ImportsAnotherFile.sol"; //~NOTE: use named imports '{A, B}' or alias 'import ".." as X' -import "some_file_2.sol" as SomeFile2; -import "another_file_2.sol" as AnotherFile2; //~NOTE: unused imports should be removed +import "./auxiliary/ImportsSomeFile2.sol" as SomeFile2; +import "./auxiliary/ImportsAnotherFile2.sol" as AnotherFile2; //~NOTE: unused imports should be removed -import * as Utils from "utils.sol"; -import * as OtherUtils from "utils2.sol"; //~NOTE: unused imports should be removed +import * as Utils from "./auxiliary/ImportsUtils.sol"; +import * as OtherUtils from "./auxiliary/ImportsUtils2.sol"; //~NOTE: unused imports should be removed -contract UnusedImport is IContract { +contract UnusedImport is IContract, BaseContract { using mySymbol for address; /// @inheritdoc docSymbol @@ -66,14 +66,14 @@ contract UnusedImport is IContract { SomeFile2.Baz public myStruct2; symbol4 public myVar; - function foo(uint256 a, symbol5 b) public view override(overrideSymbol) returns (uint256) { - emit eventSymbol.foo(c); + function foo(uint256 a, symbol5 b) public view override(BaseContract) returns (uint256) { uint256 c = Utils.calculate(a, b); + emit eventSymbol.foo(c); return c; } function convert(address addr) public pure returns (MyOtherType) { - MyType a = MyTpe.wrap(123); + MyType a = MyType.wrap(123); return MyOtherType.wrap(a); } } diff --git a/crates/lint/testdata/Imports.stderr b/crates/lint/testdata/Imports.stderr index e8b9365c9ff37..d9f4d89e87b56 100644 --- a/crates/lint/testdata/Imports.stderr +++ b/crates/lint/testdata/Imports.stderr @@ -1,16 +1,16 @@ note[unaliased-plain-import]: use named imports '{A, B}' or alias 'import ".." as X' --> ROOT/testdata/Imports.sol:LL:CC | -40 | import "SomeFile.sol"; - | -------------- +40 | import "./auxiliary/ImportsSomeFile.sol"; + | --------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unaliased-plain-import note[unaliased-plain-import]: use named imports '{A, B}' or alias 'import ".." as X' --> ROOT/testdata/Imports.sol:LL:CC | -41 | import "AnotherFile.sol"; - | ----------------- +41 | import "./auxiliary/ImportsAnotherFile.sol"; + | ------------------------------------ | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unaliased-plain-import @@ -73,16 +73,16 @@ note[unused-import]: unused imports should be removed note[unused-import]: unused imports should be removed --> ROOT/testdata/Imports.sol:LL:CC | -44 | import "another_file_2.sol" as AnotherFile2; - | -------------------------------------------- +44 | import "./auxiliary/ImportsAnotherFile2.sol" as AnotherFile2; + | ------------------------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unused-import note[unused-import]: unused imports should be removed --> ROOT/testdata/Imports.sol:LL:CC | -47 | import * as OtherUtils from "utils2.sol"; - | ----------------------------------------- +47 | import * as OtherUtils from "./auxiliary/ImportsUtils2.sol"; + | ------------------------------------------------------------ | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unused-import diff --git a/crates/lint/testdata/Keccak256.sol b/crates/lint/testdata/Keccak256.sol index f955939136476..bc63e08dd4b6c 100644 --- a/crates/lint/testdata/Keccak256.sol +++ b/crates/lint/testdata/Keccak256.sol @@ -2,32 +2,33 @@ contract AsmKeccak256 { // constants are optimized by the compiler bytes32 constant HASH = keccak256("hello"); - bytes32 constant OTHER_HASH = keccak256(1234); + bytes32 constant OTHER_HASH = keccak256(0x1234); - constructor(uint256 a, uint256 b) { + constructor(uint256 a, uint256 b, address c) { // forge-lint: disable-next-line(asm-keccak256) keccak256(abi.encodePacked(a, b)); keccak256(abi.encodePacked(a, b)); // forge-lint: disable-line(asm-keccak256) // lints fire before the disabled block - keccak256(abi.encodePacked(a, b)); // before disabled block + address c = address(1); + bytes32 hash = keccak256(abi.encodePacked(a, b, bytes32(c))); //~NOTE: inefficient hashing mechanism uint256 MixedCase_Variable = 1; //~NOTE: mutable variables should use mixedCase - // forge-lint: disable-start(asm-keccak256) ------------------------------------ - keccak256(abi.encodePacked(a, b)); // | + // forge-lint: disable-start(asm-keccak256) ------------------------------------- + keccak256(abi.encodePacked(a, b)); // disabled | // | // non-disabled lints still fire | uint256 Another_MixedCase = 2; //~NOTE: mutable variables should use mixedCase // | - // forge-lint: disable-start(asm-keccak256) --- | - keccak256(abi.encodePacked(a, b)); // | | - // | | - // forge-lint: disable-end(asm-keccak256) ----- | - // forge-lint: disable-end(asm-keccak256) -------------------------------------- + // forge-lint: disable-start(asm-keccak256) ------- | + keccak256(abi.encodePacked(a, b)); // disabled | | + // | | + // forge-lint: disable-end(asm-keccak256) --------- | + // forge-lint: disable-end(asm-keccak256) --------------------------------------- // lints still fire after the disabled block - keccak256(abi.encodePacked(a, b)); // after disabled block + bytes32 afterDisabledBlock = keccak256(abi.encode(a, b, c)); //~NOTE: inefficient hashing mechanism uint256 YetAnother_MixedCase = 3; //~NOTE: mutable variables should use mixedCase } @@ -37,9 +38,12 @@ contract AsmKeccak256 { return keccak256(abi.encodePacked(a, b)); } - function solidityHash(uint256 a, uint256 b) public view returns (bytes32) { - bytes32 hash = keccak256(a); //~NOTE: hash using inline assembly to save gas - return keccak256(abi.encodePacked(a, b)); //~NOTE: hash using inline assembly to save gas + function solidityHash(bytes calldata z, uint256 a, uint256 b, address c) public view returns (bytes32) { + bytes32 loadsFromCalldata = keccak256(z); //~NOTE: inefficient hashing mechanism + bytes memory y = z; + bytes32 loadsFromMemory = keccak256(y); //~NOTE: inefficient hashing mechanism + bytes32 lintWithoutFix = keccak256(abi.encodePacked(a, b, c)); //~NOTE: inefficient hashing mechanism + return keccak256(abi.encode(a, b, c)); //~NOTE: inefficient hashing mechanism } function assemblyHash(uint256 a, uint256 b) public view returns (bytes32){ @@ -57,7 +61,7 @@ contract OtherAsmKeccak256 { uint256 Enabled_MixedCase_Variable; //~NOTE: mutable variables should use mixedCase function contratDisabledHash(uint256 a, uint256 b) public view returns (bytes32) { - return keccak256(abi.encodePacked(a, b)); + return keccak256(abi.encode(a, b)); } function contratDisabledHash2(uint256 a, uint256 b) public view returns (bytes32) { @@ -66,8 +70,10 @@ contract OtherAsmKeccak256 { } contract YetAnotherAsmKeccak256 { - function nonDisabledHash(uint256 a, uint256 b) public view returns (bytes32) { - return keccak256(abi.encodePacked(a, b)); //~NOTE: hash using inline assembly to save gas + function nonDisabledHash(uint256 x, uint256 y) public view returns (bytes32) { + bytes32 doesNotUseScratchSpace = keccak256(abi.encode(x, y, x, y, x, y)); //~NOTE: inefficient hashing mechanism + bytes32 doesUseScratchSpace = keccak256(abi.encode(x)); //~NOTE: inefficient hashing mechanism + return keccak256(abi.encode(doesUseScratchSpace, doesNotUseScratchSpace)); //~NOTE: inefficient hashing mechanism } // forge-lint: disable-next-item(asm-keccak256) diff --git a/crates/lint/testdata/Keccak256.stderr b/crates/lint/testdata/Keccak256.stderr index 6fbf464bdb38f..32cab3039107d 100644 --- a/crates/lint/testdata/Keccak256.stderr +++ b/crates/lint/testdata/Keccak256.stderr @@ -1,80 +1,152 @@ -note[asm-keccak256]: hash using inline assembly to save gas +note[mixed-case-variable]: mutable variables should use mixedCase --> ROOT/testdata/Keccak256.sol:LL:CC | -14 | keccak256(abi.encodePacked(a, b)); // before disabled block - | --------- +16 | uint256 MixedCase_Variable = 1; + | ------------------ | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable note[mixed-case-variable]: mutable variables should use mixedCase --> ROOT/testdata/Keccak256.sol:LL:CC | -15 | uint256 MixedCase_Variable = 1; - | ------------------ +22 | uint256 Another_MixedCase = 2; + | ----------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable note[mixed-case-variable]: mutable variables should use mixedCase --> ROOT/testdata/Keccak256.sol:LL:CC | -21 | uint256 Another_MixedCase = 2; - | ----------------- +32 | uint256 YetAnother_MixedCase = 3; + | -------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable -note[asm-keccak256]: hash using inline assembly to save gas +note[mixed-case-variable]: mutable variables should use mixedCase --> ROOT/testdata/Keccak256.sol:LL:CC | -30 | keccak256(abi.encodePacked(a, b)); // after disabled block - | --------- +61 | uint256 Enabled_MixedCase_Variable; + | -------------------------- | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable note[mixed-case-variable]: mutable variables should use mixedCase --> ROOT/testdata/Keccak256.sol:LL:CC | -31 | uint256 YetAnother_MixedCase = 3; - | -------------------- +81 | uint256 Enabled_MixedCase_Variable = 1; + | -------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable -note[asm-keccak256]: hash using inline assembly to save gas +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly --> ROOT/testdata/Keccak256.sol:LL:CC | -41 | bytes32 hash = keccak256(a); - | --------- +15 | bytes32 hash = keccak256(abi.encodePacked(a, b, bytes32(c))); + | --------------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 -note[asm-keccak256]: hash using inline assembly to save gas +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly --> ROOT/testdata/Keccak256.sol:LL:CC | -42 | return keccak256(abi.encodePacked(a, b)); - | --------- +31 | bytes32 afterDisabledBlock = keccak256(abi.encode(a, b, c)); + | ------------------------------ | = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 -note[mixed-case-variable]: mutable variables should use mixedCase +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly --> ROOT/testdata/Keccak256.sol:LL:CC | -57 | uint256 Enabled_MixedCase_Variable; - | -------------------------- +37 | bytes32 hash = keccak256(a); + | ------------ | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 -note[asm-keccak256]: hash using inline assembly to save gas +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly --> ROOT/testdata/Keccak256.sol:LL:CC | -70 | return keccak256(abi.encodePacked(a, b)); - | --------- +38 | return keccak256(abi.encodePacked(a, b)); + | --------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 -note[mixed-case-variable]: mutable variables should use mixedCase +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly --> ROOT/testdata/Keccak256.sol:LL:CC | -75 | uint256 Enabled_MixedCase_Variable = 1; - | -------------------------- +42 | bytes32 loadsFromCalldata = keccak256(z); + | ------------ | - = help: https://book.getfoundry.sh/reference/forge/forge-lint#mixed-case-variable + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +44 | bytes32 loadsFromMemory = keccak256(y); + | ------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +45 | bytes32 lintWithoutFix = keccak256(abi.encodePacked(a, b, c)); + | ------------------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +46 | return keccak256(abi.encode(a, b, c)); + | ------------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +64 | return keccak256(abi.encode(a, b)); + | --------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +68 | return keccak256(abi.encodePacked(a, b)); + | --------------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +74 | bytes32 doesNotUseScratchSpace = keccak256(abi.encode(x, y, x, y, x, y)); + | --------------------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +75 | bytes32 doesUseScratchSpace = keccak256(abi.encode(x)); + | ------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +76 | return keccak256(abi.encode(doesUseScratchSpace, doesNotUseScratchSpace)); + | ------------------------------------------------------------------ + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 + +note[asm-keccak256]: use of inefficient hashing mechanism; consider using inline assembly + --> ROOT/testdata/Keccak256.sol:LL:CC + | +82 | return keccak256(abi.encodePacked(a, b)); + | --------------------------------- + | + = help: https://book.getfoundry.sh/reference/forge/forge-lint#asm-keccak256 diff --git a/crates/lint/testdata/UncheckedCall.sol b/crates/lint/testdata/UncheckedCall.sol index c2f71637a7b33..a9fdf7db4b9db 100644 --- a/crates/lint/testdata/UncheckedCall.sol +++ b/crates/lint/testdata/UncheckedCall.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.0; contract UncheckedCall { + event CallResult(bool, bytes); // SHOULD PASS: Properly checked low-level calls function checkedCallWithTuple(address target, bytes memory data) public { diff --git a/crates/lint/testdata/UncheckedCall.stderr b/crates/lint/testdata/UncheckedCall.stderr index a5a1010955f9b..123e02022bf91 100644 --- a/crates/lint/testdata/UncheckedCall.stderr +++ b/crates/lint/testdata/UncheckedCall.stderr @@ -1,7 +1,7 @@ warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -58 | target.call(data); +59 | target.call(data); | ----------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call @@ -9,7 +9,7 @@ warning[unchecked-call]: Low-level calls should check the success return value warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -62 | target.call{value: value}(""); +63 | target.call{value: value}(""); | ----------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call @@ -17,7 +17,7 @@ warning[unchecked-call]: Low-level calls should check the success return value warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -66 | target.delegatecall(data); +67 | target.delegatecall(data); | ------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call @@ -25,7 +25,7 @@ warning[unchecked-call]: Low-level calls should check the success return value warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -70 | target.staticcall(data); +71 | target.staticcall(data); | ----------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call @@ -33,7 +33,7 @@ warning[unchecked-call]: Low-level calls should check the success return value warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -74 | target1.call(""); +75 | target1.call(""); | ---------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call @@ -41,7 +41,7 @@ warning[unchecked-call]: Low-level calls should check the success return value warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -75 | target2.delegatecall(""); +76 | target2.delegatecall(""); | ------------------------ | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call @@ -49,7 +49,7 @@ warning[unchecked-call]: Low-level calls should check the success return value warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -79 | (, bytes memory data) = target.call(""); +80 | (, bytes memory data) = target.call(""); | ---------------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call @@ -57,7 +57,7 @@ warning[unchecked-call]: Low-level calls should check the success return value warning[unchecked-call]: Low-level calls should check the success return value --> ROOT/testdata/UncheckedCall.sol:LL:CC | -85 | (, existingData) = target.call(""); +86 | (, existingData) = target.call(""); | ---------------------------------- | = help: https://book.getfoundry.sh/reference/forge/forge-lint#unchecked-call diff --git a/crates/lint/testdata/auxiliary/ImportsAnotherFile.sol b/crates/lint/testdata/auxiliary/ImportsAnotherFile.sol new file mode 100644 index 0000000000000..628b66690c57b --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsAnotherFile.sol @@ -0,0 +1,3 @@ +contract AnotherFile { + // This contract is not used +} diff --git a/crates/lint/testdata/auxiliary/ImportsAnotherFile2.sol b/crates/lint/testdata/auxiliary/ImportsAnotherFile2.sol new file mode 100644 index 0000000000000..d4bf7c1b54309 --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsAnotherFile2.sol @@ -0,0 +1,3 @@ +contract AnotherDummy { + // This file is not used +} diff --git a/crates/lint/testdata/auxiliary/ImportsConstants.sol b/crates/lint/testdata/auxiliary/ImportsConstants.sol new file mode 100644 index 0000000000000..d1f8c758d0bae --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsConstants.sol @@ -0,0 +1,2 @@ +uint256 constant CONSTANT_0 = 42; +uint256 constant CONSTANT_1 = 99; diff --git a/crates/lint/testdata/auxiliary/ImportsFile.sol b/crates/lint/testdata/auxiliary/ImportsFile.sol new file mode 100644 index 0000000000000..64924d4054d3d --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsFile.sol @@ -0,0 +1,32 @@ +library symbol0 { + function isUsed(address) internal pure returns (bool) { + return true; + } +} + +type symbol1 is uint128; +type symbol3 is bytes32; +type symbol4 is uint256; +type symbol5 is uint256; +type symbol2 is bool; +type symbolNotUsed is address; +type symbolNotUsed2 is address; +type symbolNotUsed3 is address; + +contract BaseContract {} +interface IContract { + function foo(uint256 a, uint256 b) external view returns (uint256); + function convert(address addr) external pure returns (uint256); +} + +interface IContractNotUsed { + function doSomething() external; +} + +interface docSymbol {} +interface docSymbol2 {} +interface docSymbolWrongTag {} + +interface eventSymbol { + event foo(uint256 bar); +} diff --git a/crates/lint/testdata/auxiliary/ImportsSomeFile.sol b/crates/lint/testdata/auxiliary/ImportsSomeFile.sol new file mode 100644 index 0000000000000..591c1936b02b7 --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsSomeFile.sol @@ -0,0 +1,6 @@ +library SomeFile { + struct Baz { + uint256 amount; + address owner; + } +} diff --git a/crates/lint/testdata/auxiliary/ImportsSomeFile2.sol b/crates/lint/testdata/auxiliary/ImportsSomeFile2.sol new file mode 100644 index 0000000000000..4ca8ea00a3ec4 --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsSomeFile2.sol @@ -0,0 +1,4 @@ +struct Baz { + address sender; + uint256 value; +} diff --git a/crates/lint/testdata/auxiliary/ImportsTypes.sol b/crates/lint/testdata/auxiliary/ImportsTypes.sol new file mode 100644 index 0000000000000..dc035c1b407ce --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsTypes.sol @@ -0,0 +1,3 @@ +type MyType is uint256; +type MyOtherType is uint256; +type YetAnotherType is bool; diff --git a/crates/lint/testdata/auxiliary/ImportsUtils.sol b/crates/lint/testdata/auxiliary/ImportsUtils.sol new file mode 100644 index 0000000000000..9ca4a764d6c88 --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsUtils.sol @@ -0,0 +1,3 @@ +function process(bool flag) pure returns (uint256) { + return flag ? 1 : 0; +} diff --git a/crates/lint/testdata/auxiliary/ImportsUtils2.sol b/crates/lint/testdata/auxiliary/ImportsUtils2.sol new file mode 100644 index 0000000000000..9ca4a764d6c88 --- /dev/null +++ b/crates/lint/testdata/auxiliary/ImportsUtils2.sol @@ -0,0 +1,3 @@ +function process(bool flag) pure returns (uint256) { + return flag ? 1 : 0; +} diff --git a/crates/test-utils/src/ui_runner.rs b/crates/test-utils/src/ui_runner.rs index 6e9f94d3c6ee9..e4414cf954791 100644 --- a/crates/test-utils/src/ui_runner.rs +++ b/crates/test-utils/src/ui_runner.rs @@ -63,7 +63,7 @@ fn config<'a>( program: ui_test::CommandBuilder { program: cmd_path.into(), args: { - let args = vec![cmd, "--json"]; + let args = vec![cmd, "--json", "--root", testdata.to_str().expect("invalid root")]; args.into_iter().map(Into::into).collect() }, out_dir_flag: None, diff --git a/docs/dev/lintrules.md b/docs/dev/lintrules.md index 5dc7513736040..5d5b0b5d0122f 100644 --- a/docs/dev/lintrules.md +++ b/docs/dev/lintrules.md @@ -5,12 +5,13 @@ It helps enforce best practices and improve code quality within Foundry projects ## Architecture -The `forge-lint` system operates by analyzing Solidity source code: +The `forge-lint` system operates by analyzing Solidity source code through a dual-pass system: -1. **Parsing**: Solidity source files are parsed into an Abstract Syntax Tree (AST) using `solar-parse`. This AST represents the structure of the code. -2. **AST Traversal**: The generated AST is then traversed using a Visitor pattern. The `EarlyLintVisitor` is responsible for walking through the AST nodes. -3. **Applying Lint Passes**: As the visitor encounters different AST nodes (like functions, expressions, variable definitions), it invokes registered "lint passes" (`EarlyLintPass` implementations). Each pass is designed to check for a specific code pattern. -4. **Emitting Diagnostics**: If a lint pass identifies a violation of its rule, it uses the `LintContext` to emit a diagnostic (either `warning` or `note`) that pinpoints the issue in the source code. +1. **Parsing**: Solidity source files are parsed into an Abstract Syntax Tree (AST) using `solar-parse`. This AST represents the syntactic structure of the code. +2. **HIR Generation**: The AST is then lowered into a High-level Intermediate Representation (HIR) that includes type information and semantic analysis. +3. **Early Lint Passes**: The `EarlyLintVisitor` traverses the AST, invoking registered "early lint passes" (`EarlyLintPass` implementations) for syntax-level checks. +4. **Late Lint Passes**: The `LateLintVisitor` traverses the HIR, invoking registered "late lint passes" (`LateLintPass` implementations) for semantic analysis. +5. **Emitting Diagnostics**: If a lint pass identifies a violation, it uses the `LintContext` to emit a diagnostic (either `warning` or `note`) that pinpoints the issue. Lints can now also provide code fix suggestions through snippets. ### Key Components @@ -18,9 +19,12 @@ The `forge-lint` system operates by analyzing Solidity source code: - **`Lint` Trait & `SolLint` Struct**: - `Lint`: A trait that defines the essential properties of a lint rule, such as its unique ID, severity, description, and an optional help message/URL. - `SolLint`: A struct implementing the `Lint` trait, used to hold the metadata for each specific Solidity lint rule. -- **`EarlyLintPass<'ast>` Trait**: Lints that operate directly on AST nodes implement this trait. It contains methods (like `check_expr`, `check_item_function`, etc.) called by the visitor. -- **`LintContext<'s>`**: Provides contextual information to lint passes during execution, such as access to the session for emitting diagnostics. -- **`EarlyLintVisitor<'a, 's, 'ast>`**: The core visitor that traverses the AST and dispatches checks to the registered `EarlyLintPass` instances. +- **`EarlyLintPass<'ast>` Trait**: Lints that operate directly on AST nodes implement this trait. It contains methods (like `check_expr`, `check_item_function`, etc.) called by the AST visitor. +- **`LateLintPass<'hir>` Trait**: Lints that require type information and semantic analysis implement this trait. It contains methods (like `check_contract`, `check_function`, etc.) called by the HIR visitor. +- **`LintContext<'s>`**: Provides contextual information to lint passes during execution, such as access to the session for emitting diagnostics and methods for emitting fixes. +- **`EarlyLintVisitor<'a, 's, 'ast>`**: The visitor that traverses the AST and dispatches checks to the registered `EarlyLintPass` instances. +- **`LateLintVisitor<'a, 's, 'hir>`**: The visitor that traverses the HIR and dispatches checks to the registered `LateLintPass` instances. +- **`Snippet` Enum**: Represents code fix suggestions that can be either a code block or a diff, with optional descriptions. ## Developing a new lint rule @@ -33,25 +37,69 @@ The `forge-lint` system operates by analyzing Solidity source code: MIXED_CASE_FUNCTION, // The Rust identifier for this SolLint static Severity::Info, // The default severity of the lint "mixed-case-function", // A unique string ID for configuration/CLI - "function names should use mixedCase", // A brief description - "https://docs.soliditylang.org/en/latest/style-guide.html#function-names" // Optional help link + "function names should use mixedCase" // A brief description ); + // Note: The macro automatically generates a help link to the Foundry book ``` -- Register the pass struct and the lint using `register_lints!` in the `mod.rs` of its corresponding severity category. This macro generates the necessary boilerplate to make them discoverable by the linter, and creates helper functions to instantiate them. +- Register the pass struct and the lint using `register_lints!` in the `mod.rs` of its corresponding severity category. Specify the pass type (`early`, `late`, or both): ```rust register_lints!( - (PascalCaseStruct, (PASCAL_CASE_STRUCT)), - (MixedCaseVariable, (MIXED_CASE_VARIABLE)), - (MixedCaseFunction, (MIXED_CASE_FUNCTION)) + (PascalCaseStruct, early, (PASCAL_CASE_STRUCT)), + (MixedCaseVariable, early, (MIXED_CASE_VARIABLE)), + (MixedCaseFunction, early, (MIXED_CASE_FUNCTION)), + (AsmKeccak256, late, (ASM_KECCAK256)) ); - // The structs `PascalCaseStruct`, `MixedCaseVariable`, etc., would have to manually implement `EarlyLintPass`. + // The macro automatically generates the pass structs and helper functions ``` -- Implement the `EarlyLintPass` trait logic for the pass struct. Do it in a new file within the relevant severity module (e.g., `src/sol/med/my_new_lint.rs`). +- Implement the appropriate trait logic (`EarlyLintPass` or `LateLintPass`) for your lint. Do it in a new file within the relevant severity module (e.g., `src/sol/med/my_new_lint.rs`). + +### Choosing Between Early and Late Passes + +- **Use `EarlyLintPass`** for: + - Syntax-level checks (naming conventions, formatting) + - Simple pattern matching that doesn't require type information + - Lints that can be determined from the AST alone + +- **Use `LateLintPass`** for: + - Semantic analysis requiring type information + - Cross-reference checks between different parts of the code + - Complex patterns that need to understand the actual behavior + - Avoiding false positives through type-aware analysis + +### Providing Code Fix Suggestions + +Lints can now provide actionable code fix suggestions using the `emit_with_fix` method: + +```rust +// Example: Suggesting a code diff +cx.emit_with_fix( + lint, + node.span, + "consider using inline assembly", + Snippet::Diff { + desc: Some("use inline assembly for gas optimization"), + rmv: original_code, + add: optimized_assembly_code, + } +); + +// Example: Suggesting a code block +cx.emit_with_fix( + lint, + node.span, + "add this implementation", + Snippet::Block { + desc: Some("suggested implementation"), + code: suggested_code, + } +); +``` 3. Add comprehensive tests in `lint/testdata/`: - Create `MyNewLint.sol` with various examples (triggering and non-triggering cases, edge cases). + - If your test requires imports, add those files under `lint/testdata/auxiliary/` so that the ui runner doesn't lint them. - Generate the corresponding blessed file with the expected output. ### Testing a lint rule