Vendor latest annotate-snippets - #27033
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 96.84%. The percentage of expected errors that received a diagnostic held steady at 92.11%. The number of fully passing files held steady at 99/133. |
Memory usage reportMemory usage unchanged ✅ |
|
|
5bfbeb6 to
ddbb335
Compare
| ----- stdout ----- | ||
| unformatted: File would be reformatted | ||
| --> CRATE_ROOT/resources/test/fixtures/unformatted.ipynb:cell 1:1:1 | ||
| --> CRATE_ROOT/resources/test/fixtures/unformatted.ipynb:cell 1:2:1 |
| 6 | class FormFeedIndent: | ||
| 7 | def __init__(self, a=[]): | ||
| | ^^ | ||
| 7 | ␌ def __init__(self, a=[]): |
| --> has_syntax_error.py:1:1 | ||
| --> has_syntax_error.py:2:1 | ||
| | | ||
| 2 | | ||
| | ^ | ||
|
|
There was a problem hiding this comment.
While more correct, this may degrade #24528
| 3 | elf: | ||
| 4 | pass | ||
| | ^ Syntax Error: Expected a statement | ||
| 5 | else: | ||
| | ^ Syntax Error: Expected a statement | ||
| 6 | pass |
There was a problem hiding this comment.
The span is between the \n and e, so this is the more correct placement but unsure what the intended placement is
|
I'm a bit confused by the changes in Bless added pipes and Bless removed pipes. It's not clear what's different between the pipes that were removed/added. Overall, it might be helpful to give a short explanation for each of those blessed commits. E.g. why did the locations in Bless origin location changes. Were they incorrect, why? Same for added/removed pipes. Why were they removed/added? Were they incorrect? |
| info: This violates the Liskov Substitution Principle | ||
| help: It is recommended for `__eq__` to work with arbitrary objects, for example: | ||
| help | ||
| help: |
There was a problem hiding this comment.
This now renders with a trailing whitespace. Any chance we can avoid printing the whitespace if not followed by a text?
There was a problem hiding this comment.
I would prefer to keep this as-is and instead open a follow up issue to switch this to using a Snippet to render it. If it is important enough to this PR, I could instead make that change before hand.
Looking at it closer, it appears that the "added" commit just undoes some of the "removed" commit. I've squashed them. |
| --> lib/module.py:1:1 | ||
| --> lib/module.py:1:1 | ||
| | | ||
| 1 | |
There was a problem hiding this comment.
Hmm, I think this is some special casing that @ntBre introduced to prevent this. I don't remember why we did that. Is there another way of disabling rendering of the snippet (at a diagnostic level)?
There was a problem hiding this comment.
Looks like we should call hide_snippet in places where we don't want to show an empty span.
There was a problem hiding this comment.
I looked through each case
- One is that the annotated line is empty
- The rest are when a snippet is being shown for context but the source for that snippet is empty
In each case, I think this PR is doing the right thing in showing these snippets.
Two potential improvements that I think are reasonable to defer are
- Showing an ␃ for empty spans at the end of a file
- Showing a line of context before an error on an empty line
If there is a reason we should special case to hide empty lines, I would appreciate better understanding it
There was a problem hiding this comment.
Hmm, I think this is some special casing that @ntBre introduced to prevent this. I don't remember why we did that.
Also, I dug through the history and changes, and didn't see what this was referring to.
There was a problem hiding this comment.
Oh, you might mean the file-level stuff.
And looking closer, some of those look like they should be file level (go-to-definition for a module takes you to the module)
There was a problem hiding this comment.
Oh, yeah, these file-level snippets are for an internal-only render to visualize IDE actions so it doesn't really matter.
| 7 | | ||
| | ^ Syntax Error: Expected a statement | ||
| 8 | a = 10 | ||
| | ^ Syntax Error: Expected a statement |
There was a problem hiding this comment.
I'd have to take a look at the parser but this certainly is confusing now. Because this is the start of a statement.
There was a problem hiding this comment.
This seems accurate for what span the parser picks. I'd be nice if we can use a more meaningful range (or omit this error all together, if it always follow another error). If you're interested, feel free to take a look at it. I otherwise suggest that we open a follow up issue for this.
We could also consider slihgtly changing the wording, if that helps
There was a problem hiding this comment.
Wow, #27170 removed all but one of these
|
Codex explained to me that the line changes were cases where the diagnostic incorrectly used the range of a secondary annotation. Whoops. Nice catch.
|
|
This looks great overall. Thank you |
78e5fe0 implemented support for hiding severities from upstream annotate-snippets. Likely, this was just an implementation oversight upstream but the exact details of what it should look like upstream are TBD.
I believe the new rendered is collapsing the trailing padding line with the annotation line.
The origin line and column is now determined by the primary annotation.
Reasons - Primary now has precedence over context when overlapping - The primary now always has the same symbol, independent of level
The old renderer allowed the snipper to go past the term width.
The annotation is between the newline and the first character, so this is the more correct place to render it.
When to go past the end-of-line to represent the newline has changed. The two renderers diverged in their newline behavior and I tried to restore the old behavior as much as possible and I hit my timebox for getting this exactly right since this didn't seem the most important to get perfectly the same.
|
I'm going to go ahead and merge and continue to iterate as this is conflict heavy. If someone has a concern with this, we can revert. |
Summary
This is a first step towards #20411. After this, we would work to either upstream changes or adjust things within Ruff.
Performance improvements have already been upstreamed (rust-lang/annotate-snippets-rs#437, rust-lang/annotate-snippets-rs#440)
Re-implemented the following from the fork:
Originrust-lang/annotate-snippets-rs#448)Renderer::plain()rust-lang/annotate-snippets-rs#393)lineno_offset(maybe not needed if Move diff rendering intoannotate-snippets#20648 is taken care of)Additional hacks to minimize behavior change include
Levelshouldn't hide the label rust-lang/annotate-snippets-rs#452)simdby defaultAll of the above represents work that needs to be resolved to address #20411.
lib.namehack is used to simplify maintenance with the assumption that we'll continue working towards #20411.Behavior changes that were included
ruff format --checkoutputs wrong line line/column numbers on syntax errors #24528)New or upcoming features annotate-snippets features that we will be able to leverage by moving this forward
AnnotationKind::Visibleto improve the quality of built-in code-folding if Ruff were to switch to itDecorStyle::UnicodeDeferred:
Groups into the sameReportsecondary_titlefor anyGroupafter the first in aReportSnippet, instead of titles, for "virtual code" like Vendor latest annotate-snippets #27033 (comment)OriginTest Plan
Within cpython, I ran
Before:

After:
