Normalize before erasing late-bound regions in `equal_up_to_regions` #101437

compiler-errors · 2022-09-05T07:15:18Z

Normalize erasing regions first, before passing the type through a BottomUpFolder which erases late-bound regions too.

The root cause of this issue is due to 96d4137, which removes a normalize_erasing_regions that happens before this call to equal_up_to_regions. While reverting that commit might be a fix, I think it was suspicious to be erasing late-bound regions first then normalizing types in the first place in equal_up_to_regions.

I am tempted to ask the reviewer to review and r+ this without a UI test, since the existing issues that I think this fixes are all incredibly difficult to minimize (anything hyper/warp related, given the nature of those libraries 😓) or impossible to reproduce locally (the miri test), namely:

This recently reported issue with tokio + warp: rustc nightly(289279de1 2022-09-04) ICE for tokio+warp #101430
This issue from @RalfJung about Miri being broken: miri no longer builds after rust-lang/rust#101333 #101344
This additional issue reported in a comment by @tmandry (issue with fuchsia + hyper): miri no longer builds after rust-lang/rust#101333 #101344 (comment)

I have locally verified that the repro in #101430 is fixed with this PR, but after a couple of hours of attempting to minimize this error and either failing to actually repro the ICE, or being overwhelmed with the number of traits and functions I need to inline into a UI test, I have basically given up. Thoughts are appreciated on how best to handle this.

r? @oli-obk who is at the intersection of MIR and types-related stuff who may be able to give advice 😅

compiler-errors · 2022-09-05T07:16:31Z

I wonder if a crater run would shed some light on real-world crates that are fixed by this PR, since it's very likely that something is broken out in the wild due to this.

@bors try

bors · 2022-09-05T07:16:39Z

⌛ Trying commit 76b494a with merge e3fba958e26aa56a364ba6d36376f1800cd40fa5...

Noratrieb · 2022-09-05T08:33:20Z

compiler/rustc_const_eval/src/transform/validate.rs

+                // FIXME: We erase all late-bound lifetimes, but this is not fully correct.
+                // If you have a type like `<for<'a> fn(&'a u32) as SomeTrait>::Assoc`,
+                // this is not necessarily equivalent to `<fn(&'static u32) as SomeTrait>::Assoc`,
+                // since one may have an `impl SomeTrait for fn(&32)` and


This FIXME can be removed now, right? Since we now normalize before erasing, so the <for<'a> fn(&'a u32) as SomeTrait>::Assoc will be normalized to the correct type.

This still applies to the can_eq call that happens below

Also normalizing the associated type might fail if this is in a generic context.

bors · 2022-09-05T09:07:02Z

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

compiler-errors · 2022-09-05T12:13:01Z

@craterbot check

craterbot · 2022-09-05T12:13:05Z

👌 Experiment pr-101437 created and queued.
🤖 Automatically detected try build e3fba958e26aa56a364ba6d36376f1800cd40fa5
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2022-09-05T12:14:04Z

🚧 Experiment pr-101437 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

RalfJung · 2022-09-05T19:34:46Z

@rust-timer queue
does this work if the try build is already done...?

rust-timer · 2022-09-05T19:34:48Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

compiler-errors · 2022-09-05T19:41:22Z

@rust-timer build e3fba958e26aa56a364ba6d36376f1800cd40fa5

rust-timer · 2022-09-05T19:41:23Z

Queued e3fba958e26aa56a364ba6d36376f1800cd40fa5 with parent 406e03f, future comparison URL.

rust-timer · 2022-09-05T23:19:04Z

Finished benchmarking commit (e3fba958e26aa56a364ba6d36376f1800cd40fa5): comparison URL.

Overall result: no relevant changes - 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-review -S-waiting-on-perf -perf-regression

Instruction count

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

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.4%, -2.8%]	2
All ❌✅ (primary)	-	-	0

Cycles

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)	-	-	0
Regressions ❌ (secondary)	4.2%	[3.8%, 4.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

craterbot · 2022-09-06T14:21:01Z

🎉 Experiment pr-101437 is completed!
📊 67 regressed and 6 fixed (242719 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2022-09-06T16:23:33Z

I don't think these regressions are related to my PR. I checked the regressions based on syn locally, e.g. https://crater-reports.s3.amazonaws.com/pr-101437/try%23e3fba958e26aa56a364ba6d36376f1800cd40fa5/reg/prefer-dynamic-0.1.2/log.txt and I can't reproduce them.

tmandry · 2022-09-06T22:15:49Z

@compiler-errors Have you rebased on 406e03f? crater tested with that commit and with this PR merged into that commit.

I haven't done very much crater triage, but I don't think I've seen spurious failures like this from syn before and wouldn't expect them.

compiler-errors · 2022-09-06T22:36:18Z

Yes, I just cherry-picked my change back onto the base 406e03f and still can't repro when I run with cargo check --all --all-targets (with my locally built toolchain).

Worthwhile re-checking the regressed crates to see if they're spurious I guess 🤷

@craterbot run mode=check-only list=https://crater-reports.s3.amazonaws.com/pr-101437/retry-regressed-list.txt

craterbot · 2022-09-06T22:36:20Z

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2022-09-06T22:37:18Z

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-101437/retry-regressed-list.txt

craterbot · 2022-09-06T22:37:31Z

🚧 Experiment pr-101437-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2022-09-06T22:51:11Z

🎉 Experiment pr-101437-1 is completed!
📊 15 regressed and 0 fixed (67 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2022-09-06T23:15:19Z

Ok I'm pretty certain craterbot is broken -- these are definitely failing due to build.rs not being able to get RUSTC env variable. We can see the same packages failed in Santiago's crater run here: #101345 (comment)

e.g. https://crater-reports.s3.amazonaws.com/pr-101345/try%23c3491e21996dc67a8e37d9e5e74ed156563e9854/reg/prefer-dynamic-0.1.2/log.txt

I am going to test this in #101509 with a no-op change.

compiler-errors · 2022-09-07T01:10:37Z

Ok, this is a crater bug.

Mark-Simulacrum · 2022-09-07T02:02:29Z

Cc rust-lang/crater#663 (similar in shape at least, though maybe not exactly this case)

tmandry · 2022-09-07T17:53:52Z

Should we go ahead and land this PR then?

compiler-errors · 2022-09-07T17:56:03Z

I wouldn't be opposed to landing this, though coincidentally @lcnr is also reworking this function in a different way in #101478, though that will take longer to land in case that crater run comes back with issues.

tmandry · 2022-09-07T18:02:01Z

Okay, then given @oli-obk's approval I'm going to r+ since this fixes a regression.

@bors r+ rollup=maybe (given no perf impact above; idk if "maybe" is right)

bors · 2022-09-07T18:02:03Z

📌 Commit 76b494a has been approved by tmandry

It is now in the queue for this repository.

compiler-errors · 2022-09-08T16:22:04Z

@bors rollup=never

bors · 2022-09-08T19:01:42Z

⌛ Testing commit 76b494a with merge 1120c5e...

bors · 2022-09-08T21:45:17Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 1120c5e to master...

rust-timer · 2022-09-09T02:12:37Z

Finished benchmarking commit (1120c5e): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	1.2%	[1.1%, 1.3%]	2
Regressions ❌ (secondary)	3.4%	[3.0%, 3.9%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.1%, 1.3%]	2

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

Cycles

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)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.0%, 3.6%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-09-13T03:16:12Z

cranelift-codegen and keccak have been noisy lately.

@rustbot label: +perf-regression-triaged

Normalize before erasing late-bound regions in equal_up_to_regions

76b494a

rust-highfive assigned oli-obk Sep 5, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 5, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2022

Noratrieb reviewed Sep 5, 2022

View reviewed changes

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 5, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 5, 2022

oli-obk approved these changes Sep 6, 2022

View reviewed changes

craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Sep 6, 2022

craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Sep 6, 2022

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 6, 2022

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 Sep 7, 2022

This was referenced Sep 7, 2022

Nightly ICE: no errors encountered even though delay_span_bug issued #101546

Closed

no errors encountered even though delay_span_bug issued #101548

Closed

compiler-errors mentioned this pull request Sep 8, 2022

error: internal compiler error: no errors encountered even though delay_span_bug issued broken MIR in DropGlue(DefId(2:2767 ~ core[178b]::ptr::drop_in_place) #101581

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 8, 2022

bors merged commit 1120c5e into rust-lang:master Sep 8, 2022

rustbot added this to the 1.65.0 milestone Sep 8, 2022

saethlin mentioned this pull request Sep 9, 2022

ICE: Broken MIR in DropGlue + no errors even though delay_span_bug issued #101597

Closed

rustbot added the perf-regression Performance regression. label Sep 9, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 13, 2022

compiler-errors deleted the erase-normalize-ordering branch November 2, 2022 02:58

matthiaskrgr mentioned this pull request Jun 27, 2024

error: internal compiler error: compiler/rustc_infer/src/infer/lexical_region_resolve/mod.rs:486:17: cannot relate region: LUB('{erased}, '{erased}) #127033

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize before erasing late-bound regions in `equal_up_to_regions` #101437

Normalize before erasing late-bound regions in `equal_up_to_regions` #101437

compiler-errors commented Sep 5, 2022 •

edited

Loading

compiler-errors commented Sep 5, 2022 •

edited

Loading

bors commented Sep 5, 2022

Noratrieb Sep 5, 2022

compiler-errors Sep 5, 2022

RalfJung Sep 5, 2022

bors commented Sep 5, 2022

compiler-errors commented Sep 5, 2022

craterbot commented Sep 5, 2022

craterbot commented Sep 5, 2022

RalfJung commented Sep 5, 2022

rust-timer commented Sep 5, 2022

compiler-errors commented Sep 5, 2022

rust-timer commented Sep 5, 2022

rust-timer commented Sep 5, 2022

craterbot commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

tmandry commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

craterbot commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

craterbot commented Sep 6, 2022

craterbot commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

compiler-errors commented Sep 7, 2022

Mark-Simulacrum commented Sep 7, 2022

tmandry commented Sep 7, 2022

compiler-errors commented Sep 7, 2022

tmandry commented Sep 7, 2022

bors commented Sep 7, 2022

compiler-errors commented Sep 8, 2022

bors commented Sep 8, 2022

bors commented Sep 8, 2022

rust-timer commented Sep 9, 2022

nnethercote commented Sep 13, 2022

Normalize before erasing late-bound regions in equal_up_to_regions #101437

Normalize before erasing late-bound regions in equal_up_to_regions #101437

Conversation

compiler-errors commented Sep 5, 2022 • edited Loading

compiler-errors commented Sep 5, 2022 • edited Loading

bors commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 5, 2022

compiler-errors commented Sep 5, 2022

craterbot commented Sep 5, 2022

craterbot commented Sep 5, 2022

RalfJung commented Sep 5, 2022

rust-timer commented Sep 5, 2022

compiler-errors commented Sep 5, 2022

rust-timer commented Sep 5, 2022

rust-timer commented Sep 5, 2022

Overall result: no relevant changes - no action needed

Footnotes

craterbot commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

tmandry commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

craterbot commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

craterbot commented Sep 6, 2022

craterbot commented Sep 6, 2022

compiler-errors commented Sep 6, 2022

compiler-errors commented Sep 7, 2022

Mark-Simulacrum commented Sep 7, 2022

tmandry commented Sep 7, 2022

compiler-errors commented Sep 7, 2022

tmandry commented Sep 7, 2022

bors commented Sep 7, 2022

compiler-errors commented Sep 8, 2022

bors commented Sep 8, 2022

bors commented Sep 8, 2022

rust-timer commented Sep 9, 2022

Overall result: ❌ regressions - ACTION NEEDED

Footnotes

nnethercote commented Sep 13, 2022

Normalize before erasing late-bound regions in `equal_up_to_regions` #101437

Normalize before erasing late-bound regions in `equal_up_to_regions` #101437

compiler-errors commented Sep 5, 2022 •

edited

Loading

compiler-errors commented Sep 5, 2022 •

edited

Loading