From 158a79eba509dac83a6d9a50e4085d0250605e32 Mon Sep 17 00:00:00 2001 From: Xuan <97ssps30212@gmail.com> Date: Wed, 8 Mar 2023 15:36:05 +0800 Subject: [PATCH] refactor(fixer): add span to message (#146) * refactor(fixer): add span to message * refactor(fixer): use error.labels() to sort * fix(fixer): make clippy happy * refactor(fixer): use sort_by_cached_key --- crates/oxc_linter/src/autofix/mod.rs | 23 ------- crates/oxc_linter/src/context.rs | 20 +----- crates/oxc_linter/src/{autofix => }/fixer.rs | 67 ++++++++++++++------ crates/oxc_linter/src/lib.rs | 5 +- crates/oxc_linter/src/rules/eq_eq_eq.rs | 8 ++- crates/oxc_linter/src/rules/no_debugger.rs | 2 +- 6 files changed, 56 insertions(+), 69 deletions(-) delete mode 100644 crates/oxc_linter/src/autofix/mod.rs rename crates/oxc_linter/src/{autofix => }/fixer.rs (85%) diff --git a/crates/oxc_linter/src/autofix/mod.rs b/crates/oxc_linter/src/autofix/mod.rs deleted file mode 100644 index b22887ee2..000000000 --- a/crates/oxc_linter/src/autofix/mod.rs +++ /dev/null @@ -1,23 +0,0 @@ -use std::borrow::Cow; - -use oxc_ast::Span; - -mod fixer; - -pub use fixer::{Fixer, Message}; - -#[derive(Debug, Default)] -pub struct Fix<'a> { - pub content: Cow<'a, str>, - pub span: Span, -} - -impl<'a> Fix<'a> { - pub const fn delete(span: Span) -> Self { - Self { content: Cow::Borrowed(""), span } - } - - pub fn new>>(content: T, span: Span) -> Self { - Self { content: content.into(), span } - } -} diff --git a/crates/oxc_linter/src/context.rs b/crates/oxc_linter/src/context.rs index 4c702f367..25b372cbd 100644 --- a/crates/oxc_linter/src/context.rs +++ b/crates/oxc_linter/src/context.rs @@ -5,7 +5,7 @@ use oxc_diagnostics::Error; use oxc_semantic::{AstNodes, Semantic}; use crate::{ - autofix::{Fix, Message}, + fixer::{Fix, Message}, AstNode, }; @@ -18,20 +18,11 @@ pub struct LintContext<'a> { /// Whether or not to apply code fixes during linting. fix: bool, - - /// Collection of applicable fixes based on reported issues. - fixes: RefCell>>, } impl<'a> LintContext<'a> { pub fn new(source_text: &'a str, semantic: Rc>, fix: bool) -> Self { - Self { - source_text, - semantic, - diagnostics: RefCell::new(vec![]), - fix, - fixes: RefCell::new(vec![]), - } + Self { source_text, semantic, diagnostics: RefCell::new(vec![]), fix } } pub fn source_text(&self) -> &'a str { @@ -58,13 +49,6 @@ impl<'a> LintContext<'a> { } } - pub fn fix(&self, fix: Fix<'a>) { - if !self.fix { - return; - } - self.fixes.borrow_mut().push(fix); - } - #[inline] pub fn semantic(&self) -> &Semantic<'a> { &self.semantic diff --git a/crates/oxc_linter/src/autofix/fixer.rs b/crates/oxc_linter/src/fixer.rs similarity index 85% rename from crates/oxc_linter/src/autofix/fixer.rs rename to crates/oxc_linter/src/fixer.rs index bdc1d3bcb..bc203a4ca 100644 --- a/crates/oxc_linter/src/autofix/fixer.rs +++ b/crates/oxc_linter/src/fixer.rs @@ -1,8 +1,23 @@ use std::borrow::Cow; -use oxc_diagnostics::Error; +use oxc_ast::Span; +use oxc_diagnostics::{miette::LabeledSpan, Error}; -use super::Fix; +#[derive(Debug, Default)] +pub struct Fix<'a> { + pub content: Cow<'a, str>, + pub span: Span, +} + +impl<'a> Fix<'a> { + pub const fn delete(span: Span) -> Self { + Self { content: Cow::Borrowed(""), span } + } + + pub fn new>>(content: T, span: Span) -> Self { + Self { content: content.into(), span } + } +} pub struct FixResult<'a> { pub fixed: bool, @@ -18,6 +33,7 @@ pub struct Message<'a> { } impl<'a> Message<'a> { + #[must_use] pub fn new(error: Error, fix: Option>) -> Self { Self { error, fix, fixed: false } } @@ -74,7 +90,16 @@ impl<'a> Fixer<'a> { let offset = usize::try_from(last_pos.max(0)).ok().unwrap(); output.push_str(&source_text[offset..]); - let messages = self.messages.into_iter().filter(|m| !m.fixed).collect::>(); + let mut messages = self.messages.into_iter().filter(|m| !m.fixed).collect::>(); + messages.sort_by_cached_key(|m| { + let span = m + .error + .labels() + .expect("should specify a span for a rule") + .min_by_key(LabeledSpan::offset) + .expect("should contain at least one span"); + (span.offset(), span.len()) + }); return FixResult { fixed, fixed_code: Cow::Owned(output), messages }; } @@ -88,8 +113,7 @@ mod test { use oxc_ast::Span; use oxc_diagnostics::{thiserror::Error, Error}; - use super::{FixResult, Fixer, Message}; - use crate::autofix::Fix; + use super::{Fix, FixResult, Fixer, Message}; const TEST_CODE: &str = "var answer = 6 * 7;"; @@ -132,7 +156,7 @@ mod test { #[derive(Debug, Error, Diagnostic)] #[error("removemiddle")] - struct RemoveMiddle(); + struct RemoveMiddle(#[label] pub Span); const REMOVE_MIDDLE: Fix = Fix::delete(Span { start: 5, end: 10 }); #[derive(Debug, Error, Diagnostic)] @@ -147,15 +171,15 @@ mod test { #[derive(Debug, Error, Diagnostic)] #[error("nofix")] - struct NoFix(); + struct NoFix(#[label] pub Span); #[derive(Debug, Error, Diagnostic)] #[error("nofix1")] - struct NoFix1(); + struct NoFix1(#[label] pub Span); #[derive(Debug, Error, Diagnostic)] #[error("nofix2")] - struct NoFix2(); + struct NoFix2(#[label] pub Span); fn get_fix_result(messages: Vec) -> FixResult { Fixer::new(TEST_CODE, messages).fix() @@ -262,7 +286,10 @@ mod test { #[test] fn remove_at_the_middle() { - let result = get_fix_result(vec![create_message(RemoveMiddle(), Some(REMOVE_MIDDLE))]); + let result = get_fix_result(vec![create_message( + RemoveMiddle(Span::default()), + Some(REMOVE_MIDDLE), + )]); assert_eq!(result.fixed_code, TEST_CODE.replace("answer", "a")); assert_eq!(result.messages.len(), 0); assert!(result.fixed); @@ -291,7 +318,7 @@ mod test { #[test] fn apply_one_fix_when_spans_overlap() { let result = get_fix_result(vec![ - create_message(RemoveMiddle(), Some(REMOVE_MIDDLE)), + create_message(RemoveMiddle(Span::default()), Some(REMOVE_MIDDLE)), create_message(ReplaceId(), Some(REPLACE_ID)), ]); assert_eq!(result.fixed_code, TEST_CODE.replace("answer", "foo")); @@ -315,9 +342,9 @@ mod test { #[test] fn apply_one_fix_when_range_overlap_and_one_message_has_no_fix() { let result = get_fix_result(vec![ - create_message(RemoveMiddle(), Some(REMOVE_MIDDLE)), + create_message(RemoveMiddle(Span::default()), Some(REMOVE_MIDDLE)), create_message(ReplaceId(), Some(REPLACE_ID)), - create_message(NoFix(), None), + create_message(NoFix(Span::default()), None), ]); assert_eq!(result.fixed_code, TEST_CODE.replace("answer", "foo")); assert_eq!(result.messages.len(), 2); @@ -329,38 +356,36 @@ mod test { #[test] fn apply_same_fix_when_span_overlap_regardless_of_order() { let result1 = get_fix_result(vec![ - create_message(RemoveMiddle(), Some(REMOVE_MIDDLE)), + create_message(RemoveMiddle(Span::default()), Some(REMOVE_MIDDLE)), create_message(ReplaceId(), Some(REPLACE_ID)), ]); let result2 = get_fix_result(vec![ create_message(ReplaceId(), Some(REPLACE_ID)), - create_message(RemoveMiddle(), Some(REMOVE_MIDDLE)), + create_message(RemoveMiddle(Span::default()), Some(REMOVE_MIDDLE)), ]); assert_eq!(result1.fixed_code, result2.fixed_code); } #[test] fn should_not_apply_fix_with_one_no_fix() { - let result = get_fix_result(vec![create_message(NoFix(), None)]); + let result = get_fix_result(vec![create_message(NoFix(Span::default()), None)]); assert_eq!(result.fixed_code, TEST_CODE); assert_eq!(result.messages.len(), 1); assert_eq!(result.messages[0].error.to_string(), "nofix"); assert!(!result.fixed); } - #[ignore] #[test] fn sort_no_fix_messages_correctly() { let result = get_fix_result(vec![ create_message(ReplaceId(), Some(REPLACE_ID)), - create_message(NoFix2(), None), - create_message(NoFix1(), None), + Message::new(NoFix2(Span { start: 1, end: 7 }).into(), None), + Message::new(NoFix1(Span { start: 1, end: 3 }).into(), None), ]); assert_eq!(result.fixed_code, TEST_CODE.replace("answer", "foo")); assert_eq!(result.messages.len(), 2); - // We should sort the error to pass the following assertion. assert_eq!(result.messages[0].error.to_string(), "nofix1"); - assert_eq!(result.messages[0].error.to_string(), "nofix2"); + assert_eq!(result.messages[1].error.to_string(), "nofix2"); assert!(result.fixed); } } diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 26be2ad18..47512e06b 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -3,18 +3,17 @@ #[cfg(test)] mod tester; -mod autofix; mod context; +mod fixer; pub mod rule; mod rules; use std::{fs, rc::Rc}; -use autofix::Message; +pub use fixer::{Fixer, Message}; pub(crate) use oxc_semantic::AstNode; use oxc_semantic::Semantic; -pub use crate::autofix::Fixer; use crate::{ context::LintContext, rules::{RuleEnum, RULES}, diff --git a/crates/oxc_linter/src/rules/eq_eq_eq.rs b/crates/oxc_linter/src/rules/eq_eq_eq.rs index ab03dd3ba..c22c07726 100644 --- a/crates/oxc_linter/src/rules/eq_eq_eq.rs +++ b/crates/oxc_linter/src/rules/eq_eq_eq.rs @@ -8,7 +8,7 @@ use oxc_diagnostics::{ }; use oxc_macros::declare_oxc_lint; -use crate::{autofix::Fix, context::LintContext, rule::Rule, AstNode}; +use crate::{context::LintContext, fixer::Fix, rule::Rule, AstNode}; #[derive(Debug, Error, Diagnostic)] #[error("eslint(eqeqeq): Expected {1:?} and instead saw {0:?}")] @@ -64,8 +64,10 @@ impl Rule for EqEqEq { if !is_valid_comparison { let operator = binary_expr.operator.as_str(); let prefered_operator = to_strict_operator(binary_expr.operator).as_str(); - ctx.diagnostic(EqEqEqDiagnostic(operator, prefered_operator, binary_expr.span)); - ctx.fix(Fix::new(prefered_operator, binary_expr.span)); + ctx.diagnostic_with_fix( + EqEqEqDiagnostic(operator, prefered_operator, binary_expr.span), + || Fix::new(prefered_operator, binary_expr.span), + ); } } } diff --git a/crates/oxc_linter/src/rules/no_debugger.rs b/crates/oxc_linter/src/rules/no_debugger.rs index 1b2021688..928c8833b 100644 --- a/crates/oxc_linter/src/rules/no_debugger.rs +++ b/crates/oxc_linter/src/rules/no_debugger.rs @@ -5,7 +5,7 @@ use oxc_diagnostics::{ }; use oxc_macros::declare_oxc_lint; -use crate::{autofix::Fix, context::LintContext, rule::Rule, AstNode}; +use crate::{context::LintContext, fixer::Fix, rule::Rule, AstNode}; #[derive(Debug, Error, Diagnostic)] #[error("eslint(no-debugger): `debugger` statement is not allowed")]