Show diff suggestion format on verbose replacement #127541

estebank · 2024-07-09T22:32:41Z

error[E0610]: `{integer}` is a primitive type and therefore doesn't have fields
  --> $DIR/attempted-access-non-fatal.rs:7:15
   |
LL |     let _ = 2.l;
   |               ^
   |
help: if intended to be a floating point literal, consider adding a `0` after the period and a `f64` suffix
   |
LL -     let _ = 2.l;
LL +     let _ = 2.0f64;
   |

before:

error[E0610]: `{integer}` is a primitive type and therefore doesn't have fields
  --> $DIR/attempted-access-non-fatal.rs:7:15
   |
LL |     let _ = 2.l;
   |               ^
   |
help: if intended to be a floating point literal, consider adding a `0` after the period and a `f64` suffix
   |
LL +     let _ = 2.0f64;
   |               ~~~~

r? @oli-obk

rustbot · 2024-07-09T22:32:50Z

Some changes occurred in tests/ui/check-cfg

cc @Urgau

HIR ty lowering was modified

cc @fmease

estebank · 2024-07-09T22:41:37Z

This is in response to #127407 (comment). This also highlights that there are multiple places where the suggestions could have more accurate spans. We should also improve the diff presentation to automatically skip pieces that haven't changed so that suggestions can continue to be built easily when possible.

tests/ui/imports/issue-45829/rename-use-vs-extern.stderr

Urgau · 2024-07-10T09:39:55Z

Does the verbose suggestion highlight (in color mode) the thing being replaced?

What I mean is that before, the ^^^^ pointed directly at the offended place, now users will have to scan both lines to see the difference, at least without color; but with color on, we could show the character difference by highlighting them in bold/color/italic or whatever.

Adding a --color=always/SVG output test, might be helpful to see what most users will actually experience.

tests/rustdoc-ui/invalid_associated_const.stderr

tests/rustdoc-ui/issues/issue-120444-1.stderr

tests/ui/argument-suggestions/issue-101097.stderr

tests/ui/argument-suggestions/issue-97197.stderr

estebank · 2024-07-11T17:15:35Z

@oli-obk sorry about the last commit, it was an effort to simultaneously fix a papercut and have an SVG file to show different patch renderings, but even that backfired because GH can't render it :-/

I think it's failing because the contents are too long, I'll split it up and see.

Can easily remove it.

rustbot · 2024-07-11T17:26:43Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

estebank · 2024-07-11T18:11:45Z

Does the verbose suggestion highlight (in color mode) the thing being replaced?

What I mean is that before, the ^^^^ pointed directly at the offended place, now users will have to scan both lines to see the difference, at least without color; but with color on, we could show the character difference by highlighting them in bold/color/italic or whatever.

Adding a --color=always/SVG output test, might be helpful to see what most users will actually experience.

@Urgau https://github.com/rust-lang/rust/blob/82684d02db0b89766a46f1efeb7f9214f690cb3c/tests/ui/suggestions/incorrect-variant-literal.svg

estebank · 2024-07-16T16:39:28Z

I'll move all the specific suggestion changes themselves to other PRs, but they will all clash with the diff output rendering change, so we'll have to be mindful of what lands first and then rebase properly.

bors · 2025-02-07T21:00:29Z

☔ The latest upstream changes (presumably #136697) made this pull request unmergeable. Please resolve the merge conflicts.

``` error[E0610]: `{integer}` is a primitive type and therefore doesn't have fields --> $DIR/attempted-access-non-fatal.rs:7:15 | LL | let _ = 2.l; | ^ | help: if intended to be a floating point literal, consider adding a `0` after the period and a `f64` suffix | LL - let _ = 2.l; LL + let _ = 2.0f64; | ```

estebank · 2025-02-10T21:52:29Z

@bors r=petrochenkov p=1

Raising priority to try and avoid more merge conflicts (given how many stderr files are being touched).

bors · 2025-02-10T21:52:31Z

📌 Commit f0845ad has been approved by petrochenkov

It is now in the queue for this repository.

jieyouxu · 2025-02-10T22:07:59Z

@bors p=10 (conflict-prone)

bors · 2025-02-11T02:35:10Z

⌛ Testing commit f0845ad with merge ffa9afe...

bors · 2025-02-11T05:26:41Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing ffa9afe to master...

rust-timer · 2025-02-11T07:47:42Z

Finished benchmarking commit (ffa9afe): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 4.6%, secondary 7.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.6%	[4.6%, 4.6%]	1
Regressions ❌ (secondary)	7.2%	[7.2%, 7.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.6%	[4.6%, 4.6%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 786.626s -> 786.457s (-0.02%)
Artifact size: 348.30 MiB -> 348.34 MiB (0.01%)

estebank · 2025-02-11T21:33:21Z

Apologies to all the PR authors coming to see what broke you. You just need to rebase + rebless.

compiler-errors · 2025-02-13T01:40:05Z

It's pretty disappointing that this PR got no discussion before it was applied. I would've definitely pushed back on the fallout of this PR had I been aware of it past it landing and changing like every UI test. :(

…nt, r=estebank Fix presentation of purely "additive" replacement suggestion parts rust-lang#127541 changes replacement suggestions to use the "diff" view always, which I think is really verbose in cases where a replacement snippet is a "superset" of the snippet that is being replaced. Consider: ``` LL - Self::Baz: Clone, LL + Self::Baz: Clone, T: std::clone::Clone ``` In this code, we suggest replacing `", "` with `", T: std::clone::Clone"`. This is a consequence of how the snippet is constructed. I believe that since the string that is being replaced is a subset of the replacement string, it's not providing much value to present this as a diff. Users should be able to clearly understand what's being suggested here using the `~` underline view we've been suggesting for some time now. Given that this affects ~100 tests out of the ~1000 UI tests affected, I expect this to be a pretty meaningful improvement of the fallout of rust-lang#127541. --- In the last commit, this PR also "trims" replacement parts so that they are turned into their purely additive subset, if possible. See the diff for what this means. --- r? estebank

Rollup merge of rust-lang#136958 - compiler-errors:additive-replacmeent, r=estebank Fix presentation of purely "additive" replacement suggestion parts rust-lang#127541 changes replacement suggestions to use the "diff" view always, which I think is really verbose in cases where a replacement snippet is a "superset" of the snippet that is being replaced. Consider: ``` LL - Self::Baz: Clone, LL + Self::Baz: Clone, T: std::clone::Clone ``` In this code, we suggest replacing `", "` with `", T: std::clone::Clone"`. This is a consequence of how the snippet is constructed. I believe that since the string that is being replaced is a subset of the replacement string, it's not providing much value to present this as a diff. Users should be able to clearly understand what's being suggested here using the `~` underline view we've been suggesting for some time now. Given that this affects ~100 tests out of the ~1000 UI tests affected, I expect this to be a pretty meaningful improvement of the fallout of rust-lang#127541. --- In the last commit, this PR also "trims" replacement parts so that they are turned into their purely additive subset, if possible. See the diff for what this means. --- r? estebank

rustbot assigned oli-obk Jul 9, 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 9, 2024

Urgau reviewed Jul 10, 2024

View reviewed changes

tests/ui/imports/issue-45829/rename-use-vs-extern.stderr Outdated Show resolved Hide resolved

oli-obk reviewed Jul 10, 2024

View reviewed changes

tests/rustdoc-ui/invalid_associated_const.stderr Outdated Show resolved Hide resolved

oli-obk reviewed Jul 10, 2024

View reviewed changes

tests/rustdoc-ui/issues/issue-120444-1.stderr Show resolved Hide resolved

oli-obk reviewed Jul 10, 2024

View reviewed changes

tests/ui/argument-suggestions/issue-101097.stderr Show resolved Hide resolved

oli-obk reviewed Jul 10, 2024

View reviewed changes

tests/ui/argument-suggestions/issue-97197.stderr Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

estebank force-pushed the diff-suggestions branch from b00adfa to d470334 Compare July 11, 2024 17:13

This comment has been minimized.

Sign in to view

estebank mentioned this pull request Jul 11, 2024

Make parse error suggestions verbose and fix spans #127407

Merged

This comment was marked as resolved.

Sign in to view

estebank force-pushed the diff-suggestions branch from 9548c77 to 729f12b Compare July 16, 2024 16:37

This comment has been minimized.

Sign in to view

estebank force-pushed the diff-suggestions branch from 729f12b to 68fce46 Compare July 17, 2024 19:32

This comment was marked as resolved.

Sign in to view

estebank force-pushed the diff-suggestions branch from 68fce46 to 9642d8c Compare July 18, 2024 19:19

This comment was marked as resolved.

Sign in to view

estebank force-pushed the diff-suggestions branch from 9642d8c to a9dc677 Compare July 19, 2024 04:54

estebank force-pushed the diff-suggestions branch from b266cbd to 6e995a4 Compare February 10, 2025 20:03

estebank force-pushed the diff-suggestions branch from 6e995a4 to f0845ad Compare February 10, 2025 20:21

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 Feb 10, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 11, 2025

bors merged commit ffa9afe into rust-lang:master Feb 11, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Feb 11, 2025

This was referenced Feb 11, 2025

add error message for c# style named arguments #118733

Closed

Warn about C-style octal literals #131309

Open

workingjubilee mentioned this pull request Feb 11, 2025

Resolve named regions when reporting type test failures in NLL #136559

Merged

compiler-errors mentioned this pull request Feb 13, 2025

Fix presentation of purely "additive" replacement suggestion parts #136958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show diff suggestion format on verbose replacement #127541

Show diff suggestion format on verbose replacement #127541

estebank commented Jul 9, 2024 •

edited

Loading

rustbot commented Jul 9, 2024

estebank commented Jul 9, 2024

Urgau commented Jul 10, 2024 •

edited

Loading

This comment was marked as resolved.

estebank commented Jul 11, 2024 •

edited

Loading

rustbot commented Jul 11, 2024

This comment has been minimized.

estebank commented Jul 11, 2024

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

estebank commented Jul 16, 2024

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

bors commented Feb 7, 2025

estebank commented Feb 10, 2025

bors commented Feb 10, 2025

jieyouxu commented Feb 10, 2025

bors commented Feb 11, 2025

bors commented Feb 11, 2025

rust-timer commented Feb 11, 2025

estebank commented Feb 11, 2025

compiler-errors commented Feb 13, 2025 •

edited

Loading

Show diff suggestion format on verbose replacement #127541

Show diff suggestion format on verbose replacement #127541

Conversation

estebank commented Jul 9, 2024 • edited Loading

rustbot commented Jul 9, 2024

estebank commented Jul 9, 2024

Urgau commented Jul 10, 2024 • edited Loading

This comment was marked as resolved.

estebank commented Jul 11, 2024 • edited Loading

rustbot commented Jul 11, 2024

This comment has been minimized.

estebank commented Jul 11, 2024

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

estebank commented Jul 16, 2024

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

bors commented Feb 7, 2025

estebank commented Feb 10, 2025

bors commented Feb 10, 2025

jieyouxu commented Feb 10, 2025

bors commented Feb 11, 2025

bors commented Feb 11, 2025

rust-timer commented Feb 11, 2025

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

estebank commented Feb 11, 2025

compiler-errors commented Feb 13, 2025 • edited Loading

estebank commented Jul 9, 2024 •

edited

Loading

Urgau commented Jul 10, 2024 •

edited

Loading

estebank commented Jul 11, 2024 •

edited

Loading

compiler-errors commented Feb 13, 2025 •

edited

Loading