More sophisticated span trimming for suggestions #137348

compiler-errors · 2025-02-20T23:09:20Z

Previously #136958 only cared about prefixes or suffixes. Now it detects more cases where a suggestion is "sandwiched" by unchanged code on the left or the right. Would be cool if we could detect several insertions, like ACE going to ABCDE, extracting B and D, but that seems unwieldy.

r? @estebank

rustbot · 2025-02-21T00:54:28Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler-errors · 2025-02-21T00:55:50Z

compiler/rustc_errors/src/lib.rs

@@ -380,7 +403,12 @@ impl CodeSuggestion {
                // or deleted code in order to point at the correct column *after* substitution.
                let mut acc = 0;
                let mut only_capitalization = false;
-                for part in &substitution.parts {
+                for part in &mut substitution.parts {


We trim before we create highlights to avoid highlighting unchanged portions of the line, and to avoid random bugs (preexisting? #136958 (comment)) with how we render suggestions.

compiler-errors · 2025-02-21T00:56:26Z

src/tools/clippy/tests/ui/manual_flatten.stderr

-LL +         Some(1),
-LL +         Some(2),
-LL +         Some(3)
+LL |     for n in vec![


This diff is a bit weird but 🤷

The + on lines 200-202 were wrong anyways, the only thing that this PR is suggesting to change is adding .flatten().

The fact that we don't suggesting putting the ++++ under .flatten() is because the suggestion span is multiline. That's unfortunate, but more difficult to fix.

compiler-errors · 2025-02-21T00:58:55Z

src/tools/clippy/tests/ui/too_long_first_doc_paragraph.stderr

-LL ~     //! A very short summary.
-LL +     //!
+LL |     //! A very short summary.
+LL ~     //!


Kinda a shame that this isn't rendered using a + on the line, but it's pretty difficult to debug tbh.

I wonder if it is related to the finicky \n handling from the previous line... But lets ignore for now.

tests/ui/impl-trait/in-trait/refine-captures.stderr

estebank · 2025-02-21T16:32:49Z

src/tools/clippy/tests/ui/implicit_return.stderr

   |
 LL |     return true
-   |


Intrigued about this existing rendering issue...

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

estebank · 2025-02-21T17:59:17Z

tests/ui/fn/issue-3044.stderr

+LL |     needlesArr.iter().fold(|x, y| {
+LL |
 LL ~     }, /* f */);


Looking at these, we shorten the suggestion parts after we've determined what the suggestion "window" will be, which means that we display this with the "multiline" format, instead of just adding , /* f */. That's unfortunate and I'll see if we can address it easily in this PR, but if not let's keep a ticket open for that investigation.

Yeah, I looked into this. I don't think it's fixable in this PR, because if we shorten the suggestion window, then we lose the necessary context and the suggestion renders really poorly, like:

LL + }, /* f */}; ++++++++++

but with no preceding lines, which makes it really hard to know what the line being edited even is. This is even worse for other examples that changed.

If you want to look into this, then please do, but I don't think it should block this PR. It would be nice if someone cleaned up the suggestion window/highlight mode selection code.

estebank · 2025-02-21T18:05:55Z

tests/ui/suggestions/return-bindings.stderr

 help: consider returning the local binding `s`
   |
-LL ~     let s: String = if let Some(s) = opt_str {
-LL +         s
+LL |     let s: String = if let Some(s) = opt_str {
+LL ~         s
 LL ~


This looks like there might be something iffy, related to the other multiline "issues".

Yes, it's the same root cause.

estebank · 2025-02-21T18:07:35Z

tests/ui/typeck/issue-13853-5.stderr

-LL ~     fn deserialize_token<D: Deserializer<'a>>(_x: D, _y: &'a str) -> &'a str {
-LL +         _y
+LL |     fn deserialize_token<D: Deserializer<'a>>(_x: D, _y: &'a str) -> &'a str {
+LL ~         _y


Yeah, this case should absolutely be + instead of ~. (again, we can deal with this later, just marking for future reference)

estebank · 2025-02-21T18:15:22Z

compiler/rustc_errors/src/lib.rs

+        .chars()
+        .zip(suggestion.chars())
+        .take_while(|(c1, c2)| c1 == c2)
+        .map(|(c, _)| c.len_utf8())


This might interact poorly with with unicode codepoints that get "normalized" (rustc_errors::emitter::normalize_whitespace) irrespective of their actual unicode rendering. This is very niche. I do not immediately know if emit_suggestion_default does terminal output normalization. Intuitively, we might not want to do any because the user might be copy pasting from the terminal (instead of using rustfix). Either way, not a blocker.

The source map (afaict) already uses the length of substrings to adjust spans. If this is a problem, then it's (afaict) already a problem 🤔.

estebank · 2025-02-21T18:18:49Z

compiler/rustc_errors/src/lib.rs

+        .sum();
+    let original = &original[common_prefix..];
+    let suggestion = &suggestion[common_prefix..];
+    if suggestion.ends_with(original) {


Can't you instead use original.chars().rev() to do the same thing for the suffix as with the prefix?

It's simpler to just call ends_with because we want to make sure that the "original" is a subset of the "suggestion". Otherwise, we'd need to do one more check that the trimmed original string is empty.

estebank · 2025-02-21T18:20:32Z

Love these changes.

r=me with high prio, either addressing the suffix change or not.

compiler-errors · 2025-02-21T19:24:37Z

@bors r=estebank p=1 rollup

bors · 2025-02-21T19:24:41Z

📌 Commit 160905b has been approved by estebank

It is now in the queue for this repository.

More sophisticated span trimming for suggestions Previously rust-lang#136958 only cared about prefixes or suffixes. Now it detects more cases where a suggestion is "sandwiched" by unchanged code on the left or the right. Would be cool if we could detect several insertions, like `ACE` going to `ABCDE`, extracting `B` and `D`, but that seems unwieldy. r? `@estebank`

More sophisticated span trimming for suggestions Previously rust-lang#136958 only cared about prefixes or suffixes. Now it detects more cases where a suggestion is "sandwiched" by unchanged code on the left or the right. Would be cool if we could detect several insertions, like `ACE` going to `ABCDE`, extracting `B` and `D`, but that seems unwieldy. r? ``@estebank``

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136458 (Do not deduplicate list of associated types provided by dyn principal) - rust-lang#136474 ([`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing) - rust-lang#136592 (Make sure we don't overrun the stack in canonicalizer) - rust-lang#136787 (Remove `lifetime_capture_rules_2024` feature) - rust-lang#137180 (Give `global_asm` a fake body to store typeck results, represent `sym fn` as a hir expr to fix `sym fn` operands with lifetimes) - rust-lang#137257 (Ignore fake borrows for packed field check) - rust-lang#137348 (More sophisticated span trimming for suggestions) - rust-lang#137399 (fix ICE in layout computation with unnormalizable const) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2025-02-21T23:59:11Z

⌛ Testing commit 160905b with merge dc37ff8...

bors · 2025-02-22T03:04:19Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing dc37ff8 to master...

rust-timer · 2025-02-22T06:13:28Z

Finished benchmarking commit (dc37ff8): 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 (secondary 4.8%)

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)	-	-	0
Regressions ❌ (secondary)	4.8%	[4.8%, 4.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -1.4%)

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.4%	[-1.4%, -1.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.4%	[-1.4%, -1.4%]	1

Binary size

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

Bootstrap: 774s -> 772.814s (-0.15%)
Artifact size: 361.02 MiB -> 361.01 MiB (-0.00%)

More sophisticated span trimming for suggestions Previously rust-lang#136958 only cared about prefixes or suffixes. Now it detects more cases where a suggestion is "sandwiched" by unchanged code on the left or the right. Would be cool if we could detect several insertions, like `ACE` going to `ABCDE`, extracting `B` and `D`, but that seems unwieldy. r? `@estebank`

rustbot assigned estebank Feb 20, 2025

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

compiler-errors mentioned this pull request Feb 20, 2025

Make some suggestions "verbose" #137343

Open

This comment has been minimized.

Sign in to view

compiler-errors added 2 commits February 21, 2025 00:41

More sophisticated span trimming

0a7ab1d

Trim suggestion part before generating highlights

160905b

compiler-errors force-pushed the span-trim branch from d6579da to 160905b Compare February 21, 2025 00:54

compiler-errors commented Feb 21, 2025

View reviewed changes

estebank reviewed Feb 21, 2025

View reviewed changes

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

estebank reviewed Feb 21, 2025

View reviewed changes

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 21, 2025

matthiaskrgr mentioned this pull request Feb 21, 2025

Rollup of 9 pull requests #137401

Closed

matthiaskrgr mentioned this pull request Feb 21, 2025

Rollup of 8 pull requests #137402

Closed

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

bors merged commit dc37ff8 into rust-lang:master Feb 22, 2025
7 checks passed

rustbot added this to the 1.87.0 milestone Feb 22, 2025

This was referenced Feb 22, 2025

Warn about C-style octal literals #131309

Open

Clippy subtree update #137317

Closed

fmease mentioned this pull request Feb 25, 2025

fix: overflowing bin hex #136424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More sophisticated span trimming for suggestions #137348

More sophisticated span trimming for suggestions #137348

compiler-errors commented Feb 20, 2025

This comment has been minimized.

rustbot commented Feb 21, 2025

compiler-errors Feb 21, 2025 •

edited

Loading

compiler-errors Feb 21, 2025

compiler-errors Feb 21, 2025

compiler-errors Feb 21, 2025

estebank Feb 21, 2025

estebank Feb 21, 2025

estebank Feb 21, 2025

compiler-errors Feb 21, 2025

estebank Feb 21, 2025

compiler-errors Feb 21, 2025

estebank Feb 21, 2025

estebank Feb 21, 2025

compiler-errors Feb 21, 2025

estebank Feb 21, 2025

compiler-errors Feb 21, 2025

estebank commented Feb 21, 2025 •

edited

Loading

compiler-errors commented Feb 21, 2025

bors commented Feb 21, 2025

bors commented Feb 21, 2025

bors commented Feb 22, 2025

rust-timer commented Feb 22, 2025

More sophisticated span trimming for suggestions #137348

More sophisticated span trimming for suggestions #137348

Conversation

compiler-errors commented Feb 20, 2025

This comment has been minimized.

rustbot commented Feb 21, 2025

compiler-errors Feb 21, 2025 • 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

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

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

estebank commented Feb 21, 2025 • edited Loading

compiler-errors commented Feb 21, 2025

bors commented Feb 21, 2025

bors commented Feb 21, 2025

bors commented Feb 22, 2025

rust-timer commented Feb 22, 2025

Overall result: no relevant changes - no action needed

compiler-errors Feb 21, 2025 •

edited

Loading

estebank commented Feb 21, 2025 •

edited

Loading