Skip to content

Commit 696784b

Browse files
ntBresecond-ed
authored andcommitted
Use new diff rendering format in tests (astral-sh#20101)
## Summary I spun this off from astral-sh#19919 to separate the rendering code change and snapshot updates from the (much smaller) changes to expose this in the CLI. I grouped all of the `ruff_linter` snapshot changes in the final commit in an effort to make this easier to review. The code changes are in [this range](https://github.com/astral-sh/ruff/pull/20101/files/619395eb417d2030e31fbb0e487a72dac58902db). I went through all of the snapshots, albeit fairly quickly, and they all looked correct to me. In the last few commits I was trying to resolve an existing issue in the alignment of the line number separator: https://github.com/astral-sh/ruff/blob/73720c73be981df6f71ce837a67ca1167da0265e/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap#L87-L89 In the snapshot above on `main`, you can see that a double-digit line number at the end of the context lines for a snippet was causing a misalignment with the other separators. That's now resolved. The one downside is that this can lead to a mismatch with the diagnostic above: ``` C409 [*] Unnecessary list literal passed to `tuple()` (rewrite as a tuple literal) --> C409.py:4:6 | 2 | t2 = tuple([1, 2]) 3 | t3 = tuple((1, 2)) 4 | t4 = tuple([ | ______^ 5 | | 1, 6 | | 2 7 | | ]) | |__^ 8 | t5 = tuple( 9 | (1, 2) | help: Rewrite as a tuple literal 1 | t1 = tuple([]) 2 | t2 = tuple([1, 2]) 3 | t3 = tuple((1, 2)) - t4 = tuple([ 4 + t4 = ( 5 | 1, 6 | 2 - ]) 7 + ) 8 | t5 = tuple( 9 | (1, 2) 10 | ) note: This is an unsafe fix and may remove comments or change runtime behavior ``` But I don't think we can avoid that without really reworking this rendering to make the diagnostic and diff rendering aware of each other. Anyway, this should only happen in relatively rare cases where the diagnostic is near a digit boundary and also near a context boundary. Most of our diagnostics line up nicely. Another potential downside of the new rendering format is its handling of long stretches of `+` or `-` lines: ``` help: Replace with `Literal[...] | None` 21 | ... 22 | 23 | - def func6(arg1: Literal[ - "hello", - None # Comment 1 - , "world" - ]): 24 + def func6(arg1: Literal["hello", "world"] | None): 25 | ... 26 | 27 | note: This is an unsafe fix and may remove comments or change runtime behavior ``` To me it just seems a little hard to tell what's going on with just a long streak of `-`-prefixed lines. I saw an even more exaggerated example at some point, but I think this is also fairly rare. Most of the snapshots seem more like the examples we looked at on Discord with plenty of `|` lines and pairs of `+` and `-` lines. ## Test Plan Existing tests plus one new test in `ruff_db` to isolate a line separator alignment issue
1 parent eb32ba3 commit 696784b

File tree

957 files changed

+68352
-80125
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

957 files changed

+68352
-80125
lines changed

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

Lines changed: 172 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
11
use std::borrow::Cow;
22
use std::num::NonZeroUsize;
33

4-
use anstyle::Style;
5-
use ruff_notebook::NotebookIndex;
64
use similar::{ChangeTag, TextDiff};
75

86
use ruff_annotate_snippets::Renderer as AnnotateRenderer;
97
use ruff_diagnostics::{Applicability, Fix};
8+
use ruff_notebook::NotebookIndex;
109
use ruff_source_file::OneIndexed;
1110
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};
1211

@@ -58,13 +57,14 @@ impl<'a> FullRenderer<'a> {
5857
for diag in renderable.diagnostics.iter() {
5958
writeln!(f, "{}", renderer.render(diag.to_annotate()))?;
6059
}
61-
writeln!(f)?;
6260

6361
if self.config.show_fix_diff {
6462
if let Some(diff) = Diff::from_diagnostic(diag, &stylesheet, self.resolver) {
65-
writeln!(f, "{diff}")?;
63+
write!(f, "{diff}")?;
6664
}
6765
}
66+
67+
writeln!(f)?;
6868
}
6969

7070
Ok(())
@@ -126,19 +126,6 @@ impl std::fmt::Display for Diff<'_> {
126126
vec![(None, source_text.text_len())]
127127
};
128128

129-
let message = match self.fix.applicability() {
130-
// TODO(zanieb): Adjust this messaging once it's user-facing
131-
Applicability::Safe => "Safe fix",
132-
Applicability::Unsafe => "Unsafe fix",
133-
Applicability::DisplayOnly => "Display-only fix",
134-
};
135-
136-
// TODO(brent) `stylesheet.separator` is cyan rather than blue, as we had before. I think
137-
// we're getting rid of this soon anyway, so I didn't think it was worth adding another
138-
// style to the stylesheet temporarily. The color doesn't appear at all in the snapshot
139-
// tests, which is the only place these are currently used.
140-
writeln!(f, "ℹ {}", fmt_styled(message, self.stylesheet.separator))?;
141-
142129
let mut last_end = TextSize::ZERO;
143130
for (cell, offset) in cells {
144131
let range = TextRange::new(last_end, offset);
@@ -167,64 +154,67 @@ impl std::fmt::Display for Diff<'_> {
167154

168155
let diff = TextDiff::from_lines(input, &output);
169156

170-
let (largest_old, largest_new) = diff
171-
.ops()
172-
.last()
173-
.map(|op| (op.old_range().start, op.new_range().start))
174-
.unwrap_or_default();
157+
let grouped_ops = diff.grouped_ops(3);
158+
159+
// Find the new line number with the largest number of digits to align all of the line
160+
// number separators.
161+
let last_op = grouped_ops.last().and_then(|group| group.last());
162+
let largest_new = last_op.map(|op| op.new_range().end).unwrap_or_default();
175163

176-
let digit_with = OneIndexed::from_zero_indexed(largest_new.max(largest_old)).digits();
164+
let digit_with = OneIndexed::new(largest_new).unwrap_or_default().digits();
177165

178166
if let Some(cell) = cell {
179-
// Room for 2 digits, 2 x 1 space before each digit, 1 space, and 1 `|`. This
180-
// centers the three colons on the pipe.
181-
writeln!(f, "{:>1$} cell {cell}", ":::", 2 * digit_with.get() + 4)?;
167+
// Room for 1 digit, 1 space, 1 `|`, and 1 more following space. This centers the
168+
// three colons on the pipe.
169+
writeln!(f, "{:>1$} cell {cell}", ":::", digit_with.get() + 3)?;
182170
}
183171

184-
for (idx, group) in diff.grouped_ops(3).iter().enumerate() {
172+
for (idx, group) in grouped_ops.iter().enumerate() {
185173
if idx > 0 {
186174
writeln!(f, "{:-^1$}", "-", 80)?;
187175
}
188176
for op in group {
189177
for change in diff.iter_inline_changes(op) {
190-
let sign = match change.tag() {
191-
ChangeTag::Delete => "-",
192-
ChangeTag::Insert => "+",
193-
ChangeTag::Equal => " ",
178+
let (sign, style, line_no_style, index) = match change.tag() {
179+
ChangeTag::Delete => (
180+
"-",
181+
self.stylesheet.deletion,
182+
self.stylesheet.deletion_line_no,
183+
None,
184+
),
185+
ChangeTag::Insert => (
186+
"+",
187+
self.stylesheet.insertion,
188+
self.stylesheet.insertion_line_no,
189+
change.new_index(),
190+
),
191+
ChangeTag::Equal => (
192+
"|",
193+
self.stylesheet.none,
194+
self.stylesheet.line_no,
195+
change.new_index(),
196+
),
194197
};
195198

196-
let line_style = LineStyle::from(change.tag(), self.stylesheet);
197-
198-
let old_index = change.old_index().map(OneIndexed::from_zero_indexed);
199-
let new_index = change.new_index().map(OneIndexed::from_zero_indexed);
199+
let line = Line {
200+
index: index.map(OneIndexed::from_zero_indexed),
201+
width: digit_with,
202+
};
200203

201204
write!(
202205
f,
203-
"{} {} |{}",
204-
Line {
205-
index: old_index,
206-
width: digit_with,
207-
},
208-
Line {
209-
index: new_index,
210-
width: digit_with,
211-
},
212-
fmt_styled(line_style.apply_to(sign), self.stylesheet.emphasis),
206+
"{line} {sign} ",
207+
line = fmt_styled(line, self.stylesheet.line_no),
208+
sign = fmt_styled(sign, line_no_style),
213209
)?;
214210

215211
for (emphasized, value) in change.iter_strings_lossy() {
216212
let value = show_nonprinting(&value);
213+
let styled = fmt_styled(value, style);
217214
if emphasized {
218-
write!(
219-
f,
220-
"{}",
221-
fmt_styled(
222-
line_style.apply_to(&value),
223-
self.stylesheet.underline
224-
)
225-
)?;
215+
write!(f, "{}", fmt_styled(styled, self.stylesheet.emphasis))?;
226216
} else {
227-
write!(f, "{}", line_style.apply_to(&value))?;
217+
write!(f, "{styled}")?;
228218
}
229219
}
230220
if change.missing_newline() {
@@ -235,31 +225,35 @@ impl std::fmt::Display for Diff<'_> {
235225
}
236226
}
237227

238-
Ok(())
239-
}
240-
}
241-
242-
struct LineStyle {
243-
style: Style,
244-
}
245-
246-
impl LineStyle {
247-
fn apply_to(&self, input: &str) -> impl std::fmt::Display {
248-
fmt_styled(input, self.style)
249-
}
250-
251-
fn from(value: ChangeTag, stylesheet: &DiagnosticStylesheet) -> LineStyle {
252-
match value {
253-
ChangeTag::Equal => LineStyle {
254-
style: stylesheet.none,
255-
},
256-
ChangeTag::Delete => LineStyle {
257-
style: stylesheet.deletion,
258-
},
259-
ChangeTag::Insert => LineStyle {
260-
style: stylesheet.insertion,
261-
},
228+
match self.fix.applicability() {
229+
Applicability::Safe => {}
230+
Applicability::Unsafe => {
231+
writeln!(
232+
f,
233+
"{note}: {msg}",
234+
note = fmt_styled("note", self.stylesheet.warning),
235+
msg = fmt_styled(
236+
"This is an unsafe fix and may change runtime behavior",
237+
self.stylesheet.emphasis
238+
)
239+
)?;
240+
}
241+
Applicability::DisplayOnly => {
242+
// Note that this is still only used in tests. There's no `--display-only-fixes`
243+
// analog to `--unsafe-fixes` for users to activate this or see the styling.
244+
writeln!(
245+
f,
246+
"{note}: {msg}",
247+
note = fmt_styled("note", self.stylesheet.error),
248+
msg = fmt_styled(
249+
"This is a display-only fix and is likely to be incorrect",
250+
self.stylesheet.emphasis
251+
)
252+
)?;
253+
}
262254
}
255+
256+
Ok(())
263257
}
264258
}
265259

@@ -297,7 +291,7 @@ fn show_nonprinting(s: &str) -> Cow<'_, str> {
297291

298292
#[cfg(test)]
299293
mod tests {
300-
use ruff_diagnostics::{Applicability, Fix};
294+
use ruff_diagnostics::{Applicability, Edit, Fix};
301295
use ruff_text_size::{TextLen, TextRange, TextSize};
302296

303297
use crate::diagnostic::{
@@ -712,11 +706,9 @@ print()
712706
| ^^
713707
|
714708
help: Remove unused import: `os`
715-
716-
ℹ Safe fix
717-
::: cell 1
718-
1 1 | # cell 1
719-
2 |-import os
709+
::: cell 1
710+
1 | # cell 1
711+
- import os
720712
721713
error[unused-import][*]: `math` imported but unused
722714
--> notebook.ipynb:cell 2:2:8
@@ -728,13 +720,11 @@ print()
728720
4 | print('hello world')
729721
|
730722
help: Remove unused import: `math`
731-
732-
ℹ Safe fix
733-
::: cell 2
734-
1 1 | # cell 2
735-
2 |-import math
736-
3 2 |
737-
4 3 | print('hello world')
723+
::: cell 2
724+
1 | # cell 2
725+
- import math
726+
2 |
727+
3 | print('hello world')
738728
739729
error[unused-variable]: Local variable `x` is assigned to but never used
740730
--> notebook.ipynb:cell 3:4:5
@@ -745,14 +735,13 @@ print()
745735
| ^
746736
|
747737
help: Remove assignment to unused variable `x`
748-
749-
ℹ Unsafe fix
750-
::: cell 3
751-
1 1 | # cell 3
752-
2 2 | def foo():
753-
3 3 | print()
754-
4 |- x = 1
755-
5 4 |
738+
::: cell 3
739+
1 | # cell 3
740+
2 | def foo():
741+
3 | print()
742+
- x = 1
743+
4 |
744+
note: This is an unsafe fix and may change runtime behavior
756745
");
757746
}
758747

@@ -780,22 +769,21 @@ print()
780769
| ^^
781770
|
782771
help: Remove unused import: `os`
783-
784-
ℹ Unsafe fix
785-
::: cell 1
786-
1 1 | # cell 1
787-
2 |-import os
788-
::: cell 2
789-
1 1 | # cell 2
790-
2 |-import math
791-
3 2 |
792-
4 3 | print('hello world')
793-
::: cell 3
794-
1 1 | # cell 3
795-
2 2 | def foo():
796-
3 3 | print()
797-
4 |- x = 1
798-
5 4 |
772+
::: cell 1
773+
1 | # cell 1
774+
- import os
775+
::: cell 2
776+
1 | # cell 2
777+
- import math
778+
2 |
779+
3 | print('hello world')
780+
::: cell 3
781+
1 | # cell 3
782+
2 | def foo():
783+
3 | print()
784+
- x = 1
785+
4 |
786+
note: This is an unsafe fix and may change runtime behavior
799787
");
800788
}
801789

@@ -901,4 +889,73 @@ print()
901889
|
902890
");
903891
}
892+
893+
/// Test that we handle the width calculation for the line number correctly even for context
894+
/// lines at the end of a diff. For example, we want it to render like this:
895+
///
896+
/// ```
897+
/// 8 |
898+
/// 9 |
899+
/// 10 |
900+
/// ```
901+
///
902+
/// and not like this:
903+
///
904+
/// ```
905+
/// 8 |
906+
/// 9 |
907+
/// 10 |
908+
/// ```
909+
#[test]
910+
fn longer_line_number_end_of_context() {
911+
let mut env = TestEnvironment::new();
912+
let contents = "\
913+
line 1
914+
line 2
915+
line 3
916+
line 4
917+
line 5
918+
line 6
919+
line 7
920+
line 8
921+
line 9
922+
line 10
923+
";
924+
env.add("example.py", contents);
925+
env.format(DiagnosticFormat::Full);
926+
env.show_fix_diff(true);
927+
928+
let mut diagnostic = env.err().primary("example.py", "3", "3", "label").build();
929+
diagnostic.help("Start of diff:");
930+
let target = "line 7";
931+
let line9 = contents.find(target).unwrap();
932+
let range = TextRange::at(TextSize::try_from(line9).unwrap(), target.text_len());
933+
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
934+
format!("fixed {target}"),
935+
range,
936+
)));
937+
938+
insta::assert_snapshot!(env.render(&diagnostic), @r"
939+
error[test-diagnostic]: main diagnostic message
940+
--> example.py:3:1
941+
|
942+
1 | line 1
943+
2 | line 2
944+
3 | line 3
945+
| ^^^^^^ label
946+
4 | line 4
947+
5 | line 5
948+
|
949+
help: Start of diff:
950+
4 | line 4
951+
5 | line 5
952+
6 | line 6
953+
- line 7
954+
7 + fixed line 7
955+
8 | line 8
956+
9 | line 9
957+
10 | line 10
958+
note: This is an unsafe fix and may change runtime behavior
959+
");
960+
}
904961
}

0 commit comments

Comments
 (0)