From 2083d334f2b8302b2d6d6bbeadab45b1f384df6d Mon Sep 17 00:00:00 2001 From: Sysix <3897725+Sysix@users.noreply.github.com> Date: Fri, 30 May 2025 15:15:21 +0000 Subject: [PATCH] feat(linter/language_server): add second editor suggestion for `react/forward-ref-uses-ref` (#11375) ![feat-second-editor-suggestion](https://github.com/user-attachments/assets/41db3436-ad31-4aea-ac57-54568f7b326a) closes #9561 ~~blocked by #11379~~ --- .../multiple_suggestions/.oxlintrc.json | 9 ++ .../multiple_suggestions/forward_ref.ts | 1 + .../linter/multiple_suggestions/package.json | 5 + .../src/linter/server_linter.rs | 15 +++ ...r_multiple_suggestions@forward_ref.ts.snap | 15 +++ crates/oxc_linter/src/fixer/mod.rs | 14 ++- .../src/rules/react/forward_ref_uses_ref.rs | 46 +++++++-- crates/oxc_linter/src/tester.rs | 94 +++++++++++++------ 8 files changed, 157 insertions(+), 42 deletions(-) create mode 100644 crates/oxc_language_server/fixtures/linter/multiple_suggestions/.oxlintrc.json create mode 100644 crates/oxc_language_server/fixtures/linter/multiple_suggestions/forward_ref.ts create mode 100644 crates/oxc_language_server/fixtures/linter/multiple_suggestions/package.json create mode 100644 crates/oxc_language_server/src/snapshots/fixtures_linter_multiple_suggestions@forward_ref.ts.snap diff --git a/crates/oxc_language_server/fixtures/linter/multiple_suggestions/.oxlintrc.json b/crates/oxc_language_server/fixtures/linter/multiple_suggestions/.oxlintrc.json new file mode 100644 index 0000000000000..8f494fbce5761 --- /dev/null +++ b/crates/oxc_language_server/fixtures/linter/multiple_suggestions/.oxlintrc.json @@ -0,0 +1,9 @@ +{ + "plugins": ["react"], + "categories": { + "correctness": "off" + }, + "rules": { + "react/forward-ref-uses-ref": "error" + } +} diff --git a/crates/oxc_language_server/fixtures/linter/multiple_suggestions/forward_ref.ts b/crates/oxc_language_server/fixtures/linter/multiple_suggestions/forward_ref.ts new file mode 100644 index 0000000000000..d495f3daae78d --- /dev/null +++ b/crates/oxc_language_server/fixtures/linter/multiple_suggestions/forward_ref.ts @@ -0,0 +1 @@ +forwardRef((props) => {}); diff --git a/crates/oxc_language_server/fixtures/linter/multiple_suggestions/package.json b/crates/oxc_language_server/fixtures/linter/multiple_suggestions/package.json new file mode 100644 index 0000000000000..e3bf5e190e283 --- /dev/null +++ b/crates/oxc_language_server/fixtures/linter/multiple_suggestions/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "react": "latest" + } +} diff --git a/crates/oxc_language_server/src/linter/server_linter.rs b/crates/oxc_language_server/src/linter/server_linter.rs index ed3ae5874c937..b235fb9391879 100644 --- a/crates/oxc_language_server/src/linter/server_linter.rs +++ b/crates/oxc_language_server/src/linter/server_linter.rs @@ -350,4 +350,19 @@ mod test { Tester::new("fixtures/linter/cross_module_extended_config", None) .test_and_snapshot_single_file("dep-a.ts"); } + + #[test] + fn test_multiple_suggestions() { + Tester::new( + "fixtures/linter/multiple_suggestions", + Some(Options { + flags: FxHashMap::from_iter([( + "fix_kind".to_string(), + "safe_fix_or_suggestion".to_string(), + )]), + ..Options::default() + }), + ) + .test_and_snapshot_single_file("forward_ref.ts"); + } } diff --git a/crates/oxc_language_server/src/snapshots/fixtures_linter_multiple_suggestions@forward_ref.ts.snap b/crates/oxc_language_server/src/snapshots/fixtures_linter_multiple_suggestions@forward_ref.ts.snap new file mode 100644 index 0000000000000..d378963bd1dc6 --- /dev/null +++ b/crates/oxc_language_server/src/snapshots/fixtures_linter_multiple_suggestions@forward_ref.ts.snap @@ -0,0 +1,15 @@ +--- +source: crates/oxc_language_server/src/tester.rs +input_file: crates/oxc_language_server/fixtures/linter/multiple_suggestions/forward_ref.ts +--- +code: "eslint-plugin-react(forward-ref-uses-ref)" +code_description.href: "https://oxc.rs/docs/guide/usage/linter/rules/react/forward-ref-uses-ref.html" +message: "Components wrapped with `forwardRef` must have a `ref` parameter\nhelp: Add a `ref` parameter, or remove `forwardRef`" +range: Range { start: Position { line: 0, character: 11 }, end: Position { line: 0, character: 24 } } +related_information[0].message: "" +related_information[0].location.uri: "file:///fixtures/linter/multiple_suggestions/forward_ref.ts" +related_information[0].location.range: Range { start: Position { line: 0, character: 11 }, end: Position { line: 0, character: 24 } } +severity: Some(Error) +source: Some("oxc") +tags: None +fixed: Multiple([FixedContent { message: Some("remove `forwardRef` wrapper"), code: "(props) => {}", range: Range { start: Position { line: 0, character: 0 }, end: Position { line: 0, character: 25 } } }, FixedContent { message: Some("add `ref` parameter"), code: "(props, ref)", range: Range { start: Position { line: 0, character: 11 }, end: Position { line: 0, character: 18 } } }]) diff --git a/crates/oxc_linter/src/fixer/mod.rs b/crates/oxc_linter/src/fixer/mod.rs index 70623dc037da2..6f880e5e4e04f 100644 --- a/crates/oxc_linter/src/fixer/mod.rs +++ b/crates/oxc_linter/src/fixer/mod.rs @@ -311,11 +311,21 @@ impl GetSpan for Message<'_> { pub struct Fixer<'a> { source_text: &'a str, messages: Vec>, + + // To test different fixes, we need to override the default behavior. + // The behavior is oriented by `oxlint` where only one PossibleFixes is applied. + fix_index: u8, } impl<'a> Fixer<'a> { pub fn new(source_text: &'a str, messages: Vec>) -> Self { - Self { source_text, messages } + Self { source_text, messages, fix_index: 0 } + } + + #[cfg(test)] + pub fn with_fix_index(mut self, fix_index: u8) -> Self { + self.fix_index = fix_index; + self } /// # Panics @@ -343,7 +353,7 @@ impl<'a> Fixer<'a> { PossibleFixes::Single(fix) => Some(fix), // For multiple fixes, we take the first one as a representative fix. // Applying all possible fixes at once is not possible in this context. - PossibleFixes::Multiple(multiple) => multiple.first(), + PossibleFixes::Multiple(multiple) => multiple.get(self.fix_index as usize), }; let Some(Fix { content, span, .. }) = fix else { filtered_messages.push(m); diff --git a/crates/oxc_linter/src/rules/react/forward_ref_uses_ref.rs b/crates/oxc_linter/src/rules/react/forward_ref_uses_ref.rs index 951d921383d5f..eafb04fdf53a7 100644 --- a/crates/oxc_linter/src/rules/react/forward_ref_uses_ref.rs +++ b/crates/oxc_linter/src/rules/react/forward_ref_uses_ref.rs @@ -6,7 +6,7 @@ use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::Span; -use crate::{AstNode, context::LintContext, rule::Rule}; +use crate::{AstNode, context::LintContext, fixer::RuleFixer, rule::Rule}; fn forward_ref_uses_ref_diagnostic(span: Span) -> OxcDiagnostic { OxcDiagnostic::warn("Components wrapped with `forwardRef` must have a `ref` parameter") @@ -60,9 +60,14 @@ declare_oxc_lint!( react, correctness, suggestion + suggestion ); -fn check_forward_ref_inner(exp: &Expression, call_expr: &CallExpression, ctx: &LintContext) { +fn check_forward_ref_inner<'a>( + exp: &Expression, + call_expr: &CallExpression, + ctx: &LintContext<'a>, +) { let (params, span) = match exp { Expression::ArrowFunctionExpression(f) => (&f.params, f.span), Expression::FunctionExpression(f) => (&f.params, f.span), @@ -71,9 +76,26 @@ fn check_forward_ref_inner(exp: &Expression, call_expr: &CallExpression, ctx: &L if params.parameters_count() != 1 || params.rest.is_some() { return; } - ctx.diagnostic_with_suggestion(forward_ref_uses_ref_diagnostic(span), |fixer| { - fixer.replace_with(call_expr, exp) - }); + + ctx.diagnostics_with_multiple_fixes( + forward_ref_uses_ref_diagnostic(span), + (FixKind::Suggestion, |fixer: RuleFixer<'_, 'a>| { + fixer.replace_with(call_expr, exp).with_message("remove `forwardRef` wrapper") + }), + (FixKind::Suggestion, |fixer: RuleFixer<'_, 'a>| { + let fixed = ctx.source_range(params.span); + // remove the trailing `)`, `` and `,` if they exist + let fixed = fixed.strip_suffix(')').unwrap_or(fixed).trim_end(); + let mut fixed = fixed.strip_suffix(',').unwrap_or(fixed).to_string(); + + if !fixed.starts_with('(') { + fixed.insert(0, '('); + } + fixed.push_str(", ref)"); + + fixer.replace(params.span, fixed).with_message("add `ref` parameter") + }), + ); } impl Rule for ForwardRefUsesRef { @@ -192,11 +214,15 @@ fn test() { ]; let fix = vec![ - ("forwardRef((a) => {})", "(a) => {}"), - ("forwardRef(a => {})", "a => {}"), - ("forwardRef(function (a) {})", "function (a) {}"), - ("forwardRef(function(a,) {})", "function(a,) {}"), - ("React.forwardRef(function(a) {})", "function(a) {}"), + ("forwardRef((a) => {})", ("(a) => {}", "forwardRef((a, ref) => {})")), + ("forwardRef(a => {})", ("a => {}", "forwardRef((a, ref) => {})")), + ("forwardRef(function (a) {})", ("function (a) {}", "forwardRef(function (a, ref) {})")), + ("forwardRef(function(a,) {})", ("function(a,) {}", "forwardRef(function(a, ref) {})")), + ("forwardRef(function(a, ) {})", ("function(a, ) {}", "forwardRef(function(a, ref) {})")), + ( + "React.forwardRef(function(a) {})", + ("function(a) {}", "React.forwardRef(function(a, ref) {})"), + ), ]; Tester::new(ForwardRefUsesRef::NAME, ForwardRefUsesRef::PLUGIN, pass, fail) diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index a895a908ded1b..21b5748e34387 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -123,37 +123,54 @@ impl From> for ExpectFixKind { } #[derive(Debug, Clone)] -pub struct ExpectFix { +pub struct ExpectFixTestCase { /// Source code being tested source: String, + expected: Vec, + rule_config: Option, +} + +#[derive(Debug, Clone)] +struct ExpectFix { /// Expected source code after fix has been applied expected: String, kind: ExpectFixKind, - rule_config: Option, } -impl> From<(S, S, Option)> for ExpectFix { +impl> From<(S, S, Option)> for ExpectFixTestCase { fn from(value: (S, S, Option)) -> Self { Self { source: value.0.into(), - expected: value.1.into(), - kind: ExpectFixKind::Any, + expected: vec![ExpectFix { expected: value.1.into(), kind: ExpectFixKind::Any }], rule_config: value.2, } } } -impl> From<(S, S)> for ExpectFix { +impl> From<(S, S)> for ExpectFixTestCase { fn from(value: (S, S)) -> Self { Self { source: value.0.into(), - expected: value.1.into(), - kind: ExpectFixKind::Any, + expected: vec![ExpectFix { expected: value.1.into(), kind: ExpectFixKind::Any }], rule_config: None, } } } -impl From<(S, S, Option, F)> for ExpectFix + +impl> From<(S, (S, S))> for ExpectFixTestCase { + fn from(value: (S, (S, S))) -> Self { + Self { + source: value.0.into(), + expected: vec![ + ExpectFix { expected: value.1.0.into(), kind: ExpectFixKind::Any }, + ExpectFix { expected: value.1.1.into(), kind: ExpectFixKind::Any }, + ], + rule_config: None, + } + } +} + +impl From<(S, S, Option, F)> for ExpectFixTestCase where S: Into, F: Into, @@ -161,8 +178,7 @@ where fn from((source, expected, config, kind): (S, S, Option, F)) -> Self { Self { source: source.into(), - expected: expected.into(), - kind: kind.into(), + expected: vec![ExpectFix { expected: expected.into(), kind: kind.into() }], rule_config: config, } } @@ -206,7 +222,7 @@ pub struct Tester { /// /// Note that disabling this check should be done as little as possible, and /// never in bad faith (e.g. no `#[test]` functions have fixer cases at all). - expect_fix: Option>, + expect_fix: Option>, snapshot: String, /// Suffix added to end of snapshot name. /// @@ -345,7 +361,7 @@ impl Tester { /// Tester::new("no-undef", pass, fail).expect_fix(fix).test(); /// ``` #[must_use] - pub fn expect_fix>(mut self, expect_fix: Vec) -> Self { + pub fn expect_fix>(mut self, expect_fix: Vec) -> Self { // prevent `expect_fix` abuse assert!( !expect_fix.is_empty(), @@ -398,8 +414,14 @@ impl Tester { fn test_pass(&mut self) { for TestCase { source, rule_config, eslint_config, path } in self.expect_pass.clone() { - let result = - self.run(&source, rule_config.clone(), &eslint_config, path, ExpectFixKind::None); + let result = self.run( + &source, + rule_config.clone(), + &eslint_config, + path, + ExpectFixKind::None, + 0, + ); let passed = result == TestResult::Passed; let config = rule_config.map_or_else( || "\n\n------------------------\n".to_string(), @@ -420,8 +442,14 @@ impl Tester { fn test_fail(&mut self) { for TestCase { source, rule_config, eslint_config, path } in self.expect_fail.clone() { - let result = - self.run(&source, rule_config.clone(), &eslint_config, path, ExpectFixKind::None); + let result = self.run( + &source, + rule_config.clone(), + &eslint_config, + path, + ExpectFixKind::None, + 0, + ); let failed = result == TestResult::Failed; let config = rule_config.map_or_else( || "\n\n------------------------".to_string(), @@ -439,6 +467,7 @@ impl Tester { } } + #[expect(clippy::cast_possible_truncation)] // there are no rules with over 255 different possible fixes fn test_fix(&mut self) { // If auto-fixes are reported, make sure some fix test cases are provided let rule = self.find_rule(); @@ -453,15 +482,19 @@ impl Tester { }; for fix in fix_test_cases { - let ExpectFix { source, expected, kind, rule_config: config } = fix; - let result = self.run(&source, config, &None, None, kind); - match result { - TestResult::Fixed(fixed_str) => assert_eq!( - expected, fixed_str, - r#"Expected "{source}" to be fixed into "{expected}""# - ), - TestResult::Passed => panic!("Expected a fix, but test passed: {source}"), - TestResult::Failed => panic!("Expected a fix, but test failed: {source}"), + let ExpectFixTestCase { source, expected, rule_config: config } = fix; + for (index, expect) in expected.iter().enumerate() { + let result = + self.run(&source, config.clone(), &None, None, expect.kind, index as u8); + match result { + TestResult::Fixed(fixed_str) => assert_eq!( + expect.expected, fixed_str, + r#"Expected "{source}" to be fixed into "{}""#, + expect.expected + ), + TestResult::Passed => panic!("Expected a fix, but test passed: {source}"), + TestResult::Failed => panic!("Expected a fix, but test failed: {source}"), + } } } } @@ -473,7 +506,8 @@ impl Tester { rule_config: Option, eslint_config: &Option, path: Option, - fix: ExpectFixKind, + fix_kind: ExpectFixKind, + fix_index: u8, ) -> TestResult { let allocator = Allocator::default(); let rule = self.find_rule().read_json(rule_config.unwrap_or_default()); @@ -492,7 +526,7 @@ impl Tester { FxHashMap::default(), ), ) - .with_fix(fix.into()); + .with_fix(fix_kind.into()); let path_to_lint = if self.plugins.has_import() { assert!(path.is_none(), "import plugin does not support path"); @@ -520,8 +554,8 @@ impl Tester { return TestResult::Passed; } - if fix.is_some() { - let fix_result = Fixer::new(source_text, result).fix(); + if fix_kind.is_some() { + let fix_result = Fixer::new(source_text, result).with_fix_index(fix_index).fix(); return TestResult::Fixed(fix_result.fixed_code.to_string()); }