Combine lint and syntax error handling (#18471)
Some checks are pending
CI / Determine changes (push) Waiting to run
CI / cargo fmt (push) Waiting to run
CI / cargo clippy (push) Blocked by required conditions
CI / cargo test (linux) (push) Blocked by required conditions
CI / cargo test (linux, release) (push) Blocked by required conditions
CI / cargo test (windows) (push) Blocked by required conditions
CI / cargo test (wasm) (push) Blocked by required conditions
CI / cargo build (release) (push) Waiting to run
CI / cargo build (msrv) (push) Blocked by required conditions
CI / cargo fuzz build (push) Blocked by required conditions
CI / fuzz parser (push) Blocked by required conditions
CI / test scripts (push) Blocked by required conditions
CI / ecosystem (push) Blocked by required conditions
CI / Fuzz for new ty panics (push) Blocked by required conditions
CI / cargo shear (push) Blocked by required conditions
CI / python package (push) Waiting to run
CI / pre-commit (push) Waiting to run
CI / mkdocs (push) Waiting to run
CI / formatter instabilities and black similarity (push) Blocked by required conditions
CI / test ruff-lsp (push) Blocked by required conditions
CI / check playground (push) Blocked by required conditions
CI / benchmarks (push) Blocked by required conditions
[ty Playground] Release / publish (push) Waiting to run

## Summary

This is a spin-off from
https://github.com/astral-sh/ruff/pull/18447#discussion_r2125844669 to
avoid using `Message::noqa_code` to differentiate between lints and
syntax errors. I went through all of the calls on `main` and on the
branch from #18447, and the instance in `ruff_server` noted in the
linked comment was actually the primary place where this was being done.
Other calls to `noqa_code` are typically some variation of
`message.noqa_code().map_or(String::new, format!(...))`, with the major
exception of the gitlab output format:


a120610b5b/crates/ruff_linter/src/message/gitlab.rs (L93-L105)

which obviously assumes that `None` means syntax error. A simple fix
here would be to use `message.name()` for `check_name` instead of the
noqa code, but I'm not sure how breaking that would be. This could just
be:

```rust
 let description = message.body();
 let description = description.strip_prefix("SyntaxError: ").unwrap_or(description).to_string();
 let check_name = message.name();
```

In that case. This sounds reasonable based on the [Code Quality report
format](https://docs.gitlab.com/ci/testing/code_quality/#code-quality-report-format)
docs:

> | Name | Type | Description|
> |-----|-----|----|
> |`check_name` | String | A unique name representing the check, or
rule, associated with this violation. |

## Test Plan

Existing tests
This commit is contained in:
Brent Westbrook 2025-06-05 08:50:02 -04:00 committed by GitHub
parent 8485dbb324
commit 74a4e9af3d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 46 additions and 98 deletions

View file

@ -12,7 +12,6 @@ use crate::{
use ruff_diagnostics::{Applicability, Edit, Fix}; use ruff_diagnostics::{Applicability, Edit, Fix};
use ruff_linter::{ use ruff_linter::{
Locator, Locator,
codes::Rule,
directives::{Flags, extract_directives}, directives::{Flags, extract_directives},
generate_noqa_edits, generate_noqa_edits,
linter::check_path, linter::check_path,
@ -166,26 +165,17 @@ pub(crate) fn check(
messages messages
.into_iter() .into_iter()
.zip(noqa_edits) .zip(noqa_edits)
.filter_map(|(message, noqa_edit)| match message.to_rule() { .filter_map(|(message, noqa_edit)| {
Some(rule) => Some(to_lsp_diagnostic( if message.is_syntax_error() && !show_syntax_errors {
rule, None
&message, } else {
noqa_edit, Some(to_lsp_diagnostic(
&source_kind, &message,
locator.to_index(), noqa_edit,
encoding, &source_kind,
)), locator.to_index(),
None => { encoding,
if show_syntax_errors { ))
Some(syntax_error_to_lsp_diagnostic(
&message,
&source_kind,
locator.to_index(),
encoding,
))
} else {
None
}
} }
}); });
@ -241,7 +231,6 @@ pub(crate) fn fixes_for_diagnostics(
/// Generates an LSP diagnostic with an associated cell index for the diagnostic to go in. /// Generates an LSP diagnostic with an associated cell index for the diagnostic to go in.
/// If the source kind is a text document, the cell index will always be `0`. /// If the source kind is a text document, the cell index will always be `0`.
fn to_lsp_diagnostic( fn to_lsp_diagnostic(
rule: Rule,
diagnostic: &Message, diagnostic: &Message,
noqa_edit: Option<Edit>, noqa_edit: Option<Edit>,
source_kind: &SourceKind, source_kind: &SourceKind,
@ -253,11 +242,13 @@ fn to_lsp_diagnostic(
let body = diagnostic.body().to_string(); let body = diagnostic.body().to_string();
let fix = diagnostic.fix(); let fix = diagnostic.fix();
let suggestion = diagnostic.suggestion(); let suggestion = diagnostic.suggestion();
let code = diagnostic.to_noqa_code();
let fix = fix.and_then(|fix| fix.applies(Applicability::Unsafe).then_some(fix)); let fix = fix.and_then(|fix| fix.applies(Applicability::Unsafe).then_some(fix));
let data = (fix.is_some() || noqa_edit.is_some()) let data = (fix.is_some() || noqa_edit.is_some())
.then(|| { .then(|| {
let code = code?.to_string();
let edits = fix let edits = fix
.into_iter() .into_iter()
.flat_map(Fix::edits) .flat_map(Fix::edits)
@ -274,14 +265,12 @@ fn to_lsp_diagnostic(
title: suggestion.unwrap_or(name).to_string(), title: suggestion.unwrap_or(name).to_string(),
noqa_edit, noqa_edit,
edits, edits,
code: rule.noqa_code().to_string(), code,
}) })
.ok() .ok()
}) })
.flatten(); .flatten();
let code = rule.noqa_code().to_string();
let range: lsp_types::Range; let range: lsp_types::Range;
let cell: usize; let cell: usize;
@ -297,14 +286,25 @@ fn to_lsp_diagnostic(
range = diagnostic_range.to_range(source_kind.source_code(), index, encoding); range = diagnostic_range.to_range(source_kind.source_code(), index, encoding);
} }
let (severity, tags, code) = if let Some(code) = code {
let code = code.to_string();
(
Some(severity(&code)),
tags(&code),
Some(lsp_types::NumberOrString::String(code)),
)
} else {
(None, None, None)
};
( (
cell, cell,
lsp_types::Diagnostic { lsp_types::Diagnostic {
range, range,
severity: Some(severity(&code)), severity,
tags: tags(&code), tags,
code: Some(lsp_types::NumberOrString::String(code)), code,
code_description: rule.url().and_then(|url| { code_description: diagnostic.to_url().and_then(|url| {
Some(lsp_types::CodeDescription { Some(lsp_types::CodeDescription {
href: lsp_types::Url::parse(&url).ok()?, href: lsp_types::Url::parse(&url).ok()?,
}) })
@ -317,45 +317,6 @@ fn to_lsp_diagnostic(
) )
} }
fn syntax_error_to_lsp_diagnostic(
syntax_error: &Message,
source_kind: &SourceKind,
index: &LineIndex,
encoding: PositionEncoding,
) -> (usize, lsp_types::Diagnostic) {
let range: lsp_types::Range;
let cell: usize;
if let Some(notebook_index) = source_kind.as_ipy_notebook().map(Notebook::index) {
NotebookRange { cell, range } = syntax_error.range().to_notebook_range(
source_kind.source_code(),
index,
notebook_index,
encoding,
);
} else {
cell = usize::default();
range = syntax_error
.range()
.to_range(source_kind.source_code(), index, encoding);
}
(
cell,
lsp_types::Diagnostic {
range,
severity: Some(lsp_types::DiagnosticSeverity::ERROR),
tags: None,
code: None,
code_description: None,
source: Some(DIAGNOSTIC_NAME.into()),
message: syntax_error.body().to_string(),
related_information: None,
data: None,
},
)
}
fn diagnostic_edit_range( fn diagnostic_edit_range(
range: TextRange, range: TextRange,
source_kind: &SourceKind, source_kind: &SourceKind,

View file

@ -207,36 +207,23 @@ impl Workspace {
let messages: Vec<ExpandedMessage> = messages let messages: Vec<ExpandedMessage> = messages
.into_iter() .into_iter()
.map(|msg| { .map(|msg| ExpandedMessage {
let message = msg.body().to_string(); code: msg.to_noqa_code().map(|code| code.to_string()),
let range = msg.range(); message: msg.body().to_string(),
match msg.to_noqa_code() { start_location: source_code.line_column(msg.start()).into(),
Some(code) => ExpandedMessage { end_location: source_code.line_column(msg.end()).into(),
code: Some(code.to_string()), fix: msg.fix().map(|fix| ExpandedFix {
message, message: msg.suggestion().map(ToString::to_string),
start_location: source_code.line_column(range.start()).into(), edits: fix
end_location: source_code.line_column(range.end()).into(), .edits()
fix: msg.fix().map(|fix| ExpandedFix { .iter()
message: msg.suggestion().map(ToString::to_string), .map(|edit| ExpandedEdit {
edits: fix location: source_code.line_column(edit.start()).into(),
.edits() end_location: source_code.line_column(edit.end()).into(),
.iter() content: edit.content().map(ToString::to_string),
.map(|edit| ExpandedEdit { })
location: source_code.line_column(edit.start()).into(), .collect(),
end_location: source_code.line_column(edit.end()).into(), }),
content: edit.content().map(ToString::to_string),
})
.collect(),
}),
},
None => ExpandedMessage {
code: None,
message,
start_location: source_code.line_column(range.start()).into(),
end_location: source_code.line_column(range.end()).into(),
fix: None,
},
}
}) })
.collect(); .collect();