Skip to content
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

fix: Tweak rendering of source when it's a single ASCII whitespace #169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BurntSushi
Copy link
Member

This PR includes a small fix (that I am not at all sure about) and a
regression test that fails on current master.

The input here is just a single ASCII whitespace with an annotation
pointing to immediately after that whitespace. In current master, it
gets rendered like this:

error: missing trailing newline at end of file
  |
1 | ...
  | ^ W292
  |
  = help: Add trailing newline

But I think the insertion of an ellipsis here is not quite right. I was
certainly confused by the output.

I don't really understand the formatting code at all, but I believe the
ellipsis is being inserted by this code:

if self.margin.was_cut_left() {
    // We have stripped some code/whitespace from the beginning, make it clear.
    buffer.puts(line_offset, code_offset, "...", *lineno_color);
}

And self.margin.was_cut_left() was returning true because
computed_left was set to 18446744073709551593. This kind of value
seems like a bug, although some margin values are explicitly
initialized to usize::MAX, so maybe not.

Anywho, I tried removing the condition gating the setting of
whitespace_margin, and that seems to fix this specific case without
any other known regressions (including across all of ruff's test
suite), but this was mostly a result of me feeling around in the dark
here.

This PR includes a small fix (that I am not at all sure about) and a
regression test that fails on current master.

The input here is just a single ASCII whitespace with an annotation
pointing to immediately after that whitespace. In current master, it
gets rendered like this:

```
error: missing trailing newline at end of file
  |
1 | ...
  | ^ W292
  |
  = help: Add trailing newline
```

But I think the insertion of an ellipsis here is not quite right. I was
certainly confused by the output.

I don't really understand the formatting code at all, but I believe the
ellipsis is being inserted by this code:

```rust
if self.margin.was_cut_left() {
    // We have stripped some code/whitespace from the beginning, make it clear.
    buffer.puts(line_offset, code_offset, "...", *lineno_color);
}
```

And `self.margin.was_cut_left()` was returning true because
`computed_left` was set to `18446744073709551593`. This kind of value
_seems_ like a bug, although some margin values are explicitly
initialized to `usize::MAX`, so maybe not.

Anywho, I tried removing the condition gating the setting of
`whitespace_margin`, and that seems to fix this specific case without
any other _known_ regressions (including across all of `ruff`'s test
suite), but this was mostly a result of me feeling around in the dark
here.
@BurntSushi BurntSushi force-pushed the ag/handle-single-space branch from 910bfef to b1c4112 Compare January 8, 2025 16:41
BurntSushi added a commit to astral-sh/ruff that referenced this pull request Jan 8, 2025
…ource

The change to the rendering code is elaborated on in more detail here,
where I attempted to upstream it:
rust-lang/annotate-snippets-rs#169

Otherwise, the snapshot diff also shows a bug fix: a `^` is now rendered
where as it previously was not.
BurntSushi added a commit to astral-sh/ruff that referenced this pull request Jan 8, 2025
…ource

The change to the rendering code is elaborated on in more detail here,
where I attempted to upstream it:
rust-lang/annotate-snippets-rs#169

Otherwise, the snapshot diff also shows a bug fix: a `^` is now rendered
where as it previously was not.
BurntSushi added a commit to astral-sh/ruff that referenced this pull request Jan 8, 2025
…ource

The change to the rendering code is elaborated on in more detail here,
where I attempted to upstream it:
rust-lang/annotate-snippets-rs#169

Otherwise, the snapshot diff also shows a bug fix: a `^` is now rendered
where as it previously was not.
@BurntSushi BurntSushi changed the title renderer: fix rendering of source when it's a single ASCII whitespace fix: Tweak rendering of source when it's a single ASCII whitespace Jan 8, 2025
BurntSushi added a commit to astral-sh/ruff that referenced this pull request Jan 9, 2025
…ource

The change to the rendering code is elaborated on in more detail here,
where I attempted to upstream it:
rust-lang/annotate-snippets-rs#169

Otherwise, the snapshot diff also shows a bug fix: a `^` is now rendered
where as it previously was not.
BurntSushi added a commit to astral-sh/ruff that referenced this pull request Jan 10, 2025
…ource

The change to the rendering code is elaborated on in more detail here,
where I attempted to upstream it:
rust-lang/annotate-snippets-rs#169

Otherwise, the snapshot diff also shows a bug fix: a `^` is now rendered
where as it previously was not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant