From 8d98d2c2e605144eaa918abdf98424bc99b4741d Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Fri, 12 Jul 2024 00:03:45 -0400 Subject: [PATCH] feat(linter): support suggestions and dangerous fixes --- apps/oxlint/src/command/lint.rs | 27 ++- apps/oxlint/src/lint/mod.rs | 2 +- crates/oxc_language_server/src/linter.rs | 4 +- crates/oxc_language_server/src/main.rs | 6 +- crates/oxc_linter/src/context.rs | 99 ++++++++- crates/oxc_linter/src/fixer/fix.rs | 201 +++++++++++++++++- crates/oxc_linter/src/fixer/mod.rs | 117 +++++++--- crates/oxc_linter/src/lib.rs | 5 +- crates/oxc_linter/src/options.rs | 13 +- .../src/rules/jest/prefer_expect_resolves.rs | 4 +- .../oxc_linter/src/rules/jest/prefer_todo.rs | 4 +- .../typescript/consistent_type_imports.rs | 73 ++++--- .../oxc_linter/src/rules/unicorn/no_null.rs | 4 +- .../src/rules/unicorn/no_useless_spread.rs | 6 +- .../rules/unicorn/prefer_query_selector.rs | 4 +- crates/oxc_linter/src/service.rs | 2 +- crates/oxc_linter/src/tester.rs | 6 +- 17 files changed, 477 insertions(+), 100 deletions(-) diff --git a/apps/oxlint/src/command/lint.rs b/apps/oxlint/src/command/lint.rs index 672e5a77fcf939..db325c4fccafd0 100644 --- a/apps/oxlint/src/command/lint.rs +++ b/apps/oxlint/src/command/lint.rs @@ -1,7 +1,7 @@ use std::{path::PathBuf, str::FromStr}; use bpaf::Bpaf; -use oxc_linter::AllowWarnDeny; +use oxc_linter::{AllowWarnDeny, FixKind}; use super::{ expand_glob, @@ -122,6 +122,31 @@ pub struct FixOptions { /// Fix as many issues as possible. Only unfixed issues are reported in the output #[bpaf(switch)] pub fix: bool, + /// Apply all safe fixes and more-risky suggestions. Overrides `--fix`. + #[bpaf(switch)] + pub fix_suggestions: bool, + + /// Apply all fixes and suggestions, including dangerous ones. Overrides + /// `--fix` and `--fix-suggestions`. These fixes may create incorrect code. + #[bpaf(switch)] + pub fix_dangerously: bool, +} +impl FixOptions { + pub fn fix_kind(&self) -> Option { + if self.fix_dangerously { + Some(FixKind::DangerousFix) + } else if self.fix_suggestions { + Some(FixKind::Suggestion) + } else if self.fix { + Some(FixKind::SafeFix) + } else { + None + } + } + + pub fn is_enabled(&self) -> bool { + self.fix || self.fix_suggestions || self.fix_dangerously + } } /// Handle Warnings diff --git a/apps/oxlint/src/lint/mod.rs b/apps/oxlint/src/lint/mod.rs index 609239796c192c..dd78c771e3e026 100644 --- a/apps/oxlint/src/lint/mod.rs +++ b/apps/oxlint/src/lint/mod.rs @@ -93,7 +93,7 @@ impl Runner for LintRunner { let lint_options = LintOptions::default() .with_filter(filter) .with_config_path(basic_options.config) - .with_fix(fix_options.fix) + .with_fix(fix_options.fix_kind()) .with_react_plugin(enable_plugins.react_plugin) .with_unicorn_plugin(enable_plugins.unicorn_plugin) .with_typescript_plugin(enable_plugins.typescript_plugin) diff --git a/crates/oxc_language_server/src/linter.rs b/crates/oxc_language_server/src/linter.rs index 6df7c2f98071ac..b33746293caac9 100644 --- a/crates/oxc_language_server/src/linter.rs +++ b/crates/oxc_language_server/src/linter.rs @@ -13,7 +13,7 @@ use oxc_linter::{ AstroPartialLoader, JavaScriptSource, SveltePartialLoader, VuePartialLoader, LINT_PARTIAL_LOADER_EXT, }, - LintContext, Linter, + FixKind, LintContext, Linter, }; use oxc_parser::Parser; use oxc_semantic::SemanticBuilder; @@ -388,7 +388,7 @@ pub struct ServerLinter { impl ServerLinter { pub fn new() -> Self { - let linter = Linter::default().with_fix(true); + let linter = Linter::default().with_fix(Some(FixKind::SafeFix)); Self { linter: Arc::new(linter) } } diff --git a/crates/oxc_language_server/src/main.rs b/crates/oxc_language_server/src/main.rs index 2e4512213361a0..aa367d23f72d22 100644 --- a/crates/oxc_language_server/src/main.rs +++ b/crates/oxc_language_server/src/main.rs @@ -7,7 +7,7 @@ use futures::future::join_all; use globset::Glob; use ignore::gitignore::Gitignore; use log::{debug, error, info}; -use oxc_linter::{LintOptions, Linter}; +use oxc_linter::{FixKind, LintOptions, Linter}; use serde::{Deserialize, Serialize}; use tokio::sync::{Mutex, OnceCell, RwLock, SetError}; use tower_lsp::{ @@ -345,7 +345,9 @@ impl Backend { let mut linter = self.server_linter.write().await; *linter = ServerLinter::new_with_linter( Linter::from_options( - LintOptions::default().with_fix(true).with_config_path(Some(config_path)), + LintOptions::default() + .with_fix(Some(FixKind::SafeFix)) + .with_config_path(Some(config_path)), ) .expect("should have initialized linter with new options"), ); diff --git a/crates/oxc_linter/src/context.rs b/crates/oxc_linter/src/context.rs index 19268430fce856..74ecb43bc1b1fc 100644 --- a/crates/oxc_linter/src/context.rs +++ b/crates/oxc_linter/src/context.rs @@ -10,7 +10,7 @@ use oxc_syntax::module_record::ModuleRecord; use crate::{ config::OxlintRules, disable_directives::{DisableDirectives, DisableDirectivesBuilder}, - fixer::{CompositeFix, Message, RuleFixer}, + fixer::{FixKind, Message, RuleFix, RuleFixer}, javascript_globals::GLOBALS, AllowWarnDeny, OxlintConfig, OxlintEnv, OxlintGlobals, OxlintSettings, }; @@ -29,7 +29,7 @@ pub struct LintContext<'a> { /// Whether or not to apply code fixes during linting. Defaults to `false`. /// /// Set via the `--fix` CLI flag. - fix: bool, + fix: Option, file_path: Rc, @@ -69,7 +69,7 @@ impl<'a> LintContext<'a> { semantic, diagnostics: RefCell::new(Vec::with_capacity(DIAGNOSTICS_INITIAL_CAPACITY)), disable_directives: Rc::new(disable_directives), - fix: false, + fix: None, file_path: file_path.into(), eslint_config: Arc::new(OxlintConfig::default()), current_rule_name: "", @@ -79,7 +79,7 @@ impl<'a> LintContext<'a> { /// Enable/disable automatic code fixes. #[must_use] - pub fn with_fix(mut self, fix: bool) -> Self { + pub fn with_fix(mut self, fix: Option) -> Self { self.fix = fix; self } @@ -190,6 +190,7 @@ impl<'a> LintContext<'a> { /// Report a lint rule violation. /// /// Use [`LintContext::diagnostic_with_fix`] to provide an automatic fix. + #[inline] pub fn diagnostic(&self, diagnostic: OxcDiagnostic) { self.add_diagnostic(Message::new(diagnostic, None)); } @@ -197,19 +198,95 @@ impl<'a> LintContext<'a> { /// Report a lint rule violation and provide an automatic fix. /// /// The second argument is a [closure] that takes a [`RuleFixer`] and - /// returns something that can turn into a [`CompositeFix`]. + /// returns something that can turn into a `CompositeFix`. + /// + /// Fixes created this way should not create parse errors or change the + /// semantics of the linted code. If your fix may change the code's + /// semantics, use [`LintContext::diagnostic_with_suggestion`] instead. If + /// your fix has the potential to create parse errors, use + /// [`LintContext::diagnostic_with_dangerous_fix`]. /// /// [closure]: + #[inline] pub fn diagnostic_with_fix(&self, diagnostic: OxcDiagnostic, fix: F) where - C: Into>, + C: Into>, + F: FnOnce(RuleFixer<'_, 'a>) -> C, + { + self.diagnostic_with_fix_of_kind(diagnostic, FixKind::SafeFix, fix); + } + + /// Report a lint rule violation and provide a suggestion for fixing it. + /// + /// The second argument is a [closure] that takes a [`RuleFixer`] and + /// returns something that can turn into a `CompositeFix`. + /// + /// Fixes created this way should not create parse errors, but have the + /// potential to change the code's semantics. If your fix is completely safe + /// and definitely does not change semantics, use [`LintContext::diagnostic_with_fix`]. + /// If your fix has the potential to create parse errors, use + /// [`LintContext::diagnostic_with_dangerous_fix`]. + /// + /// [closure]: + #[inline] + pub fn diagnostic_with_suggestion(&self, diagnostic: OxcDiagnostic, fix: F) + where + C: Into>, + F: FnOnce(RuleFixer<'_, 'a>) -> C, + { + self.diagnostic_with_fix_of_kind(diagnostic, FixKind::Suggestion, fix); + } + + /// Report a lint rule violation and provide a potentially dangerous + /// automatic fix for it. + /// + /// The second argument is a [closure] that takes a [`RuleFixer`] and + /// returns something that can turn into a `CompositeFix`. + /// + /// Dangerous fixes should be avoided and are not applied by default with + /// `--fix`. Use this method if: + /// - Your fix is experimental and you want to test it out in the wild + /// before marking it as safe. + /// - Your fix is extremely aggressive and risky, but you want to provide + /// it as an option to users. + /// + /// When possible, prefer [`LintContext::diagnostic_with_fix`]. If the only + /// risk your fix poses is minor(ish) changes to code semantics, use + /// [`LintContext::diagnostic_with_suggestion`] instead. + /// + /// [closure]: + /// + #[inline] + pub fn diagnostic_with_dangerous_fix(&self, diagnostic: OxcDiagnostic, fix: F) + where + C: Into>, F: FnOnce(RuleFixer<'_, 'a>) -> C, { - if self.fix { - let fixer = RuleFixer::new(self); - let composite_fix: CompositeFix = fix(fixer).into(); - let fix = composite_fix.normalize_fixes(self.source_text()); - self.add_diagnostic(Message::new(diagnostic, Some(fix))); + self.diagnostic_with_fix_of_kind(diagnostic, FixKind::DangerousFix, fix); + } + + pub fn diagnostic_with_fix_of_kind( + &self, + diagnostic: OxcDiagnostic, + fix_kind: FixKind, + fix: F, + ) where + C: Into>, + F: FnOnce(RuleFixer<'_, 'a>) -> C, + { + if let Some(accepted_fix_kind) = self.fix { + let fixer = RuleFixer::new(fix_kind, self); + let rule_fix: RuleFix<'a> = fix(fixer).into(); + let diagnostic = match (rule_fix.message(), &diagnostic.help) { + (Some(message), None) => diagnostic.with_help(message.to_owned()), + _ => diagnostic, + }; + if rule_fix.kind() <= accepted_fix_kind { + let fix = rule_fix.into_fix(self.source_text()); + self.add_diagnostic(Message::new(diagnostic, Some(fix))); + } else { + self.diagnostic(diagnostic); + } } else { self.diagnostic(diagnostic); } diff --git a/crates/oxc_linter/src/fixer/fix.rs b/crates/oxc_linter/src/fixer/fix.rs index 8213921f596bb2..552d40e1af5ed0 100644 --- a/crates/oxc_linter/src/fixer/fix.rs +++ b/crates/oxc_linter/src/fixer/fix.rs @@ -1,7 +1,133 @@ -use std::borrow::Cow; +use std::{borrow::Cow, ops::Deref}; -use oxc_span::{Span, SPAN}; +use oxc_span::{GetSpan, Span, SPAN}; +/// Ordered from safest to least safe. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum FixKind { + /// An automatic code fix that we are confident won't + /// - Cause any parse errors + /// - Change the code's semantics + #[default] + SafeFix, + /// A recommendation about how to fix a rule violation. These are safe to + /// apply, in that they shouldn't cause parse or runtime errors, but may + /// change the meaning of the code. + Suggestion, + /// An automatic code fix that may break the code. This covers fixes that: + /// - Are aggressive + /// - Are under development + DangerousFix, +} + +// TODO: rename +#[derive(Debug, Default)] +#[must_use = "Fixes must be used. If you don't need a fix, use `LintContext::diagnostic`, or create an empty fix using `RuleFixer::noop`."] +pub struct RuleFix<'a> { + kind: FixKind, + /// A suggestion message. Will be shown in editors via code actions. + /// + /// NOTE: code actions aren't implemented yet. + message: Option>, + /// The actual that will be applied to the source code. + /// + /// See: [`Fix`] + fix: CompositeFix<'a>, +} + +macro_rules! impl_from { + ($($ty:ty),*) => { + $( + impl<'a> From<$ty> for RuleFix<'a> { + fn from(fix: $ty) -> Self { + Self { kind: FixKind::SafeFix, message: None, fix: fix.into() } + } + } + )* + }; +} +// I'd like to use +// impl<'a, F: Into>> From for RuleFix<'a> b +// but this breaks when implementing `From> for CompositeFix<'a>`. +impl_from!(CompositeFix<'a>, Fix<'a>, Option>, Vec>); + +impl<'a> From> for CompositeFix<'a> { + fn from(val: RuleFix<'a>) -> Self { + val.fix + } +} + +impl<'a> RuleFix<'a> { + pub(super) fn new(kind: FixKind, message: Option>, fix: CompositeFix<'a>) -> Self { + Self { kind, message, fix } + } + + /// Create a new safe fix. + #[inline] + pub fn fix(fix: CompositeFix<'a>) -> Self { + Self { kind: FixKind::SafeFix, message: None, fix } + } + + /// Create a new suggestion + #[inline] + pub const fn suggestion(fix: CompositeFix<'a>, message: Cow<'a, str>) -> Self { + Self { kind: FixKind::Suggestion, message: Some(message), fix } + } + + /// Create a dangerous fix. + #[inline] + pub fn dangerous(fix: CompositeFix<'a>) -> Self { + Self { kind: FixKind::DangerousFix, message: None, fix } + } + + pub fn with_message>>(mut self, message: S) -> Self { + self.message = Some(message.into()); + self + } + + #[inline] + pub fn kind(&self) -> FixKind { + self.kind + } + + #[inline] + pub fn message(&self) -> Option<&str> { + self.message.as_deref() + } + + #[must_use] + #[inline] + pub fn into_fix(self, source_text: &str) -> Fix<'a> { + self.fix.normalize_fixes(source_text) + } + + pub fn extend>>(mut self, fix: F) -> Self { + self.fix = self.fix.concat(fix.into()); + self + } + + #[inline] + pub fn push>>(&mut self, fix: F) { + self.fix.push(fix.into()); + } +} + +impl GetSpan for RuleFix<'_> { + fn span(&self) -> Span { + self.fix.span() + } +} + +impl<'a> Deref for RuleFix<'a> { + type Target = CompositeFix<'a>; + fn deref(&self) -> &Self::Target { + &self.fix + } +} + +/// A completed, normalized fix ready to be applied to the source code. +/// +/// Used internally by this module. Lint rules should use [`RuleFix`]. #[derive(Debug, Clone)] #[non_exhaustive] pub struct Fix<'a> { @@ -31,6 +157,10 @@ impl<'a> Fix<'a> { } } +// NOTE (@DonIsaac): having these variants is effectively the same as interning +// single or 0-element Vecs. I experimented with using smallvec here, but the +// resulting struct size was larger (40 bytes vs 32). So, we're sticking with +// this (at least for now). #[derive(Debug, Default)] pub enum CompositeFix<'a> { /// No fixes @@ -66,7 +196,72 @@ impl<'a> From>> for CompositeFix<'a> { } } +impl GetSpan for CompositeFix<'_> { + fn span(&self) -> Span { + match self { + CompositeFix::Single(fix) => fix.span, + CompositeFix::Multiple(fixes) => { + fixes.iter().map(|fix| fix.span).reduce(|a, b| a.merge(&b)).unwrap_or(SPAN) + } + CompositeFix::None => SPAN, + } + } +} + impl<'a> CompositeFix<'a> { + pub fn push(&mut self, fix: CompositeFix<'a>) { + match self { + Self::None => *self = fix, + Self::Single(fix1) => { + *self = Self::Multiple(vec![fix1.clone(), fix.normalize_fixes("")]); + } + Self::Multiple(fixes) => fixes.push(fix.normalize_fixes("")), + } + } + + #[must_use] + pub fn concat(self, fix: CompositeFix<'a>) -> Self { + match (self, fix) { + (Self::None, f) | (f, Self::None) => f, + (Self::Single(fix1), Self::Single(fix2)) => Self::Multiple(vec![fix1, fix2]), + (Self::Single(fix), Self::Multiple(mut fixes)) => { + fixes.insert(0, fix); + Self::Multiple(fixes) + } + (Self::Multiple(mut fixes), Self::Single(fix)) => { + fixes.push(fix); + Self::Multiple(fixes) + } + (Self::Multiple(mut fixes1), Self::Multiple(fixes2)) => { + fixes1.extend(fixes2); + Self::Multiple(fixes1) + } + } + } + // TODO: do we want this? + // pub fn extend(&mut self, fix: CompositeFix<'a>) { + // match self { + // Self::None => *self = fix, + // Self::Single(fix1) => { + // match fix { + // Self::None => {}, + // Self::Single(fix2) => *self = Self::Multiple(vec![fix1.clone(), fix2]), + // Self::Multiple(mut fixes) => { + // fixes.insert(0, fix1.clone()); + // *self = Self::Multiple(fixes); + // } + // } + // } + // Self::Multiple(fixes) => { + // match fix { + // Self::None => {}, + // Self::Single(fix2) => fixes.push(fix2), + // Self::Multiple(fixes2) => fixes.extend(fixes2), + // } + // } + // } + // } + /// Gets one fix from the fixes. If we retrieve multiple fixes, this merges those into one. /// pub fn normalize_fixes(self, source_text: &str) -> Fix<'a> { @@ -76,7 +271,7 @@ impl<'a> CompositeFix<'a> { CompositeFix::None => Fix::empty(), } } - /// Merges multiple fixes to one, returns an `Fix::default`(which will not fix anything) if: + /// Merges multiple fixes to one, returns an [`Fix::empty`] (which will not fix anything) if: /// /// 1. `fixes` is empty /// 2. contains overlapped ranges diff --git a/crates/oxc_linter/src/fixer/mod.rs b/crates/oxc_linter/src/fixer/mod.rs index b62d20dae8f985..1e34611e089b73 100644 --- a/crates/oxc_linter/src/fixer/mod.rs +++ b/crates/oxc_linter/src/fixer/mod.rs @@ -8,82 +8,150 @@ use oxc_span::{GetSpan, Span}; use crate::LintContext; -pub use fix::{CompositeFix, Fix}; +pub use fix::{CompositeFix, Fix, FixKind, RuleFix}; -/// Inspired by ESLint's [`RuleFixer`]. +/// Produces [`RuleFix`] instances. Inspired by ESLint's [`RuleFixer`]. /// /// [`RuleFixer`]: https://github.com/eslint/eslint/blob/main/lib/linter/rule-fixer.js #[derive(Clone, Copy)] pub struct RuleFixer<'c, 'a: 'c> { + /// What kind of fixes will factory methods produce? + /// + /// Controlled via `diagnostic_with_fix`, `diagnostic_with_suggestion`, and + /// `diagnostic_with_dangerous_fix` methods on [`LintContext`] + kind: FixKind, + /// Enable/disable automatic creation of suggestion messages. + /// + /// Auto-messaging is useful for single fixes, but not so much when we know + /// multiple fixes will be applied. Some [`RuleFix`] factory methods + /// allocate strings on the heap, which would then just get thrown away. + /// Turning this off prevents uneeded allocations. + /// + /// Defaults to `true` + auto_message: bool, ctx: &'c LintContext<'a>, } impl<'c, 'a: 'c> RuleFixer<'c, 'a> { - pub fn new(ctx: &'c LintContext<'a>) -> Self { - Self { ctx } + pub(super) fn new(kind: FixKind, ctx: &'c LintContext<'a>) -> Self { + Self { kind, auto_message: true, ctx } + } + + /// Hint to the [`RuleFixer`] that it will be creating [`CompositeFix`]es + /// containing more than one [`Fix`]. + /// + /// Calling this method in such cases is _highly recommended_ as it has a + /// sizeable performance impact, but is not _stricly_ necessary. + #[must_use] + pub fn for_multifix(mut self) -> Self { + self.auto_message = false; + self + } + + // NOTE(@DonIsaac): Internal methods shouldn't use `T: Into` generics to optimize binary + // size. Only use such generics in public APIs. + fn new_fix(&self, fix: CompositeFix<'a>, message: Option>) -> RuleFix<'a> { + RuleFix::new(self.kind, message, fix) + } + + /// Create a new [`RuleFix`] with pre-allocated memory for multiple fixes. + pub fn new_fix_with_capacity(&self, capacity: usize) -> RuleFix<'a> { + RuleFix::new(self.kind, None, CompositeFix::Multiple(Vec::with_capacity(capacity))) } /// Get a snippet of source text covered by the given [`Span`]. For details, /// see [`Span::source_text`]. - pub fn source_range(self, span: Span) -> &'a str { + #[inline] + pub fn source_range(&self, span: Span) -> &'a str { self.ctx.source_range(span) } - /// Create a [`Fix`] that deletes the text covered by the given [`Span`] or - /// AST node. - pub fn delete(self, spanned: &S) -> Fix<'a> { + /// Create a [`RuleFix`] that deletes the text covered by the given [`Span`] + /// or AST node. + #[inline] + pub fn delete(&self, spanned: &S) -> RuleFix<'a> { self.delete_range(spanned.span()) } + /// Delete text covered by a [`Span`] #[allow(clippy::unused_self)] - pub fn delete_range(self, span: Span) -> Fix<'a> { - Fix::delete(span) + pub fn delete_range(&self, span: Span) -> RuleFix<'a> { + self.new_fix( + CompositeFix::Single(Fix::delete(span)), + self.auto_message.then_some(Cow::Borrowed("Delete this code.")), + ) } /// Replace a `target` AST node with the source code of a `replacement` node.. - pub fn replace_with(self, target: &T, replacement: &S) -> Fix<'a> { + pub fn replace_with(&self, target: &T, replacement: &S) -> RuleFix<'a> { let replacement_text = self.ctx.source_range(replacement.span()); - Fix::new(replacement_text, target.span()) + let fix = Fix::new(replacement_text, target.span()); + let message = self.auto_message.then(|| { + let target_text = self.ctx.source_range(target.span()); + Cow::Owned(format!("Replace `{target_text}` with `{replacement_text}`.")) + }); + + self.new_fix(CompositeFix::Single(fix), message) } /// Replace a `target` AST node with a `replacement` string. #[allow(clippy::unused_self)] - pub fn replace>>(self, target: Span, replacement: S) -> Fix<'a> { - Fix::new(replacement, target) + pub fn replace>>(&self, target: Span, replacement: S) -> RuleFix<'a> { + let fix = Fix::new(replacement, target); + let target_text = self.ctx.source_range(target); + let message = self + .auto_message + .then(|| Cow::Owned(format!("Replace `{target_text}` code with `{}`.", &fix.content))); + + self.new_fix(CompositeFix::Single(fix), message) } /// Creates a fix command that inserts text before the given node. + #[inline] pub fn insert_text_before>>( - self, + &self, target: &T, text: S, - ) -> Fix<'a> { + ) -> RuleFix<'a> { self.insert_text_before_range(target.span(), text) } /// Creates a fix command that inserts text before the specified range in the source text. - pub fn insert_text_before_range>>(self, span: Span, text: S) -> Fix<'a> { + #[inline] + pub fn insert_text_before_range>>( + &self, + span: Span, + text: S, + ) -> RuleFix<'a> { self.insert_text_at(span.start, text) } /// Creates a fix command that inserts text after the given node. + #[inline] pub fn insert_text_after>>( - self, + &self, target: &T, text: S, - ) -> Fix<'a> { + ) -> RuleFix<'a> { self.insert_text_after_range(target.span(), text) } /// Creates a fix command that inserts text after the specified range in the source text. - pub fn insert_text_after_range>>(self, span: Span, text: S) -> Fix<'a> { + #[inline] + pub fn insert_text_after_range>>( + &self, + span: Span, + text: S, + ) -> RuleFix<'a> { self.insert_text_at(span.end, text) } /// Creates a fix command that inserts text at the specified index in the source text. #[allow(clippy::unused_self)] - fn insert_text_at>>(self, index: u32, text: S) -> Fix<'a> { - Fix::new(text, Span::new(index, index)) + fn insert_text_at>>(&self, index: u32, text: S) -> RuleFix<'a> { + let fix = Fix::new(text, Span::new(index, index)); + let message = self.auto_message.then(|| Cow::Owned(format!("Insert `{}`", &fix.content))); + self.new_fix(CompositeFix::Single(fix), message) } #[allow(clippy::unused_self)] @@ -92,8 +160,9 @@ impl<'c, 'a: 'c> RuleFixer<'c, 'a> { } #[allow(clippy::unused_self)] - pub fn noop(self) -> Fix<'a> { - Fix::empty() + #[inline] + pub fn noop(&self) -> RuleFix<'a> { + self.new_fix(CompositeFix::None, None) } } diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 35eebc4e99d6a4..4b3434a4a272df 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -27,13 +27,14 @@ use oxc_semantic::AstNode; pub use crate::{ config::OxlintConfig, context::LintContext, + fixer::FixKind, options::{AllowWarnDeny, LintOptions}, rule::{RuleCategory, RuleMeta, RuleWithSeverity}, service::{LintService, LintServiceOptions}, }; use crate::{ config::{OxlintEnv, OxlintGlobals, OxlintSettings}, - fixer::{Fix, Fixer, Message}, + fixer::{Fixer, Message}, rules::RuleEnum, table::RuleTable, }; @@ -86,7 +87,7 @@ impl Linter { /// Enable/disable automatic code fixes. #[must_use] - pub fn with_fix(mut self, yes: bool) -> Self { + pub fn with_fix(mut self, yes: Option) -> Self { self.options.fix = yes; self } diff --git a/crates/oxc_linter/src/options.rs b/crates/oxc_linter/src/options.rs index 3bb9c4c0bc62f1..d222cc83cd0085 100644 --- a/crates/oxc_linter/src/options.rs +++ b/crates/oxc_linter/src/options.rs @@ -6,8 +6,8 @@ use schemars::{schema::SchemaObject, JsonSchema}; use serde_json::{Number, Value}; use crate::{ - config::OxlintConfig, rules::RULES, utils::is_jest_rule_adapted_to_vitest, RuleCategory, - RuleEnum, RuleWithSeverity, + config::OxlintConfig, fixer::FixKind, rules::RULES, utils::is_jest_rule_adapted_to_vitest, + RuleCategory, RuleEnum, RuleWithSeverity, }; #[derive(Debug)] @@ -16,7 +16,10 @@ pub struct LintOptions { /// Defaults to [("deny", "correctness")] pub filter: Vec<(AllowWarnDeny, String)>, pub config_path: Option, - pub fix: bool, + /// Enable automatic code fixes. Set to [`None`] to disable. + /// + /// The kind represents the riskiest fix that the linter can apply. + pub fix: Option, pub react_plugin: bool, pub unicorn_plugin: bool, @@ -37,7 +40,7 @@ impl Default for LintOptions { Self { filter: vec![(AllowWarnDeny::Warn, String::from("correctness"))], config_path: None, - fix: false, + fix: None, react_plugin: true, unicorn_plugin: true, typescript_plugin: true, @@ -70,7 +73,7 @@ impl LintOptions { } #[must_use] - pub fn with_fix(mut self, yes: bool) -> Self { + pub fn with_fix(mut self, yes: Option) -> Self { self.fix = yes; self } diff --git a/crates/oxc_linter/src/rules/jest/prefer_expect_resolves.rs b/crates/oxc_linter/src/rules/jest/prefer_expect_resolves.rs index 1289c3ce2b5c9c..be9c781522907e 100644 --- a/crates/oxc_linter/src/rules/jest/prefer_expect_resolves.rs +++ b/crates/oxc_linter/src/rules/jest/prefer_expect_resolves.rs @@ -8,7 +8,7 @@ use oxc_span::Span; use crate::{ context::LintContext, - fixer::{Fix, RuleFixer}, + fixer::{RuleFix, RuleFixer}, rule::Rule, utils::{ collect_possible_jest_call_node, parse_expect_jest_fn_call, ParsedExpectFnCall, @@ -116,7 +116,7 @@ impl PreferExpectResolves { jest_expect_fn_call: &ParsedExpectFnCall<'a>, call_expr: &CallExpression<'a>, ident_span: Span, - ) -> Fix<'a> { + ) -> RuleFix<'a> { let mut formatter = fixer.codegen(); let first = call_expr.arguments.first().unwrap(); let Argument::AwaitExpression(await_expr) = first else { diff --git a/crates/oxc_linter/src/rules/jest/prefer_todo.rs b/crates/oxc_linter/src/rules/jest/prefer_todo.rs index f08b8f3b152a0c..01c7824b72f6c1 100644 --- a/crates/oxc_linter/src/rules/jest/prefer_todo.rs +++ b/crates/oxc_linter/src/rules/jest/prefer_todo.rs @@ -8,7 +8,7 @@ use oxc_span::{GetSpan, Span}; use crate::{ context::LintContext, - fixer::{Fix, RuleFixer}, + fixer::{RuleFix, RuleFixer}, rule::Rule, utils::{ collect_possible_jest_call_node, is_type_of_jest_fn_call, JestFnKind, JestGeneralFnKind, @@ -137,7 +137,7 @@ fn is_empty_function(expr: &CallExpression) -> bool { } } -fn build_code<'a>(fixer: RuleFixer<'_, 'a>, expr: &CallExpression<'a>) -> Fix<'a> { +fn build_code<'a>(fixer: RuleFixer<'_, 'a>, expr: &CallExpression<'a>) -> RuleFix<'a> { let mut formatter = fixer.codegen(); match &expr.callee { diff --git a/crates/oxc_linter/src/rules/typescript/consistent_type_imports.rs b/crates/oxc_linter/src/rules/typescript/consistent_type_imports.rs index 9cf66fa38d0450..5474c6da2d1e07 100644 --- a/crates/oxc_linter/src/rules/typescript/consistent_type_imports.rs +++ b/crates/oxc_linter/src/rules/typescript/consistent_type_imports.rs @@ -15,7 +15,7 @@ use oxc_span::{CompactStr, GetSpan, Span}; use crate::{ context::LintContext, - fixer::{Fix, RuleFixer}, + fixer::{RuleFix, RuleFixer}, rule::Rule, AstNode, }; @@ -256,7 +256,7 @@ impl Rule for ConsistentTypeImports { Ok(fixes) => fixes, Err(err) => { debug_assert!(false, "Failed to fix: {err}"); - vec![] + fixer.noop() } } }; @@ -329,8 +329,9 @@ fn fixer_error, T>(message: S) -> FixerResult { // import { Foo, Bar } from 'foo' => import type { Foo, Bar } from 'foo' #[allow(clippy::unnecessary_cast, clippy::cast_possible_truncation)] -fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResult>> { +fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResult> { let FixOptions { fixer, import_decl, type_names, fix_style, ctx } = options; + let fixer = fixer.for_multifix(); let GroupedSpecifiers { namespace_specifier, named_specifiers, default_specifier } = classify_specifier(import_decl); @@ -389,8 +390,8 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu let fixes_named_specifiers = get_fixes_named_specifiers(options, &type_names_specifiers, &named_specifiers)?; - let mut fixes = vec![]; - let mut after_fixes = vec![]; + let mut rule_fixes = fixer.new_fix_with_capacity(4); + let mut after_fixes = fixer.new_fix_with_capacity(4); if !type_names_specifiers.is_empty() { // definitely all type references: `import type { A, B } from 'foo'` @@ -409,7 +410,7 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu &fixes_named_specifiers.type_named_specifiers_text, )?; if type_only_named_import.span.end <= import_decl.span.start { - fixes.push(fix); + rule_fixes.push(fix); } else { after_fixes.push(fix); } @@ -427,9 +428,9 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu .join(", "), ctx.source_range(import_decl.source.span), ); - fixes.push(fixer.insert_text_before(*import_decl, text)); + rule_fixes.push(fixer.insert_text_before(*import_decl, text)); } else { - fixes.push(fixer.insert_text_before( + rule_fixes.push(fixer.insert_text_before( *import_decl, format!( "import type {{{}}} from {};\n", @@ -441,7 +442,7 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu } } - let mut fixes_remove_type_namespace_specifier = vec![]; + let mut fixes_remove_type_namespace_specifier = fixer.new_fix_with_capacity(0); if let Some(namespace_specifier) = namespace_specifier { if type_names.iter().contains(&namespace_specifier.local.name.as_str()) { @@ -459,7 +460,7 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu // import type * as Ns from 'foo' // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ insert - fixes.push(fixer.insert_text_before( + rule_fixes.push(fixer.insert_text_before( *import_decl, format!( "import type {} from {};\n", @@ -475,7 +476,7 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu if type_names.len() == import_decl.specifiers.as_ref().map_or(0, |s| s.len()) { // import type Type from 'foo' // ^^^^^ insert - fixes.push(fixer.insert_text_after( + rule_fixes.push(fixer.insert_text_after( &Span::new(import_decl.span().start, import_decl.span().start + 6), " type", )); @@ -490,7 +491,7 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu default_specifier.span.start, import_decl.span().start + comma, )); - fixes.push(fixer.insert_text_before( + rule_fixes.push(fixer.insert_text_before( *import_decl, format!( "import type {default_text} from {};\n", @@ -502,7 +503,7 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu find_first_non_white_space(&import_text[(comma + 1) as usize..]) { let after_token = comma as usize + 1 + after_token.0; - fixes.push(fixer.delete_range(Span::new( + rule_fixes.push(fixer.delete_range(Span::new( default_specifier.span.start, import_decl.span().start + after_token as u32, ))); @@ -511,16 +512,16 @@ fn fix_to_type_import_declaration<'a>(options: &FixOptions<'a, '_>) -> FixerResu } } - fixes.extend(fixes_named_specifiers.remove_type_name_specifiers); - fixes.extend(fixes_remove_type_namespace_specifier); - fixes.extend(after_fixes); - Ok(fixes) + Ok(rule_fixes + .extend(fixes_named_specifiers.remove_type_name_specifiers) + .extend(fixes_remove_type_namespace_specifier) + .extend(after_fixes)) } fn fix_insert_named_specifiers_in_named_specifier_list<'a>( options: &FixOptions<'a, '_>, insert_text: &str, -) -> FixerResult> { +) -> FixerResult> { let FixOptions { fixer, import_decl, ctx, .. } = options; let import_text = ctx.source_range(import_decl.span); let close_brace = try_find_char(import_text, '}')?; @@ -541,7 +542,7 @@ fn fix_insert_named_specifiers_in_named_specifier_list<'a>( #[derive(Default, Debug)] struct FixNamedSpecifiers<'a> { type_named_specifiers_text: String, - remove_type_name_specifiers: Vec>, + remove_type_name_specifiers: RuleFix<'a>, } // get the type-only named import declaration with same source @@ -578,15 +579,16 @@ fn get_fixes_named_specifiers<'a>( options: &FixOptions<'a, '_>, subset_named_specifiers: &[&ImportSpecifier<'a>], all_named_specifiers: &[&ImportSpecifier<'a>], -) -> Result, Box> { +) -> FixerResult> { let FixOptions { fixer, import_decl, ctx, .. } = options; + let fixer = fixer.for_multifix(); if all_named_specifiers.is_empty() { return Ok(FixNamedSpecifiers::default()); } let mut type_named_specifiers_text: Vec<&str> = vec![]; - let mut remove_type_named_specifiers: Vec = vec![]; + let mut remove_type_named_specifiers = fixer.new_fix_with_capacity(1); if subset_named_specifiers.len() == all_named_specifiers.len() { // import Foo, {Type1, Type2} from 'foo' @@ -736,10 +738,11 @@ fn try_find_char(text: &str, c: char) -> Result> { fn fix_inline_type_import_declaration<'a>( options: &FixOptions<'a, '_>, -) -> FixerResult>> { +) -> FixerResult> { let FixOptions { fixer, import_decl, type_names, ctx, .. } = options; + let fixer = fixer.for_multifix(); - let mut fixes = vec![]; + let mut rule_fixes = fixer.new_fix_with_capacity(0); let Some(specifiers) = &import_decl.specifiers else { return fixer_error("Missing specifiers in import declaration"); @@ -748,7 +751,7 @@ fn fix_inline_type_import_declaration<'a>( for specifier in specifiers { if let ImportDeclarationSpecifier::ImportSpecifier(specifier) = specifier { if type_names.iter().contains(&specifier.local.name.as_str()) { - fixes.push( + rule_fixes.push( fixer.replace( specifier.span, format!("type {}", ctx.source_range(specifier.span)), @@ -758,20 +761,21 @@ fn fix_inline_type_import_declaration<'a>( } } - Ok(fixes) + Ok(rule_fixes) } fn fix_insert_type_specifier_for_import_declaration<'a>( options: &FixOptions<'a, '_>, is_default_import: bool, -) -> FixerResult>> { +) -> FixerResult> { let FixOptions { fixer, import_decl, ctx, .. } = options; + let fixer = fixer.for_multifix(); let import_source = ctx.source_range(import_decl.span); - let mut fixes = vec![]; + let mut rule_fixes = fixer.new_fix_with_capacity(1); // "import { Foo, Bar } from 'foo'" => "import type { Foo, Bar } from 'foo'" // ^^^^ add - fixes.push( + rule_fixes.push( fixer.replace(Span::new(import_decl.span.start, import_decl.span.start + 6), "import type"), ); @@ -784,7 +788,7 @@ fn fix_insert_type_specifier_for_import_declaration<'a>( let base = import_decl.span.start; // import foo, {} from 'foo' // ^^^^ delete - fixes.push( + rule_fixes.push( fixer.delete(&Span::new(base + comma_token, base + (closing_brace_token + 1))), ); if import_decl.specifiers.as_ref().is_some_and(|specifiers| specifiers.len() > 1) { @@ -800,7 +804,7 @@ fn fix_insert_type_specifier_for_import_declaration<'a>( )); }; - fixes.push(fixer.insert_text_after( + rule_fixes.push(fixer.insert_text_after( *import_decl, format!( "\nimport type {} from {}", @@ -818,7 +822,7 @@ fn fix_insert_type_specifier_for_import_declaration<'a>( if specifier.import_kind.is_type() { // import { type A } from 'foo.js' // ^^^^^^^^ delete - fixes.push( + rule_fixes.push( fixer.delete(&Span::new( specifier.span.start, specifier.imported.span().start, @@ -829,7 +833,7 @@ fn fix_insert_type_specifier_for_import_declaration<'a>( } } - Ok(fixes) + Ok(rule_fixes) } struct GroupedSpecifiers<'a, 'b> { @@ -866,11 +870,12 @@ fn classify_specifier<'a, 'b>(import_decl: &'b ImportDeclaration<'a>) -> Grouped // import type Foo from 'foo' // ^^^^ remove +// note:(don): RuleFix added fn fix_remove_type_specifier_from_import_declaration<'a>( fixer: RuleFixer<'_, 'a>, import_decl_span: Span, ctx: &LintContext<'a>, -) -> Fix<'a> { +) -> RuleFix<'a> { let import_source = ctx.source_range(import_decl_span); let new_import_source = import_source // ` type Foo from 'foo'` @@ -896,7 +901,7 @@ fn fix_remove_type_specifier_from_import_specifier<'a>( fixer: RuleFixer<'_, 'a>, specifier_span: Span, ctx: &LintContext<'a>, -) -> Fix<'a> { +) -> RuleFix<'a> { let specifier_source = ctx.source_range(specifier_span); let new_specifier_source = specifier_source.strip_prefix("type "); diff --git a/crates/oxc_linter/src/rules/unicorn/no_null.rs b/crates/oxc_linter/src/rules/unicorn/no_null.rs index 6b25f7b29aa252..2dfa16ce204b09 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_null.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_null.rs @@ -12,7 +12,7 @@ use oxc_syntax::operator::BinaryOperator; use crate::{ ast_util::is_method_call, context::LintContext, - fixer::{Fix, RuleFixer}, + fixer::{RuleFix, RuleFixer}, rule::Rule, AstNode, }; @@ -220,7 +220,7 @@ impl Rule for NoNull { } } -fn fix_null<'a>(fixer: RuleFixer<'_, 'a>, null: &NullLiteral) -> Fix<'a> { +fn fix_null<'a>(fixer: RuleFixer<'_, 'a>, null: &NullLiteral) -> RuleFix<'a> { fixer.replace(null.span, "undefined") } #[test] diff --git a/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs b/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs index e19f9cc2b426c3..95b5fb847cddef 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs @@ -14,7 +14,7 @@ use crate::{ get_new_expr_ident_name, is_method_call, is_new_expression, outermost_paren_parent, }, context::LintContext, - fixer::{Fix, RuleFixer}, + fixer::{RuleFix, RuleFixer}, rule::Rule, AstNode, }; @@ -447,7 +447,7 @@ fn fix_replace<'a, T: GetSpan, U: GetSpan>( fixer: RuleFixer<'_, 'a>, target: &T, replacement: &U, -) -> Fix<'a> { +) -> RuleFix<'a> { let replacement = fixer.source_range(replacement.span()); fixer.replace(target.span(), replacement) } @@ -457,7 +457,7 @@ fn fix_by_removing_spread<'a, S: GetSpan>( fixer: RuleFixer<'_, 'a>, iterable: &S, spread: &SpreadElement<'a>, -) -> Fix<'a> { +) -> RuleFix<'a> { fixer.replace(iterable.span(), fixer.source_range(spread.argument.span())) } diff --git a/crates/oxc_linter/src/rules/unicorn/prefer_query_selector.rs b/crates/oxc_linter/src/rules/unicorn/prefer_query_selector.rs index 006e2d2dd121ba..5fd38e4aa990c3 100644 --- a/crates/oxc_linter/src/rules/unicorn/prefer_query_selector.rs +++ b/crates/oxc_linter/src/rules/unicorn/prefer_query_selector.rs @@ -4,7 +4,7 @@ use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; use phf::phf_map; -use crate::{context::LintContext, rule::Rule, utils::is_node_value_not_dom_node, AstNode, Fix}; +use crate::{context::LintContext, rule::Rule, utils::is_node_value_not_dom_node, AstNode}; fn prefer_query_selector_diagnostic(x0: &str, x1: &str, span2: Span) -> OxcDiagnostic { OxcDiagnostic::warn(format!("eslint-plugin-unicorn(prefer-query-selector): Prefer `.{x0}()` over `.{x1}()`.")) @@ -107,7 +107,7 @@ impl Rule for PreferQuerySelector { if let Some(literal_value) = literal_value { return ctx.diagnostic_with_fix(diagnostic, |fixer| { if literal_value.is_empty() { - return Fix::new(*preferred_selector, property_span); + return fixer.replace(property_span, *preferred_selector); } // let source_text = diff --git a/crates/oxc_linter/src/service.rs b/crates/oxc_linter/src/service.rs index 36704600284bda..c01f70e0d3e41f 100644 --- a/crates/oxc_linter/src/service.rs +++ b/crates/oxc_linter/src/service.rs @@ -227,7 +227,7 @@ impl Runtime { self.process_source(path, &allocator, source_text, source_type, true, tx_error); // TODO: Span is wrong, ban this feature for file process by `PartialLoader`. - if !is_processed_by_partial_loader && self.linter.options().fix { + if !is_processed_by_partial_loader && self.linter.options().fix.is_some() { let fix_result = Fixer::new(source_text, messages).fix(); fs::write(path, fix_result.fixed_code.as_bytes()).unwrap(); messages = fix_result.messages; diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index 09742d8fb3957d..35ac5a87263a5e 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -9,8 +9,8 @@ use serde::Deserialize; use serde_json::Value; use crate::{ - rules::RULES, AllowWarnDeny, Fixer, LintOptions, LintService, LintServiceOptions, Linter, - OxlintConfig, RuleEnum, RuleWithSeverity, + fixer::FixKind, rules::RULES, AllowWarnDeny, Fixer, LintOptions, LintService, + LintServiceOptions, Linter, OxlintConfig, RuleEnum, RuleWithSeverity, }; #[derive(Eq, PartialEq)] @@ -259,7 +259,7 @@ impl Tester { let allocator = Allocator::default(); let rule = self.find_rule().read_json(rule_config.unwrap_or_default()); let options = LintOptions::default() - .with_fix(is_fix) + .with_fix(is_fix.then_some(FixKind::DangerousFix)) .with_import_plugin(self.import_plugin) .with_jest_plugin(self.jest_plugin) .with_vitest_plugin(self.vitest_plugin)