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>
This commit is contained in:
Brent Westbrook 2025-07-23 11:43:32 -04:00 committed by GitHub
parent 88bd82938f
commit 4daf59e5e7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 326 additions and 106 deletions

View File

@ -64,7 +64,7 @@ jobs:
cd ..
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@f0eec0e549684d8e1d7b8bc3e351202124b63bda"
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@27dd66d9e397d986ef9c631119ee09556eab8af9"
ecosystem-analyzer \
--repository ruff \

View File

@ -49,7 +49,7 @@ jobs:
cd ..
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@f0eec0e549684d8e1d7b8bc3e351202124b63bda"
uv tool install "git+https://github.com/astral-sh/ecosystem-analyzer@27dd66d9e397d986ef9c631119ee09556eab8af9"
ecosystem-analyzer \
--verbose \

View File

@ -264,6 +264,7 @@ impl Printer {
.with_show_fix_diff(self.flags.intersects(Flags::SHOW_FIX_DIFF))
.with_show_source(self.format == OutputFormat::Full)
.with_unsafe_fixes(self.unsafe_fixes)
.with_preview(preview)
.emit(writer, &diagnostics.inner, &context)?;
if self.flags.intersects(Flags::SHOW_FIX_SUMMARY) {

View File

@ -1,6 +1,6 @@
use std::{fmt::Formatter, path::Path, sync::Arc};
use ruff_diagnostics::Fix;
use ruff_diagnostics::{Applicability, Fix};
use ruff_source_file::{LineColumn, SourceCode, SourceFile};
use ruff_annotate_snippets::Level as AnnotateLevel;
@ -1188,7 +1188,9 @@ impl Severity {
/// Like [`Severity`] but exclusively for sub-diagnostics.
///
/// This supports an additional `Help` severity that may not be needed in main diagnostics.
/// This type only exists to add an additional `Help` severity that isn't present in `Severity` or
/// used for main diagnostics. If we want to add `Severity::Help` in the future, this type could be
/// deleted and the two combined again.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, get_size2::GetSize)]
pub enum SubDiagnosticSeverity {
Help,
@ -1236,6 +1238,15 @@ pub struct DisplayDiagnosticConfig {
reason = "This is currently only used for JSON but will be needed soon for other formats"
)]
preview: bool,
/// Whether to hide the real `Severity` of diagnostics.
///
/// This is intended for temporary use by Ruff, which only has a single `error` severity at the
/// moment. We should be able to remove this option when Ruff gets more severities.
hide_severity: bool,
/// Whether to show the availability of a fix in a diagnostic.
show_fix_status: bool,
/// The lowest applicability that should be shown when reporting diagnostics.
fix_applicability: Applicability,
}
impl DisplayDiagnosticConfig {
@ -1264,6 +1275,35 @@ impl DisplayDiagnosticConfig {
..self
}
}
/// Whether to hide a diagnostic's severity or not.
pub fn hide_severity(self, yes: bool) -> DisplayDiagnosticConfig {
DisplayDiagnosticConfig {
hide_severity: yes,
..self
}
}
/// Whether to show a fix's availability or not.
pub fn show_fix_status(self, yes: bool) -> DisplayDiagnosticConfig {
DisplayDiagnosticConfig {
show_fix_status: yes,
..self
}
}
/// Set the lowest fix applicability that should be shown.
///
/// In other words, an applicability of `Safe` (the default) would suppress showing fixes or fix
/// availability for unsafe or display-only fixes.
///
/// Note that this option is currently ignored when `hide_severity` is false.
pub fn fix_applicability(self, applicability: Applicability) -> DisplayDiagnosticConfig {
DisplayDiagnosticConfig {
fix_applicability: applicability,
..self
}
}
}
impl Default for DisplayDiagnosticConfig {
@ -1273,6 +1313,9 @@ impl Default for DisplayDiagnosticConfig {
color: false,
context: 2,
preview: false,
hide_severity: false,
show_fix_status: false,
fix_applicability: Applicability::Safe,
}
}
}

View File

@ -9,7 +9,7 @@ use ruff_notebook::{Notebook, NotebookIndex};
use ruff_source_file::{LineIndex, OneIndexed, SourceCode};
use ruff_text_size::{TextRange, TextSize};
use crate::diagnostic::stylesheet::{DiagnosticStylesheet, fmt_styled};
use crate::diagnostic::stylesheet::DiagnosticStylesheet;
use crate::{
Db,
files::File,
@ -18,14 +18,16 @@ use crate::{
};
use super::{
Annotation, Diagnostic, DiagnosticFormat, DiagnosticSource, DisplayDiagnosticConfig, Severity,
Annotation, Diagnostic, DiagnosticFormat, DiagnosticSource, DisplayDiagnosticConfig,
SubDiagnostic, UnifiedFile,
};
use azure::AzureRenderer;
use concise::ConciseRenderer;
use pylint::PylintRenderer;
mod azure;
mod concise;
mod full;
#[cfg(feature = "serde")]
mod json;
@ -105,48 +107,7 @@ impl std::fmt::Display for DisplayDiagnostics<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self.config.format {
DiagnosticFormat::Concise => {
let stylesheet = if self.config.color {
DiagnosticStylesheet::styled()
} else {
DiagnosticStylesheet::plain()
};
for diag in self.diagnostics {
let (severity, severity_style) = match diag.severity() {
Severity::Info => ("info", stylesheet.info),
Severity::Warning => ("warning", stylesheet.warning),
Severity::Error => ("error", stylesheet.error),
Severity::Fatal => ("fatal", stylesheet.error),
};
write!(
f,
"{severity}[{id}]",
severity = fmt_styled(severity, severity_style),
id = fmt_styled(diag.id(), stylesheet.emphasis)
)?;
if let Some(span) = diag.primary_span() {
write!(
f,
" {path}",
path = fmt_styled(span.file().path(self.resolver), stylesheet.emphasis)
)?;
if let Some(range) = span.range() {
let diagnostic_source = span.file().diagnostic_source(self.resolver);
let start = diagnostic_source
.as_source_code()
.line_column(range.start());
write!(
f,
":{line}:{col}",
line = fmt_styled(start.line, stylesheet.emphasis),
col = fmt_styled(start.column, stylesheet.emphasis),
)?;
}
write!(f, ":")?;
}
writeln!(f, " {message}", message = diag.concise_message())?;
}
ConciseRenderer::new(self.resolver, self.config).render(f, self.diagnostics)?;
}
DiagnosticFormat::Full => {
let stylesheet = if self.config.color {
@ -862,7 +823,7 @@ fn relativize_path<'p>(cwd: &SystemPath, path: &'p str) -> &'p str {
#[cfg(test)]
mod tests {
use ruff_diagnostics::{Edit, Fix};
use ruff_diagnostics::{Applicability, Edit, Fix};
use crate::diagnostic::{
Annotation, DiagnosticId, IntoDiagnosticMessage, SecondaryCode, Severity, Span,
@ -2310,6 +2271,27 @@ watermelon
self.config = config;
}
/// Hide diagnostic severity when rendering.
pub(super) fn hide_severity(&mut self, yes: bool) {
let mut config = std::mem::take(&mut self.config);
config = config.hide_severity(yes);
self.config = config;
}
/// Show fix availability when rendering.
pub(super) fn show_fix_status(&mut self, yes: bool) {
let mut config = std::mem::take(&mut self.config);
config = config.show_fix_status(yes);
self.config = config;
}
/// The lowest fix applicability to show when rendering.
pub(super) fn fix_applicability(&mut self, applicability: Applicability) {
let mut config = std::mem::take(&mut self.config);
config = config.fix_applicability(applicability);
self.config = config;
}
/// Add a file with the given path and contents to this environment.
pub(super) fn add(&mut self, path: &str, contents: &str) {
let path = SystemPath::new(path);
@ -2675,6 +2657,25 @@ if call(foo
}
/// Create Ruff-style diagnostics for testing the various output formats for a notebook.
///
/// The concatenated cells look like this:
///
/// ```python
/// # cell 1
/// import os
/// # cell 2
/// import math
///
/// print('hello world')
/// # cell 3
/// def foo():
/// print()
/// x = 1
/// ```
///
/// The first diagnostic is on the unused `os` import with location cell 1, row 2, column 8
/// (`cell 1:2:8`). The second diagnostic is the unused `math` import at `cell 2:2:8`, and the
/// third diagnostic is an unfixable unused variable at `cell 3:4:5`.
#[allow(
dead_code,
reason = "This is currently only used for JSON but will be needed soon for other formats"

View File

@ -0,0 +1,195 @@
use crate::diagnostic::{
Diagnostic, DisplayDiagnosticConfig, Severity,
stylesheet::{DiagnosticStylesheet, fmt_styled},
};
use super::FileResolver;
pub(super) struct ConciseRenderer<'a> {
resolver: &'a dyn FileResolver,
config: &'a DisplayDiagnosticConfig,
}
impl<'a> ConciseRenderer<'a> {
pub(super) fn new(resolver: &'a dyn FileResolver, config: &'a DisplayDiagnosticConfig) -> Self {
Self { resolver, config }
}
pub(super) fn render(
&self,
f: &mut std::fmt::Formatter,
diagnostics: &[Diagnostic],
) -> std::fmt::Result {
let stylesheet = if self.config.color {
DiagnosticStylesheet::styled()
} else {
DiagnosticStylesheet::plain()
};
let sep = fmt_styled(":", stylesheet.separator);
for diag in diagnostics {
if let Some(span) = diag.primary_span() {
write!(
f,
"{path}",
path = fmt_styled(
span.file().relative_path(self.resolver).to_string_lossy(),
stylesheet.emphasis
)
)?;
if let Some(range) = span.range() {
let diagnostic_source = span.file().diagnostic_source(self.resolver);
let start = diagnostic_source
.as_source_code()
.line_column(range.start());
if let Some(notebook_index) = self.resolver.notebook_index(span.file()) {
write!(
f,
"{sep}cell {cell}{sep}{line}{sep}{col}",
cell = notebook_index.cell(start.line).unwrap_or_default(),
line = notebook_index.cell_row(start.line).unwrap_or_default(),
col = start.column,
)?;
} else {
write!(
f,
"{sep}{line}{sep}{col}",
line = start.line,
col = start.column,
)?;
}
}
write!(f, "{sep} ")?;
}
if self.config.hide_severity {
if let Some(code) = diag.secondary_code() {
write!(
f,
"{code} ",
code = fmt_styled(code, stylesheet.secondary_code)
)?;
}
if self.config.show_fix_status {
if let Some(fix) = diag.fix() {
// Do not display an indicator for inapplicable fixes
if fix.applies(self.config.fix_applicability) {
write!(f, "[{fix}] ", fix = fmt_styled("*", stylesheet.separator))?;
}
}
}
} else {
let (severity, severity_style) = match diag.severity() {
Severity::Info => ("info", stylesheet.info),
Severity::Warning => ("warning", stylesheet.warning),
Severity::Error => ("error", stylesheet.error),
Severity::Fatal => ("fatal", stylesheet.error),
};
write!(
f,
"{severity}[{id}] ",
severity = fmt_styled(severity, severity_style),
id = fmt_styled(diag.id(), stylesheet.emphasis)
)?;
}
writeln!(f, "{message}", message = diag.concise_message())?;
}
Ok(())
}
}
#[cfg(test)]
mod tests {
use ruff_diagnostics::Applicability;
use crate::diagnostic::{
DiagnosticFormat,
render::tests::{
TestEnvironment, create_diagnostics, create_notebook_diagnostics,
create_syntax_error_diagnostics,
},
};
#[test]
fn output() {
let (env, diagnostics) = create_diagnostics(DiagnosticFormat::Concise);
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
fib.py:1:8: error[unused-import] `os` imported but unused
fib.py:6:5: error[unused-variable] Local variable `x` is assigned to but never used
undef.py:1:4: error[undefined-name] Undefined name `a`
");
}
#[test]
fn show_fixes() {
let (mut env, diagnostics) = create_diagnostics(DiagnosticFormat::Concise);
env.hide_severity(true);
env.show_fix_status(true);
env.fix_applicability(Applicability::DisplayOnly);
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
fib.py:1:8: F401 [*] `os` imported but unused
fib.py:6:5: F841 [*] Local variable `x` is assigned to but never used
undef.py:1:4: F821 Undefined name `a`
");
}
#[test]
fn show_fixes_preview() {
let (mut env, diagnostics) = create_diagnostics(DiagnosticFormat::Concise);
env.hide_severity(true);
env.show_fix_status(true);
env.fix_applicability(Applicability::DisplayOnly);
env.preview(true);
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
fib.py:1:8: F401 [*] `os` imported but unused
fib.py:6:5: F841 [*] Local variable `x` is assigned to but never used
undef.py:1:4: F821 Undefined name `a`
");
}
#[test]
fn show_fixes_syntax_errors() {
let (mut env, diagnostics) = create_syntax_error_diagnostics(DiagnosticFormat::Concise);
env.hide_severity(true);
env.show_fix_status(true);
env.fix_applicability(Applicability::DisplayOnly);
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
syntax_errors.py:1:15: SyntaxError: Expected one or more symbol names after import
syntax_errors.py:3:12: SyntaxError: Expected ')', found newline
");
}
#[test]
fn syntax_errors() {
let (env, diagnostics) = create_syntax_error_diagnostics(DiagnosticFormat::Concise);
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
syntax_errors.py:1:15: error[invalid-syntax] SyntaxError: Expected one or more symbol names after import
syntax_errors.py:3:12: error[invalid-syntax] SyntaxError: Expected ')', found newline
");
}
#[test]
fn notebook_output() {
let (env, diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Concise);
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
notebook.ipynb:cell 1:2:8: error[unused-import] `os` imported but unused
notebook.ipynb:cell 2:2:8: error[unused-import] `math` imported but unused
notebook.ipynb:cell 3:4:5: error[unused-variable] Local variable `x` is assigned to but never used
");
}
#[test]
fn missing_file() {
let mut env = TestEnvironment::new();
env.format(DiagnosticFormat::Concise);
let diag = env.err().build();
insta::assert_snapshot!(
env.render(&diag),
@"error[test-diagnostic] main diagnostic message",
);
}
}

View File

@ -41,6 +41,8 @@ pub struct DiagnosticStylesheet {
pub(crate) line_no: Style,
pub(crate) emphasis: Style,
pub(crate) none: Style,
pub(crate) separator: Style,
pub(crate) secondary_code: Style,
}
impl Default for DiagnosticStylesheet {
@ -62,6 +64,8 @@ impl DiagnosticStylesheet {
line_no: bright_blue.effects(Effects::BOLD),
emphasis: Style::new().effects(Effects::BOLD),
none: Style::new(),
separator: AnsiColor::Cyan.on_default(),
secondary_code: AnsiColor::Red.on_default().effects(Effects::BOLD),
}
}
@ -75,6 +79,8 @@ impl DiagnosticStylesheet {
line_no: Style::new(),
emphasis: Style::new(),
none: Style::new(),
separator: Style::new(),
secondary_code: Style::new(),
}
}
}

View File

@ -6,13 +6,12 @@ use bitflags::bitflags;
use colored::Colorize;
use ruff_annotate_snippets::{Level, Renderer, Snippet};
use ruff_db::diagnostic::{Diagnostic, SecondaryCode};
use ruff_db::diagnostic::{Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig, SecondaryCode};
use ruff_notebook::NotebookIndex;
use ruff_source_file::{LineColumn, OneIndexed};
use ruff_source_file::OneIndexed;
use ruff_text_size::{TextLen, TextRange, TextSize};
use crate::Locator;
use crate::fs::relativize_path;
use crate::line_width::{IndentWidth, LineWidthBuilder};
use crate::message::diff::Diff;
use crate::message::{Emitter, EmitterContext};
@ -21,8 +20,6 @@ use crate::settings::types::UnsafeFixes;
bitflags! {
#[derive(Default)]
struct EmitterFlags: u8 {
/// Whether to show the fix status of a diagnostic.
const SHOW_FIX_STATUS = 1 << 0;
/// Whether to show the diff of a fix, for diagnostics that have a fix.
const SHOW_FIX_DIFF = 1 << 1;
/// Whether to show the source code of a diagnostic.
@ -30,17 +27,27 @@ bitflags! {
}
}
#[derive(Default)]
pub struct TextEmitter {
flags: EmitterFlags,
unsafe_fixes: UnsafeFixes,
config: DisplayDiagnosticConfig,
}
impl Default for TextEmitter {
fn default() -> Self {
Self {
flags: EmitterFlags::default(),
config: DisplayDiagnosticConfig::default()
.format(DiagnosticFormat::Concise)
.hide_severity(true)
.color(!cfg!(test) && colored::control::SHOULD_COLORIZE.should_colorize()),
}
}
}
impl TextEmitter {
#[must_use]
pub fn with_show_fix_status(mut self, show_fix_status: bool) -> Self {
self.flags
.set(EmitterFlags::SHOW_FIX_STATUS, show_fix_status);
self.config = self.config.show_fix_status(show_fix_status);
self
}
@ -58,7 +65,15 @@ impl TextEmitter {
#[must_use]
pub fn with_unsafe_fixes(mut self, unsafe_fixes: UnsafeFixes) -> Self {
self.unsafe_fixes = unsafe_fixes;
self.config = self
.config
.fix_applicability(unsafe_fixes.required_applicability());
self
}
#[must_use]
pub fn with_preview(mut self, preview: bool) -> Self {
self.config = self.config.preview(preview);
self
}
}
@ -71,51 +86,10 @@ impl Emitter for TextEmitter {
context: &EmitterContext,
) -> anyhow::Result<()> {
for message in diagnostics {
write!(writer, "{}", message.display(context, &self.config))?;
let filename = message.expect_ruff_filename();
write!(
writer,
"{path}{sep}",
path = relativize_path(&filename).bold(),
sep = ":".cyan(),
)?;
let start_location = message.expect_ruff_start_location();
let notebook_index = context.notebook_index(&filename);
// Check if we're working on a jupyter notebook and translate positions with cell accordingly
let diagnostic_location = if let Some(notebook_index) = notebook_index {
write!(
writer,
"cell {cell}{sep}",
cell = notebook_index
.cell(start_location.line)
.unwrap_or(OneIndexed::MIN),
sep = ":".cyan(),
)?;
LineColumn {
line: notebook_index
.cell_row(start_location.line)
.unwrap_or(OneIndexed::MIN),
column: start_location.column,
}
} else {
start_location
};
writeln!(
writer,
"{row}{sep}{col}{sep} {code_and_body}",
row = diagnostic_location.line,
col = diagnostic_location.column,
sep = ":".cyan(),
code_and_body = RuleCodeAndBody {
message,
show_fix_status: self.flags.intersects(EmitterFlags::SHOW_FIX_STATUS),
unsafe_fixes: self.unsafe_fixes,
}
)?;
if self.flags.intersects(EmitterFlags::SHOW_SOURCE) {
// The `0..0` range is used to highlight file-level diagnostics.
if message.expect_range() != TextRange::default() {

View File

@ -676,7 +676,7 @@ fn invalid_include_pattern_concise_output() -> anyhow::Result<()> {
----- stderr -----
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
ty failed
Cause: error[invalid-glob] ty.toml:4:5: Invalid include pattern: Too many stars at position 5
Cause: ty.toml:4:5: error[invalid-glob] Invalid include pattern: Too many stars at position 5
");
Ok(())

View File

@ -592,8 +592,8 @@ fn concise_diagnostics() -> anyhow::Result<()> {
success: false
exit_code: 1
----- stdout -----
warning[unresolved-reference] test.py:2:7: Name `x` used when not defined
error[non-subscriptable] test.py:3:7: Cannot subscript object of type `Literal[4]` with no `__getitem__` method
test.py:2:7: warning[unresolved-reference] Name `x` used when not defined
test.py:3:7: error[non-subscriptable] Cannot subscript object of type `Literal[4]` with no `__getitem__` method
Found 2 diagnostics
----- stderr -----
@ -627,7 +627,7 @@ fn concise_revealed_type() -> anyhow::Result<()> {
success: true
exit_code: 0
----- stdout -----
info[revealed-type] test.py:5:13: Revealed type: `Literal["hello"]`
test.py:5:13: info[revealed-type] Revealed type: `Literal["hello"]`
Found 1 diagnostic
----- stderr -----