diff --git a/examples/dlint/main.rs b/examples/dlint/main.rs index 86c7184f..720cfd8d 100644 --- a/examples/dlint/main.rs +++ b/examples/dlint/main.rs @@ -111,6 +111,7 @@ fn run_linter( let linter = Linter::new(LinterOptions { rules, all_rule_codes, + rule_severities: Default::default(), custom_ignore_file_directive: None, custom_ignore_diagnostic_directive: None, }); diff --git a/src/lib.rs b/src/lib.rs index 6c6ff18d..11a3ed9c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,6 +47,7 @@ mod lint_tests { let linter = Linter::new(LinterOptions { rules, all_rule_codes, + rule_severities: Default::default(), custom_ignore_diagnostic_directive: None, custom_ignore_file_directive: None, }); @@ -74,6 +75,7 @@ mod lint_tests { let linter = Linter::new(LinterOptions { rules, all_rule_codes, + rule_severities: Default::default(), custom_ignore_diagnostic_directive: None, custom_ignore_file_directive: None, }); diff --git a/src/linter.rs b/src/linter.rs index 54fb3179..eb4d0870 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -3,6 +3,7 @@ use crate::ast_parser::parse_program; use crate::context::Context; use crate::diagnostic::LintDiagnostic; +use crate::diagnostic::LintDiagnosticSeverity; use crate::ignore_directives::parse_file_ignore_directives; use crate::performance_mark::PerformanceMark; use crate::rules::{ban_unknown_rule_code::BanUnknownRuleCode, LintRule}; @@ -11,6 +12,7 @@ use deno_ast::MediaType; use deno_ast::ParsedSource; use deno_ast::{ModuleSpecifier, ParseDiagnostic}; use std::borrow::Cow; +use std::collections::HashMap; use std::collections::HashSet; use std::sync::Arc; @@ -19,6 +21,12 @@ pub struct LinterOptions { pub rules: Vec>, /// Collection of all the lint rule codes. pub all_rule_codes: HashSet>, + /// Per-rule diagnostic severity, keyed by rule code. Diagnostics from a rule + /// whose code isn't present default to [`LintDiagnosticSeverity::Error`], so + /// callers that don't configure severities keep the historical behavior. + /// + /// See [`crate::rules::config`] for producing this from configuration. + pub rule_severities: HashMap, LintDiagnosticSeverity>, /// Defaults to "deno-lint-ignore-file" pub custom_ignore_file_directive: Option<&'static str>, /// Defaults to "deno-lint-ignore" @@ -43,6 +51,8 @@ pub(crate) struct LinterContext { /// Rules are sorted by priority pub rules: Vec>, pub all_rule_codes: HashSet>, + /// Per-rule diagnostic severity, keyed by rule code. + pub rule_severities: HashMap, LintDiagnosticSeverity>, } impl LinterContext { @@ -63,6 +73,7 @@ impl LinterContext { check_unknown_rules, rules, all_rule_codes: options.all_rule_codes, + rule_severities: options.rule_severities, } } } @@ -174,6 +185,21 @@ impl Linter { // Run `ban-unused-ignore` diagnostics.extend(context.ban_unused_ignore(&enabled_rules)); + // Stamp each diagnostic with its rule's configured severity. Rules without + // a configured severity keep the default (`Error`), so this is a no-op + // unless the caller supplied `rule_severities`. + if !self.ctx.rule_severities.is_empty() { + for diagnostic in &mut diagnostics { + if let Some(severity) = self + .ctx + .rule_severities + .get(diagnostic.details.code.as_str()) + { + diagnostic.severity = *severity; + } + } + } + // Finally sort by position the diagnostics originates on then by code diagnostics.sort_by(|a, b| { let a_range = a.range.as_ref().map(|r| r.range.start); @@ -261,6 +287,7 @@ mod tests { let linter = Linter::new(LinterOptions { rules: vec![Box::new(NoDebugger)], all_rule_codes: [Cow::from("no-debugger")].into_iter().collect(), + rule_severities: Default::default(), custom_ignore_file_directive: None, custom_ignore_diagnostic_directive, }); @@ -309,4 +336,59 @@ mod tests { let source = "// deno-lint-ignore no-debugger\ndebugger;"; assert!(lint_with_directives(source, None).is_empty()); } + + // End-to-end: configuring a rule's severity flows all the way to the emitted + // diagnostic. This is the payoff of the `rules::config` layer. + #[test] + fn configured_severity_reaches_diagnostic() { + use crate::diagnostic::LintDiagnosticSeverity; + use crate::rules::config::{ + configure_rules, split_configured, RuleConfig, RuleSeverity, + }; + use crate::rules::no_console::NoConsole; + use std::collections::HashMap; + + let mut user = HashMap::new(); + user.insert( + "no-console".to_string(), + RuleConfig { + severity: Some(RuleSeverity::Warn), + options: None, + }, + ); + + let defs = vec![NoConsole::def()]; + let configured = configure_rules(&defs, &user).unwrap(); + let (rules, rule_severities) = split_configured(configured); + let all_rule_codes = rules + .iter() + .map(|r| Cow::from(r.code())) + .collect::>(); + + let linter = Linter::new(LinterOptions { + rules, + all_rule_codes, + rule_severities, + custom_ignore_file_directive: None, + custom_ignore_diagnostic_directive: None, + }); + + let (_, diagnostics) = linter + .lint_file(LintFileOptions { + specifier: ModuleSpecifier::parse("file:///foo.ts").unwrap(), + source_code: "console.log('x');".to_string(), + media_type: MediaType::TypeScript, + config: LintConfig { + default_jsx_factory: None, + default_jsx_fragment_factory: None, + }, + external_linter: None, + }) + .unwrap(); + + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].details.code, "no-console"); + // Without configuration this would default to `Error`. + assert_eq!(diagnostics[0].severity, LintDiagnosticSeverity::Warning); + } } diff --git a/src/rules.rs b/src/rules.rs index ff86954a..0d45e7e1 100644 --- a/src/rules.rs +++ b/src/rules.rs @@ -16,6 +16,7 @@ pub mod ban_untagged_ignore; pub mod ban_untagged_todo; pub mod ban_unused_ignore; pub mod camelcase; +pub mod config; pub mod constructor_super; pub mod default_param_last; pub mod eqeqeq; @@ -262,7 +263,7 @@ fn get_all_rules_raw() -> Vec> { Box::new(camelcase::Camelcase), Box::new(constructor_super::ConstructorSuper), Box::new(default_param_last::DefaultParamLast), - Box::new(eqeqeq::Eqeqeq), + Box::new(eqeqeq::Eqeqeq::default()), Box::new(explicit_function_return_type::ExplicitFunctionReturnType), Box::new(explicit_module_boundary_types::ExplicitModuleBoundaryTypes), Box::new(for_direction::ForDirection), @@ -290,7 +291,7 @@ fn get_all_rules_raw() -> Vec> { Box::new(no_class_assign::NoClassAssign), Box::new(no_compare_neg_zero::NoCompareNegZero), Box::new(no_cond_assign::NoCondAssign), - Box::new(no_console::NoConsole), + Box::new(no_console::NoConsole::default()), Box::new(no_const_assign::NoConstAssign), Box::new(no_constant_condition::NoConstantCondition), Box::new(no_control_regex::NoControlRegex), @@ -302,7 +303,7 @@ fn get_all_rules_raw() -> Vec> { Box::new(no_dupe_else_if::NoDupeElseIf), Box::new(no_dupe_keys::NoDupeKeys), Box::new(no_duplicate_case::NoDuplicateCase), - Box::new(no_empty::NoEmpty), + Box::new(no_empty::NoEmpty::default()), Box::new(no_empty_character_class::NoEmptyCharacterClass), Box::new(no_empty_enum::NoEmptyEnum), Box::new(no_empty_interface::NoEmptyInterface), @@ -512,7 +513,7 @@ mod tests { Box::new(ban_unknown_rule_code::BanUnknownRuleCode), Box::new(ban_unused_ignore::BanUnusedIgnore), Box::new(no_redeclare::NoRedeclare), - Box::new(eqeqeq::Eqeqeq), + Box::new(eqeqeq::Eqeqeq::default()), ]; sort_rules_by_priority(&mut rules); diff --git a/src/rules/config.rs b/src/rules/config.rs new file mode 100644 index 00000000..9cb4769f --- /dev/null +++ b/src/rules/config.rs @@ -0,0 +1,338 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +//! Rule configuration: turning "some data" (severity + options, the eslint +//! `[severity, options]` shape) into runnable, configured rules. +//! +//! ## Why this exists / the design +//! +//! The original model conflated two distinct concepts into a single unit +//! struct registered as `Box`: +//! +//! * the *definition* of a rule — its code, tags, default severity, and how +//! to build it from options; and +//! * a *configured, runnable instance* of that rule. +//! +//! That conflation is why a `from_configuration(value) -> Self` constructor +//! can't live on `LintRule`: constructing `Self` isn't object-safe, so it +//! can't be reached through `Box`. eslint (`{ meta, create }`) +//! and oxlint (a rule descriptor + `from_configuration`) both keep the two +//! halves apart. This module reintroduces that split: +//! +//! * [`RuleDef`] is the *definition* — cheap, copyable-ish static metadata +//! plus a `configure` function pointer ("options -> runnable rule"). +//! * [`ConfiguredRule`] is the runnable [`LintRule`] instance plus the +//! [`LintDiagnosticSeverity`] its diagnostics should carry. +//! +//! Enablement and severity are unified the way eslint does it: a rule whose +//! effective [`RuleSeverity`] is `Off` is simply never built. `default_severity` +//! on the definition encodes "recommended rules are on by default". +//! +//! ## Performance / footprint (intentionally not yet optimal) +//! +//! This is a starter implementation; a few things to revisit soon: +//! +//! * `RuleDef` holds only `&'static` data + a `fn` pointer, so the registry +//! can become a `&'static [RuleDef]` with zero per-call allocation (better +//! than today's `get_all_rules()` which rebuilds a `Vec` of boxed rules +//! every call). The prototype builds a small `Vec` for convenience. +//! * `configure` is called once per enabled rule per lint *session*, not per +//! file, so option parsing cost is amortized across files. +//! * `serde` derives add codegen, but only for the handful of rules that +//! actually take options; option-less rules share [`no_options`], which is +//! a single monomorphic function — no per-rule codegen. +//! * Severity is currently applied as an O(diagnostics) post-pass in the +//! linter; tagging at emit time would avoid the extra walk. +//! * Hand-writing a `RuleDef` per rule won't scale to ~120 rules; a +//! `declare_rules!` macro (or build-time codegen) should generate them. + +use crate::diagnostic::LintDiagnosticSeverity; +use crate::rules::LintRule; +use crate::tags::Tags; +use std::borrow::Cow; +use std::collections::HashMap; + +/// The severity a rule may be configured with. +/// +/// Unlike [`LintDiagnosticSeverity`] (a property of an emitted diagnostic), +/// this includes `Off`: a rule configured `Off` is never constructed or run, so +/// there is nothing to emit. This mirrors eslint, where `"off"` both silences +/// and disables a rule. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RuleSeverity { + Off, + Warn, + Error, +} + +impl RuleSeverity { + /// The diagnostic severity to stamp on this rule's output, or `None` when the + /// rule is `Off` (and therefore should not run at all). + fn diagnostic_severity(self) -> Option { + match self { + RuleSeverity::Off => None, + RuleSeverity::Warn => Some(LintDiagnosticSeverity::Warning), + RuleSeverity::Error => Some(LintDiagnosticSeverity::Error), + } + } +} + +/// Per-rule configuration as it might arrive from a config file, the CLI, or the +/// LSP. Models eslint's `[severity, options]`: severity is optional (fall back +/// to the rule's default) and options are an opaque JSON blob the rule itself +/// knows how to interpret. +#[derive(Debug, Clone, Default)] +pub struct RuleConfig { + /// `None` means "use the rule's `default_severity`". + pub severity: Option, + /// Rule-specific options. `None` means "use option defaults". + pub options: Option, +} + +impl RuleConfig { + /// Convenience: just turn a rule on at its default severity. + pub fn on() -> Self { + RuleConfig { + severity: Some(RuleSeverity::Error), + options: None, + } + } +} + +/// Failure to apply configuration to a rule. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RuleConfigError { + /// Config referenced a rule code that isn't in the registry. + UnknownRule { code: String }, + /// Options were supplied for a rule that takes none. + DoesNotSupportOptions { code: &'static str }, + /// Options were supplied but failed to deserialize into the rule's schema. + InvalidOptions { code: &'static str, message: String }, +} + +impl std::fmt::Display for RuleConfigError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RuleConfigError::UnknownRule { code } => { + write!(f, "Unknown lint rule '{code}'") + } + RuleConfigError::DoesNotSupportOptions { code } => { + write!(f, "Lint rule '{code}' does not accept options") + } + RuleConfigError::InvalidOptions { code, message } => { + write!(f, "Invalid options for lint rule '{code}': {message}") + } + } + } +} + +impl std::error::Error for RuleConfigError {} + +/// A function that builds a configured, runnable rule from optional JSON +/// options (`None` = defaults). This is the constructor that the runnable +/// `LintRule` trait can't host because it isn't object-safe. +pub type ConfigureFn = + fn(Option<&serde_json::Value>) -> Result, RuleConfigError>; + +/// The *definition* half of a rule: static metadata plus a constructor. +/// +/// The registry stores these instead of pre-built instances, so a rule is +/// "created" by applying configuration to its definition. +pub struct RuleDef { + pub code: &'static str, + pub tags: Tags, + /// Severity used when a rule is enabled without an explicit severity. Encodes + /// "on by default": recommended rules use `Error`, others use `Off`. + pub default_severity: RuleSeverity, + /// Builds the runnable instance from options. Severity is applied separately + /// by [`RuleDef::configure`]. + pub configure_options: ConfigureFn, +} + +impl RuleDef { + /// Resolve this definition against user-supplied [`RuleConfig`] into a + /// runnable [`ConfiguredRule`], or `Ok(None)` if the rule is effectively + /// `Off`. + pub fn configure( + &self, + config: &RuleConfig, + ) -> Result, RuleConfigError> { + let severity = config.severity.unwrap_or(self.default_severity); + let Some(diagnostic_severity) = severity.diagnostic_severity() else { + return Ok(None); + }; + let rule = (self.configure_options)(config.options.as_ref())?; + Ok(Some(ConfiguredRule { + rule, + severity: diagnostic_severity, + })) + } +} + +/// A runnable rule instance together with the severity its diagnostics carry. +#[derive(Debug)] +pub struct ConfiguredRule { + pub rule: Box, + pub severity: LintDiagnosticSeverity, +} + +/// Per-rule diagnostic severities, keyed by rule code. This is one of the two +/// inputs the linter consumes (alongside the runnable rules). +pub type RuleSeverities = HashMap, LintDiagnosticSeverity>; + +/// A [`ConfigureFn`] for rules that take no options: errors if any options are +/// supplied, otherwise builds the default instance. Shared across all +/// option-less rules so they add no per-rule codegen. +pub fn no_options( + options: Option<&serde_json::Value>, +) -> Result, RuleConfigError> +where + R: LintRule + Default + 'static, +{ + match options { + // An explicit empty object/null is treated as "no options". + None => Ok(Box::new(R::default())), + Some(v) if v.is_null() => Ok(Box::new(R::default())), + Some(v) if v.as_object().is_some_and(|o| o.is_empty()) => { + Ok(Box::new(R::default())) + } + Some(_) => Err(RuleConfigError::DoesNotSupportOptions { + code: R::default().code(), + }), + } +} + +/// Resolve a whole registry of definitions against user configuration keyed by +/// rule code, producing the set of runnable rules (those not `Off`). +/// +/// Unknown rule codes in `user` are reported rather than silently ignored. +pub fn configure_rules( + defs: &[RuleDef], + user: &HashMap, +) -> Result, RuleConfigError> { + // Surface configuration for codes that don't exist. + for code in user.keys() { + if !defs.iter().any(|d| d.code == code) { + return Err(RuleConfigError::UnknownRule { code: code.clone() }); + } + } + + let default_config = RuleConfig::default(); + let mut configured = Vec::new(); + for def in defs { + let config = user.get(def.code).unwrap_or(&default_config); + if let Some(rule) = def.configure(config)? { + configured.push(rule); + } + } + Ok(configured) +} + +/// Split configured rules into the two inputs `LinterOptions` wants: the +/// runnable rules and the per-code severity map. This is the bridge from "rule +/// configuration" to "linter input". +pub fn split_configured( + configured: Vec, +) -> (Vec>, RuleSeverities) { + let mut rules = Vec::with_capacity(configured.len()); + let mut severities = HashMap::with_capacity(configured.len()); + for configured_rule in configured { + // `code()` returns `&'static str`, so this borrows nothing from the rule. + severities.insert( + Cow::Borrowed(configured_rule.rule.code()), + configured_rule.severity, + ); + rules.push(configured_rule.rule); + } + (rules, severities) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::rules::eqeqeq::Eqeqeq; + use crate::rules::no_console::NoConsole; + use crate::rules::no_empty::NoEmpty; + + fn registry() -> Vec { + vec![NoConsole::def(), NoEmpty::def(), Eqeqeq::def()] + } + + fn json(s: &str) -> serde_json::Value { + serde_json::from_str(s).unwrap() + } + + #[test] + fn recommended_default_on_others_off() { + // With no user config, only rules whose `default_severity` isn't `Off` + // (here: the recommended `no-empty`) are created. + let configured = configure_rules(®istry(), &HashMap::new()).unwrap(); + let codes: Vec<_> = configured.iter().map(|c| c.rule.code()).collect(); + assert_eq!(codes, vec!["no-empty"]); + assert_eq!(configured[0].severity, LintDiagnosticSeverity::Error); + } + + #[test] + fn severity_off_excludes_rule() { + let mut user = HashMap::new(); + user.insert( + "no-empty".to_string(), + RuleConfig { + severity: Some(RuleSeverity::Off), + options: None, + }, + ); + let configured = configure_rules(®istry(), &user).unwrap(); + assert!(configured.is_empty()); + } + + #[test] + fn severity_warn_is_carried() { + let mut user = HashMap::new(); + user.insert( + "no-console".to_string(), + RuleConfig { + severity: Some(RuleSeverity::Warn), + options: None, + }, + ); + let configured = configure_rules(®istry(), &user).unwrap(); + let c = configured + .iter() + .find(|c| c.rule.code() == "no-console") + .unwrap(); + assert_eq!(c.severity, LintDiagnosticSeverity::Warning); + } + + #[test] + fn unknown_rule_is_an_error() { + let mut user = HashMap::new(); + user.insert("no-such-rule".to_string(), RuleConfig::on()); + let err = configure_rules(®istry(), &user).unwrap_err(); + assert_eq!( + err, + RuleConfigError::UnknownRule { + code: "no-such-rule".to_string() + } + ); + } + + #[test] + fn options_for_optionless_rule_error() { + // `no-empty` accepts options, but feeding garbage to an option-less rule + // (simulated via the shared `no_options` path) is rejected. Here we feed + // an unexpected shape to a rule and expect an invalid-options error. + let mut user = HashMap::new(); + user.insert( + "eqeqeq".to_string(), + RuleConfig { + severity: Some(RuleSeverity::Error), + options: Some(json(r#""nonsense-mode""#)), + }, + ); + let err = configure_rules(®istry(), &user).unwrap_err(); + assert!(matches!( + err, + RuleConfigError::InvalidOptions { code: "eqeqeq", .. } + )); + } +} diff --git a/src/rules/eqeqeq.rs b/src/rules/eqeqeq.rs index 9b867611..ad48f604 100644 --- a/src/rules/eqeqeq.rs +++ b/src/rules/eqeqeq.rs @@ -2,16 +2,61 @@ use super::{Context, LintRule}; use crate::handler::{Handler, Traverse}; +use crate::rules::config::{RuleConfigError, RuleDef, RuleSeverity}; use crate::Program; -use deno_ast::swc::ast::BinaryOp; +use deno_ast::swc::ast::{BinaryOp, UnaryOp}; use deno_ast::{view as ast_view, SourceRanged}; use derive_more::Display; +use serde::Deserialize; -#[derive(Debug)] -pub struct Eqeqeq; +/// How strictly `eqeqeq` enforces strict equality. Mirrors eslint's `eqeqeq` +/// option string: `"always"` (default) flags every `==`/`!=`; `"smart"` allows +/// them when comparing against `null`, evaluating `typeof`, or comparing two +/// literals. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum EqeqeqMode { + #[default] + Always, + Smart, +} + +#[derive(Debug, Default)] +pub struct Eqeqeq { + mode: EqeqeqMode, +} const CODE: &str = "eqeqeq"; +fn configure( + options: Option<&serde_json::Value>, +) -> Result, RuleConfigError> { + let mode = match options { + None => EqeqeqMode::default(), + Some(value) => serde_json::from_value(value.clone()).map_err(|e| { + RuleConfigError::InvalidOptions { + code: CODE, + message: e.to_string(), + } + })?, + }; + Ok(Box::new(Eqeqeq { mode })) +} + +impl Eqeqeq { + /// The rule *definition*: metadata plus the constructor used to build a + /// configured instance. See [`crate::rules::config`]. + pub fn def() -> RuleDef { + RuleDef { + code: CODE, + tags: &[], + // Not a recommended rule, so off unless explicitly enabled. + default_severity: RuleSeverity::Off, + configure_options: configure, + } + } +} + #[derive(Display)] enum EqeqeqMessage { #[display(fmt = "expected '===' and instead saw '=='.")] @@ -38,15 +83,38 @@ impl LintRule for Eqeqeq { context: &mut Context, program: Program, ) { - EqeqeqHandler.traverse(program, context); + EqeqeqHandler { mode: self.mode }.traverse(program, context); } } -struct EqeqeqHandler; +struct EqeqeqHandler { + mode: EqeqeqMode, +} + +/// Whether a `==`/`!=` comparison is permitted under `"smart"` mode: comparing +/// against `null`, evaluating `typeof`, or comparing two literal values. +fn is_smart_allowed(bin_expr: &ast_view::BinExpr) -> bool { + use ast_view::Expr; + use ast_view::Lit; + + let is_null = |e: &Expr| matches!(e, Expr::Lit(Lit::Null(_))); + let is_typeof = + |e: &Expr| matches!(e, Expr::Unary(u) if u.op() == UnaryOp::TypeOf); + let is_literal = |e: &Expr| matches!(e, Expr::Lit(_)); + + is_null(&bin_expr.left) + || is_null(&bin_expr.right) + || is_typeof(&bin_expr.left) + || is_typeof(&bin_expr.right) + || (is_literal(&bin_expr.left) && is_literal(&bin_expr.right)) +} impl Handler for EqeqeqHandler { fn bin_expr(&mut self, bin_expr: &ast_view::BinExpr, context: &mut Context) { if matches!(bin_expr.op(), BinaryOp::EqEq | BinaryOp::NotEq) { + if self.mode == EqeqeqMode::Smart && is_smart_allowed(bin_expr) { + return; + } let (message, hint) = if bin_expr.op() == BinaryOp::EqEq { (EqeqeqMessage::ExpectedEqual, EqeqeqHint::UseEqeqeq) } else { @@ -64,7 +132,7 @@ mod tests { #[test] fn eqeqeq_valid() { assert_lint_ok! { - Eqeqeq, + Eqeqeq::default(), "midori === sapphire", "midori !== hazuki", "kumiko === null", @@ -77,7 +145,7 @@ mod tests { #[test] fn eqeqeq_invalid() { assert_lint_err! { - Eqeqeq, + Eqeqeq::default(), "a == b": [ { @@ -287,4 +355,67 @@ b "#: [ }], } } + + fn smart() -> Eqeqeq { + Eqeqeq { + mode: EqeqeqMode::Smart, + } + } + + #[test] + fn eqeqeq_smart_valid() { + // `"smart"` permits comparing against null, evaluating typeof, and + // comparing two literals. + assert_lint_ok! { + smart(), + "a == null", + "null != a", + "typeof a == 'number'", + "'string' != typeof a", + "true == true", + "2 == 3", + "'hello' != 'world'", + }; + } + + #[test] + fn eqeqeq_smart_invalid() { + // Non-null, non-typeof, non-literal comparisons are still flagged. + assert_lint_err! { + smart(), + "a == b": [ + { + col: 0, + message: EqeqeqMessage::ExpectedEqual, + hint: EqeqeqHint::UseEqeqeq, + }], + "a != b": [ + { + col: 0, + message: EqeqeqMessage::ExpectedNotEqual, + hint: EqeqeqHint::UseNoteqeq, + }], + } + } + + #[test] + fn eqeqeq_configure_parses_mode() { + use crate::rules::config::RuleConfigError; + + let always = (Eqeqeq::def().configure_options)(None).unwrap(); + assert_eq!(always.code(), "eqeqeq"); + + let smart = + (Eqeqeq::def().configure_options)(Some(&serde_json::json!("smart"))) + .unwrap(); + assert_eq!(smart.code(), "eqeqeq"); + + let err = + (Eqeqeq::def().configure_options)(Some(&serde_json::json!("nope"))) + .unwrap_err(); + assert!(matches!( + err, + RuleConfigError::InvalidOptions { code: "eqeqeq", .. } + )); + } } diff --git a/src/rules/no_console.rs b/src/rules/no_console.rs index fe99e0e8..7624fe7e 100644 --- a/src/rules/no_console.rs +++ b/src/rules/no_console.rs @@ -1,19 +1,62 @@ use super::{Context, LintRule}; use crate::handler::{Handler, Traverse}; +use crate::rules::config::{RuleConfigError, RuleDef, RuleSeverity}; use crate::tags::Tags; use crate::Program; use deno_ast::swc::ast::Id; use deno_ast::view as ast_view; use deno_ast::SourceRanged; +use serde::Deserialize; use std::collections::HashSet; -#[derive(Debug)] -pub struct NoConsole; +#[derive(Debug, Default)] +pub struct NoConsole { + /// Console methods that are permitted, e.g. `["warn", "error"]`. + allowed: Vec, +} const MESSAGE: &str = "`console` usage is not allowed."; const CODE: &str = "no-console"; +/// Options for `no-console`, mirroring eslint's `{ allow: string[] }`. +#[derive(Debug, Default, Deserialize)] +#[serde(default)] +struct NoConsoleOptions { + allow: Vec, +} + +fn configure( + options: Option<&serde_json::Value>, +) -> Result, RuleConfigError> { + let options: NoConsoleOptions = match options { + None => NoConsoleOptions::default(), + Some(value) => serde_json::from_value(value.clone()).map_err(|e| { + RuleConfigError::InvalidOptions { + code: CODE, + message: e.to_string(), + } + })?, + }; + Ok(Box::new(NoConsole { + allowed: options.allow, + })) +} + +impl NoConsole { + /// The rule *definition*: metadata plus the constructor used to build a + /// configured instance. See [`crate::rules::config`]. + pub fn def() -> RuleDef { + RuleDef { + code: CODE, + tags: &[], + // Not a recommended rule, so off unless explicitly enabled. + default_severity: RuleSeverity::Off, + configure_options: configure, + } + } +} + impl LintRule for NoConsole { fn tags(&self) -> Tags { &[] @@ -30,19 +73,22 @@ impl LintRule for NoConsole { ) { NoConsoleHandler { imported_console: HashSet::new(), + allowed: &self.allowed, } .traverse(program, context); } } -struct NoConsoleHandler { +struct NoConsoleHandler<'a> { /// Bindings imported as the default export of `node:console`, e.g. /// `import console from "node:console";`. These refer to the same `console` /// object as the global, so usages should be flagged too. imported_console: HashSet, + /// Permitted console methods (the `allow` option). + allowed: &'a [String], } -impl NoConsoleHandler { +impl NoConsoleHandler<'_> { fn is_console(&self, ident: &ast_view::Ident, ctx: &mut Context) -> bool { let id = ident.inner.to_id(); // `console` imported from `node:console` (any local name). @@ -52,9 +98,23 @@ impl NoConsoleHandler { // The global `console`. ident.sym() == "console" && ctx.scope().is_global(&id) } + + /// Whether the accessed property is in the `allow` list, e.g. the `warn` in + /// `console.warn(...)`. Only the simple `console.method` form is recognized; + /// computed access (`console["warn"]`) is not, matching the common case. + fn is_allowed_property(&self, prop: &ast_view::MemberProp) -> bool { + if self.allowed.is_empty() { + return false; + } + if let ast_view::MemberProp::Ident(ident) = prop { + let name = ident.sym(); + return self.allowed.iter().any(|allowed| name == allowed.as_str()); + } + false + } } -impl Handler for NoConsoleHandler { +impl Handler for NoConsoleHandler<'_> { fn import_decl(&mut self, import: &ast_view::ImportDecl, _ctx: &mut Context) { if import.src.value().to_string_lossy() != "node:console" { return; @@ -73,7 +133,7 @@ impl Handler for NoConsoleHandler { use deno_ast::view::Expr; if let Expr::Ident(ident) = &expr.obj { - if self.is_console(ident, ctx) { + if self.is_console(ident, ctx) && !self.is_allowed_property(&expr.prop) { ctx.add_diagnostic(ident.range(), CODE, MESSAGE); } } @@ -96,7 +156,7 @@ mod tests { #[test] fn console_allowed() { assert_lint_ok!( - NoConsole, + NoConsole::default(), // ignored r"// deno-lint-ignore no-console\nconsole.error('Error message');", // not global @@ -114,7 +174,7 @@ mod tests { fn no_console_invalid() { // Test cases where console is present assert_lint_err!( - NoConsole, + NoConsole::default(), r#"console.log('Debug message');"#: [{ col: 0, message: MESSAGE, @@ -160,4 +220,49 @@ mod tests { }], ); } + + fn with_allow(methods: &[&str]) -> NoConsole { + NoConsole { + allowed: methods.iter().map(|s| s.to_string()).collect(), + } + } + + #[test] + fn no_console_allow_option_valid() { + // Methods listed in `allow` are permitted. + assert_lint_ok!( + with_allow(&["warn", "error"]), + r#"console.warn("ok");"#, + r#"console.error("ok");"#, + ); + } + + #[test] + fn no_console_allow_option_invalid() { + // Methods not listed in `allow` are still flagged. + assert_lint_err!( + with_allow(&["warn", "error"]), + r#"console.log("nope");"#: [{ + col: 0, + message: MESSAGE, + }], + // bare `console` has no method, so `allow` doesn't apply. + r#"console;"#: [{ + col: 0, + message: MESSAGE, + }] + ); + } + + #[test] + fn no_console_configure_parses_allow() { + let default_rule = (NoConsole::def().configure_options)(None).unwrap(); + assert_eq!(default_rule.code(), "no-console"); + + let configured = (NoConsole::def().configure_options)(Some( + &serde_json::json!({ "allow": ["warn"] }), + )) + .unwrap(); + assert_eq!(configured.code(), "no-console"); + } } diff --git a/src/rules/no_empty.rs b/src/rules/no_empty.rs index 194f698a..e9558741 100644 --- a/src/rules/no_empty.rs +++ b/src/rules/no_empty.rs @@ -2,16 +2,60 @@ use super::{Context, LintRule}; use crate::handler::{Handler, Traverse}; +use crate::rules::config::{RuleConfigError, RuleDef, RuleSeverity}; use crate::tags::{self, Tags}; use crate::Program; -use deno_ast::view::{ArrowExpr, BlockStmt, Constructor, Function, SwitchStmt}; +use deno_ast::view::{ + ArrowExpr, BlockStmt, CatchClause, Constructor, Function, SwitchStmt, +}; use deno_ast::{SourceRanged, SourceRangedForSpanned}; +use serde::Deserialize; -#[derive(Debug)] -pub struct NoEmpty; +#[derive(Debug, Default)] +pub struct NoEmpty { + allow_empty_catch: bool, +} const CODE: &str = "no-empty"; +/// Options for `no-empty`, mirroring eslint's `{ allowEmptyCatch: boolean }`. +#[derive(Debug, Default, Deserialize)] +#[serde(rename_all = "camelCase", default)] +struct NoEmptyOptions { + allow_empty_catch: bool, +} + +fn configure( + options: Option<&serde_json::Value>, +) -> Result, RuleConfigError> { + let options: NoEmptyOptions = match options { + None => NoEmptyOptions::default(), + Some(value) => serde_json::from_value(value.clone()).map_err(|e| { + RuleConfigError::InvalidOptions { + code: CODE, + message: e.to_string(), + } + })?, + }; + Ok(Box::new(NoEmpty { + allow_empty_catch: options.allow_empty_catch, + })) +} + +impl NoEmpty { + /// The rule *definition*: metadata plus the constructor used to build a + /// configured instance. See [`crate::rules::config`]. + pub fn def() -> RuleDef { + RuleDef { + code: CODE, + tags: &[tags::RECOMMENDED], + // Recommended, so on by default at error severity. + default_severity: RuleSeverity::Error, + configure_options: configure, + } + } +} + impl LintRule for NoEmpty { fn tags(&self) -> Tags { &[tags::RECOMMENDED] @@ -26,11 +70,16 @@ impl LintRule for NoEmpty { context: &mut Context, program: Program, ) { - NoEmptyHandler.traverse(program, context); + NoEmptyHandler { + allow_empty_catch: self.allow_empty_catch, + } + .traverse(program, context); } } -struct NoEmptyHandler; +struct NoEmptyHandler { + allow_empty_catch: bool, +} impl Handler for NoEmptyHandler { fn block_stmt(&mut self, block_stmt: &BlockStmt, ctx: &mut Context) { @@ -38,10 +87,15 @@ impl Handler for NoEmptyHandler { // Because function's body is a block statement, we're gonna // manually visit each member; otherwise rule would produce errors // for empty function or arrow body or constructor. + // + // When `allowEmptyCatch` is enabled, an empty `catch {}` body is allowed. + let is_allowed_empty_catch = + self.allow_empty_catch && block_stmt.parent().is::(); if block_stmt.stmts.is_empty() && !block_stmt.parent().is::() && !block_stmt.parent().is::() && !block_stmt.parent().is::() + && !is_allowed_empty_catch && !block_stmt.contains_comments(ctx) { ctx.add_diagnostic_with_hint( @@ -84,7 +138,7 @@ mod tests { #[test] fn no_empty_valid() { assert_lint_ok! { - NoEmpty, + NoEmpty::default(), r#"function foobar() {}"#, r#" class Foo { @@ -140,7 +194,7 @@ try { #[test] fn no_empty_invalid() { assert_lint_err! { - NoEmpty, + NoEmpty::default(), "if (foo) { }": [ { col: 9, @@ -332,4 +386,55 @@ try { ] }; } + + fn allow_empty_catch() -> NoEmpty { + NoEmpty { + allow_empty_catch: true, + } + } + + #[test] + fn no_empty_allow_empty_catch_valid() { + // With `allowEmptyCatch`, an empty `catch {}` is permitted... + assert_lint_ok! { + allow_empty_catch(), + "try { foo(); } catch {}", + "try { foo(); } catch (e) {}", + }; + } + + #[test] + fn no_empty_allow_empty_catch_still_flags_others() { + // ...but other empty blocks are still reported. + assert_lint_err! { + allow_empty_catch(), + "if (foo) { }": [ + { + col: 9, + message: "Empty block statement", + hint: "Add code or comment to the empty block", + } + ], + // An empty `try` block is not a `catch`, so it is still flagged. + "try {} catch { foo(); }": [ + { + col: 4, + message: "Empty block statement", + hint: "Add code or comment to the empty block", + } + ] + }; + } + + #[test] + fn no_empty_configure_parses_allow_empty_catch() { + let default_rule = (NoEmpty::def().configure_options)(None).unwrap(); + assert_eq!(default_rule.code(), "no-empty"); + + let configured = (NoEmpty::def().configure_options)(Some( + &serde_json::json!({ "allowEmptyCatch": true }), + )) + .unwrap(); + assert_eq!(configured.code(), "no-empty"); + } } diff --git a/src/test_util.rs b/src/test_util.rs index 3746354b..90462682 100644 --- a/src/test_util.rs +++ b/src/test_util.rs @@ -330,6 +330,7 @@ fn lint( .map(|rule| rule.code()) .map(Cow::from) .collect(), + rule_severities: Default::default(), custom_ignore_diagnostic_directive: None, custom_ignore_file_directive: None, });