From ef19895cc3f37d0a723363b4a8683e09e1771822 Mon Sep 17 00:00:00 2001 From: Boshen Date: Sun, 26 Mar 2023 04:34:31 -0700 Subject: [PATCH] feat(cli): add -A all -D isolated-declarations (#211) * feat(linter): add restriction category for lint rules * feat(cli): add "--allow" and "--deny" commands * feat(linter): use a single instance of linter * feat(cli): derive rules from args * feat(cli): print number of rules --- Cargo.lock | 1 + crates/oxc_cli/Cargo.toml | 1 + crates/oxc_cli/src/lint/command.rs | 26 ++++++ crates/oxc_cli/src/lint/mod.rs | 83 ++++++++++++++++--- crates/oxc_cli/src/lint/runner.rs | 68 +++++++++++++-- crates/oxc_cli/src/result.rs | 10 ++- crates/oxc_linter/src/lib.rs | 24 ++++-- crates/oxc_linter/src/rule.rs | 33 ++++++-- .../oxc_macros/src/declare_all_lint_rules.rs | 26 ++++++ 9 files changed, 235 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 79bf0cf02..3c67792c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -877,6 +877,7 @@ dependencies = [ "oxc_parser", "oxc_semantic", "rayon", + "rustc-hash", ] [[package]] diff --git a/crates/oxc_cli/Cargo.toml b/crates/oxc_cli/Cargo.toml index aa2eb721f..39b06852c 100644 --- a/crates/oxc_cli/Cargo.toml +++ b/crates/oxc_cli/Cargo.toml @@ -26,6 +26,7 @@ oxc_linter = { path = "../oxc_linter" } clap = { workspace = true } rayon = { workspace = true } miette = { workspace = true, features = ["fancy-no-backtrace"] } +rustc-hash = { workspace = true } num_cpus = "1.15.0" ignore = { version = "0.4.20", features = ["simd-accel"] } diff --git a/crates/oxc_cli/src/lint/command.rs b/crates/oxc_cli/src/lint/command.rs index 56a665e23..30fc087f7 100644 --- a/crates/oxc_cli/src/lint/command.rs +++ b/crates/oxc_cli/src/lint/command.rs @@ -5,6 +5,16 @@ pub fn lint_command() -> Command { .alias("check") .about("Lint this repository.") .arg_required_else_help(true) + .after_help( + "To allow or deny a rule, multiple -A or -D . +For example: -D correctness -A no-debugger. + +The categories are: + * correctness - code that is outright wrong or useless + * nursery - new lints that are still under development + * all - all the categories listed above + +The default category is -D correctness.") .arg( Arg::new("path") .value_name("PATH") @@ -13,6 +23,22 @@ pub fn lint_command() -> Command { .value_parser(ValueParser::path_buf()) .help("File or Directory paths to scan. Directories are scanned recursively.") ) + .arg( + Arg::new("allow") + .long("allow") + .short('A') + .required(false) + .action(ArgAction::Append) + .help("Allow a rule or a category") + ) + .arg( + Arg::new("deny") + .long("deny") + .short('D') + .required(false) + .action(ArgAction::Append) + .help("Deny a rule or a category") + ) .arg( Arg::new("fix") .long("fix") diff --git a/crates/oxc_cli/src/lint/mod.rs b/crates/oxc_cli/src/lint/mod.rs index f03c34418..ee77e93bc 100644 --- a/crates/oxc_cli/src/lint/mod.rs +++ b/crates/oxc_cli/src/lint/mod.rs @@ -1,7 +1,8 @@ -use std::path::PathBuf; mod command; mod runner; +use std::{collections::BTreeMap, path::PathBuf}; + use clap::ArgMatches; pub use self::{command::lint_command, runner::LintRunner}; @@ -9,6 +10,9 @@ pub use self::{command::lint_command, runner::LintRunner}; #[derive(Debug)] pub struct LintOptions { pub paths: Vec, + /// Allow / Deny rules in order. [("allow" / "deny", rule name)] + /// Defaults to [("deny", "correctness")] + pub rules: Vec<(AllowWarnDeny, String)>, pub fix: bool, pub quiet: bool, pub ignore_path: PathBuf, @@ -17,6 +21,23 @@ pub struct LintOptions { pub max_warnings: Option, } +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub enum AllowWarnDeny { + Allow, + // Warn, + Deny, +} + +impl From<&'static str> for AllowWarnDeny { + fn from(s: &'static str) -> Self { + match s { + "allow" => Self::Allow, + "deny" => Self::Deny, + _ => unreachable!(), + } + } +} + impl<'a> From<&'a ArgMatches> for LintOptions { fn from(matches: &'a ArgMatches) -> Self { Self { @@ -24,6 +45,7 @@ impl<'a> From<&'a ArgMatches> for LintOptions { || vec![PathBuf::from(".")], |paths| paths.into_iter().cloned().collect(), ), + rules: Self::get_rules(matches), fix: matches.get_flag("fix"), quiet: matches.get_flag("quiet"), ignore_path: matches @@ -39,11 +61,34 @@ impl<'a> From<&'a ArgMatches> for LintOptions { } } +impl LintOptions { + /// Get all rules in order, e.g. + /// `-A all -D no-var -D -eqeqeq` => [("allow", "all"), ("deny", "no-var"), ("deny", "eqeqeq")] + /// Defaults to [("deny", "correctness")]; + fn get_rules(matches: &ArgMatches) -> Vec<(AllowWarnDeny, String)> { + let mut map: BTreeMap = BTreeMap::new(); + for key in ["allow", "deny"] { + let allow_warn_deny = AllowWarnDeny::from(key); + if let Some(values) = matches.get_many::(key) { + let indices = matches.indices_of(key).unwrap(); + let zipped = + values.zip(indices).map(|(value, i)| (i, (allow_warn_deny, value.clone()))); + map.extend(zipped); + } + } + if map.is_empty() { + vec![(AllowWarnDeny::Deny, "correctness".into())] + } else { + map.into_values().collect() + } + } +} + #[cfg(test)] mod test { use std::path::PathBuf; - use super::{lint_command, LintOptions}; + use super::{lint_command, AllowWarnDeny, LintOptions}; #[test] fn verify_command() { @@ -56,7 +101,7 @@ mod test { } #[test] - fn test_default() { + fn default() { let options = get_lint_options("lint ."); assert_eq!(options.paths, vec![PathBuf::from(".")]); assert!(!options.fix); @@ -68,7 +113,7 @@ mod test { } #[test] - fn test_multiple_paths() { + fn multiple_paths() { let options = get_lint_options("lint foo bar baz"); assert_eq!( options.paths, @@ -77,43 +122,59 @@ mod test { } #[test] - fn test_quiet_true() { + fn rules_with_deny_and_allow() { + let options = get_lint_options( + "lint src -D suspicious --deny pedantic -A no-debugger --allow no-var", + ); + assert_eq!( + options.rules, + vec![ + (AllowWarnDeny::Deny, "suspicious".into()), + (AllowWarnDeny::Deny, "pedantic".into()), + (AllowWarnDeny::Allow, "no-debugger".into()), + (AllowWarnDeny::Allow, "no-var".into()) + ] + ); + } + + #[test] + fn quiet_true() { let options = get_lint_options("lint foo.js --quiet"); assert!(options.quiet); } #[test] - fn test_fix_true() { + fn fix_true() { let options = get_lint_options("lint foo.js --fix"); assert!(options.fix); } #[test] - fn test_max_warnings() { + fn max_warnings() { let options = get_lint_options("lint --max-warnings 10 foo.js"); assert_eq!(options.max_warnings, Some(10)); } #[test] - fn test_ignore_path() { + fn ignore_path() { let options = get_lint_options("lint --ignore-path .xxx foo.js"); assert_eq!(options.ignore_path, PathBuf::from(".xxx")); } #[test] - fn test_no_ignore() { + fn no_ignore() { let options = get_lint_options("lint --no-ignore foo.js"); assert!(options.no_ignore); } #[test] - fn test_single_ignore_pattern() { + fn single_ignore_pattern() { let options = get_lint_options("lint --ignore-pattern ./test foo.js"); assert_eq!(options.ignore_pattern, vec![String::from("./test")]); } #[test] - fn test_multiple_ignore_pattern() { + fn multiple_ignore_pattern() { let options = get_lint_options("lint --ignore-pattern ./test --ignore-pattern bar.js foo.js"); assert_eq!(options.ignore_pattern, vec![String::from("./test"), String::from("bar.js")]); diff --git a/crates/oxc_cli/src/lint/runner.rs b/crates/oxc_cli/src/lint/runner.rs index b5698ecfb..88b65cb32 100644 --- a/crates/oxc_cli/src/lint/runner.rs +++ b/crates/oxc_cli/src/lint/runner.rs @@ -13,21 +13,71 @@ use miette::NamedSource; use oxc_allocator::Allocator; use oxc_ast::SourceType; use oxc_diagnostics::{Error, MinifiedFileError, Severity}; -use oxc_linter::{Fixer, Linter}; +use oxc_linter::{Fixer, Linter, RuleCategory, RuleEnum, RULES}; use oxc_parser::Parser; use oxc_semantic::SemanticBuilder; +use rustc_hash::FxHashSet; -use super::LintOptions; +use super::{AllowWarnDeny, LintOptions}; use crate::{CliRunResult, Walk}; pub struct LintRunner { options: LintOptions, + + linter: Arc, } impl LintRunner { #[must_use] pub fn new(options: LintOptions) -> Self { - Self { options } + let linter = Linter::from_rules(Self::derive_rules(&options)).with_fix(options.fix); + Self { options, linter: Arc::new(linter) } + } + + fn derive_rules(options: &LintOptions) -> Vec { + let mut rules: FxHashSet = FxHashSet::default(); + + for (allow_warn_deny, name_or_category) in &options.rules { + let maybe_category = RuleCategory::from(name_or_category.as_str()); + match allow_warn_deny { + AllowWarnDeny::Deny => { + match maybe_category { + Some(category) => rules.extend( + RULES.iter().filter(|rule| rule.category() == category).cloned(), + ), + None => { + if name_or_category == "all" { + rules.extend(RULES.iter().cloned()); + } else { + rules.extend( + RULES + .iter() + .filter(|rule| rule.name() == name_or_category) + .cloned(), + ); + } + } + }; + } + AllowWarnDeny::Allow => { + match maybe_category { + Some(category) => rules.retain(|rule| rule.category() != category), + None => { + if name_or_category == "all" { + rules.drain(); + } else { + rules.retain(|rule| rule.name() == name_or_category); + } + } + }; + } + } + } + + let mut rules = rules.into_iter().collect::>(); + // for stable diagnostics output ordering + rules.sort_unstable_by_key(|rule| rule.name()); + rules } /// # Panics @@ -45,6 +95,7 @@ impl LintRunner { CliRunResult::LintResult { duration: now.elapsed(), + number_of_rules: self.linter.number_of_rules(), number_of_files: number_of_files.load(Ordering::Relaxed), number_of_diagnostics, number_of_warnings, @@ -73,12 +124,13 @@ impl LintRunner { number_of_files.store(count, Ordering::Relaxed); }); - let fix = self.options.fix; + let linter = Arc::clone(&self.linter); rayon::spawn(move || { while let Ok(path) = rx_path.recv() { let tx_error = tx_error.clone(); + let linter = Arc::clone(&linter); rayon::spawn(move || { - if let Some(diagnostics) = Self::lint_path(&path, fix) { + if let Some(diagnostics) = Self::lint_path(&linter, &path) { tx_error.send(diagnostics).unwrap(); } drop(tx_error); @@ -128,7 +180,7 @@ impl LintRunner { (number_of_warnings, number_of_diagnostics) } - fn lint_path(path: &Path, fix: bool) -> Option<(PathBuf, Vec)> { + fn lint_path(linter: &Linter, path: &Path) -> Option<(PathBuf, Vec)> { let source_text = fs::read_to_string(path).unwrap_or_else(|_| panic!("{path:?} not found")); let allocator = Allocator::default(); let source_type = @@ -147,13 +199,13 @@ impl LintRunner { return Some(Self::wrap_diagnostics(path, &source_text, semantic_ret.errors)); }; - let result = Linter::new().with_fix(fix).run(&Rc::new(semantic_ret.semantic)); + let result = linter.run(&Rc::new(semantic_ret.semantic)); if result.is_empty() { return None; } - if fix { + if linter.has_fix() { let fix_result = Fixer::new(&source_text, result).fix(); fs::write(path, fix_result.fixed_code.as_bytes()).unwrap(); let errors = fix_result.messages.into_iter().map(|m| m.error).collect(); diff --git a/crates/oxc_cli/src/result.rs b/crates/oxc_cli/src/result.rs index 35c754982..24e9ca167 100644 --- a/crates/oxc_cli/src/result.rs +++ b/crates/oxc_cli/src/result.rs @@ -10,11 +10,12 @@ pub enum CliRunResult { paths: Vec, }, LintResult { + duration: std::time::Duration, + number_of_rules: usize, number_of_files: usize, number_of_warnings: usize, number_of_diagnostics: usize, max_warnings_exceeded: bool, - duration: std::time::Duration, }, } @@ -27,15 +28,18 @@ impl Termination for CliRunResult { ExitCode::from(1) } Self::LintResult { + duration, + number_of_rules, number_of_files, number_of_warnings, number_of_diagnostics, max_warnings_exceeded, - duration, } => { let ms = duration.as_millis(); let cpus = num_cpus::get(); - println!("Checked {number_of_files} files in {ms}ms using {cpus} cores."); + println!( + "Finished in {ms}ms on {number_of_files} files with {number_of_rules} rules using {cpus} cores." + ); if max_warnings_exceeded { println!("Exceeded maximum number of warnings. Found {number_of_warnings}."); diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 940f00f10..5046a8828 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -17,11 +17,13 @@ use std::{fs, rc::Rc}; pub use fixer::{Fixer, Message}; pub(crate) use oxc_semantic::AstNode; use oxc_semantic::Semantic; -use rule::{Rule, RuleCategory}; use crate::{ - context::LintContext, - rules::{early_error::javascript::EarlyErrorJavaScript, RuleEnum, RULES}, + context::LintContext, rule::Rule, rules::early_error::javascript::EarlyErrorJavaScript, +}; +pub use crate::{ + rule::RuleCategory, + rules::{RuleEnum, RULES}, }; #[derive(Debug)] @@ -37,17 +39,11 @@ impl Linter { #[must_use] #[allow(clippy::new_without_default)] pub fn new() -> Self { - // let rules_config = Self::read_rules_configuration(); - // let rules = rules_config.map_or_else( - // || RULES.to_vec(), - // |rules_config| { let rules = RULES .iter() .cloned() .filter(|rule| rule.category() == RuleCategory::Correctness) .collect::>(); - // }, - // ); Self::from_rules(rules) } @@ -56,6 +52,16 @@ impl Linter { Self { rules, early_error_javascript: EarlyErrorJavaScript, fix: false } } + #[must_use] + pub fn has_fix(&self) -> bool { + self.fix + } + + #[must_use] + pub fn number_of_rules(&self) -> usize { + self.rules.len() + } + #[must_use] pub fn with_fix(mut self, yes: bool) -> Self { self.fix = yes; diff --git a/crates/oxc_linter/src/rule.rs b/crates/oxc_linter/src/rule.rs index 2fde9edbc..71e0aa6fa 100644 --- a/crates/oxc_linter/src/rule.rs +++ b/crates/oxc_linter/src/rule.rs @@ -4,12 +4,6 @@ use oxc_semantic::Symbol; use crate::{context::LintContext, AstNode}; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum RuleCategory { - Correctness, - Nursery, -} - pub trait Rule: Sized + Default + Debug { /// Initialize from eslint json configuration #[must_use] @@ -32,3 +26,30 @@ pub trait RuleMeta { None } } + +/// Rule categories defined by rust-clippy +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RuleCategory { + /// Code that is outright wrong or useless + Correctness, + /// Lints which prevent the use of language and library features + /// The restriction category should, emphatically, not be enabled as a whole. + /// The contained lints may lint against perfectly reasonable code, may not have an alternative suggestion, + /// and may contradict any other lints (including other categories). + /// Lints should be considered on a case-by-case basis before enabling. + Restriction, + /// New lints that are still under development + Nursery, +} + +impl RuleCategory { + #[must_use] + pub fn from(input: &str) -> Option { + match input { + "correctness" => Some(Self::Correctness), + "restriction" => Some(Self::Restriction), + "nursery" => Some(Self::Nursery), + _ => None, + } + } +} diff --git a/crates/oxc_macros/src/declare_all_lint_rules.rs b/crates/oxc_macros/src/declare_all_lint_rules.rs index 5261431a6..3a2af5c16 100644 --- a/crates/oxc_macros/src/declare_all_lint_rules.rs +++ b/crates/oxc_macros/src/declare_all_lint_rules.rs @@ -126,6 +126,32 @@ pub fn declare_all_lint_rules(metadata: AllLintRulesMeta) -> TokenStream { } } + impl std::hash::Hash for RuleEnum { + fn hash(&self, state: &mut H) { + self.name().hash(state); + } + } + + impl PartialEq for RuleEnum { + fn eq(&self, other: &Self) -> bool { + self.name() == other.name() + } + } + + impl Eq for RuleEnum {} + + impl Ord for RuleEnum { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.name().cmp(&other.name()) + } + } + + impl PartialOrd for RuleEnum { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(&other)) + } + } + lazy_static::lazy_static! { pub static ref RULES: Vec = vec![ #(RuleEnum::#struct_names(#struct_names::default())),*