Account for removal of multiline span in suggestion by estebank · Pull Request #134664 · rust-lang/rust

estebank · 2024-12-22T20:21:08Z

When highlighting the removed parts of a suggestion, properly account for spans that cover more than one line.

rustbot · 2024-12-22T20:21:16Z

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

jieyouxu · 2024-12-22T20:35:07Z

r? jieyouxu

jieyouxu

Thanks! The removal highlight does indeed looks much nicer. I left a few questions, but LGTM otherwise.

jieyouxu · 2024-12-23T11:03:54Z

compiler/rustc_errors/src/emitter.rs

+                            // too bad to begin with, so we side-step that issue here.
+                            for (i, line) in snippet.lines().enumerate() {
+                                let line = normalize_whitespace(line);
+                                let row = row_num - 2 - (newlines - i - 1);


Question [OFFSETS 1/2]: what's the significance of row_num - 2? There's several +/- n in this region of code which are not immediately obvious (modulo the - 1 cases), but this one in particular is somewhat mysterious (at least to me).

row_num at this point corresponds to

| LL | CODE | ++++ <- this row

in the buffer. When we have a diff format output, we end up with

| LL - OLDER LL + NEWER | <- this row

The row_num - 2 is to select the buffer line that has the "old version of the diff" at that point. When the removal is a single line, i is 0, newlines is 1 so (newlines - i - 1) ends up being 0, so row points at LL - OLDER. When the removal corresponds to multiple lines, we end up with newlines > 1 and i being 0..newlines - 1.

| LL - OLDER LL - CODE LL - BEING LL - REMOVED LL + NEWER | <- this row

How should I communicate this better in a comment?

Honestly? I can't think of any shorter explanation that's also unambiguous. I would just copy-pasta what you said as a comment, even at the risk of the comment becoming outdated. Because until you explained this to me (thank you), I don't think anyone else would've been able to interpret this without extensive local testing...

jieyouxu · 2024-12-23T11:07:27Z

compiler/rustc_errors/src/emitter.rs

+                        } else {
+                            // The removed code fits all in one line.
+                            buffer.set_style_range(
+                                row_num - 2,


Question [OFFSETS 2/2]: same here, what's the significance of row_num - 2?

We can probably unify the two branches. Do note that this is the previously existing code being moved around.

Yeah I see that, it's just that this one was equally mysterious lol.

jieyouxu · 2024-12-23T11:08:08Z

tests/ui/error-emitter/multiline-removal-suggestion.rs

+// ignore-tidy-tab
+use std::collections::{HashMap, HashSet};
+fn foo() -> Vec<(bool, HashSet<u8>)> {
+	let mut hm = HashMap::<bool, Vec<HashSet<u8>>>::new();


Question: is there a specific reason why the indents in this file use tabs?

To ensure that the code normalization logic still kicks in correctly, given that I'm doing length calculations involving the original code. \t gets always replaced with .

Could you add that as a comment in the test? I imagine someone else who looked at this test might notice the tabs and go "huh?" because other ui tests typically only use spaces for indent purposes.

jieyouxu · 2024-12-23T11:09:15Z

tests/ui/error-emitter/multiline-removal-suggestion.svg

Remark: the svg snapshots are very nice :3 The removal span highlighting does indeed look much nicer with the changes in this PR!

jieyouxu · 2024-12-24T06:13:51Z

Thanks, you can r=me after some commit nits.
@rustbot author

jieyouxu · 2024-12-26T07:21:12Z

Thanks! The comments made the logic much easier to follow.

jieyouxu · 2024-12-26T07:21:32Z

@bors r+ rollup

bors · 2024-12-26T07:21:35Z

📌 Commit 5c818c3 has been approved by jieyouxu

It is now in the queue for this repository.

…ouxu Account for removal of multiline span in suggestion When highlighting the removed parts of a suggestion, properly account for spans that cover more than one line. Fix rust-lang#134485. ![Screenshot of the highlighted output](https://github.com/user-attachments/assets/18bcd9bc-3bec-4b79-a9d7-e4ea4e6289ad)

Rollup of 4 pull requests Successful merges: - rust-lang#134664 (Account for removal of multiline span in suggestion) - rust-lang#134774 (fix default-backtrace-ice test) - rust-lang#134781 (Add more `begin_panic` normalizations to panic backtrace tests) - rust-lang#134784 (Miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2024-12-26T13:10:57Z

Failed on msvc in rollup.
@bors r-

jieyouxu · 2024-12-26T13:25:03Z

FTR this does not fail locally on msvc for me...

jieyouxu · 2024-12-26T13:30:43Z

Ok nvm I know exactly what the failure is...

{closure@C:\a\rust\rust\tests\ui\error-emitter\multiline-removal-suggestion.rs:30:10: 30:13}

it's #132752 that I haven't gotten around to fixing, will take a look tmrw

estebank · 2024-12-26T17:29:46Z

That's a great find. I can also mark the test as linux only for now.

When highlighting the removed parts of a suggestion, properly account for spans that cover more than one line. Fix rust-lang#134485.

estebank · 2024-12-26T17:42:31Z

@bors r=jieyouxu

bors · 2024-12-26T17:42:34Z

📌 Commit 12d66d9 has been approved by jieyouxu

It is now in the queue for this repository.

…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#134656 (Migrate `incr-add-rust-src-component` to rmake) - rust-lang#134664 (Account for removal of multiline span in suggestion) - rust-lang#134772 (Improve/cleanup rustdoc code) - rust-lang#134781 (Add more `begin_panic` normalizations to panic backtrace tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134664 - estebank:sugg-highlighting, r=jieyouxu Account for removal of multiline span in suggestion When highlighting the removed parts of a suggestion, properly account for spans that cover more than one line. Fix rust-lang#134485. ![Screenshot of the highlighted output](https://github.com/user-attachments/assets/18bcd9bc-3bec-4b79-a9d7-e4ea4e6289ad)

…ouxu Account for removal of multiline span in suggestion When highlighting the removed parts of a suggestion, properly account for spans that cover more than one line. Fix rust-lang#134485. ![Screenshot of the highlighted output](https://github.com/user-attachments/assets/18bcd9bc-3bec-4b79-a9d7-e4ea4e6289ad)

…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#134656 (Migrate `incr-add-rust-src-component` to rmake) - rust-lang#134664 (Account for removal of multiline span in suggestion) - rust-lang#134772 (Improve/cleanup rustdoc code) - rust-lang#134781 (Add more `begin_panic` normalizations to panic backtrace tests) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2025-01-08T22:11:37Z

Ok nvm I know exactly what the failure is...

Actually I was wrong, because I looked at the unnormalized svg... The real reason I believe this:

I believe we use different colors between Unixes and Windows (Windows have some legacy workarounds to use different bright colors and such because the cmd.exe bright colors are ass).

@estebank lol

rustbot assigned Nadrieril Dec 22, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 22, 2024

estebank marked this pull request as ready for review December 22, 2024 20:21

This comment has been minimized.

Sign in to view

rustbot assigned jieyouxu and unassigned Nadrieril Dec 22, 2024

estebank force-pushed the sugg-highlighting branch from f401572 to 0edaf9a Compare December 22, 2024 20:41

This comment has been minimized.

Sign in to view

estebank force-pushed the sugg-highlighting branch from 0edaf9a to a42e529 Compare December 22, 2024 21:02

jieyouxu approved these changes Dec 23, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 24, 2024

estebank force-pushed the sugg-highlighting branch from a42e529 to d3d86a3 Compare December 25, 2024 23:37

This comment has been minimized.

Sign in to view

estebank force-pushed the sugg-highlighting branch from d3d86a3 to 5c818c3 Compare December 26, 2024 00:32

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 26, 2024

jieyouxu mentioned this pull request Dec 26, 2024

Rollup of 4 pull requests #134785

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 26, 2024

jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 26, 2024

Account for removal of multiline span in suggestion

12d66d9

When highlighting the removed parts of a suggestion, properly account for spans that cover more than one line. Fix rust-lang#134485.

estebank force-pushed the sugg-highlighting branch from 5c818c3 to 12d66d9 Compare December 26, 2024 17:41

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 26, 2024

GuillaumeGomez mentioned this pull request Dec 26, 2024

Rollup of 4 pull requests #134795

Merged

bors merged commit 88687d4 into rust-lang:master Dec 26, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 26, 2024

jieyouxu mentioned this pull request Jan 8, 2025

compiletest: error emitter pretty svg tests don't normalize color differences between Windows and Unixes #135270

Open

Uh oh!

Conversation

estebank commented Dec 22, 2024

Uh oh!

rustbot commented Dec 22, 2024

Uh oh!

This comment has been minimized.

jieyouxu commented Dec 22, 2024

Uh oh!

This comment has been minimized.

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Dec 24, 2024

Uh oh!

This comment has been minimized.

jieyouxu commented Dec 26, 2024

Uh oh!

jieyouxu commented Dec 26, 2024

Uh oh!

bors commented Dec 26, 2024

Uh oh!

jieyouxu commented Dec 26, 2024

Uh oh!

jieyouxu commented Dec 26, 2024

Uh oh!

jieyouxu commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank commented Dec 26, 2024

Uh oh!

estebank commented Dec 26, 2024

Uh oh!

bors commented Dec 26, 2024

Uh oh!

Uh oh!

jieyouxu commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jieyouxu left a comment •

edited

Loading

jieyouxu Dec 24, 2024 •

edited

Loading

jieyouxu commented Dec 26, 2024 •

edited

Loading