diagnostic rendering for import lint I001 is sub-optimal in some cases

[BurntSushi]

On current main, here's what one example of a I001 diagnostic looks like:

lines_after_imports.pyi:1:1: I001 [*] Import block is un-sorted or un-formatted
   |
 1 | / from __future__ import annotations
 2 | |
 3 | | from typing import Any
 4 | |
 5 | | from requests import Session
 6 | |
 7 | | from my_first_party import my_first_party_object
 8 | |
 9 | | from . import my_local_folder_object
10 | |
11 | |
12 | |
13 | | class Thing(object):
   | |_^ I001
14 |     name: str
15 |     def __init__(self, name: str):
   |
   = help: Organize imports

In particular, the above comes from this snapshot:

https://github.com/astral-sh/ruff/blob/55a7f72035390cf1093e4da0cf2462e793bfbce1/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__lines_after_imports.pyi.snap

In the above rendering, the ^ is pointing to a class definition, but it should be pointing to an import statement. After #15359 is merged, the rendering will improve somewhat to this:

lines_after_imports.pyi:1:1: I001 [*] Import block is un-sorted or un-formatted
   |
 1 | / from __future__ import annotations
 2 | |
 3 | | from typing import Any
 4 | |
 5 | | from requests import Session
 6 | |
 7 | | from my_first_party import my_first_party_object
 8 | |
 9 | | from . import my_local_folder_object
10 | |
11 | |
12 | |
   | |__^ I001
13 |   class Thing(object):
14 |     name: str
15 |     def __init__(self, name: str):
   |
   = help: Organize imports

This is better, but still not ideal. However, this isn't a problem in the diagnostic rendering itself (which is what #15359 didn't address it), but rather, in the spans generated by the I001 lint. To demonstrate, this is a minimal reproducer I made by debug printing the annotate-snippets::Message when running the above test:

use annotate_snippets::{Level, Renderer, Snippet};

fn main() {
    let source = "from __future__ import annotations\n\nfrom typing import Any\n\nfrom requests import Session\n\nfrom my_first_party import my_first_party_object\n\nfrom . import my_local_folder_object\n\n\n\nclass Thing(object):\n  name: str\n  def __init__(self, name: str):\n";
    let src_annotation = Level::Error.span(0..180).label("I001");
    let snippet =
        Snippet::source(source).line_start(1).annotation(src_annotation);
    let message = Level::Error.title("").snippet(snippet);

    println!("{}", Renderer::plain().render(message));
}

This outputs the same rendering as above. And the span, 0..180, corresponds to this substring:

from __future__ import annotations\n\nfrom typing import Any\n\nfrom requests import Session\n\nfrom my_first_party import my_first_party
_object\n\nfrom . import my_local_folder_object\n\n\n\n

So whatever is generating the spans here is including those empty lines at the end. We should fix the span generation to be tighter. (It's perhaps as simple as trimming the trailing lines from the range.)

[dylwil3]

Edit: Ignore this, see comment below

This is confusing because if I add a debug print to display the line number, it seems to give the right answer for the range emitted from the lint:

> cargo run -p ruff -- check tmp.py --no-cache --preview --isolated --select I001
[crates/ruff_linter/src/rules/isort/rules/organize_imports.rs:96:5] &range = 0..176
[crates/ruff_linter/src/rules/isort/rules/organize_imports.rs:97:5] locator.compute_source_location(range.end()) = SourceLocation {
    row: 9,
    column: 37,
}
tmp.py:1:1: I001 [*] Import block is un-sorted or un-formatted
   |
 1 | / from __future__ import annotations
 2 | |
 3 | | from typing import Any
 4 | |
 5 | | from requests import Session
 6 | |
 7 | | from my_first_party import my_first_party_object
 8 | |
 9 | | from . import my_local_folder_object
10 | |
11 | |
12 | |
   | |__^ I001
13 |   class Foo:...
   |
   = help: Organize imports

Found 1 error.
[*] 1 fixable with the `--fix` option.

Is there some transformation done to the range before it gets to the diagnostic/user?

[MichaReiser]

annotated-snippet determines where to draw the arrow. It's quiet possible that there's a bug if the range points to the end of the line?

[dylwil3]

ah, actually my mistake - I was printing the range from the "early return" diagnostic. there's additional logic in the lint that makes a different range, which explicitly includes trailing whitespace:

ruff/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs

Lines 135 to 136 in c034e28

	// Expand the span the entire range, including leading and trailing space.
	let range = TextRange::new(locator.line_start(range.start()), trailing_line_end);

It looks like this choice is related to implementing the fix.