Skip to content

rollback tweak to empty spans generated by some lints #15509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BurntSushi opened this issue Jan 15, 2025 · 4 comments
Closed

rollback tweak to empty spans generated by some lints #15509

BurntSushi opened this issue Jan 15, 2025 · 4 comments
Labels
bug Something isn't working diagnostics Related to reporting of diagnostics. internal An internal refactor or improvement

Comments

@BurntSushi
Copy link
Member

In #15359, in order to upgrade to the latest version of annotate-snippets, we ended up tweaking the spans generated by some lints. The essential problem is described in an upstream issue: rust-lang/annotate-snippets-rs#176

To work around this, instead of generating an empty span immediately after a line terminator, we instead generate a span that is one character wide immediately after a line terminator. The former would point to the end of the preceding line, but the latter points to the beginning of the following line (which is what we want).

#15359 did this in the following places:

  • crates/ruff_linter/src/checkers/logical_lines.rs
  • crates/ruff_linter/src/rules/pydocstyle/rules/indent.rs

Then look for uses of ceil_char_boundary, which was a new method added to Locator to facilitate the creation of these ranges.

I think ideally, we would fix annotate-snippets to render empty spans after a line terminator to point to the beginning of the following line. I did attempt to do this in our vendored copy of annotate-snippets, but any change I tried ended up regressing a bunch of other cases. So I think in order to fix this for real, more investment will be needed to better understand how annotate-snippets's renderer works.

@BurntSushi BurntSushi added the internal An internal refactor or improvement label Jan 15, 2025
@MichaReiser MichaReiser added bug Something isn't working diagnostics Related to reporting of diagnostics. labels Jan 15, 2025
@MichaReiser
Copy link
Member

Copying my comment from the PR to add some context why I think this is a bug


Okay, somewhat "bad" news. The new diagnostic ranges do regress the LSP experience. It's not in a significant way that I think is worth blocking this PR but it is reason enough that we should change annotation-snippet to support empty ranges pointing at the start of a line. This isn't something we have to follow up on immediately but I consider it "serious" enough that it's something we should act on in the coming months.

I tested it with E115 using

if False:  
print()

Before

The quick fix was only shown when the cursor is at the start of the line:

if False:  
print()
^-- cursor has to be here

Now

The quick fix is now shown if the cursor is at the start of the line or between p and r. This is incorrect because the action doesn't make sense at that position:

if False:  
print()
^ ---- cursor can be here
 ^ ----- or here

@BurntSushi
Copy link
Member Author

@MichaReiser Hah you beat me to it. I was just going to copy that here.

@BurntSushi
Copy link
Member Author

Note that I moved the span fixup to be right before rendering and it applies to all diagnostics:

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 locator = Locator::new(&self.text);
        let start = self.annotation_range.start();
        let end = locator.ceil_char_boundary(start + TextSize::from(1));
        SourceCode {
            annotation_range: TextRange::new(start, end),
            ..self
        }
    }
}

This doesn't help with the regression in the Python parser diagnostic, but this might help with the LSP regression since we no longer change the spans in the lint, but only right before rendering.

@BurntSushi
Copy link
Member Author

Given that the LSP regression is fixed and this is fixed at the level of rendering (which is right before the call out to annotate-snippets), I think it's safe to call this issue fixed. I think it would be nice to not need to fix up the spans by fixing this properly in annotate-snippets, but the current approach shouldn't have any user visible downsides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working diagnostics Related to reporting of diagnostics. internal An internal refactor or improvement
Projects
None yet
Development

No branches or pull requests

2 participants