Skip to content

fix(language_server): append rule to existing disable comment instead of creating duplicate #16081

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+169 −36
Open

fix(language_server): append rule to existing disable comment instead of creating duplicate #16081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
141 changes: 119 additions & 22 deletions crates/oxc_language_server/src/linter/error_with_position.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use tower_lsp_server::lsp_types::{

use oxc_data_structures::rope::{Rope, get_line_column};
use oxc_diagnostics::{OxcCode, Severity};
use oxc_linter::{Fix, Message, PossibleFixes};
use oxc_linter::{DisableDirectives, Fix, Message, PossibleFixes, RuleCommentType};

#[derive(Debug, Clone, Default)]
pub struct DiagnosticReport {
Expand Down Expand Up @@ -38,6 +38,7 @@ pub fn message_to_lsp_diagnostic(
uri: &Uri,
source_text: &str,
rope: &Rope,
directives: Option<&DisableDirectives>,
) -> DiagnosticReport {
let severity = match message.error.severity {
Severity::Error => Some(lsp_types::DiagnosticSeverity::ERROR),
Expand Down Expand Up @@ -130,6 +131,7 @@ pub fn message_to_lsp_diagnostic(
section_offset,
rope,
source_text,
directives,
);

DiagnosticReport { diagnostic, fixed_content }
Expand Down Expand Up @@ -203,6 +205,7 @@ fn add_ignore_fixes(
section_offset: u32,
rope: &Rope,
source_text: &str,
directives: Option<&DisableDirectives>,
) -> PossibleFixContent {
// do not append ignore code actions when the error is the ignore action
if matches!(fixes, PossibleFixContent::Single(ref fix) if fix.message.as_ref().is_some_and(|message| message.starts_with("remove unused disable directive")))
Expand All @@ -218,13 +221,13 @@ fn add_ignore_fixes(
}

if let Some(rule_name) = code.number.as_ref() {
// TODO: doesn't support disabling multiple rules by name for a given line.
new_fixes.push(disable_for_this_line(
rule_name,
error_offset,
section_offset,
rope,
source_text,
directives,
Copy link
Member

@Sysix Sysix Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes only for the "fix on this line" and not "on the whole file" (section for partial loaders).
Probably need to update the PR title

));
new_fixes.push(disable_for_this_section(rule_name, section_offset, rope, source_text));
}
Expand All @@ -244,6 +247,76 @@ fn disable_for_this_line(
section_offset: u32,
rope: &Rope,
source_text: &str,
directives: Option<&DisableDirectives>,
) -> FixedContent {
if let Some(directives) = directives
&& let Some(existing_comment) =
directives.find_disable_next_line_comment_for_position(error_offset)
{
return append_rule_to_existing_comment(rule_name, existing_comment, rope, source_text);
}

create_new_disable_comment(rule_name, error_offset, section_offset, rope, source_text)
}

/// Append a rule to an existing disable-next-line comment, or return a no-op if the rule already exists.
fn append_rule_to_existing_comment(
rule_name: &str,
existing_comment: &oxc_linter::DisableRuleComment,
rope: &Rope,
source_text: &str,
) -> FixedContent {
let comment_span = existing_comment.span;
let comment_text = &source_text[comment_span.start as usize..comment_span.end as usize];

// Get the existing rules from the comment
let existing_rules: Vec<&str> = match &existing_comment.r#type {
RuleCommentType::All => {
// If it's an "all" directive, just return a no-op (can't add more rules)
let start_position = offset_to_position(rope, comment_span.start, source_text);
let end_position = offset_to_position(rope, comment_span.end, source_text);
return FixedContent {
message: Some(format!("Disable {rule_name} for this line")),
code: comment_text.to_string(),
range: Range::new(start_position, end_position),
};
}
RuleCommentType::Single(rules) => rules.iter().map(|r| r.rule_name.as_str()).collect(),
};

// Check if the rule is already in the comment
if existing_rules.contains(&rule_name) {
Copy link
Member

@Sysix Sysix Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should never happen. The fix/diagnostic is only shown when the rule is not in the comment.
Adding a (debug_)assert! would be helpful :)

// Rule already exists, return a no-op fix (same content)
let start_position = offset_to_position(rope, comment_span.start, source_text);
let end_position = offset_to_position(rope, comment_span.end, source_text);
return FixedContent {
message: Some(format!("Disable {rule_name} for this line")),
code: comment_text.to_string(),
range: Range::new(start_position, end_position),
};
}

// Append the new rule to the comment using comma separation (ESLint standard format)
// The comment_text is just the content inside the comment (without // prefix for line comments)
let new_comment = format!("{comment_text}, {rule_name}");

let start_position = offset_to_position(rope, comment_span.start, source_text);
let end_position = offset_to_position(rope, comment_span.end, source_text);

FixedContent {
message: Some(format!("Disable {rule_name} for this line")),
code: new_comment,
range: Range::new(start_position, end_position),
}
}

/// Create a new disable-next-line comment when no existing comment is found.
fn create_new_disable_comment(
rule_name: &str,
error_offset: u32,
section_offset: u32,
rope: &Rope,
source_text: &str,
) -> FixedContent {
let bytes = source_text.as_bytes();
// Find the line break before the error
Expand Down Expand Up @@ -436,7 +509,7 @@ mod test {
fn disable_for_this_line_single_line() {
Copy link
Member

@Sysix Sysix Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some tests please? Specially with partial source text (section offset > 0).
I am afraid that this will be skipped by the internal linter.

let source = "console.log('hello');";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 0, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 0, 0, &rope, source, None);

assert_eq!(fix.code, "// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 0);
Expand All @@ -447,7 +520,7 @@ mod test {
fn disable_for_this_line_with_spaces() {
let source = " console.log('hello');";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 10, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 10, 0, &rope, source, None);

assert_eq!(fix.code, " // oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 0);
Expand All @@ -458,7 +531,7 @@ mod test {
fn disable_for_this_line_with_tabs() {
let source = "\t\tconsole.log('hello');";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 10, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 10, 0, &rope, source, None);

assert_eq!(fix.code, "\t\t// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 0);
Expand All @@ -469,7 +542,7 @@ mod test {
fn disable_for_this_line_mixed_tabs_spaces() {
let source = "\t \tconsole.log('hello');";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 12, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 12, 0, &rope, source, None);

assert_eq!(fix.code, "\t \t// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 0);
Expand All @@ -480,7 +553,7 @@ mod test {
fn disable_for_this_line_multiline_with_tabs() {
let source = "function test() {\n\tconsole.log('hello');\n}";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 27, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 27, 0, &rope, source, None);

assert_eq!(fix.code, "\t// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 1);
Expand All @@ -491,7 +564,7 @@ mod test {
fn disable_for_this_line_multiline_with_spaces() {
let source = "function test() {\n console.log('hello');\n}";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 30, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 30, 0, &rope, source, None);

assert_eq!(fix.code, " // oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 1);
Expand All @@ -502,7 +575,7 @@ mod test {
fn disable_for_this_line_complex_indentation() {
let source = "function test() {\n\t \t console.log('hello');\n}";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 33, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 33, 0, &rope, source, None);

assert_eq!(fix.code, "\t \t // oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 1);
Expand All @@ -513,7 +586,7 @@ mod test {
fn disable_for_this_line_no_indentation() {
let source = "function test() {\nconsole.log('hello');\n}";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 26, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 26, 0, &rope, source, None);

assert_eq!(fix.code, "// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 1);
Expand All @@ -524,7 +597,7 @@ mod test {
fn disable_for_this_line_crlf_with_tabs() {
let source = "function test() {\r\n\tconsole.log('hello');\r\n}";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 28, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 28, 0, &rope, source, None);

assert_eq!(fix.code, "\t// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 1);
Expand All @@ -535,7 +608,7 @@ mod test {
fn disable_for_this_line_deeply_nested() {
let source = "if (true) {\n\t\tif (nested) {\n\t\t\tconsole.log('deep');\n\t\t}\n}";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 40, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 40, 0, &rope, source, None);

assert_eq!(fix.code, "\t\t\t// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 2);
Expand All @@ -546,7 +619,7 @@ mod test {
fn disable_for_this_line_at_start_of_file() {
let source = "console.log('hello');";
let rope = Rope::from_str(source);
let fix = super::disable_for_this_line("no-console", 0, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 0, 0, &rope, source, None);

assert_eq!(fix.code, "// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 0);
Expand All @@ -559,7 +632,7 @@ mod test {
let source = "function test() {\n \tcode \there\n}";
let rope = Rope::from_str(source);
// Error at position of 'code' (after " \t")
let fix = super::disable_for_this_line("no-console", 21, 0, &rope, source);
let fix = super::disable_for_this_line("no-console", 21, 0, &rope, source, None);

// Should only capture " \t" at the beginning, not the spaces around "here"
assert_eq!(fix.code, " \t// oxlint-disable-next-line no-console\n");
Expand All @@ -574,8 +647,14 @@ mod test {
let rope = Rope::from_str(source);
let section_offset = 8; // At the \n after "<script>"
let error_offset = 17; // At 'console'
let fix =
super::disable_for_this_line("no-console", error_offset, section_offset, &rope, source);
let fix = super::disable_for_this_line(
"no-console",
error_offset,
section_offset,
&rope,
source,
None,
);

assert_eq!(fix.code, "// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 1);
Expand All @@ -589,8 +668,14 @@ mod test {
let rope = Rope::from_str(source);
let section_offset = 8; // After "<script>"
let error_offset = 16; // At 'console'
let fix =
super::disable_for_this_line("no-console", error_offset, section_offset, &rope, source);
let fix = super::disable_for_this_line(
"no-console",
error_offset,
section_offset,
&rope,
source,
None,
);

assert_eq!(fix.code, "\n// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 0);
Expand All @@ -604,8 +689,14 @@ mod test {
let rope = Rope::from_str(source);
let section_offset = 31; // At \n after "<script>"
let error_offset = 36; // At 'console' (after " ")
let fix =
super::disable_for_this_line("no-console", error_offset, section_offset, &rope, source);
let fix = super::disable_for_this_line(
"no-console",
error_offset,
section_offset,
&rope,
source,
None,
);

assert_eq!(fix.code, " // oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 3);
Expand All @@ -619,8 +710,14 @@ mod test {
let rope = Rope::from_str(source);
let section_offset = 8; // At the \n after "<script>"
let error_offset = 8; // Error exactly at section offset
let fix =
super::disable_for_this_line("no-console", error_offset, section_offset, &rope, source);
let fix = super::disable_for_this_line(
"no-console",
error_offset,
section_offset,
&rope,
source,
None,
);

assert_eq!(fix.code, "// oxlint-disable-next-line no-console\n");
assert_eq!(fix.range.start.line, 1);
Expand Down
22 changes: 13 additions & 9 deletions crates/oxc_language_server/src/linter/isolated_lint_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,25 @@ impl IsolatedLintHandler {

let fs = IsolatedLintHandlerFileSystem::new(path.to_path_buf(), Arc::from(source_text));

let mut messages: Vec<DiagnosticReport> = self
.runner
.run_source(&Arc::from(path.as_os_str()), source_text.to_string(), &fs)
let lint_messages =
self.runner.run_source(&Arc::from(path.as_os_str()), source_text.to_string(), &fs);

let directives = self.runner.directives_coordinator().get(path);

let mut messages: Vec<DiagnosticReport> = lint_messages
.iter()
.map(|message| message_to_lsp_diagnostic(message, uri, source_text, rope))
.map(|message| {
message_to_lsp_diagnostic(message, uri, source_text, rope, directives.as_ref())
})
.collect();

// Add unused directives if configured
if let Some(severity) = self.unused_directives_severity
&& let Some(directives) = self.runner.directives_coordinator().get(path)
&& let Some(directives) = &directives
{
messages.extend(
create_unused_directives_messages(&directives, severity, source_text)
.iter()
.map(|message| message_to_lsp_diagnostic(message, uri, source_text, rope)),
create_unused_directives_messages(directives, severity, source_text).iter().map(
|message| message_to_lsp_diagnostic(message, uri, source_text, rope, None),
),
);
}

Expand Down
10 changes: 5 additions & 5 deletions ...guage_server/src/linter/snapshots/[email protected]
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,15 @@ Title: Disable no-console for this line
TextEdit: TextEdit {
range: Range {
start: Position {
line: 9,
character: 0,
line: 8,
character: 2,
},
end: Position {
line: 9,
character: 0,
line: 8,
character: 52,
},
},
new_text: "// oxlint-disable-next-line no-console\n",
new_text: " oxlint-disable-next-line no-debugger, no-for-loop, no-console",
}


Expand Down
Loading
Loading