Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Clean up unused rendering code in `ruff_linter` (#19832) 

## Summary

This is a follow-up to
https://github.com/astral-sh/ruff/pull/19415#discussion_r2263456740 to
remove some unused code. As Micha noticed,
`GroupedEmitter::with_show_source` was only used in local unit tests[^1]
and was safe to remove. This allowed deleting `MessageCodeFrame` and a
lot more helper code previously shared with the `full` output format.

I also moved some other code from `text.rs` and `message/mod.rs` into
`grouped.rs` that is now only used for the `grouped` format. With a
little refactoring of the `concise` rendering logic in `ruff_db`, we
could probably remove `RuleCodeAndBody` too. The only difference I see
from the `concise` output is whether we print the filename next to the
row and column or not:

```shell
> ruff check --output-format concise
try.py:1:8: F401 [*] `math` imported but unused
> ruff check --output-format grouped
try.py:
  1:8 F401 [*] `math` imported but unused
```

But I didn't try to do that here.

## Test Plan

Existing tests, with the source code no longer displayed. I also deleted
one test, as it was now a duplicate of the `default` test.

[^1]: "Local unit tests" as opposed to all of our linter snapshot tests,
as is the case for `TextEmitter::with_show_fix_diff`. We also want to
expose that to users eventually
(https://github.com/astral-sh/ruff/issues/7352), which I don't believe
is the case for the `grouped` format.
This commit is contained in:
Brent Westbrook 2025-08-09 14:20:48 -04:00 committed by GitHub
parent 5a116e48c3
commit 8230b79829
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 79 additions and 448 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@ -3022,7 +3022,6 @@ dependencies = [
"pep440_rs",
"pyproject-toml",
"regex",
"ruff_annotate_snippets",
"ruff_cache",
"ruff_db",
"ruff_diagnostics",

1
crates/ruff_linter/Cargo.toml
View File

@ -13,7 +13,6 @@ license = { workspace = true }
[lib]
[dependencies]
ruff_annotate_snippets = { workspace = true }
ruff_cache = { workspace = true }
ruff_db = { workspace = true, features = ["junit", "serde"] }
ruff_diagnostics = { workspace = true, features = ["serde"] }

145
crates/ruff_linter/src/message/grouped.rs
View File

@ -1,3 +1,4 @@
use std::collections::BTreeMap;
use std::fmt::{Display, Formatter};
use std::io::Write;
use std::num::NonZeroUsize;
@ -6,18 +7,16 @@ use colored::Colorize;
use ruff_db::diagnostic::Diagnostic;
use ruff_notebook::NotebookIndex;
use ruff_source_file::OneIndexed;
use ruff_source_file::{LineColumn, OneIndexed};
use crate::fs::relativize_path;
use crate::message::diff::calculate_print_width;
use crate::message::text::{MessageCodeFrame, RuleCodeAndBody};
use crate::message::{Emitter, EmitterContext, MessageWithLocation, group_diagnostics_by_filename};
use crate::message::{Emitter, EmitterContext};
use crate::settings::types::UnsafeFixes;
#[derive(Default)]
pub struct GroupedEmitter {
show_fix_status: bool,
show_source: bool,
unsafe_fixes: UnsafeFixes,
}
@ -28,12 +27,6 @@ impl GroupedEmitter {
self
}
#[must_use]
pub fn with_show_source(mut self, show_source: bool) -> Self {
self.show_source = show_source;
self
}
#[must_use]
pub fn with_unsafe_fixes(mut self, unsafe_fixes: UnsafeFixes) -> Self {
self.unsafe_fixes = unsafe_fixes;
@ -76,29 +69,53 @@ impl Emitter for GroupedEmitter {
message,
show_fix_status: self.show_fix_status,
unsafe_fixes: self.unsafe_fixes,
show_source: self.show_source,
row_length,
column_length,
}
)?;
}
// Print a blank line between files, unless we're showing the source, in which case
// we'll have already printed a blank line between messages.
if !self.show_source {
writeln!(writer)?;
}
// Print a blank line between files.
writeln!(writer)?;
}
Ok(())
}
}
struct MessageWithLocation<'a> {
message: &'a Diagnostic,
start_location: LineColumn,
}
impl std::ops::Deref for MessageWithLocation<'_> {
type Target = Diagnostic;
fn deref(&self) -> &Self::Target {
self.message
}
}
fn group_diagnostics_by_filename(
diagnostics: &[Diagnostic],
) -> BTreeMap<String, Vec<MessageWithLocation<'_>>> {
let mut grouped_messages = BTreeMap::default();
for diagnostic in diagnostics {
grouped_messages
.entry(diagnostic.expect_ruff_filename())
.or_insert_with(Vec::new)
.push(MessageWithLocation {
message: diagnostic,
start_location: diagnostic.expect_ruff_start_location(),
});
}
grouped_messages
}
struct DisplayGroupedMessage<'a> {
message: MessageWithLocation<'a>,
show_fix_status: bool,
unsafe_fixes: UnsafeFixes,
show_source: bool,
row_length: NonZeroUsize,
column_length: NonZeroUsize,
notebook_index: Option<&'a NotebookIndex>,
@ -152,51 +169,50 @@ impl Display for DisplayGroupedMessage<'_> {
},
)?;
if self.show_source {
use std::fmt::Write;
let mut padded = PadAdapter::new(f);
writeln!(
padded,
"{}",
MessageCodeFrame {
message,
notebook_index: self.notebook_index
Ok(())
}
}
pub(super) struct RuleCodeAndBody<'a> {
pub(crate) message: &'a Diagnostic,
pub(crate) show_fix_status: bool,
pub(crate) unsafe_fixes: UnsafeFixes,
}
impl Display for RuleCodeAndBody<'_> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
if self.show_fix_status {
if let Some(fix) = self.message.fix() {
// Do not display an indicator for inapplicable fixes
if fix.applies(self.unsafe_fixes.required_applicability()) {
if let Some(code) = self.message.secondary_code() {
write!(f, "{} ", code.red().bold())?;
}
return write!(
f,
"{fix}{body}",
fix = format_args!("[{}] ", "*".cyan()),
body = self.message.body(),
);
}
)?;
}
Ok(())
}
}
/// Adapter that adds a ' ' at the start of every line without the need to copy the string.
/// Inspired by Rust's `debug_struct()` internal implementation that also uses a `PadAdapter`.
struct PadAdapter<'buf> {
buf: &'buf mut (dyn std::fmt::Write + 'buf),
on_newline: bool,
}
impl<'buf> PadAdapter<'buf> {
fn new(buf: &'buf mut (dyn std::fmt::Write + 'buf)) -> Self {
Self {
buf,
on_newline: true,
}
}
}
impl std::fmt::Write for PadAdapter<'_> {
fn write_str(&mut self, s: &str) -> std::fmt::Result {
for s in s.split_inclusive('\n') {
if self.on_newline {
self.buf.write_str(" ")?;
}
self.on_newline = s.ends_with('\n');
self.buf.write_str(s)?;
}
Ok(())
if let Some(code) = self.message.secondary_code() {
write!(
f,
"{code} {body}",
code = code.red().bold(),
body = self.message.body(),
)
} else {
write!(
f,
"{code}: {body}",
code = self.message.id().as_str().red().bold(),
body = self.message.body(),
)
}
}
}
@ -226,19 +242,9 @@ mod tests {
assert_snapshot!(content);
}
#[test]
fn show_source() {
let mut emitter = GroupedEmitter::default().with_show_source(true);
let content = capture_emitter_output(&mut emitter, &create_diagnostics());
assert_snapshot!(content);
}
#[test]
fn fix_status() {
let mut emitter = GroupedEmitter::default()
.with_show_fix_status(true)
.with_show_source(true);
let mut emitter = GroupedEmitter::default().with_show_fix_status(true);
let content = capture_emitter_output(&mut emitter, &create_diagnostics());
assert_snapshot!(content);
@ -248,7 +254,6 @@ mod tests {
fn fix_status_unsafe() {
let mut emitter = GroupedEmitter::default()
.with_show_fix_status(true)
.with_show_source(true)
.with_unsafe_fixes(UnsafeFixes::Enabled);
let content = capture_emitter_output(&mut emitter, &create_diagnostics());

33
crates/ruff_linter/src/message/mod.rs
View File

@ -1,7 +1,5 @@
use std::collections::BTreeMap;
use std::fmt::Display;
use std::io::Write;
use std::ops::Deref;
use rustc_hash::FxHashMap;
@ -15,7 +13,7 @@ pub use github::GithubEmitter;
pub use gitlab::GitlabEmitter;
pub use grouped::GroupedEmitter;
use ruff_notebook::NotebookIndex;
use ruff_source_file::{LineColumn, SourceFile};
use ruff_source_file::SourceFile;
use ruff_text_size::{Ranged, TextRange, TextSize};
pub use sarif::SarifEmitter;
pub use text::TextEmitter;
@ -134,35 +132,6 @@ impl FileResolver for EmitterContext<'_> {
}
}
struct MessageWithLocation<'a> {
message: &'a Diagnostic,
start_location: LineColumn,
}
impl Deref for MessageWithLocation<'_> {
type Target = Diagnostic;
fn deref(&self) -> &Self::Target {
self.message
}
}
fn group_diagnostics_by_filename(
diagnostics: &[Diagnostic],
) -> BTreeMap<String, Vec<MessageWithLocation<'_>>> {
let mut grouped_messages = BTreeMap::default();
for diagnostic in diagnostics {
grouped_messages
.entry(diagnostic.expect_ruff_filename())
.or_insert_with(Vec::new)
.push(MessageWithLocation {
message: diagnostic,
start_location: diagnostic.expect_ruff_start_location(),
});
}
grouped_messages
}
/// Display format for [`Diagnostic`]s.
///
/// The emitter serializes a slice of [`Diagnostic`]s and writes them to a [`Write`].

22
crates/ruff_linter/src/message/snapshots/ruff_linter__message__grouped__tests__fix_status.snap
View File

@ -1,30 +1,10 @@
---
source: crates/ruff_linter/src/message/grouped.rs
expression: content
snapshot_kind: text
---
fib.py:
1:8 F401 `os` imported but unused
|
1 | import os
| ^^ F401
|
= help: Remove unused import: `os`
6:5 F841 Local variable `x` is assigned to but never used
|
4 | def fibonacci(n):
5 | """Compute the nth number in the Fibonacci sequence."""
6 | x = 1
| ^ F841
7 | if n == 0:
8 | return 0
|
= help: Remove assignment to unused variable `x`
undef.py:
1:4 F821 Undefined name `a`
|
1 | if a == 1: pass
| ^ F821
|

22
crates/ruff_linter/src/message/snapshots/ruff_linter__message__grouped__tests__fix_status_unsafe.snap
View File

@ -1,30 +1,10 @@
---
source: crates/ruff_linter/src/message/grouped.rs
expression: content
snapshot_kind: text
---
fib.py:
1:8 F401 [*] `os` imported but unused
|
1 | import os
| ^^ F401
|
= help: Remove unused import: `os`
6:5 F841 [*] Local variable `x` is assigned to but never used
|
4 | def fibonacci(n):
5 | """Compute the nth number in the Fibonacci sequence."""
6 | x = 1
| ^ F841
7 | if n == 0:
8 | return 0
|
= help: Remove assignment to unused variable `x`
undef.py:
1:4 F821 Undefined name `a`
|
1 | if a == 1: pass
| ^ F821
|

30
crates/ruff_linter/src/message/snapshots/ruff_linter__message__grouped__tests__show_source.snap
View File

@ -1,30 +0,0 @@
---
source: crates/ruff_linter/src/message/grouped.rs
expression: content
snapshot_kind: text
---
fib.py:
1:8 F401 `os` imported but unused
|
1 | import os
| ^^ F401
|
= help: Remove unused import: `os`
6:5 F841 Local variable `x` is assigned to but never used
|
4 | def fibonacci(n):
5 | """Compute the nth number in the Fibonacci sequence."""
6 | x = 1
| ^ F841
7 | if n == 0:
8 | return 0
|
= help: Remove assignment to unused variable `x`
undef.py:
1:4 F821 Undefined name `a`
|
1 | if a == 1: pass
| ^ F821
|

273
crates/ruff_linter/src/message/text.rs
View File

@ -1,16 +1,6 @@
use std::borrow::Cow;
use std::fmt::{Display, Formatter};
use std::io::Write;
use colored::Colorize;
use ruff_annotate_snippets::{Level, Renderer, Snippet};
use ruff_db::diagnostic::{
Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig, SecondaryCode, ceil_char_boundary,
};
use ruff_notebook::NotebookIndex;
use ruff_source_file::OneIndexed;
use ruff_text_size::{TextLen, TextRange, TextSize};
use ruff_db::diagnostic::{Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig};
use crate::message::diff::Diff;
use crate::message::{Emitter, EmitterContext};
@ -101,267 +91,6 @@ impl Emitter for TextEmitter {
}
}
pub(super) struct RuleCodeAndBody<'a> {
pub(crate) message: &'a Diagnostic,
pub(crate) show_fix_status: bool,
pub(crate) unsafe_fixes: UnsafeFixes,
}
impl Display for RuleCodeAndBody<'_> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
if self.show_fix_status {
if let Some(fix) = self.message.fix() {
// Do not display an indicator for inapplicable fixes
if fix.applies(self.unsafe_fixes.required_applicability()) {
if let Some(code) = self.message.secondary_code() {
write!(f, "{} ", code.red().bold())?;
}
return write!(
f,
"{fix}{body}",
fix = format_args!("[{}] ", "*".cyan()),
body = self.message.body(),
);
}
}
}
if let Some(code) = self.message.secondary_code() {
write!(
f,
"{code} {body}",
code = code.red().bold(),
body = self.message.body(),
)
} else {
write!(
f,
"{code}: {body}",
code = self.message.id().as_str().red().bold(),
body = self.message.body(),
)
}
}
}
pub(super) struct MessageCodeFrame<'a> {
pub(crate) message: &'a Diagnostic,
pub(crate) notebook_index: Option<&'a NotebookIndex>,
}
impl Display for MessageCodeFrame<'_> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
let suggestion = self.message.first_help_text();
let footers = if let Some(suggestion) = suggestion {
vec![Level::Help.title(suggestion)]
} else {
Vec::new()
};
let source_file = self.message.expect_ruff_source_file();
let source_code = source_file.to_source_code();
let content_start_index = source_code.line_index(self.message.expect_range().start());
let mut start_index = content_start_index.saturating_sub(2);
// If we're working with a Jupyter Notebook, skip the lines which are
// outside of the cell containing the diagnostic.
if let Some(index) = self.notebook_index {
let content_start_cell = index.cell(content_start_index).unwrap_or(OneIndexed::MIN);
while start_index < content_start_index {
if index.cell(start_index).unwrap_or(OneIndexed::MIN) == content_start_cell {
break;
}
start_index = start_index.saturating_add(1);
}
}
// Trim leading empty lines.
while start_index < content_start_index {
if !source_code.line_text(start_index).trim().is_empty() {
break;
}
start_index = start_index.saturating_add(1);
}
let content_end_index = source_code.line_index(self.message.expect_range().end());
let mut end_index = content_end_index
.saturating_add(2)
.min(OneIndexed::from_zero_indexed(source_code.line_count()));
// If we're working with a Jupyter Notebook, skip the lines which are
// outside of the cell containing the diagnostic.
if let Some(index) = self.notebook_index {
let content_end_cell = index.cell(content_end_index).unwrap_or(OneIndexed::MIN);
while end_index > content_end_index {
if index.cell(end_index).unwrap_or(OneIndexed::MIN) == content_end_cell {
break;
}
end_index = end_index.saturating_sub(1);
}
}
// Trim trailing empty lines.
while end_index > content_end_index {
if !source_code.line_text(end_index).trim().is_empty() {
break;
}
end_index = end_index.saturating_sub(1);
}
let start_offset = source_code.line_start(start_index);
let end_offset = source_code.line_end(end_index);
let source = replace_unprintable(
source_code.slice(TextRange::new(start_offset, end_offset)),
self.message.expect_range() - start_offset,
)
.fix_up_empty_spans_after_line_terminator();
let label = self
.message
.secondary_code()
.map(SecondaryCode::as_str)
.unwrap_or_default();
let line_start = self.notebook_index.map_or_else(
|| start_index.get(),
|notebook_index| {
notebook_index
.cell_row(start_index)
.unwrap_or(OneIndexed::MIN)
.get()
},
);
let span = usize::from(source.annotation_range.start())
..usize::from(source.annotation_range.end());
let annotation = Level::Error.span(span).label(label);
let snippet = Snippet::source(&source.text)
.line_start(line_start)
.annotation(annotation)
.fold(false);
let message = Level::None.title("").snippet(snippet).footers(footers);
let renderer = if !cfg!(test) && colored::control::SHOULD_COLORIZE.should_colorize() {
Renderer::styled()
} else {
Renderer::plain()
}
.cut_indicator("…");
let rendered = renderer.render(message);
writeln!(f, "{rendered}")
}
}
/// Given some source code and an annotation range, this routine replaces
/// unprintable characters with printable representations of them.
///
/// The source code returned has an annotation that is updated to reflect
/// changes made to the source code (if any).
///
/// We don't need to normalize whitespace, such as converting tabs to spaces,
/// because `annotate-snippets` handles that internally. Similarly, it's safe to
/// modify the annotation ranges by inserting 3-byte Unicode replacements
/// because `annotate-snippets` will account for their actual width when
/// rendering and displaying the column to the user.
fn replace_unprintable(source: &str, annotation_range: TextRange) -> SourceCode<'_> {
let mut result = String::new();
let mut last_end = 0;
let mut range = annotation_range;
// Updates the range given by the caller whenever a single byte (at
// `index` in `source`) is replaced with `len` bytes.
//
// When the index occurs before the start of the range, the range is
// offset by `len`. When the range occurs after or at the start but before
// the end, then the end of the range only is offset by `len`.
let mut update_range = |index, len| {
if index < usize::from(annotation_range.start()) {
range += TextSize::new(len - 1);
} else if index < usize::from(annotation_range.end()) {
range = range.add_end(TextSize::new(len - 1));
}
};
// If `c` is an unprintable character, then this returns a printable
// representation of it (using a fancier Unicode codepoint).
let unprintable_replacement = |c: char| -> Option<char> {
match c {
'\x07' => Some('␇'),
'\x08' => Some('␈'),
'\x1b' => Some('␛'),
'\x7f' => Some('␡'),
_ => None,
}
};
for (index, c) in source.char_indices() {
if let Some(printable) = unprintable_replacement(c) {
result.push_str(&source[last_end..index]);
result.push(printable);
last_end = index + 1;
let len = printable.text_len().to_u32();
update_range(index, len);
}
}
// No tabs or unprintable chars
if result.is_empty() {
SourceCode {
annotation_range,
text: Cow::Borrowed(source),
}
} else {
result.push_str(&source[last_end..]);
SourceCode {
annotation_range: range,
text: Cow::Owned(result),
}
}
}
struct SourceCode<'a> {
text: Cow<'a, str>,
annotation_range: TextRange,
}
impl<'a> SourceCode<'a> {
/// This attempts to "fix up" the span on `SourceCode` in the case where
/// it's an empty span immediately following a line terminator.
///
/// At present, `annotate-snippets` (both upstream and our vendored copy)
/// will render annotations of such spans to point to the space immediately
/// following the previous line. But ideally, this should point to the space
/// immediately preceding the next line.
///
/// After attempting to fix `annotate-snippets` and giving up after a couple
/// hours, this routine takes a different tact: it adjusts the span to be
/// non-empty and it will cover the first codepoint of the following line.
/// This forces `annotate-snippets` to point to the right place.
///
/// See also: <https://github.com/astral-sh/ruff/issues/15509>
fn fix_up_empty_spans_after_line_terminator(self) -> SourceCode<'a> {
if !self.annotation_range.is_empty()
|| self.annotation_range.start() == TextSize::from(0)
|| self.annotation_range.start() >= self.text.text_len()
{
return self;
}
if self.text.as_bytes()[self.annotation_range.start().to_usize() - 1] != b'\n' {
return self;
}
let start = self.annotation_range.start();
let end = ceil_char_boundary(&self.text, start + TextSize::from(1));
SourceCode {
annotation_range: TextRange::new(start, end),
..self
}
}
}
#[cfg(test)]
mod tests {
use insta::assert_snapshot;