Account for tabs when highlighting multiline code suggestions #87976

estebank · 2021-08-12T15:53:08Z

Address '\t' case in #87972.

Before:

After:

rust-highfive · 2021-08-12T15:53:11Z

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

m-ou-se · 2021-08-12T16:14:15Z

compiler/rustc_errors/src/emitter.rs

+                            .take(*start)
+                            .map(|ch| match ch {
+                                '\t' => 4,
+                                _ => unicode_width::UnicodeWidthChar::width(ch).unwrap_or(1),


I think this should just be 1. Counted in chars and not in columns.

Current nightly:

This PR:

(The second 'A' is a double-width 'Ａ'.)

Can you verify the current version of the PR against your test cases? Sadly, we don't currently have regression tests for the coloring of the output in our tests :-/

This is what I'm seeing in my local tests:

Yup, the highlighting looks good now!

In particular, I want to confirm that the output of the terminal without emoji support you showed here is reasonable (as I fully expect it to be, because it relies on what we were doing for the multiline case, which looks ok in that screenshot).

Sadly, now I'm seeing other issues :(

Here's the output in four more terminals:

cjgillot · 2021-08-14T13:43:44Z

Can you take care of this PR @m-ou-se?

compiler/rustc_errors/src/lib.rs

estebank · 2021-08-19T16:33:38Z

@m-ou-se do let me know if there's anything left here. I want to avoid this not reaching master before the next beta cutoff.

m-ou-se

I found one more (unlikely) edge case (which is not very important, but not hard to fix), and a have a small comment on the code.

r=me either way

compiler/rustc_errors/src/lib.rs

m-ou-se · 2021-08-19T21:24:57Z

compiler/rustc_errors/src/emitter.rs

+                        max_line_num_len + 3 + start + tabs,
+                        max_line_num_len + 3 + end + tabs,


This assumes the addition/replacement doesn't contain tabs itself. It rarely happens, but it can occur:

Here, the entire closure body up to and including the final } is supposed to be highlighted, but i put some tabs in the string literal which broke it.

Ah, yes. I assumed that we didn't have tabs in the suggestion because I didn't account for cases where snippets from the user include them :-/

I suppose all cases where user snippets are included should be replaced by multispan suggestions. (This specific one is already fixed by #87958, but I think there's quite a few places left where this happens.)

Could you give me the repro case? I think I fixed this, but the suggestions seem to be doing the right thing for me.

With #87958 merged, the test case in the screenshot above doesn't trigger the problem anymore. We'll have to find another case where a bunch of user code gets put verbatim in the suggestion. Grepping for span_to_snippet now. ^^

Found one:

mod a { pub const A: String = String::new(); } fn main() { a :: A; //^ TAB here }

The current code handles this correctly:

estebank · 2021-08-23T15:19:59Z

@bors r=m-ou-se

bors · 2021-08-23T15:20:01Z

📌 Commit 955e913 has been approved by m-ou-se

Rollup of 6 pull requests Successful merges: - rust-lang#87976 (Account for tabs when highlighting multiline code suggestions) - rust-lang#88174 (Clarify some wording in Rust 2021 lint docs) - rust-lang#88188 (Greatly improve limitation handling on parallel rustdoc GUI test run) - rust-lang#88230 (Fix typos “a”→“an”) - rust-lang#88232 (Add notes to macro-not-found diagnostics to point out how things with the same name were not a match.) - rust-lang#88259 (Do not mark `-Z thir-unsafeck` as unsound anymore) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

camsteffen · 2021-09-03T22:05:37Z

I'm getting this when I run ./x.py test src/tools/clippy (line 345 in this PR diff)

thread 'rustc' panicked at 'attempt to subtract with overflow', compiler/rustc_errors/src/lib.rs:347:47

estebank · 2021-09-05T17:48:03Z

@camsteffen it sounds like there might be a malformed Span involved, where the end is earlier than the end or len is smaller than the Span... Although now that I see it the if might be incorrect (we compare prev_hi and cur_lo but then calculate using cur_hi and cur_lo. What are the steps to reproduce this, just running that command?

camsteffen · 2021-09-05T20:18:58Z

Yes I am seeing that error on multiple Clippy UI tests just running that command on 4878034 (and probably current master).

estebank · 2021-09-10T10:32:18Z

I wasn't able to reproduce this :-/

camsteffen · 2021-09-10T15:35:08Z

Well shoot. Must've been an artifact of my local setup. I'm not seeing it either and it hasn't shown up in CI.

rust-highfive assigned cjgillot Aug 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2021

m-ou-se reviewed Aug 12, 2021

View reviewed changes

estebank force-pushed the fix-suggestion-span-coloring branch from 655a555 to 1a519ff Compare August 12, 2021 18:32

This comment has been minimized.

Sign in to view

estebank force-pushed the fix-suggestion-span-coloring branch 2 times, most recently from d8fc003 to e86547e Compare August 13, 2021 13:44

m-ou-se assigned m-ou-se and unassigned cjgillot Aug 14, 2021

m-ou-se reviewed Aug 16, 2021

View reviewed changes

compiler/rustc_errors/src/lib.rs Show resolved Hide resolved

estebank force-pushed the fix-suggestion-span-coloring branch from e86547e to 1cfe7c0 Compare August 17, 2021 15:46

m-ou-se added the P-high High priority label Aug 19, 2021

m-ou-se added this to the 1.56.0 milestone Aug 19, 2021

m-ou-se approved these changes Aug 19, 2021

View reviewed changes

estebank added 4 commits August 23, 2021 11:58

Account for tabs when highlighting multiline code suggestions

75fd1bf

wip

a29a624

Fixes to span locations

5626346

remove unnecessary info!() logging

31d07ed

estebank force-pushed the fix-suggestion-span-coloring branch from 5398b6b to 8f746d2 Compare August 23, 2021 12:42

This comment has been minimized.

Sign in to view

bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 23, 2021

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 23, 2021

This comment has been minimized.

Sign in to view

review comments

955e913

estebank force-pushed the fix-suggestion-span-coloring branch from 8f746d2 to 955e913 Compare August 23, 2021 14:32

This comment has been minimized.

Sign in to view

m-ou-se mentioned this pull request Aug 23, 2021

Rollup of 6 pull requests #88265

Merged

bors merged commit 70aec8d into rust-lang:master Aug 23, 2021

estebank mentioned this pull request Aug 27, 2021

Wrong part colored in diagnostic suggestions after tabs #87972

Closed

apiraino removed the P-high High priority label Feb 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for tabs when highlighting multiline code suggestions #87976

Account for tabs when highlighting multiline code suggestions #87976

estebank commented Aug 12, 2021 •

edited

Loading

rust-highfive commented Aug 12, 2021

m-ou-se Aug 12, 2021 •

edited

Loading

estebank Aug 12, 2021 •

edited

Loading

m-ou-se Aug 12, 2021

estebank Aug 12, 2021

estebank Aug 12, 2021

m-ou-se Aug 12, 2021

This comment has been minimized.

cjgillot commented Aug 14, 2021

estebank commented Aug 19, 2021

m-ou-se left a comment

m-ou-se Aug 19, 2021 •

edited

Loading

estebank Aug 20, 2021

m-ou-se Aug 21, 2021

estebank Aug 23, 2021

m-ou-se Aug 23, 2021 •

edited

Loading

m-ou-se Aug 23, 2021

estebank Aug 23, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

estebank commented Aug 23, 2021

bors commented Aug 23, 2021

camsteffen commented Sep 3, 2021

estebank commented Sep 5, 2021

camsteffen commented Sep 5, 2021

estebank commented Sep 10, 2021

camsteffen commented Sep 10, 2021

		max_line_num_len + 3 + start + tabs,
		max_line_num_len + 3 + end + tabs,

Account for tabs when highlighting multiline code suggestions #87976

Account for tabs when highlighting multiline code suggestions #87976

Conversation

estebank commented Aug 12, 2021 • edited Loading

rust-highfive commented Aug 12, 2021

m-ou-se Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

estebank Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

cjgillot commented Aug 14, 2021

estebank commented Aug 19, 2021

m-ou-se left a comment

Choose a reason for hiding this comment

m-ou-se Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-ou-se Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

estebank commented Aug 23, 2021

bors commented Aug 23, 2021

camsteffen commented Sep 3, 2021

estebank commented Sep 5, 2021

camsteffen commented Sep 5, 2021

estebank commented Sep 10, 2021

camsteffen commented Sep 10, 2021

estebank commented Aug 12, 2021 •

edited

Loading

m-ou-se Aug 12, 2021 •

edited

Loading

estebank Aug 12, 2021 •

edited

Loading

m-ou-se Aug 19, 2021 •

edited

Loading

m-ou-se Aug 23, 2021 •

edited

Loading