Skip to content

Commit 692be72

Browse files
authored
Move diff rendering to ruff_db (#20006)
Summary -- This is a preparatory PR in support of #19919. It moves our `Diff` rendering code from `ruff_linter` to `ruff_db`, where we have direct access to the `DiagnosticStylesheet` used by our other diagnostic rendering code. As shown by the tests, this shouldn't cause any visible changes. The colors aren't exactly the same, as I note in a TODO comment, but I don't think there's any existing way to see those, even in tests. The `Diff` implementation is mostly unchanged. I just switched from a Ruff-specific `SourceFile` to a `DiagnosticSource` (removing an `expect_ruff_source_file` call) and updated the `LineStyle` struct and other styling calls to use `fmt_styled` and our existing stylesheet. In support of these changes, I added three styles to our stylesheet: `insertion` and `deletion` for the corresponding diff operations, and `underline`, which apparently we _can_ use, as I hoped on Discord. This isn't supported in all terminals, though. It worked in ghostty but not in st for me. I moved the `calculate_print_width` function from the now-deleted `diff.rs` to a method on `OneIndexed`, where it was available everywhere we needed it. I'm not sure if that's desirable, or if my other changes to the function are either (using `ilog10` instead of a loop). This does make it `const` and slightly simplifies things in my opinion, but I'm happy to revert it if preferred. I also inlined a version of `show_nonprinting` from the `ShowNonprinting` trait in `ruff_linter`: https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_linter/src/text_helpers.rs#L3-L5 This trait is now only used in `source_kind.rs`, so I'm not sure it's worth having the trait or the macro-generated implementation (which is only called once). This is obviously closely related to our unprintable character handling in diagnostic rendering, but the usage seems different enough not to try to combine them. https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_db/src/diagnostic/render.rs#L990-L998 We could also move the trait to another crate where we can use it in `ruff_db` instead of inlining here, of course. Finally, this PR makes `TextEmitter` a very thin wrapper around a `DisplayDiagnosticsConfig`. It's still used in a few places, though, unlike the other emitters we've replaced, so I figured it was worth keeping around. It's a pretty nice API for setting all of the options on the config and then passing that along to a `DisplayDiagnostics`. Test Plan -- Existing snapshot tests with diffs
1 parent 14fe122 commit 692be72

File tree

11 files changed

+266
-230
lines changed

11 files changed

+266
-230
lines changed

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/ruff_db/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ salsa = { workspace = true }
4040
schemars = { workspace = true, optional = true }
4141
serde = { workspace = true, optional = true }
4242
serde_json = { workspace = true, optional = true }
43+
similar = { workspace = true }
4344
thiserror = { workspace = true }
4445
tracing = { workspace = true }
4546
tracing-subscriber = { workspace = true, optional = true }

crates/ruff_db/src/diagnostic/mod.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,6 +1294,10 @@ pub struct DisplayDiagnosticConfig {
12941294
hide_severity: bool,
12951295
/// Whether to show the availability of a fix in a diagnostic.
12961296
show_fix_status: bool,
1297+
/// Whether to show the diff for an available fix after the main diagnostic.
1298+
///
1299+
/// This currently only applies to `DiagnosticFormat::Full`.
1300+
show_fix_diff: bool,
12971301
/// The lowest applicability that should be shown when reporting diagnostics.
12981302
fix_applicability: Applicability,
12991303
}
@@ -1341,6 +1345,14 @@ impl DisplayDiagnosticConfig {
13411345
}
13421346
}
13431347

1348+
/// Whether to show a diff for an available fix after the main diagnostic.
1349+
pub fn show_fix_diff(self, yes: bool) -> DisplayDiagnosticConfig {
1350+
DisplayDiagnosticConfig {
1351+
show_fix_diff: yes,
1352+
..self
1353+
}
1354+
}
1355+
13441356
/// Set the lowest fix applicability that should be shown.
13451357
///
13461358
/// In other words, an applicability of `Safe` (the default) would suppress showing fixes or fix
@@ -1364,6 +1376,7 @@ impl Default for DisplayDiagnosticConfig {
13641376
preview: false,
13651377
hide_severity: false,
13661378
show_fix_status: false,
1379+
show_fix_diff: false,
13671380
fix_applicability: Applicability::Safe,
13681381
}
13691382
}

crates/ruff_db/src/diagnostic/render/full.rs

Lines changed: 198 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,17 @@
1+
use std::borrow::Cow;
2+
use std::num::NonZeroUsize;
3+
4+
use anstyle::Style;
5+
use similar::{ChangeTag, TextDiff};
6+
17
use ruff_annotate_snippets::Renderer as AnnotateRenderer;
8+
use ruff_diagnostics::{Applicability, Fix};
9+
use ruff_source_file::OneIndexed;
10+
use ruff_text_size::{Ranged, TextRange, TextSize};
211

312
use crate::diagnostic::render::{FileResolver, Resolved};
4-
use crate::diagnostic::{Diagnostic, DisplayDiagnosticConfig, stylesheet::DiagnosticStylesheet};
13+
use crate::diagnostic::stylesheet::{DiagnosticStylesheet, fmt_styled};
14+
use crate::diagnostic::{Diagnostic, DiagnosticSource, DisplayDiagnosticConfig};
515

616
pub(super) struct FullRenderer<'a> {
717
resolver: &'a dyn FileResolver,
@@ -48,12 +58,199 @@ impl<'a> FullRenderer<'a> {
4858
writeln!(f, "{}", renderer.render(diag.to_annotate()))?;
4959
}
5060
writeln!(f)?;
61+
62+
if self.config.show_fix_diff {
63+
if let Some(diff) = Diff::from_diagnostic(diag, &stylesheet, self.resolver) {
64+
writeln!(f, "{diff}")?;
65+
}
66+
}
67+
}
68+
69+
Ok(())
70+
}
71+
}
72+
73+
/// Renders a diff that shows the code fixes.
74+
///
75+
/// The implementation isn't fully fledged out and only used by tests. Before using in production, try
76+
/// * Improve layout
77+
/// * Replace tabs with spaces for a consistent experience across terminals
78+
/// * Replace zero-width whitespaces
79+
/// * Print a simpler diff if only a single line has changed
80+
/// * Compute the diff from the `Edit` because diff calculation is expensive.
81+
struct Diff<'a> {
82+
fix: &'a Fix,
83+
diagnostic_source: DiagnosticSource,
84+
stylesheet: &'a DiagnosticStylesheet,
85+
}
86+
87+
impl<'a> Diff<'a> {
88+
fn from_diagnostic(
89+
diagnostic: &'a Diagnostic,
90+
stylesheet: &'a DiagnosticStylesheet,
91+
resolver: &'a dyn FileResolver,
92+
) -> Option<Diff<'a>> {
93+
Some(Diff {
94+
fix: diagnostic.fix()?,
95+
diagnostic_source: diagnostic
96+
.primary_span_ref()?
97+
.file
98+
.diagnostic_source(resolver),
99+
stylesheet,
100+
})
101+
}
102+
}
103+
104+
impl std::fmt::Display for Diff<'_> {
105+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
106+
let source_code = self.diagnostic_source.as_source_code();
107+
let source_text = source_code.text();
108+
109+
// TODO(dhruvmanila): Add support for Notebook cells once it's user-facing
110+
let mut output = String::with_capacity(source_text.len());
111+
let mut last_end = TextSize::default();
112+
113+
for edit in self.fix.edits() {
114+
output.push_str(source_code.slice(TextRange::new(last_end, edit.start())));
115+
output.push_str(edit.content().unwrap_or_default());
116+
last_end = edit.end();
117+
}
118+
119+
output.push_str(&source_text[usize::from(last_end)..]);
120+
121+
let diff = TextDiff::from_lines(source_text, &output);
122+
123+
let message = match self.fix.applicability() {
124+
// TODO(zanieb): Adjust this messaging once it's user-facing
125+
Applicability::Safe => "Safe fix",
126+
Applicability::Unsafe => "Unsafe fix",
127+
Applicability::DisplayOnly => "Display-only fix",
128+
};
129+
130+
// TODO(brent) `stylesheet.separator` is cyan rather than blue, as we had before. I think
131+
// we're getting rid of this soon anyway, so I didn't think it was worth adding another
132+
// style to the stylesheet temporarily. The color doesn't appear at all in the snapshot
133+
// tests, which is the only place these are currently used.
134+
writeln!(f, "ℹ {}", fmt_styled(message, self.stylesheet.separator))?;
135+
136+
let (largest_old, largest_new) = diff
137+
.ops()
138+
.last()
139+
.map(|op| (op.old_range().start, op.new_range().start))
140+
.unwrap_or_default();
141+
142+
let digit_with = OneIndexed::from_zero_indexed(largest_new.max(largest_old)).digits();
143+
144+
for (idx, group) in diff.grouped_ops(3).iter().enumerate() {
145+
if idx > 0 {
146+
writeln!(f, "{:-^1$}", "-", 80)?;
147+
}
148+
for op in group {
149+
for change in diff.iter_inline_changes(op) {
150+
let sign = match change.tag() {
151+
ChangeTag::Delete => "-",
152+
ChangeTag::Insert => "+",
153+
ChangeTag::Equal => " ",
154+
};
155+
156+
let line_style = LineStyle::from(change.tag(), self.stylesheet);
157+
158+
let old_index = change.old_index().map(OneIndexed::from_zero_indexed);
159+
let new_index = change.new_index().map(OneIndexed::from_zero_indexed);
160+
161+
write!(
162+
f,
163+
"{} {} |{}",
164+
Line {
165+
index: old_index,
166+
width: digit_with
167+
},
168+
Line {
169+
index: new_index,
170+
width: digit_with
171+
},
172+
fmt_styled(line_style.apply_to(sign), self.stylesheet.emphasis),
173+
)?;
174+
175+
for (emphasized, value) in change.iter_strings_lossy() {
176+
let value = show_nonprinting(&value);
177+
if emphasized {
178+
write!(
179+
f,
180+
"{}",
181+
fmt_styled(line_style.apply_to(&value), self.stylesheet.underline)
182+
)?;
183+
} else {
184+
write!(f, "{}", line_style.apply_to(&value))?;
185+
}
186+
}
187+
if change.missing_newline() {
188+
writeln!(f)?;
189+
}
190+
}
191+
}
51192
}
52193

53194
Ok(())
54195
}
55196
}
56197

198+
struct LineStyle {
199+
style: Style,
200+
}
201+
202+
impl LineStyle {
203+
fn apply_to(&self, input: &str) -> impl std::fmt::Display {
204+
fmt_styled(input, self.style)
205+
}
206+
207+
fn from(value: ChangeTag, stylesheet: &DiagnosticStylesheet) -> LineStyle {
208+
match value {
209+
ChangeTag::Equal => LineStyle {
210+
style: stylesheet.none,
211+
},
212+
ChangeTag::Delete => LineStyle {
213+
style: stylesheet.deletion,
214+
},
215+
ChangeTag::Insert => LineStyle {
216+
style: stylesheet.insertion,
217+
},
218+
}
219+
}
220+
}
221+
222+
struct Line {
223+
index: Option<OneIndexed>,
224+
width: NonZeroUsize,
225+
}
226+
227+
impl std::fmt::Display for Line {
228+
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
229+
match self.index {
230+
None => {
231+
for _ in 0..self.width.get() {
232+
f.write_str(" ")?;
233+
}
234+
Ok(())
235+
}
236+
Some(idx) => write!(f, "{:<width$}", idx, width = self.width.get()),
237+
}
238+
}
239+
}
240+
241+
fn show_nonprinting(s: &str) -> Cow<'_, str> {
242+
if s.find(['\x07', '\x08', '\x1b', '\x7f']).is_some() {
243+
Cow::Owned(
244+
s.replace('\x07', "␇")
245+
.replace('\x08', "␈")
246+
.replace('\x1b', "␛")
247+
.replace('\x7f', "␡"),
248+
)
249+
} else {
250+
Cow::Borrowed(s)
251+
}
252+
}
253+
57254
#[cfg(test)]
58255
mod tests {
59256
use ruff_diagnostics::Applicability;

crates/ruff_db/src/diagnostic/stylesheet.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,12 @@ pub struct DiagnosticStylesheet {
4040
pub(crate) help: Style,
4141
pub(crate) line_no: Style,
4242
pub(crate) emphasis: Style,
43+
pub(crate) underline: Style,
4344
pub(crate) none: Style,
4445
pub(crate) separator: Style,
4546
pub(crate) secondary_code: Style,
47+
pub(crate) insertion: Style,
48+
pub(crate) deletion: Style,
4649
}
4750

4851
impl Default for DiagnosticStylesheet {
@@ -63,9 +66,12 @@ impl DiagnosticStylesheet {
6366
help: AnsiColor::BrightCyan.on_default().effects(Effects::BOLD),
6467
line_no: bright_blue.effects(Effects::BOLD),
6568
emphasis: Style::new().effects(Effects::BOLD),
69+
underline: Style::new().effects(Effects::UNDERLINE),
6670
none: Style::new(),
6771
separator: AnsiColor::Cyan.on_default(),
6872
secondary_code: AnsiColor::Red.on_default().effects(Effects::BOLD),
73+
insertion: AnsiColor::Green.on_default(),
74+
deletion: AnsiColor::Red.on_default(),
6975
}
7076
}
7177

@@ -78,9 +84,12 @@ impl DiagnosticStylesheet {
7884
help: Style::new(),
7985
line_no: Style::new(),
8086
emphasis: Style::new(),
87+
underline: Style::new(),
8188
none: Style::new(),
8289
separator: Style::new(),
8390
secondary_code: Style::new(),
91+
insertion: Style::new(),
92+
deletion: Style::new(),
8493
}
8594
}
8695
}

0 commit comments

Comments
 (0)