Skip to content

Commit adff069

Browse files
committed
fix(language_server): don't apply "ignore this rule" fixes for fixAll code action + command (#14243)
This PR fixes a bug where on `source.fixAll.oxc` code action or `oxc.fixAll` command will append `// oxlint-disable ...` comments to the file. This bug was introduced in #14183
1 parent 7fe930c commit adff069

File tree

6 files changed

+90
-2
lines changed

6 files changed

+90
-2
lines changed

crates/oxc_language_server/src/code_actions.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use log::debug;
12
use tower_lsp_server::lsp_types::{CodeAction, CodeActionKind, TextEdit, Uri, WorkspaceEdit};
23

34
use crate::linter::error_with_position::{DiagnosticReport, FixedContent, PossibleFixContent};
@@ -87,7 +88,30 @@ pub fn apply_all_fix_code_action<'a>(
8788
PossibleFixContent::Single(fixed_content) => Some(fixed_content),
8889
// For multiple fixes, we take the first one as a representative fix.
8990
// Applying all possible fixes at once is not possible in this context.
90-
PossibleFixContent::Multiple(multi) => multi.first(),
91+
PossibleFixContent::Multiple(multi) => {
92+
// for a real linter fix, we expect at least 3 fixes
93+
if multi.len() > 2 {
94+
multi.first()
95+
} else {
96+
debug!("Multiple fixes found, but only ignore fixes available");
97+
#[cfg(debug_assertions)]
98+
{
99+
if !multi.is_empty() {
100+
debug_assert!(multi[0].message.as_ref().is_some());
101+
debug_assert!(
102+
multi[0].message.as_ref().unwrap().starts_with("Disable")
103+
);
104+
debug_assert!(
105+
multi[0].message.as_ref().unwrap().ends_with("for this line")
106+
);
107+
}
108+
}
109+
110+
// this fix is only for "ignore this line/file" fixes
111+
// do not apply them for "fix all" code action
112+
None
113+
}
114+
}
91115
};
92116

93117
if let Some(fixed_content) = &fix {

crates/oxc_language_server/src/worker.rs

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,29 @@ impl WorkspaceWorker {
294294
PossibleFixContent::Single(fixed_content) => Some(fixed_content),
295295
// For multiple fixes, we take the first one as a representative fix.
296296
// Applying all possible fixes at once is not possible in this context.
297-
PossibleFixContent::Multiple(multi) => multi.first(),
297+
PossibleFixContent::Multiple(multi) => {
298+
// for a real linter fix, we expect at least 3 fixes
299+
if multi.len() > 2 {
300+
multi.first()
301+
} else {
302+
debug!("Multiple fixes found, but only ignore fixes available");
303+
#[cfg(debug_assertions)]
304+
{
305+
if !multi.is_empty() {
306+
debug_assert!(multi[0].message.as_ref().is_some());
307+
debug_assert!(
308+
multi[0].message.as_ref().unwrap().starts_with("Disable")
309+
);
310+
debug_assert!(
311+
multi[0].message.as_ref().unwrap().ends_with("for this line")
312+
);
313+
}
314+
}
315+
// this fix is only for "ignore this line/file" fixes
316+
// do not apply them for "fix all" code action
317+
None
318+
}
319+
}
298320
};
299321

300322
if let Some(fixed_content) = &fix {
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"rules": {
3+
"no-const-assign": "error"
4+
}
5+
}
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
const a = 0;
2+
a = 1;
3+
4+
console.log(a);
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
const a = 0;
2+
a = 1;
3+
4+
console.log(a);

editors/vscode/tests/code_actions.spec.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,35 @@ suite('code actions', () => {
106106
strictEqual(content.toString(), expected.toString());
107107
});
108108

109+
// https://discord.com/channels/1079625926024900739/1080723403595591700/1422191300395929620
110+
test('code action `source.fixAll.oxc` ignores "ignore this rule for this line/file"', async () => {
111+
let file = Uri.joinPath(fixturesWorkspaceUri(), 'fixtures', 'file2.js');
112+
let expectedFile = Uri.joinPath(fixturesWorkspaceUri(), 'fixtures', 'expected.txt');
113+
114+
await workspace.getConfiguration('editor').update('codeActionsOnSave', {
115+
'source.fixAll.oxc': 'always',
116+
});
117+
await workspace.saveAll();
118+
119+
const range = new Range(new Position(0, 0), new Position(0, 0));
120+
const edit = new WorkspaceEdit();
121+
edit.replace(file, range, ' ');
122+
123+
await sleep(1000);
124+
125+
await loadFixture('fixall_code_action_ignore_only_disable_fix');
126+
await workspace.openTextDocument(file);
127+
await workspace.applyEdit(edit);
128+
await sleep(1000);
129+
await workspace.saveAll();
130+
await sleep(500);
131+
132+
const content = await workspace.fs.readFile(file);
133+
const expected = await workspace.fs.readFile(expectedFile);
134+
135+
strictEqual(content.toString(), expected.toString());
136+
});
137+
109138
test('changing configuration flag "fix_kind" will reveal more code actions', async () => {
110139
await loadFixture('changing_fix_kind');
111140
const fileUri = Uri.joinPath(fixturesWorkspaceUri(), 'fixtures', 'for_direction.ts');

0 commit comments

Comments
 (0)