Skip to content

Commit 2083d33

Browse files
SysixSysix
committed
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~~
1 parent f6424dd commit 2083d33

File tree

8 files changed

+157
-42
lines changed

8 files changed

+157
-42
lines changed

‎crates/oxc_language_server/fixtures/linter/multiple_suggestions/.oxlintrc.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"plugins": ["react"],
3+
"categories": {
4+
"correctness": "off"
5+
},
6+
"rules": {
7+
"react/forward-ref-uses-ref": "error"
8+
}
9+
}

‎crates/oxc_language_server/fixtures/linter/multiple_suggestions/forward_ref.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
forwardRef((props) => {});

‎crates/oxc_language_server/fixtures/linter/multiple_suggestions/package.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"dependencies": {
3+
"react": "latest"
4+
}
5+
}

‎crates/oxc_language_server/src/linter/server_linter.rs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,4 +350,19 @@ mod test {
350350
Tester::new("fixtures/linter/cross_module_extended_config", None)
351351
.test_and_snapshot_single_file("dep-a.ts");
352352
}
353+
354+
#[test]
355+
fn test_multiple_suggestions() {
356+
Tester::new(
357+
"fixtures/linter/multiple_suggestions",
358+
Some(Options {
359+
flags: FxHashMap::from_iter([(
360+
"fix_kind".to_string(),
361+
"safe_fix_or_suggestion".to_string(),
362+
)]),
363+
..Options::default()
364+
}),
365+
)
366+
.test_and_snapshot_single_file("forward_ref.ts");
367+
}
353368
}

‎crates/oxc_language_server/src/snapshots/fixtures_linter_multiple_suggestions@forward_ref.ts.snap‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: crates/oxc_language_server/src/tester.rs
3+
input_file: crates/oxc_language_server/fixtures/linter/multiple_suggestions/forward_ref.ts
4+
---
5+
code: "eslint-plugin-react(forward-ref-uses-ref)"
6+
code_description.href: "https://oxc.rs/docs/guide/usage/linter/rules/react/forward-ref-uses-ref.html"
7+
message: "Components wrapped with `forwardRef` must have a `ref` parameter\nhelp: Add a `ref` parameter, or remove `forwardRef`"
8+
range: Range { start: Position { line: 0, character: 11 }, end: Position { line: 0, character: 24 } }
9+
related_information[0].message: ""
10+
related_information[0].location.uri: "file://<variable>/fixtures/linter/multiple_suggestions/forward_ref.ts"
11+
related_information[0].location.range: Range { start: Position { line: 0, character: 11 }, end: Position { line: 0, character: 24 } }
12+
severity: Some(Error)
13+
source: Some("oxc")
14+
tags: None
15+
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 } } }])

‎crates/oxc_linter/src/fixer/mod.rs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -311,11 +311,21 @@ impl GetSpan for Message<'_> {
311311
pub struct Fixer<'a> {
312312
source_text: &'a str,
313313
messages: Vec<Message<'a>>,
314+
315+
// To test different fixes, we need to override the default behavior.
316+
// The behavior is oriented by `oxlint` where only one PossibleFixes is applied.
317+
fix_index: u8,
314318
}
315319

316320
impl<'a> Fixer<'a> {
317321
pub fn new(source_text: &'a str, messages: Vec<Message<'a>>) -> Self {
318-
Self { source_text, messages }
322+
Self { source_text, messages, fix_index: 0 }
323+
}
324+
325+
#[cfg(test)]
326+
pub fn with_fix_index(mut self, fix_index: u8) -> Self {
327+
self.fix_index = fix_index;
328+
self
319329
}
320330

321331
/// # Panics
@@ -343,7 +353,7 @@ impl<'a> Fixer<'a> {
343353
PossibleFixes::Single(fix) => Some(fix),
344354
// For multiple fixes, we take the first one as a representative fix.
345355
// Applying all possible fixes at once is not possible in this context.
346-
PossibleFixes::Multiple(multiple) => multiple.first(),
356+
PossibleFixes::Multiple(multiple) => multiple.get(self.fix_index as usize),
347357
};
348358
let Some(Fix { content, span, .. }) = fix else {
349359
filtered_messages.push(m);

‎crates/oxc_linter/src/rules/react/forward_ref_uses_ref.rs‎

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use oxc_diagnostics::OxcDiagnostic;
66
use oxc_macros::declare_oxc_lint;
77
use oxc_span::Span;
88

9-
use crate::{AstNode, context::LintContext, rule::Rule};
9+
use crate::{AstNode, context::LintContext, fixer::RuleFixer, rule::Rule};
1010

1111
fn forward_ref_uses_ref_diagnostic(span: Span) -> OxcDiagnostic {
1212
OxcDiagnostic::warn("Components wrapped with `forwardRef` must have a `ref` parameter")
@@ -60,9 +60,14 @@ declare_oxc_lint!(
6060
react,
6161
correctness,
6262
suggestion
63+
suggestion
6364
);
6465

65-
fn check_forward_ref_inner(exp: &Expression, call_expr: &CallExpression, ctx: &LintContext) {
66+
fn check_forward_ref_inner<'a>(
67+
exp: &Expression,
68+
call_expr: &CallExpression,
69+
ctx: &LintContext<'a>,
70+
) {
6671
let (params, span) = match exp {
6772
Expression::ArrowFunctionExpression(f) => (&f.params, f.span),
6873
Expression::FunctionExpression(f) => (&f.params, f.span),
@@ -71,9 +76,26 @@ fn check_forward_ref_inner(exp: &Expression, call_expr: &CallExpression, ctx: &L
7176
if params.parameters_count() != 1 || params.rest.is_some() {
7277
return;
7378
}
74-
ctx.diagnostic_with_suggestion(forward_ref_uses_ref_diagnostic(span), |fixer| {
75-
fixer.replace_with(call_expr, exp)
76-
});
79+
80+
ctx.diagnostics_with_multiple_fixes(
81+
forward_ref_uses_ref_diagnostic(span),
82+
(FixKind::Suggestion, |fixer: RuleFixer<'_, 'a>| {
83+
fixer.replace_with(call_expr, exp).with_message("remove `forwardRef` wrapper")
84+
}),
85+
(FixKind::Suggestion, |fixer: RuleFixer<'_, 'a>| {
86+
let fixed = ctx.source_range(params.span);
87+
// remove the trailing `)`, `` and `,` if they exist
88+
let fixed = fixed.strip_suffix(')').unwrap_or(fixed).trim_end();
89+
let mut fixed = fixed.strip_suffix(',').unwrap_or(fixed).to_string();
90+
91+
if !fixed.starts_with('(') {
92+
fixed.insert(0, '(');
93+
}
94+
fixed.push_str(", ref)");
95+
96+
fixer.replace(params.span, fixed).with_message("add `ref` parameter")
97+
}),
98+
);
7799
}
78100

79101
impl Rule for ForwardRefUsesRef {
@@ -192,11 +214,15 @@ fn test() {
192214
];
193215

194216
let fix = vec![
195-
("forwardRef((a) => {})", "(a) => {}"),
196-
("forwardRef(a => {})", "a => {}"),
197-
("forwardRef(function (a) {})", "function (a) {}"),
198-
("forwardRef(function(a,) {})", "function(a,) {}"),
199-
("React.forwardRef(function(a) {})", "function(a) {}"),
217+
("forwardRef((a) => {})", ("(a) => {}", "forwardRef((a, ref) => {})")),
218+
("forwardRef(a => {})", ("a => {}", "forwardRef((a, ref) => {})")),
219+
("forwardRef(function (a) {})", ("function (a) {}", "forwardRef(function (a, ref) {})")),
220+
("forwardRef(function(a,) {})", ("function(a,) {}", "forwardRef(function(a, ref) {})")),
221+
("forwardRef(function(a, ) {})", ("function(a, ) {}", "forwardRef(function(a, ref) {})")),
222+
(
223+
"React.forwardRef(function(a) {})",
224+
("function(a) {}", "React.forwardRef(function(a, ref) {})"),
225+
),
200226
];
201227

202228
Tester::new(ForwardRefUsesRef::NAME, ForwardRefUsesRef::PLUGIN, pass, fail)

‎crates/oxc_linter/src/tester.rs‎

Lines changed: 64 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -123,46 +123,62 @@ impl From<Option<FixKind>> for ExpectFixKind {
123123
}
124124

125125
#[derive(Debug, Clone)]
126-
pub struct ExpectFix {
126+
pub struct ExpectFixTestCase {
127127
/// Source code being tested
128128
source: String,
129+
expected: Vec<ExpectFix>,
130+
rule_config: Option<Value>,
131+
}
132+
133+
#[derive(Debug, Clone)]
134+
struct ExpectFix {
129135
/// Expected source code after fix has been applied
130136
expected: String,
131137
kind: ExpectFixKind,
132-
rule_config: Option<Value>,
133138
}
134139

135-
impl<S: Into<String>> From<(S, S, Option<Value>)> for ExpectFix {
140+
impl<S: Into<String>> From<(S, S, Option<Value>)> for ExpectFixTestCase {
136141
fn from(value: (S, S, Option<Value>)) -> Self {
137142
Self {
138143
source: value.0.into(),
139-
expected: value.1.into(),
140-
kind: ExpectFixKind::Any,
144+
expected: vec![ExpectFix { expected: value.1.into(), kind: ExpectFixKind::Any }],
141145
rule_config: value.2,
142146
}
143147
}
144148
}
145149

146-
impl<S: Into<String>> From<(S, S)> for ExpectFix {
150+
impl<S: Into<String>> From<(S, S)> for ExpectFixTestCase {
147151
fn from(value: (S, S)) -> Self {
148152
Self {
149153
source: value.0.into(),
150-
expected: value.1.into(),
151-
kind: ExpectFixKind::Any,
154+
expected: vec![ExpectFix { expected: value.1.into(), kind: ExpectFixKind::Any }],
152155
rule_config: None,
153156
}
154157
}
155158
}
156-
impl<S, F> From<(S, S, Option<Value>, F)> for ExpectFix
159+
160+
impl<S: Into<String>> From<(S, (S, S))> for ExpectFixTestCase {
161+
fn from(value: (S, (S, S))) -> Self {
162+
Self {
163+
source: value.0.into(),
164+
expected: vec![
165+
ExpectFix { expected: value.1.0.into(), kind: ExpectFixKind::Any },
166+
ExpectFix { expected: value.1.1.into(), kind: ExpectFixKind::Any },
167+
],
168+
rule_config: None,
169+
}
170+
}
171+
}
172+
173+
impl<S, F> From<(S, S, Option<Value>, F)> for ExpectFixTestCase
157174
where
158175
S: Into<String>,
159176
F: Into<ExpectFixKind>,
160177
{
161178
fn from((source, expected, config, kind): (S, S, Option<Value>, F)) -> Self {
162179
Self {
163180
source: source.into(),
164-
expected: expected.into(),
165-
kind: kind.into(),
181+
expected: vec![ExpectFix { expected: expected.into(), kind: kind.into() }],
166182
rule_config: config,
167183
}
168184
}
@@ -206,7 +222,7 @@ pub struct Tester {
206222
///
207223
/// Note that disabling this check should be done as little as possible, and
208224
/// never in bad faith (e.g. no `#[test]` functions have fixer cases at all).
209-
expect_fix: Option<Vec<ExpectFix>>,
225+
expect_fix: Option<Vec<ExpectFixTestCase>>,
210226
snapshot: String,
211227
/// Suffix added to end of snapshot name.
212228
///
@@ -345,7 +361,7 @@ impl Tester {
345361
/// Tester::new("no-undef", pass, fail).expect_fix(fix).test();
346362
/// ```
347363
#[must_use]
348-
pub fn expect_fix<F: Into<ExpectFix>>(mut self, expect_fix: Vec<F>) -> Self {
364+
pub fn expect_fix<F: Into<ExpectFixTestCase>>(mut self, expect_fix: Vec<F>) -> Self {
349365
// prevent `expect_fix` abuse
350366
assert!(
351367
!expect_fix.is_empty(),
@@ -398,8 +414,14 @@ impl Tester {
398414

399415
fn test_pass(&mut self) {
400416
for TestCase { source, rule_config, eslint_config, path } in self.expect_pass.clone() {
401-
let result =
402-
self.run(&source, rule_config.clone(), &eslint_config, path, ExpectFixKind::None);
417+
let result = self.run(
418+
&source,
419+
rule_config.clone(),
420+
&eslint_config,
421+
path,
422+
ExpectFixKind::None,
423+
0,
424+
);
403425
let passed = result == TestResult::Passed;
404426
let config = rule_config.map_or_else(
405427
|| "\n\n------------------------\n".to_string(),
@@ -420,8 +442,14 @@ impl Tester {
420442

421443
fn test_fail(&mut self) {
422444
for TestCase { source, rule_config, eslint_config, path } in self.expect_fail.clone() {
423-
let result =
424-
self.run(&source, rule_config.clone(), &eslint_config, path, ExpectFixKind::None);
445+
let result = self.run(
446+
&source,
447+
rule_config.clone(),
448+
&eslint_config,
449+
path,
450+
ExpectFixKind::None,
451+
0,
452+
);
425453
let failed = result == TestResult::Failed;
426454
let config = rule_config.map_or_else(
427455
|| "\n\n------------------------".to_string(),
@@ -439,6 +467,7 @@ impl Tester {
439467
}
440468
}
441469

470+
#[expect(clippy::cast_possible_truncation)] // there are no rules with over 255 different possible fixes
442471
fn test_fix(&mut self) {
443472
// If auto-fixes are reported, make sure some fix test cases are provided
444473
let rule = self.find_rule();
@@ -453,15 +482,19 @@ impl Tester {
453482
};
454483

455484
for fix in fix_test_cases {
456-
let ExpectFix { source, expected, kind, rule_config: config } = fix;
457-
let result = self.run(&source, config, &None, None, kind);
458-
match result {
459-
TestResult::Fixed(fixed_str) => assert_eq!(
460-
expected, fixed_str,
461-
r#"Expected "{source}" to be fixed into "{expected}""#
462-
),
463-
TestResult::Passed => panic!("Expected a fix, but test passed: {source}"),
464-
TestResult::Failed => panic!("Expected a fix, but test failed: {source}"),
485+
let ExpectFixTestCase { source, expected, rule_config: config } = fix;
486+
for (index, expect) in expected.iter().enumerate() {
487+
let result =
488+
self.run(&source, config.clone(), &None, None, expect.kind, index as u8);
489+
match result {
490+
TestResult::Fixed(fixed_str) => assert_eq!(
491+
expect.expected, fixed_str,
492+
r#"Expected "{source}" to be fixed into "{}""#,
493+
expect.expected
494+
),
495+
TestResult::Passed => panic!("Expected a fix, but test passed: {source}"),
496+
TestResult::Failed => panic!("Expected a fix, but test failed: {source}"),
497+
}
465498
}
466499
}
467500
}
@@ -473,7 +506,8 @@ impl Tester {
473506
rule_config: Option<Value>,
474507
eslint_config: &Option<Value>,
475508
path: Option<PathBuf>,
476-
fix: ExpectFixKind,
509+
fix_kind: ExpectFixKind,
510+
fix_index: u8,
477511
) -> TestResult {
478512
let allocator = Allocator::default();
479513
let rule = self.find_rule().read_json(rule_config.unwrap_or_default());
@@ -492,7 +526,7 @@ impl Tester {
492526
FxHashMap::default(),
493527
),
494528
)
495-
.with_fix(fix.into());
529+
.with_fix(fix_kind.into());
496530

497531
let path_to_lint = if self.plugins.has_import() {
498532
assert!(path.is_none(), "import plugin does not support path");
@@ -520,8 +554,8 @@ impl Tester {
520554
return TestResult::Passed;
521555
}
522556

523-
if fix.is_some() {
524-
let fix_result = Fixer::new(source_text, result).fix();
557+
if fix_kind.is_some() {
558+
let fix_result = Fixer::new(source_text, result).with_fix_index(fix_index).fix();
525559
return TestResult::Fixed(fix_result.fixed_code.to_string());
526560
}
527561

0 commit comments

Comments
 (0)