Skip to content

Commit c24ec56

Browse files
ntBresharkdp
authored andcommitted
Move concise diagnostic rendering to ruff_db (#19398)
## Summary This PR moves most of the work of rendering concise diagnostics in Ruff into `ruff_db`, where the code is shared with ty. To accomplish this without breaking backwards compatibility in Ruff, there are two main changes on the `ruff_db`/ty side: - Added the logic from Ruff for remapping notebook line numbers to cells - Reordered the fields in the diagnostic to match Ruff and rustc ```text # old error[invalid-assignment] try.py:3:1: Object of type `Literal[1]` is not assignable to `str` # new try.py:3:1: error[invalid-assignment]: Object of type `Literal[1]` is not assignable to `str` ``` I don't think the notebook change failed any tests on its own, and only a handful of snaphots changed in ty after reordering the fields, but this will obviously affect any other uses of the concise format, outside of tests, too. The other big change should only affect Ruff: - Added three new `DisplayDiagnosticConfig` options Micha and I hoped that we could get by with one option (`hide_severity`), but Ruff also toggles `show_fix_status` itself, independently (there are cases where we want neither severity nor the fix status), and during the implementation I realized we also needed access to an `Applicability`. The main goal here is to suppress the severity (`error` above) because ruff only uses the `error` severity and to use the secondary/noqa code instead of the line name (`invalid-assignment` above). ```text # ty - same as "new" above try.py:3:1: error[invalid-assignment]: Object of type `Literal[1]` is not assignable to `str` # ruff try.py:3:1: RUF123 [*] Object of type `Literal[1]` is not assignable to `str` ``` This part of the concise diagnostic is actually shared with the `full` output format in Ruff, but with the settings above, there are no snapshot changes to either format. ## Test Plan Existing tests with the handful of updates mentioned above, as well as some new tests in the `concise` module. Also this PR. Swapping the fields might have broken mypy_primer, unless it occasionally times out on its own. I also ran this script in the root of my Ruff checkout, which also has CPython in it: ```shell flags=(--isolated --no-cache --no-respect-gitignore --output-format concise .) diff <(target/release/ruff check ${flags[@]} 2> /dev/null) \ <(ruff check ${flags[@]} 2> /dev/null) ``` This yielded an expected diff due to some t-string error changes on main since 0.12.4: ```diff 33622c33622 < crates/ruff_python_parser/resources/inline/err/f_string_lambda_without_parentheses.py:1:15: SyntaxError: Expected an element of or the end of the f-string --- > crates/ruff_python_parser/resources/inline/err/f_string_lambda_without_parentheses.py:1:15: SyntaxError: Expected an f-string or t-string element or the end of the f-string or t-string 33742c33742 < crates/ruff_python_parser/resources/inline/err/implicitly_concatenated_unterminated_string_multiline.py:4:1: SyntaxError: Expected an element of or the end of the f-string --- > crates/ruff_python_parser/resources/inline/err/implicitly_concatenated_unterminated_string_multiline.py:4:1: SyntaxError: Expected an f-string or t-string element or the end of the f-string or t-string 34131c34131 < crates/ruff_python_parser/resources/inline/err/t_string_lambda_without_parentheses.py:2:15: SyntaxError: Expected an element of or the end of the t-string --- > crates/ruff_python_parser/resources/inline/err/t_string_lambda_without_parentheses.py:2:15: SyntaxError: Expected an f-string or t-string element or the end of the f-string or t-string ``` So modulo color, the results are identical on 38,186 errors in our test suite and CPython 3.10. --------- Co-authored-by: David Peter <mail@david-peter.de>
1 parent 25f570d commit c24ec56

File tree

10 files changed

+326
-106
lines changed

10 files changed

+326
-106
lines changed

.github/workflows/ty-ecosystem-analyzer.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ jobs:
6464
6565
cd ..
6666
67-
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@f0eec0e549684d8e1d7b8bc3e351202124b63bda"
67+
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@27dd66d9e397d986ef9c631119ee09556eab8af9"
6868
6969
ecosystem-analyzer \
7070
--repository ruff \

.github/workflows/ty-ecosystem-report.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ jobs:
4949
5050
cd ..
5151
52-
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@f0eec0e549684d8e1d7b8bc3e351202124b63bda"
52+
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@27dd66d9e397d986ef9c631119ee09556eab8af9"
5353
5454
ecosystem-analyzer \
5555
--verbose \

crates/ruff/src/printer.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ impl Printer {
264264
.with_show_fix_diff(self.flags.intersects(Flags::SHOW_FIX_DIFF))
265265
.with_show_source(self.format == OutputFormat::Full)
266266
.with_unsafe_fixes(self.unsafe_fixes)
267+
.with_preview(preview)
267268
.emit(writer, &diagnostics.inner, &context)?;
268269

269270
if self.flags.intersects(Flags::SHOW_FIX_SUMMARY) {

crates/ruff_db/src/diagnostic/mod.rs

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::{fmt::Formatter, path::Path, sync::Arc};
22

3-
use ruff_diagnostics::Fix;
3+
use ruff_diagnostics::{Applicability, Fix};
44
use ruff_source_file::{LineColumn, SourceCode, SourceFile};
55

66
use ruff_annotate_snippets::Level as AnnotateLevel;
@@ -1188,7 +1188,9 @@ impl Severity {
11881188

11891189
/// Like [`Severity`] but exclusively for sub-diagnostics.
11901190
///
1191-
/// This supports an additional `Help` severity that may not be needed in main diagnostics.
1191+
/// This type only exists to add an additional `Help` severity that isn't present in `Severity` or
1192+
/// used for main diagnostics. If we want to add `Severity::Help` in the future, this type could be
1193+
/// deleted and the two combined again.
11921194
#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, get_size2::GetSize)]
11931195
pub enum SubDiagnosticSeverity {
11941196
Help,
@@ -1236,6 +1238,15 @@ pub struct DisplayDiagnosticConfig {
12361238
reason = "This is currently only used for JSON but will be needed soon for other formats"
12371239
)]
12381240
preview: bool,
1241+
/// Whether to hide the real `Severity` of diagnostics.
1242+
///
1243+
/// This is intended for temporary use by Ruff, which only has a single `error` severity at the
1244+
/// moment. We should be able to remove this option when Ruff gets more severities.
1245+
hide_severity: bool,
1246+
/// Whether to show the availability of a fix in a diagnostic.
1247+
show_fix_status: bool,
1248+
/// The lowest applicability that should be shown when reporting diagnostics.
1249+
fix_applicability: Applicability,
12391250
}
12401251

12411252
impl DisplayDiagnosticConfig {
@@ -1264,6 +1275,35 @@ impl DisplayDiagnosticConfig {
12641275
..self
12651276
}
12661277
}
1278+
1279+
/// Whether to hide a diagnostic's severity or not.
1280+
pub fn hide_severity(self, yes: bool) -> DisplayDiagnosticConfig {
1281+
DisplayDiagnosticConfig {
1282+
hide_severity: yes,
1283+
..self
1284+
}
1285+
}
1286+
1287+
/// Whether to show a fix's availability or not.
1288+
pub fn show_fix_status(self, yes: bool) -> DisplayDiagnosticConfig {
1289+
DisplayDiagnosticConfig {
1290+
show_fix_status: yes,
1291+
..self
1292+
}
1293+
}
1294+
1295+
/// Set the lowest fix applicability that should be shown.
1296+
///
1297+
/// In other words, an applicability of `Safe` (the default) would suppress showing fixes or fix
1298+
/// availability for unsafe or display-only fixes.
1299+
///
1300+
/// Note that this option is currently ignored when `hide_severity` is false.
1301+
pub fn fix_applicability(self, applicability: Applicability) -> DisplayDiagnosticConfig {
1302+
DisplayDiagnosticConfig {
1303+
fix_applicability: applicability,
1304+
..self
1305+
}
1306+
}
12671307
}
12681308

12691309
impl Default for DisplayDiagnosticConfig {
@@ -1273,6 +1313,9 @@ impl Default for DisplayDiagnosticConfig {
12731313
color: false,
12741314
context: 2,
12751315
preview: false,
1316+
hide_severity: false,
1317+
show_fix_status: false,
1318+
fix_applicability: Applicability::Safe,
12761319
}
12771320
}
12781321
}

crates/ruff_db/src/diagnostic/render.rs

Lines changed: 46 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use ruff_notebook::{Notebook, NotebookIndex};
99
use ruff_source_file::{LineIndex, OneIndexed, SourceCode};
1010
use ruff_text_size::{TextRange, TextSize};
1111

12-
use crate::diagnostic::stylesheet::{DiagnosticStylesheet, fmt_styled};
12+
use crate::diagnostic::stylesheet::DiagnosticStylesheet;
1313
use crate::{
1414
Db,
1515
files::File,
@@ -18,14 +18,16 @@ use crate::{
1818
};
1919

2020
use super::{
21-
Annotation, Diagnostic, DiagnosticFormat, DiagnosticSource, DisplayDiagnosticConfig, Severity,
21+
Annotation, Diagnostic, DiagnosticFormat, DiagnosticSource, DisplayDiagnosticConfig,
2222
SubDiagnostic, UnifiedFile,
2323
};
2424

2525
use azure::AzureRenderer;
26+
use concise::ConciseRenderer;
2627
use pylint::PylintRenderer;
2728

2829
mod azure;
30+
mod concise;
2931
mod full;
3032
#[cfg(feature = "serde")]
3133
mod json;
@@ -105,48 +107,7 @@ impl std::fmt::Display for DisplayDiagnostics<'_> {
105107
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
106108
match self.config.format {
107109
DiagnosticFormat::Concise => {
108-
let stylesheet = if self.config.color {
109-
DiagnosticStylesheet::styled()
110-
} else {
111-
DiagnosticStylesheet::plain()
112-
};
113-
114-
for diag in self.diagnostics {
115-
let (severity, severity_style) = match diag.severity() {
116-
Severity::Info => ("info", stylesheet.info),
117-
Severity::Warning => ("warning", stylesheet.warning),
118-
Severity::Error => ("error", stylesheet.error),
119-
Severity::Fatal => ("fatal", stylesheet.error),
120-
};
121-
write!(
122-
f,
123-
"{severity}[{id}]",
124-
severity = fmt_styled(severity, severity_style),
125-
id = fmt_styled(diag.id(), stylesheet.emphasis)
126-
)?;
127-
if let Some(span) = diag.primary_span() {
128-
write!(
129-
f,
130-
" {path}",
131-
path = fmt_styled(span.file().path(self.resolver), stylesheet.emphasis)
132-
)?;
133-
if let Some(range) = span.range() {
134-
let diagnostic_source = span.file().diagnostic_source(self.resolver);
135-
let start = diagnostic_source
136-
.as_source_code()
137-
.line_column(range.start());
138-
139-
write!(
140-
f,
141-
":{line}:{col}",
142-
line = fmt_styled(start.line, stylesheet.emphasis),
143-
col = fmt_styled(start.column, stylesheet.emphasis),
144-
)?;
145-
}
146-
write!(f, ":")?;
147-
}
148-
writeln!(f, " {message}", message = diag.concise_message())?;
149-
}
110+
ConciseRenderer::new(self.resolver, self.config).render(f, self.diagnostics)?;
150111
}
151112
DiagnosticFormat::Full => {
152113
let stylesheet = if self.config.color {
@@ -862,7 +823,7 @@ fn relativize_path<'p>(cwd: &SystemPath, path: &'p str) -> &'p str {
862823
#[cfg(test)]
863824
mod tests {
864825

865-
use ruff_diagnostics::{Edit, Fix};
826+
use ruff_diagnostics::{Applicability, Edit, Fix};
866827

867828
use crate::diagnostic::{
868829
Annotation, DiagnosticId, IntoDiagnosticMessage, SecondaryCode, Severity, Span,
@@ -2310,6 +2271,27 @@ watermelon
23102271
self.config = config;
23112272
}
23122273

2274+
/// Hide diagnostic severity when rendering.
2275+
pub(super) fn hide_severity(&mut self, yes: bool) {
2276+
let mut config = std::mem::take(&mut self.config);
2277+
config = config.hide_severity(yes);
2278+
self.config = config;
2279+
}
2280+
2281+
/// Show fix availability when rendering.
2282+
pub(super) fn show_fix_status(&mut self, yes: bool) {
2283+
let mut config = std::mem::take(&mut self.config);
2284+
config = config.show_fix_status(yes);
2285+
self.config = config;
2286+
}
2287+
2288+
/// The lowest fix applicability to show when rendering.
2289+
pub(super) fn fix_applicability(&mut self, applicability: Applicability) {
2290+
let mut config = std::mem::take(&mut self.config);
2291+
config = config.fix_applicability(applicability);
2292+
self.config = config;
2293+
}
2294+
23132295
/// Add a file with the given path and contents to this environment.
23142296
pub(super) fn add(&mut self, path: &str, contents: &str) {
23152297
let path = SystemPath::new(path);
@@ -2675,6 +2657,25 @@ if call(foo
26752657
}
26762658

26772659
/// Create Ruff-style diagnostics for testing the various output formats for a notebook.
2660+
///
2661+
/// The concatenated cells look like this:
2662+
///
2663+
/// ```python
2664+
/// # cell 1
2665+
/// import os
2666+
/// # cell 2
2667+
/// import math
2668+
///
2669+
/// print('hello world')
2670+
/// # cell 3
2671+
/// def foo():
2672+
/// print()
2673+
/// x = 1
2674+
/// ```
2675+
///
2676+
/// The first diagnostic is on the unused `os` import with location cell 1, row 2, column 8
2677+
/// (`cell 1:2:8`). The second diagnostic is the unused `math` import at `cell 2:2:8`, and the
2678+
/// third diagnostic is an unfixable unused variable at `cell 3:4:5`.
26782679
#[allow(
26792680
dead_code,
26802681
reason = "This is currently only used for JSON but will be needed soon for other formats"

0 commit comments

Comments
 (0)