Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1273,13 +1273,17 @@ fn format_header<'a>(
..
} = item
{
if main_range >= range.0 && main_range < range.1 + max(*end_line as usize, 1) {
let end_of_range = range.1 + max(*end_line as usize, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd find a comment what's happening here useful :) Together with a comment that this is another upstream divergence

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely makes sense to put the comment here, rather than on the test!

if main_range >= range.0 && main_range < end_of_range {
let char_column = text[0..(main_range - range.0).min(text.len())]
.chars()
.count();
col = char_column + 1;
line_offset = lineno.unwrap_or(1);
break;
} else if main_range == end_of_range {
line_offset = lineno.map_or(1, |line| line + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the +1 because the line is zero indexed or because we want to point to the next line.

I think it would also be okay to say last_line:after_last_column which would better align with how the snippet is rendered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the +1 is to point to the next line to match Ruff's current behavior:

ISC_syntax_error.py:30:1: invalid-syntax: unexpected EOF while parsing
|
28 | "i" "j"
29 | )
| ^
|

I agree with you that 29:2 would make sense and more closely align with the rendering, though.

One issue I ran into here was that (naively) computing the column breaks some other annotate-snippets tests. Maybe that's a sign that it's not the right fix.

I'll look at this a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a more robust fix working, but this will also be a mismatch from the concise rendering. Should I update that as well? It looks like those numbers come from our LineIndex code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing LineIndex does sound reasonable if that's where the numbers are coming from. But I don't have a good sense of the blast radius. You might have to try to see what changes (Updating concise makes sense to me, we want the line numbers to match across formats)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I definitely want them to match, I was more wondering if you'd rather align on this new behavior or preserve the old behavior. I'll have to double check the other output formats too. Hopefully they all use the line index. I guess full is the only case where the output is noticeably questionable with the caret on a different line from what the range reports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer aligning on the new behavior. I think the existing behavior is even confusing in the context of the new newline at end of file because it suggests that there's a newline (which obviously there isn't)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference, we discussed this on Discord and decided to move ahead with preserving the old behavior for now. This seems like the same, or a closely-related, issue as #15510, so we can follow-up on resolving the header-rendering mismatch separately. I added some notes there (#15510 (comment)) from looking into it today too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Thank you for looking into it so carefully! Let's merge :)

break;
}
}
}
Expand Down
23 changes: 22 additions & 1 deletion crates/ruff_db/src/diagnostic/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,22 @@ impl<'r> RenderableSnippet<'r> {
.as_source_code()
.slice(TextRange::new(snippet_start, snippet_end));

// Strip the BOM from the beginning of the snippet, if present. Doing this here saves us the
// trouble of updating the annotation ranges in `replace_unprintable`, and also allows us to
// check that the BOM is at the very beginning of the file, not just the beginning of the
// snippet.
const BOM: char = '\u{feff}';
let bom_len = BOM.text_len();
let (snippet, snippet_start) =
if snippet_start == TextSize::default() && snippet.starts_with(BOM) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very very minor, but it might be nice if we had a TextSize::ZERO constant or something. Using default() for this feels a little funny.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do TextSize::new(0) but agree that ZERO would be nice`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! I think ONE might be helpful too, but not in this PR.

(
&snippet[bom_len.to_usize()..],
snippet_start + TextSize::new(bom_len.to_u32()),
)
} else {
(snippet, snippet_start)
};

let annotations = anns
.iter()
.map(|ann| RenderableAnnotation::new(snippet_start, ann))
Expand Down Expand Up @@ -1000,7 +1016,12 @@ fn replace_unprintable<'r>(
let mut last_end = 0;
let mut result = String::new();
for (index, c) in source.char_indices() {
if let Some(printable) = unprintable_replacement(c) {
// normalize `\r` line endings but don't double `\r\n`
if c == '\r' && !matches!(source.get(index + 1..index + 2), Some("\n")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I like doing here is &source[index + 1..].starts_width('\n')

result.push_str(&source[last_end..index]);
result.push('\n');
last_end = index + 1;
} else if let Some(printable) = unprintable_replacement(c) {
result.push_str(&source[last_end..index]);

let len = printable.text_len().to_u32();
Expand Down
84 changes: 83 additions & 1 deletion crates/ruff_db/src/diagnostic/render/full.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#[cfg(test)]
mod tests {
use ruff_diagnostics::Applicability;
use ruff_text_size::TextRange;
use ruff_text_size::{TextLen, TextRange, TextSize};

use crate::diagnostic::{
Annotation, DiagnosticFormat, Severity,
Expand Down Expand Up @@ -400,4 +400,86 @@ print()
help: Remove `print` statement
");
}

/// Carriage return (`\r`) is a valid line-ending in Python, so we should normalize this to a
/// line feed (`\n`) for rendering. Otherwise we report a single long line for this case.
#[test]
fn normalize_carriage_return() {
let mut env = TestEnvironment::new();
env.add(
"example.py",
"# Keep parenthesis around preserved CR\rint(-\r 1)\rint(+\r 1)",
);
env.format(DiagnosticFormat::Full);

let mut diagnostic = env.err().build();
let span = env
.path("example.py")
.with_range(TextRange::at(TextSize::new(39), TextSize::new(0)));
let annotation = Annotation::primary(span);
diagnostic.annotate(annotation);

insta::assert_snapshot!(env.render(&diagnostic), @r"
error[test-diagnostic]: main diagnostic message
--> example.py:2:1
|
1 | # Keep parenthesis around preserved CR
2 | int(-
| ^
3 | 1)
4 | int(+
|
");
}

/// Without stripping the BOM, we report an error in column 2, unlike Ruff.
#[test]
fn strip_bom() {
let mut env = TestEnvironment::new();
env.add("example.py", "\u{feff}import foo");
env.format(DiagnosticFormat::Full);

let mut diagnostic = env.err().build();
let span = env
.path("example.py")
.with_range(TextRange::at(TextSize::new(3), TextSize::new(0)));
let annotation = Annotation::primary(span);
diagnostic.annotate(annotation);

insta::assert_snapshot!(env.render(&diagnostic), @r"
error[test-diagnostic]: main diagnostic message
--> example.py:1:1
|
1 | import foo
| ^
|
");
}

/// We previously rendered this correctly, but the header was falling back to 1:1 for ranges
/// pointing to the final newline in a file. Like Ruff, we now use the offset of the first
/// character in the nonexistent final line in the header.
#[test]
fn end_of_file() {
let mut env = TestEnvironment::new();
let contents = "unexpected eof\n";
env.add("example.py", contents);
env.format(DiagnosticFormat::Full);

let mut diagnostic = env.err().build();
let span = env
.path("example.py")
.with_range(TextRange::at(contents.text_len(), TextSize::new(0)));
let annotation = Annotation::primary(span);
diagnostic.annotate(annotation);

insta::assert_snapshot!(env.render(&diagnostic), @r"
error[test-diagnostic]: main diagnostic message
--> example.py:2:1
|
1 | unexpected eof
| ^
|
");
}
}
Loading