Skip to content

Commit

Permalink
ruff_linter: adjust empty spans after line terminator more generally
Browse files Browse the repository at this point in the history
Instead of doing this on a lint-by-lint basis, we now just do it right
before rendering. This is more broadly applicable.

Note that this doesn't fix the diagnostic rendering for the Python
parser. But that's using a different path anyway (`annotate-snippets` is
only used in tests).
  • Loading branch information
BurntSushi committed Jan 15, 2025
1 parent 458153d commit 282de25
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 42 deletions.
10 changes: 2 additions & 8 deletions crates/ruff_linter/src/checkers/logical_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_python_parser::{TokenKind, Tokens};
use ruff_source_file::LineRanges;
use ruff_text_size::{Ranged, TextRange, TextSize};
use ruff_text_size::{Ranged, TextRange};

use crate::line_width::IndentWidth;
use crate::registry::{AsRule, Rule};
Expand Down Expand Up @@ -161,13 +161,7 @@ pub(crate) fn check_logical_lines(
let range = if first_token.kind() == TokenKind::Indent {
first_token.range()
} else {
let mut range =
TextRange::new(locator.line_start(first_token.start()), first_token.start());
if range.is_empty() {
let end = locator.ceil_char_boundary(range.start() + TextSize::from(1));
range = TextRange::new(range.start(), end);
}
range
TextRange::new(locator.line_start(first_token.start()), first_token.start())
};

let indent_level = expand_indent(locator.slice(range), settings.tab_size);
Expand Down
39 changes: 38 additions & 1 deletion crates/ruff_linter/src/message/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::line_width::{IndentWidth, LineWidthBuilder};
use crate::message::diff::Diff;
use crate::message::{Emitter, EmitterContext, Message};
use crate::settings::types::UnsafeFixes;
use crate::Locator;

bitflags! {
#[derive(Default)]
Expand Down Expand Up @@ -247,7 +248,8 @@ impl Display for MessageCodeFrame<'_> {
let source = replace_whitespace_and_unprintable(
source_code.slice(TextRange::new(start_offset, end_offset)),
self.message.range() - start_offset,
);
)
.fix_up_empty_spans_after_line_terminator();

let label = self
.message
Expand Down Expand Up @@ -365,6 +367,41 @@ struct SourceCode<'a> {
annotation_range: TextRange,
}

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
}
}
}

#[cfg(test)]
mod tests {
use insta::assert_snapshot;
Expand Down
39 changes: 6 additions & 33 deletions crates/ruff_linter/src/rules/pydocstyle/rules/indent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,20 +223,8 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) {
// We report under-indentation on every line. This isn't great, but enables
// fix.
if (is_last || !is_blank) && line_indent_size < docstring_indent_size {
// Previously, this used `TextRange::empty(line.start())`,
// but this creates an offset immediately after the line
// terminator. Probably, our renderer should create an
// annotation that points to the beginning of the following
// line. But it doesn't at present and this have proved
// difficult to fix without regressing other cases. So for now,
// we work around this by creating a range that points at the
// first codepoint in the corresponding line. This makes the
// renderer do what we want. ---AG
let start = line.start();
let end = checker
.locator()
.ceil_char_boundary(start + TextSize::from(1));
let mut diagnostic = Diagnostic::new(UnderIndentation, TextRange::new(start, end));
let mut diagnostic =
Diagnostic::new(UnderIndentation, TextRange::empty(line.start()));
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
clean_space(docstring.indentation),
TextRange::at(line.start(), line_indent.text_len()),
Expand Down Expand Up @@ -293,16 +281,8 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) {

// We report over-indentation on every line. This isn't great, but
// enables the fix capability.
//
// Also, we ensure that our range points to the first character
// of the line instead of the empty spance immediately
// preceding the line. See above for how we handle under
// indentation for more explanation. ---AG
let start = line.start();
let end = checker
.locator()
.ceil_char_boundary(start + TextSize::from(1));
let mut diagnostic = Diagnostic::new(OverIndentation, TextRange::new(start, end));
let mut diagnostic =
Diagnostic::new(OverIndentation, TextRange::empty(line.start()));

let edit = if indent.is_empty() {
// Delete the entire indent.
Expand Down Expand Up @@ -344,15 +324,8 @@ pub(crate) fn indent(checker: &mut Checker, docstring: &Docstring) {

let is_indent_only = line_indent.len() == last.len();
if last_line_over_indent > 0 && is_indent_only {
// We ensure that our range points to the first character of
// the line instead of the empty spance immediately preceding
// the line. See above for how we handle under indentation for
// more explanation. ---AG
let start = last.start();
let end = checker
.locator()
.ceil_char_boundary(start + TextSize::from(1));
let mut diagnostic = Diagnostic::new(OverIndentation, TextRange::new(start, end));
let mut diagnostic =
Diagnostic::new(OverIndentation, TextRange::empty(last.start()));
let indent = clean_space(docstring.indentation);
let range = TextRange::at(last.start(), line_indent.text_len());
let edit = if indent.is_empty() {
Expand Down

0 comments on commit 282de25

Please sign in to comment.