Replace tabs earlier in diagnostics

[jryans]

This replaces tabs earlier in the diagnostics emitting process, which allows various margin calculations to ignore the existence of tabs. It does add a string copy for the source lines that are emitted.

Fixes #78438

r? @estebank

[jryans]

It was not enough to just edit places where we check Unicode width, as many other margin calculations are also impacted. This approach seems like the simplest thing I could think of given the problem we're trying to solve.

I did also experiment with wrapper types to clearly delineate the different kinds of lengths / offsets, but it's much more verbose and a much larger change, which I am not sure is worth the trouble. I believe it would avoid the string copy though.

[jryans]

Ah, hmm, I guess there are a few remaining failures because some tabs were making it through to StyledBuffer. (It's a bit hard to read the UI test failures locally for me, as there are quite a few that fail locally but work on CI...)

Anyway, I'm curious what you think about the approach generally. If it seems broadly okay, I'll adjust things further to fix up the remaining tests.

[estebank]

I'm slightly concerned by this removal. Have you tried to see the output in your local CLI? I need to double check the code, but can you confirm that the underlines are all colored correctly?

[jryans]

I believe this is safe, since the style handling here is about adjusting them for the adding tabs, but we now handle the tabs earlier on, so there's no need to adjust at this stage any more.

The output looks correct to me as well. Here are a few examples:

[estebank]

The chances look reasonable, I need to read it again to make sure that everything is correct, but provisionally r=me. Could you address @pickfire's comment about replace and post a screenshot of the new output?

[jryans]

@estebank Thanks for the initial look! 😄 This should now be ready for a full review. Changes since last time:

• Switched to replace
• Added screenshots
• Updated other paths that print source lines to replace as well
• Added assertions to ensure no stray tabs slip through

[pickfire]

Looks good to me.

[jryans]

I made a small cleanup change after @pickfire's latest review:

• StyledBuffer.render no longer mutates, so I changed it to &self

[jryans]

@estebank Just a friendly reminder that this is ready for another review. 😄 I’m hoping to work my way up to eventually helping with the review load as well.

[estebank]

This is a bit heavy handed but ok with me (I will assume we'll see reports on nightly if this causes issues before it hits stable).

[jryans]

Ah okay, mostly I just wanted it to be ultra clear to future code readers which paths should not have tabs, but it could be relaxed to a comment of course. I'll check json output now.

[estebank]

@estebank Just a friendly reminder that this is ready for another review. 😄 I’m hoping to work my way up to eventually helping with the review load as well.

No worries! Apologies, I was offline for all of December, which is why my personal queue has grown dramatically 😅

The changes look all great. I'm approving, but could you also check that the --output=json works correctly (without triggering the asserts)? If you don't get to it, I'll try it out tomorrow on nightly.

Super happy to see this fixed!

[estebank]

@bors r+

[bors]

📌 Commit 3537bd8 has been approved by estebank

[jryans]

could you also check that the --output=json works correctly (without triggering the asserts)? If you don't get to it, I'll try it out tomorrow on nightly.

@estebank I couldn't find any --output=json flag in rustc or cargo, but I am guessing perhaps you meant rustc's --error-format=json? If yes, then that seems to work correctly for the test case in this PR (without triggering the assertions):

$ ./build/x86_64-apple-darwin/stage1/bin/rustc src/test/ui/terminal-width/tabs-trimming.rs --error-format=json
{"message":"variable `v` is not bound in all patterns","code":{"code":"E0408","explanation":"An \"or\" pattern was used where the variable bindings are not consistently bound\nacross patterns.\n\nErroneous code example:\n\n```compile_fail,E0408\nmatch x {\n    Some(y) | None => { /* use y */ } // error: variable `y` from pattern #1 is\n                                      //        not bound in pattern #2\n    _ => ()\n}\n```\n\nHere, `y` is bound to the contents of the `Some` and can be used within the\nblock corresponding to the match arm. However, in case `x` is `None`, we have\nnot specified what `y` is, and the block will use a nonexistent variable.\n\nTo fix this error, either split into multiple match arms:\n\n```\nlet x = Some(1);\nmatch x {\n    Some(y) => { /* use y */ }\n    None => { /* ... */ }\n}\n```\n\nor, bind the variable to a field of the same type in all sub-patterns of the\nor pattern:\n\n```\nlet x = (0, 2);\nmatch x {\n    (0, y) | (y, 0) => { /* use y */}\n    _ => {}\n}\n```\n\nIn this example, if `x` matches the pattern `(0, _)`, the second field is set\nto `y`. If it matches `(_, 0)`, the first field is set to `y`; so in all\ncases `y` is set to some value.\n"},"level":"error","spans":[{"file_name":"src/test/ui/terminal-width/tabs-trimming.rs","byte_start":222,"byte_end":223,"line_start":9,"line_end":9,"column_start":16,"column_end":17,"is_primary":true,"text":[{"text":"\t\t\t\t\t\t\tv @ 1 | 2 | 3 => panic!(\"You gave me too little money {}\", v), // Long text here: TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTT","highlight_start":16,"highlight_end":17}],"label":"pattern doesn't bind `v`","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"src/test/ui/terminal-width/tabs-trimming.rs","byte_start":226,"byte_end":227,"line_start":9,"line_end":9,"column_start":20,"column_end":21,"is_primary":true,"text":[{"text":"\t\t\t\t\t\t\tv @ 1 | 2 | 3 => panic!(\"You gave me too little money {}\", v), // Long text here: TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTT","highlight_start":20,"highlight_end":21}],"label":"pattern doesn't bind `v`","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"src/test/ui/terminal-width/tabs-trimming.rs","byte_start":214,"byte_end":215,"line_start":9,"line_end":9,"column_start":8,"column_end":9,"is_primary":false,"text":[{"text":"\t\t\t\t\t\t\tv @ 1 | 2 | 3 => panic!(\"You gave me too little money {}\", v), // Long text here: TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTT","highlight_start":8,"highlight_end":9}],"label":"variable not in all patterns","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0408]: variable `v` is not bound in all patterns\n --> src/test/ui/terminal-width/tabs-trimming.rs:9:16\n  |\n9 | ...   v @ 1 | 2 | 3 => panic!(\"You gave me too little money {}\", v), // Long text here: TTTTTTTT...\n  |       -       ^   ^ pattern doesn't bind `v`\n  |       |       |\n  |       |       pattern doesn't bind `v`\n  |       variable not in all patterns\n\n"}
{"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
{"message":"For more information about this error, try `rustc --explain E0408`.","code":null,"level":"failure-note","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0408`.\n"}

[bors]

⌛ Testing commit 3537bd8 with merge b6b4616...