From 63b98bddd93bd97c5f9a3681867ca609a1db055f Mon Sep 17 00:00:00 2001 From: mysteryven <33973865+mysteryven@users.noreply.github.com> Date: Wed, 26 Jun 2024 13:29:22 +0000 Subject: [PATCH] feat(linter): accept multiple fixes when fix code (#3842) Want to add this because of there are bunch of fix codes in [consistent-type-import](https://github.com/typescript-eslint/typescript-eslint/blob/e408b93e48e5199d83a8d99d1e27126d2dd8bc8f/packages/eslint-plugin/src/rules/consistent-type-imports.ts#L610-L945) are not easy to port. --- crates/oxc_linter/src/context.rs | 16 +- crates/oxc_linter/src/fixer.rs | 202 +++++++++++++++++- .../typescript/no_import_type_side_effects.rs | 52 ++--- .../oxc_linter/src/rules/unicorn/no_null.rs | 6 +- 4 files changed, 233 insertions(+), 43 deletions(-) diff --git a/crates/oxc_linter/src/context.rs b/crates/oxc_linter/src/context.rs index f30b9d6fdde75..2ccf338c6de6c 100644 --- a/crates/oxc_linter/src/context.rs +++ b/crates/oxc_linter/src/context.rs @@ -9,7 +9,7 @@ use oxc_syntax::module_record::ModuleRecord; use crate::{ config::OxlintRules, disable_directives::{DisableDirectives, DisableDirectivesBuilder}, - fixer::{Fix, Message, RuleFixer}, + fixer::{CompositeFix, Message, RuleFixer}, javascript_globals::GLOBALS, AllowWarnDeny, OxlintConfig, OxlintEnv, OxlintGlobals, OxlintSettings, }; @@ -171,14 +171,16 @@ impl<'a> LintContext<'a> { } /// Report a lint rule violation and provide an automatic fix. - pub fn diagnostic_with_fix) -> Fix<'a>>( - &self, - diagnostic: OxcDiagnostic, - fix: F, - ) { + pub fn diagnostic_with_fix(&self, diagnostic: OxcDiagnostic, fix: F) + where + C: Into>, + F: FnOnce(RuleFixer<'_, 'a>) -> C, + { if self.fix { let fixer = RuleFixer::new(self); - self.add_diagnostic(Message::new(diagnostic, Some(fix(fixer)))); + let composite_fix: CompositeFix = fix(fixer).into(); + let fix = composite_fix.normalize_fixes(self.source_text()); + self.add_diagnostic(Message::new(diagnostic, Some(fix))); } else { self.diagnostic(diagnostic); } diff --git a/crates/oxc_linter/src/fixer.rs b/crates/oxc_linter/src/fixer.rs index 1913f101a9e4e..bfa29980cc940 100644 --- a/crates/oxc_linter/src/fixer.rs +++ b/crates/oxc_linter/src/fixer.rs @@ -22,6 +22,92 @@ impl<'a> Fix<'a> { } } +pub enum CompositeFix<'a> { + Single(Fix<'a>), + Multiple(Vec>), +} + +impl<'a> From> for CompositeFix<'a> { + fn from(fix: Fix<'a>) -> Self { + CompositeFix::Single(fix) + } +} + +impl<'a> From>> for CompositeFix<'a> { + fn from(fixes: Vec>) -> Self { + CompositeFix::Multiple(fixes) + } +} + +impl<'a> CompositeFix<'a> { + /// 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> { + match self { + CompositeFix::Single(fix) => fix, + CompositeFix::Multiple(fixes) => Self::merge_fixes(fixes, source_text), + } + } + // Merges multiple fixes to one, returns an `Fix::default`(which will not fix anything) if: + // 1. `fixes` is empty + // 2. contains overlapped ranges + // 3. contains negative ranges (span.start > span.end) + // + fn merge_fixes(fixes: Vec>, source_text: &str) -> Fix<'a> { + let mut fixes = fixes; + let empty_fix = Fix::default(); + if fixes.is_empty() { + // Do nothing + return empty_fix; + } + if fixes.len() == 1 { + return fixes.pop().unwrap(); + } + + fixes.sort_by(|a, b| a.span.cmp(&b.span)); + + // safe, as fixes.len() > 1 + let start = fixes[0].span.start; + let end = fixes[fixes.len() - 1].span.end; + let mut last_pos = start; + let mut output = String::new(); + + for fix in fixes { + let Fix { ref content, span } = fix; + // negative range or overlapping ranges is invalid + if span.start > span.end { + debug_assert!(false, "Negative range is invalid: {span:?}"); + return empty_fix; + } + if last_pos > span.start { + debug_assert!( + false, + "Fix must not be overlapped, last_pos: {}, span.start: {}", + last_pos, span.start + ); + return empty_fix; + } + + let Some(before) = source_text.get((last_pos) as usize..span.start as usize) else { + debug_assert!(false, "Invalid range: {}, {}", last_pos, span.start); + return empty_fix; + }; + + output.push_str(before); + output.push_str(content); + last_pos = span.end; + } + + let Some(after) = source_text.get(last_pos as usize..end as usize) else { + debug_assert!(false, "Invalid range: {:?}", last_pos as usize..end as usize); + return empty_fix; + }; + + output.push_str(after); + Fix::new(output, Span::new(start, end)) + } +} + /// Inspired by ESLint's [`RuleFixer`]. /// /// [`RuleFixer`]: https://github.com/eslint/eslint/blob/main/lib/linter/rule-fixer.js @@ -174,7 +260,7 @@ mod test { use oxc_diagnostics::OxcDiagnostic; use oxc_span::Span; - use super::{Fix, FixResult, Fixer, Message}; + use super::{CompositeFix, Fix, FixResult, Fixer, Message}; fn insert_at_end() -> OxcDiagnostic { OxcDiagnostic::warn("End") @@ -448,4 +534,118 @@ mod test { assert_eq!(result.messages[1].error.to_string(), "nofix2"); assert!(result.fixed); } + + fn assert_fixed_corrected(source_text: &str, expected: &str, composite_fix: CompositeFix) { + let mut source_text = source_text.to_string(); + let fix = composite_fix.normalize_fixes(&source_text); + let start = fix.span.start as usize; + let end = fix.span.end as usize; + source_text.replace_range(start..end, fix.content.to_string().as_str()); + assert_eq!(source_text, expected); + } + + #[test] + fn merge_fixes_in_composite_fix() { + let source_text = "foo bar baz"; + let fixes = vec![Fix::new("quux", Span::new(0, 3)), Fix::new("qux", Span::new(4, 7))]; + let composite_fix = CompositeFix::Multiple(fixes); + assert_fixed_corrected(source_text, "quux qux baz", composite_fix); + } + + #[test] + fn one_fix_in_composite_fix() { + let source_text = "foo bar baz"; + let fix = Fix::new("quxx", Span::new(4, 7)); + let composite_fix = CompositeFix::Single(fix.clone()); + assert_fixed_corrected(source_text, "foo quxx baz", composite_fix); + + let composite_fix = CompositeFix::Multiple(vec![fix]); + assert_fixed_corrected(source_text, "foo quxx baz", composite_fix); + } + + #[test] + fn zero_fixes_in_composite_fix() { + let source_text = "foo bar baz"; + let composite_fix = CompositeFix::Multiple(vec![]); + assert_fixed_corrected(source_text, source_text, composite_fix); + } + + #[test] + #[should_panic(expected = "Fix must not be overlapped, last_pos: 3, span.start: 2")] + fn overlapping_ranges_in_composite_fix() { + let source_text = "foo bar baz"; + let fixes = vec![Fix::new("quux", Span::new(0, 3)), Fix::new("qux", Span::new(2, 5))]; + let composite_fix = CompositeFix::Multiple(fixes); + assert_fixed_corrected(source_text, source_text, composite_fix); + } + + #[test] + #[should_panic(expected = "Negative range is invalid: Span { start: 5, end: 2 }")] + fn negative_ranges_in_composite_fix() { + let source_text = "foo bar baz"; + let fixes = vec![Fix::new("quux", Span::new(0, 3)), Fix::new("qux", Span::new(5, 2))]; + let composite_fix = CompositeFix::Multiple(fixes); + assert_fixed_corrected(source_text, source_text, composite_fix); + } + + fn assert_fixes_merged(fixes: Vec, fix: &Fix, source_text: &str) { + let composite_fix = CompositeFix::from(fixes); + let merged_fix = composite_fix.normalize_fixes(source_text); + assert_eq!(merged_fix.content, fix.content); + assert_eq!(merged_fix.span, fix.span); + } + + // Remain test caces picked from eslint + // + // 1. Combining autofixes + #[test] + fn merge_fixes_into_one() { + let source_text = "foo\nbar"; + let fixes = vec![Fix::new("foo", Span::new(1, 2)), Fix::new("bar", Span::new(4, 5))]; + assert_fixes_merged(fixes, &Fix::new("fooo\nbar", Span::new(1, 5)), source_text); + } + + #[test] + fn respect_ranges_of_empty_insertions() { + let source_text = "foo\nbar"; + let fixes = vec![ + Fix::new("cd", Span::new(4, 5)), + Fix::new("", Span::new(2, 2)), + Fix::new("", Span::new(7, 7)), + ]; + assert_fixes_merged(fixes, &Fix::new("o\ncdar", Span::new(2, 7)), source_text); + } + + #[test] + fn pass_through_fixes_if_only_one_present() { + let source_text = "foo\nbar"; + let fix = Fix::new("foo", Span::new(1, 2)); + assert_fixes_merged(vec![fix.clone()], &fix, source_text); + } + + #[test] + #[should_panic(expected = "Fix must not be overlapped, last_pos: 3, span.start: 2")] + fn throw_error_when_ranges_overlap() { + let source_text = "foo\nbar"; + let fixes = vec![Fix::new("foo", Span::new(0, 3)), Fix::new("x", Span::new(2, 5))]; + assert_fixes_merged(fixes, &Fix::default(), source_text); + } + + // 2. unique `fix` and `fix.range` objects + #[test] + fn return_new_fix_when_fixes_is_one() { + let source_text = "foo\nbar"; + let fix = Fix::new("baz", Span::new(0, 3)); + let fixes = vec![fix.clone()]; + + assert_fixes_merged(fixes, &fix, source_text); + } + + #[test] + fn create_new_fix_with_new_range_when_fixes_is_multiple() { + let source_text = "foo\nbar"; + let fixes = vec![Fix::new("baz", Span::new(0, 3)), Fix::new("qux", Span::new(4, 7))]; + + assert_fixes_merged(fixes, &Fix::new("baz\nqux", Span::new(0, 7)), source_text); + } } diff --git a/crates/oxc_linter/src/rules/typescript/no_import_type_side_effects.rs b/crates/oxc_linter/src/rules/typescript/no_import_type_side_effects.rs index 0b31577774d8f..888a5f85cd248 100644 --- a/crates/oxc_linter/src/rules/typescript/no_import_type_side_effects.rs +++ b/crates/oxc_linter/src/rules/typescript/no_import_type_side_effects.rs @@ -6,7 +6,7 @@ use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{GetSpan, Span}; -use crate::{context::LintContext, rule::Rule, AstNode}; +use crate::{context::LintContext, fixer::Fix, rule::Rule, AstNode}; fn no_import_type_side_effects_diagnostic(span0: Span) -> OxcDiagnostic { OxcDiagnostic::warn("typescript-eslint(no-import-type-side-effects): TypeScript will only remove the inline type specifiers which will leave behind a side effect import at runtime.") @@ -83,45 +83,29 @@ impl Rule for NoImportTypeSideEffects { // `import { type A, type B } from 'foo.js'` ctx.diagnostic_with_fix( no_import_type_side_effects_diagnostic(import_decl.span), - |fixer| { - let mut delete_ranges = vec![]; + |_fixer| { + let raw = ctx.source_range(import_decl.span); + let mut fixes = vec![]; + + // import type A from 'foo.js' + // ^^^^ add + if raw.starts_with("import") { + fixes.push(Fix::new( + "import type", + Span::new(import_decl.span.start, import_decl.span.start + 6), + )); + } for specifier in type_specifiers { // import { type A } from 'foo.js' // ^^^^^^^^ - delete_ranges - .push(Span::new(specifier.span.start, specifier.imported.span().start)); - } - - let mut output = String::new(); - let mut last_pos = import_decl.span.start; - for range in delete_ranges { - // import { type A } from 'foo.js' - // ^^^^^^^^^^^^^^^ - // | | - // [last_pos range.start) - output.push_str(ctx.source_range(Span::new(last_pos, range.start))); - // import { type A } from 'foo.js' - // ^ - // | - // last_pos - last_pos = range.end; + fixes.push(Fix::delete(Span::new( + specifier.span.start, + specifier.imported.span().start, + ))); } - // import { type A } from 'foo.js' - // ^^^^^^^^^^^^^^^^^^ - // ^ ^ - // | | - // [last_pos import_decl_span.end) - output.push_str(ctx.source_range(Span::new(last_pos, import_decl.span.end))); - - if let Some(output) = output.strip_prefix("import ") { - let output = format!("import type {output}"); - fixer.replace(import_decl.span, output) - } else { - // Do not do anything, this should never happen - fixer.replace(import_decl.span, ctx.source_range(import_decl.span)) - } + fixes }, ); } diff --git a/crates/oxc_linter/src/rules/unicorn/no_null.rs b/crates/oxc_linter/src/rules/unicorn/no_null.rs index fd64ef6f7df3f..f35d11cf37a9f 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_null.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_null.rs @@ -10,7 +10,11 @@ use oxc_span::{GetSpan, Span}; use oxc_syntax::operator::BinaryOperator; use crate::{ - ast_util::is_method_call, context::LintContext, fixer::RuleFixer, rule::Rule, AstNode, Fix, + ast_util::is_method_call, + context::LintContext, + fixer::{Fix, RuleFixer}, + rule::Rule, + AstNode, }; fn replace_null_diagnostic(span0: Span) -> OxcDiagnostic {