Remove overlapping parts of multipart suggestions #106916

lukas-code · 2023-01-15T20:56:45Z

This PR adds a debug assertion that the parts of a single substitution cannot overlap, fixes a overlapping substitution from the testsuite, and fixes #106870.

Note that a single suggestion can still have multiple overlapping substitutions / possible edits, we just don't suggest overlapping replacements in a single edit anymore.

I've also included a fix for an unrelated bug where rustfix for explicit_outlives_requirements would produce multiple trailing commas for a where clause.

rustbot · 2023-01-15T20:56:51Z

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

WaffleLapkin · 2023-01-17T08:25:34Z

compiler/rustc_errors/src/diagnostic.rs

+        for (index, part_outer) in parts.iter().enumerate() {
+            debug_assert!(
+                !part_outer.span.is_empty() || !part_outer.snippet.is_empty(),
+                "Span must not be empty and have no suggestion",
+            );
+            for part_inner in parts.iter().skip(index + 1) {
+                debug_assert!(
+                    !part_outer.span.overlaps(part_inner.span),
+                    "suggestion must not have overlapping parts:\n{part_outer:?}\n{part_inner:?}",
+                );
+            }
+        }


Does it make sense to sort parts by the span? this way you only need to check the two adjacent ones. But, we shouldn't do this just for the debug assertions maybe...

Either way, I think it would be better to enclose this in if cfg!(debug_assertions) {}, so llvm has an easier time removing the useless loops.

Also, can we somehow test this assertion?

I think it is perfectly safe to always unconditionally sort here.

WaffleLapkin · 2023-01-17T08:26:34Z

r? @estebank

estebank · 2023-01-17T17:21:35Z

compiler/rustc_errors/src/diagnostic.rs

+        for (index, part_outer) in parts.iter().enumerate() {
+            debug_assert!(
+                !part_outer.span.is_empty() || !part_outer.snippet.is_empty(),
+                "Span must not be empty and have no suggestion",
+            );
+            for part_inner in parts.iter().skip(index + 1) {
+                debug_assert!(
+                    !part_outer.span.overlaps(part_inner.span),
+                    "suggestion must not have overlapping parts:\n{part_outer:?}\n{part_inner:?}",
+                );
+            }
+        }


After sorting suggestion:

Suggested change

for (index, part_outer) in parts.iter().enumerate() {

debug_assert!(

!part_outer.span.is_empty() || !part_outer.snippet.is_empty(),

"Span must not be empty and have no suggestion",

);

for part_inner in parts.iter().skip(index + 1) {

debug_assert!(

!part_outer.span.overlaps(part_inner.span),

"suggestion must not have overlapping parts:\n{part_outer:?}\n{part_inner:?}",

);

}

}

let mut parts = parts.iter.peekable();

while let Some(part) = parts.next() {

debug_assert!(

!partspan.is_empty() || !part.snippet.is_empty(),

"Span must not be empty and have no suggestion",

);

let Some(next) = parts.peek() else { break; }

debug_assert!(

!part.span.overlaps(next.span),

"suggestion must not have overlapping parts:\n{part:?}\n{next:?}",

);

}

I think this can be written nicer with parts.array_windows(). Something like

debug_assert!(parts.iter().all(|part| !part.span.is_empty() || !part.snipper.is_empty()), "Span must not be empty and have no suggestion"); parts.array_windows(|[a, b]| debug_assert!(!a.span.overlaps(b.span), "suggestion must not have overlapping parts:\n{a:?}\n{b:?}"));

estebank · 2023-01-17T17:22:34Z

compiler/rustc_expand/src/mbe/macro_rules.rs

+                    let rhs_tt = &rhs.tts[i];
+                    let ctxt = tt.span().ctxt();
+                    match (&mut tt, rhs_tt) {
+                        // preserve the delim spans if able


estebank · 2023-01-17T17:25:48Z

r=me after addressing the sorting/assertion nitpick

WaffleLapkin · 2023-01-17T19:31:44Z

@bors r=estebank

bors · 2023-01-17T19:31:46Z

📌 Commit d18768a has been approved by estebank

It is now in the queue for this repository.

lukas-code · 2023-01-17T19:50:46Z

For the record, this is how this assertion looks now:

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some([SubstitutionPart { span: /home/lukas/code/rust/tests/ui/parser/issues/issue-44406.rs:3:12: 3:24 (#4), snippet: " { " }, SubstitutionPart { span: /home/lukas/code/rust/tests/ui/parser/issues/issue-44406.rs:3:12: 3:24 (#4), snippet: " }" }])`: suggestion must not have overlapping parts', /home/lukas/code/rust/compiler/rustc_errors/src/diagnostic.rs:646:9

…stebank Remove overlapping parts of multipart suggestions This PR adds a debug assertion that the parts of a single substitution cannot overlap, fixes a overlapping substitution from the testsuite, and fixes rust-lang#106870. Note that a single suggestion can still have multiple overlapping substitutions / possible edits, we just don't suggest overlapping replacements in a single edit anymore. I've also included a fix for an unrelated bug where rustfix for `explicit_outlives_requirements` would produce multiple trailing commas for a where clause.

matthiaskrgr · 2023-01-18T05:58:52Z

Might break clippy tests

WaffleLapkin · 2023-01-18T08:24:19Z

It does look like it breaks clippy tests (diff from #107020):

-error: variables can be used directly in the `format!` string
-   |
-   |
-LL |     println!("{0} {0}", local_i32);
-   |
-help: change this to
-   |
-   |
-LL -     println!("{0} {0}", local_i32);
-LL +     println!("{local_i32} {local_i32}");
+thread 'rustc' panicked at 'assertion failed: `(left == right)`
+  left: `None`,
+  left: `None`,
+ right: `Some([SubstitutionPart { span: $DIR/uninlined_format_args.rs:78:23: 78:34 (#0), snippet: "" }, SubstitutionPart { span: $DIR/uninlined_format_args.rs:78:23: 78:34 (#0), snippet: "" }])`: suggestion must not have overlapping parts', /checkout/compiler/rustc_errors/src/diagnostic.rs:646:9

Not sure why clippy removes the same span twice though 🤔

WaffleLapkin · 2023-01-18T08:24:40Z

@bors r-

rustbot · 2023-01-19T20:56:27Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

lukas-code · 2023-01-19T21:48:53Z

I fixed the clippy failure and also changed the signature of multipart_suggestion_* to take impl IntoIterator instead of Vec, because I didn't like the .into_iter().collect().into_iter().collect(). I hope this doesn't affect compile time too much.

@rustbot ready

WaffleLapkin

Generally it's better to make smaller PR and separate refactorings, but this looks good overall, except for a couple of nitpicks.

WaffleLapkin · 2023-01-20T14:27:07Z

compiler/rustc_hir_analysis/src/check/wfcheck.rs

+        let sugg =
+            trait_should_be_self.iter().map(|span| (*span, "Self".to_string())).collect::<Vec<_>>();


Wouldn't it make more sense to remove the collect altogether?

This vector is moved into a MultiSpan below. Alternatively this can be .clone().into_iter().map() instead of .iter().map().collect(), but I don't know if that's any better?

WaffleLapkin · 2023-01-20T14:33:28Z

compiler/rustc_lint/src/builtin.rs

+                let lint_spans = lint_spans.into_iter().collect::<Vec<_>>();
+
                cx.emit_spanned_lint(
                    EXPLICIT_OUTLIVES_REQUIREMENTS,
                    lint_spans.clone(),


Remove collect? also the clone seems unnecessary? Can't this just be

Suggested change

let lint_spans = lint_spans.into_iter().collect::<Vec<_>>();

cx.emit_spanned_lint(

EXPLICIT_OUTLIVES_REQUIREMENTS,

lint_spans.clone(),

cx.emit_spanned_lint(

EXPLICIT_OUTLIVES_REQUIREMENTS,

lint_spans,

The clone is necessary, because we need one vector for the MultiSpan of the lint itself and one for the suggestion.

The collect is turning the HashSet into a Vec for the MultiSpan. But it would probably be better to just sort and dedup a vector instead of using a hash set, will change that.

WaffleLapkin · 2023-01-20T14:35:34Z

compiler/rustc_errors/src/diagnostic.rs

-                    .map(|(span, snippet)| SubstitutionPart { snippet, span })
-                    .collect(),
-            }],
+            substitutions: vec![Substitution { parts }],


💭 would small vec be good here?

WaffleLapkin · 2023-01-27T10:27:43Z

I assume you've removed the generic changes, so let's see what perf says @bors try @rust-timer queue

bors · 2023-01-27T10:28:01Z

⌛ Trying commit 228ddf0 with merge 5bfca626acb506f9da84b4eac21a87e0847f8785...

WaffleLapkin · 2023-01-27T10:28:01Z

@bors try

bors · 2023-01-27T10:28:10Z

⌛ Trying commit 228ddf0 with merge cddc29a75100f2566a46e957e0b0df708bc12c68...

bors · 2023-01-27T12:38:01Z

☀️ Try build successful - checks-actions
Build commit: cddc29a75100f2566a46e957e0b0df708bc12c68 (cddc29a75100f2566a46e957e0b0df708bc12c68)

bors · 2023-01-27T12:38:01Z

☀️ Try build successful - checks-actions
Build commit: cddc29a75100f2566a46e957e0b0df708bc12c68 (cddc29a75100f2566a46e957e0b0df708bc12c68)

rust-timer · 2023-01-27T14:26:58Z

Finished benchmarking commit (cddc29a75100f2566a46e957e0b0df708bc12c68): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

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

Cycles

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

lukas-code · 2023-01-27T14:33:57Z

Regression is from the improvements in #107343 (comment), so probably noise.

WaffleLapkin · 2023-01-27T15:27:10Z

@bors r=estebank

bors · 2023-01-27T15:27:12Z

📌 Commit 228ddf0 has been approved by estebank

It is now in the queue for this repository.

bors · 2023-01-28T10:01:00Z

⌛ Testing commit 228ddf0 with merge d8da513...

bors · 2023-01-28T13:12:53Z

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

rust-timer · 2023-01-28T15:47:31Z

Finished benchmarking commit (d8da513): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-1.2%, -0.1%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

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)	2.7%	[2.6%, 2.8%]	2
Regressions ❌ (secondary)	1.5%	[1.1%, 2.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[2.6%, 2.8%]	2

Cycles

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

WaffleLapkin · 2023-01-30T05:07:04Z

@lukas-code do you still have the commit/diff of making diagnostic functions accept impl Interator? I would like to experiment with this a bit

lukas-code · 2023-01-30T18:06:46Z

@WaffleLapkin I pulled this out of my reflog: 36d9953 https://github.com/lukas-code/rust/tree/multipart-impl-iterator

WaffleLapkin · 2023-01-31T07:26:37Z

@lukas-code thanks!

…ebank Remove overlapping parts of multipart suggestions This PR adds a debug assertion that the parts of a single substitution cannot overlap, fixes a overlapping substitution from the testsuite, and fixes rust-lang#106870. Note that a single suggestion can still have multiple overlapping substitutions / possible edits, we just don't suggest overlapping replacements in a single edit anymore. I've also included a fix for an unrelated bug where rustfix for `explicit_outlives_requirements` would produce multiple trailing commas for a where clause.

rustbot assigned WaffleLapkin Jan 15, 2023

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 Jan 15, 2023

WaffleLapkin reviewed Jan 17, 2023

View reviewed changes

rustbot assigned estebank and unassigned WaffleLapkin Jan 17, 2023

estebank reviewed Jan 17, 2023

View reviewed changes

lukas-code force-pushed the overlapping-substs branch from b56c49a to d18768a Compare January 17, 2023 19:28

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 Jan 17, 2023

matthiaskrgr mentioned this pull request Jan 18, 2023

Rollup of 7 pull requests #107020

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 Jan 18, 2023

lukas-code force-pushed the overlapping-substs branch from d18768a to 17a9aa9 Compare January 19, 2023 20:56

rustbot 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 Jan 19, 2023

WaffleLapkin reviewed Jan 20, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 27, 2023

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jan 27, 2023

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 Jan 27, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 28, 2023

bors merged commit d8da513 into rust-lang:master Jan 28, 2023

rustbot added this to the 1.69.0 milestone Jan 28, 2023

lukas-code deleted the overlapping-substs branch January 28, 2023 13:13

		let sugg =
		trait_should_be_self.iter().map(\|span\| (*span, "Self".to_string())).collect::<Vec<_>>();

Remove overlapping parts of multipart suggestions #106916

Remove overlapping parts of multipart suggestions #106916

Conversation

lukas-code commented Jan 15, 2023

rustbot commented Jan 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaffleLapkin commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jan 17, 2023

WaffleLapkin commented Jan 17, 2023

bors commented Jan 17, 2023

lukas-code commented Jan 17, 2023

matthiaskrgr commented Jan 18, 2023

WaffleLapkin commented Jan 18, 2023

WaffleLapkin commented Jan 18, 2023

rustbot commented Jan 19, 2023

lukas-code commented Jan 19, 2023

WaffleLapkin left a 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

WaffleLapkin commented Jan 27, 2023

This comment has been minimized.

bors commented Jan 27, 2023

WaffleLapkin commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 27, 2023

This comment has been minimized.

rust-timer commented Jan 27, 2023

Overall result: ❌ regressions - no action needed

lukas-code commented Jan 27, 2023

WaffleLapkin commented Jan 27, 2023

bors commented Jan 27, 2023

bors commented Jan 28, 2023

bors commented Jan 28, 2023

rust-timer commented Jan 28, 2023

Overall result: ✅ improvements - no action needed

WaffleLapkin commented Jan 30, 2023

lukas-code commented Jan 30, 2023

WaffleLapkin commented Jan 31, 2023