Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linter): accept multiple fixes when fix code #3842

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions crates/oxc_linter/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -171,14 +171,16 @@ impl<'a> LintContext<'a> {
}

/// Report a lint rule violation and provide an automatic fix.
pub fn diagnostic_with_fix<F: FnOnce(RuleFixer<'_, 'a>) -> Fix<'a>>(
&self,
diagnostic: OxcDiagnostic,
fix: F,
) {
pub fn diagnostic_with_fix<C, F>(&self, diagnostic: OxcDiagnostic, fix: F)
where
C: Into<CompositeFix<'a>>,
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);
}
Expand Down
202 changes: 201 additions & 1 deletion crates/oxc_linter/src/fixer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,92 @@ impl<'a> Fix<'a> {
}
}

pub enum CompositeFix<'a> {
mysteryven marked this conversation as resolved.
Show resolved Hide resolved
Single(Fix<'a>),
Multiple(Vec<Fix<'a>>),
}

impl<'a> From<Fix<'a>> for CompositeFix<'a> {
fn from(fix: Fix<'a>) -> Self {
CompositeFix::Single(fix)
}
}

impl<'a> From<Vec<Fix<'a>>> for CompositeFix<'a> {
fn from(fixes: Vec<Fix<'a>>) -> Self {
CompositeFix::Multiple(fixes)
}
}

impl<'a> CompositeFix<'a> {
/// Gets one fix from the fixes. If we retrieve multiple fixes, this merges those into one.
/// <https://github.com/eslint/eslint/blob/main/lib/linter/report-translator.js#L181-L203>
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)
// <https://github.com/eslint/eslint/blob/main/lib/linter/report-translator.js#L147-L179>
fn merge_fixes(fixes: Vec<Fix<'a>>, 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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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: &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
// <https://github.com/eslint/eslint/blob/main/tests/lib/linter/report-translator.js>
// 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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Expand Down Expand Up @@ -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
},
);
}
Expand Down
6 changes: 5 additions & 1 deletion crates/oxc_linter/src/rules/unicorn/no_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading