Fix implied outlives bounds logic for projections #101680

jackh726 · 2022-09-11T08:44:02Z

The logic here is subtly wrong. I put a bit of an explanation in a767d7b5165cea8ee5cbe494a4a636c50ef67c9c.

TL;DR: we register outlives predicates to be proved, because wf code normalizes projections (from the unnormalized types) to type variables. This causes us to register those as constraints instead of implied. This was "fine", because we later added that implied bound in the normalized type, and delayed registering constraints. When I went to cleanup free_region_relations to not delay adding constraints, this bug was uncovered.

cc. @aliemjay because this caused your test failure in #99832 (I only realized as I was writing this)

r? @nikomatsakis

compiler/rustc_traits/src/implied_outlives_bounds.rs

jackh726 · 2022-09-11T08:44:59Z

@bors try

bors · 2022-09-11T08:45:08Z

⌛ Trying commit 79367191d3c2148648e4167e4e366a538c2f7650 with merge 77b749d3ae4f115e3b8cef8a0e20da0998461320...

bors · 2022-09-11T09:08:31Z

💔 Test failed - checks-actions

jackh726 · 2022-09-11T09:11:38Z

Spurious?

@bors try

bors · 2022-09-11T09:11:45Z

⌛ Trying commit 79367191d3c2148648e4167e4e366a538c2f7650 with merge d752302224558bd75a97bd19853f642147a9d13b...

bors · 2022-09-11T09:36:26Z

💔 Test failed - checks-actions

jackh726 · 2022-09-11T16:41:00Z

@bors try

bors · 2022-09-11T16:41:09Z

⌛ Trying commit f8f8a50 with merge ee93ebe8ef8a3e16bffdd473f324fc515dec2948...

bors · 2022-09-11T17:12:30Z

💔 Test failed - checks-actions

compiler/rustc_traits/src/implied_outlives_bounds.rs

jackh726 · 2022-09-12T01:35:03Z

@bors try

bors · 2022-09-12T01:35:10Z

⌛ Trying commit 9e6234b294bc5aae0b6fcf8d7b0a73e55459d67d with merge e79c9799a5ffd821c63b799b91e8aa87fd7ebb1e...

bors · 2022-09-12T03:04:47Z

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

jackh726 · 2022-09-12T03:21:57Z

@craterbot check

lcnr

nits, then r=me

lcnr · 2023-01-13T12:45:23Z

compiler/rustc_borrowck/src/type_check/free_region_relations.rs

+        let param_env = self.param_env;
+        self.add_outlives_bounds(outlives::explicit_outlives_bounds(param_env));
+
+        // Finally:


"Finally:" seems wrong, considering that we add the implied bounds further down 😁

lcnr · 2023-01-13T12:47:05Z

compiler/rustc_borrowck/src/type_check/free_region_relations.rs

+            // We add implied bounds from both the unnormalized and normalized ty.
+            // See issue #87748
+            let constraints_unnorm = self.add_implied_bounds(ty);
+            constraints_unnorm.map(|c| constraints.push(c));


can you use if let here? map with side effects always looks a bit scary to me

lcnr · 2023-01-13T12:48:41Z

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -918,6 +918,8 @@ pub(crate) struct MirTypeckRegionConstraints<'tcx> {
 }

 impl<'tcx> MirTypeckRegionConstraints<'tcx> {
+    /// Creates a `Region` that for a given `PlaceholderRegion`, or returns the


"Creates a Region that for a given PlaceholderRegion"?

apiraino · 2023-02-02T14:17:08Z

Switching to waiting on author to incorporate last changes (see comment) then we should be set.

@rustbot author

rustbot · 2023-02-10T01:55:44Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred in src/tools/cargo

cc @ehuss

…hind normalization

jackh726 · 2023-02-10T02:16:17Z

@bors r=lcnr rollup=never

bors · 2023-02-10T02:16:20Z

📌 Commit 0637b6b has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-02-10T03:21:42Z

⌛ Testing commit 0637b6b with merge a697573...

bors · 2023-02-10T06:19:54Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing a697573 to master...

rust-timer · 2023-02-10T07:46:06Z

Finished benchmarking commit (a697573): 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

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)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	2.6%	[1.6%, 3.2%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[3.1%, 3.1%]	1

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.3%	[1.6%, 2.9%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

rust-highfive assigned nikomatsakis Sep 11, 2022

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

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

jackh726 commented Sep 11, 2022

View reviewed changes

compiler/rustc_traits/src/implied_outlives_bounds.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2022

This comment has been minimized.

Sign in to view

jackh726 force-pushed the implied-cleanup branch from 7936719 to f8f8a50 Compare September 11, 2022 16:40

jackh726 mentioned this pull request Sep 11, 2022

Need new test for incomplete compute_implied_outlives_bounds fulfillment #101694

Open

aliemjay reviewed Sep 11, 2022

View reviewed changes

compiler/rustc_traits/src/implied_outlives_bounds.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot assigned lcnr and unassigned oli-obk Jan 10, 2023

lcnr reviewed Jan 13, 2023

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2023

Cleanup free_region_relations a bit

1a663c0

jackh726 force-pushed the implied-cleanup branch from b3b3530 to 449824e Compare February 10, 2023 01:55

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Feb 10, 2023

jackh726 force-pushed the implied-cleanup branch 4 times, most recently from 22a8dae to 40e519e Compare February 10, 2023 02:08

Update implied_outlives_bounds to properly register implied bounds be…

0637b6b

…hind normalization

jackh726 force-pushed the implied-cleanup branch from 40e519e to 0637b6b Compare February 10, 2023 02:08

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 10, 2023

bors merged commit a697573 into rust-lang:master Feb 10, 2023

rustbot added this to the 1.69.0 milestone Feb 10, 2023

bors mentioned this pull request Feb 10, 2023

Implement deferred_projection_equality for erica solver #107507

Merged

jackh726 deleted the implied-cleanup branch February 10, 2023 14:13

matthiaskrgr mentioned this pull request Apr 20, 2024

ICE: expected principal trait object #124092

Closed

matthiaskrgr mentioned this pull request May 6, 2024

ICE: used byval ABI for unsized layout #124806

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix implied outlives bounds logic for projections #101680

Fix implied outlives bounds logic for projections #101680

jackh726 commented Sep 11, 2022

jackh726 commented Sep 11, 2022

bors commented Sep 11, 2022

This comment has been minimized.

bors commented Sep 11, 2022

jackh726 commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

This comment has been minimized.

jackh726 commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

This comment has been minimized.

jackh726 commented Sep 12, 2022

bors commented Sep 12, 2022

This comment has been minimized.

bors commented Sep 12, 2022

jackh726 commented Sep 12, 2022

lcnr left a comment

lcnr Jan 13, 2023 •

edited

Loading

lcnr Jan 13, 2023

lcnr Jan 13, 2023

apiraino commented Feb 2, 2023

rustbot commented Feb 10, 2023

jackh726 commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

rust-timer commented Feb 10, 2023

Fix implied outlives bounds logic for projections #101680

Fix implied outlives bounds logic for projections #101680

Conversation

jackh726 commented Sep 11, 2022

jackh726 commented Sep 11, 2022

bors commented Sep 11, 2022

This comment has been minimized.

bors commented Sep 11, 2022

jackh726 commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

This comment has been minimized.

jackh726 commented Sep 11, 2022

bors commented Sep 11, 2022

bors commented Sep 11, 2022

This comment has been minimized.

jackh726 commented Sep 12, 2022

bors commented Sep 12, 2022

This comment has been minimized.

bors commented Sep 12, 2022

jackh726 commented Sep 12, 2022

lcnr left a comment

Choose a reason for hiding this comment

lcnr Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

lcnr Jan 13, 2023

Choose a reason for hiding this comment

lcnr Jan 13, 2023

Choose a reason for hiding this comment

apiraino commented Feb 2, 2023

rustbot commented Feb 10, 2023

jackh726 commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

bors commented Feb 10, 2023

rust-timer commented Feb 10, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

lcnr Jan 13, 2023 •

edited

Loading