Do leak check after function pointer coercion #97206

jackh726 · 2022-05-20T06:15:14Z

I still need to clean diagnostics just a tad, but figured I would put this up anyways.

This change is made in order to make match arm coercion order-independent.

Basically, any time we do function pointer coercion, we follow it by doing a leak check. This is necessary because the LUB code doesn't handler higher-ranked things correctly, leading us to "coerce", but use the wrong type. A proper fix is to actually fix that code (so the type returned by unify_and is a supertype of both a and b if Ok). However, that requires a more in-depth fix, likely heavily overlapping with the new subtyping changes.

Here, I've been conservative and error early if we generate unsatisfiable constraints. Note, this should mostly only affect NLL, since migrate mode falls back to the LUB implementation (followed by leak check), whereas NLL only does sub.

There could be other coercion code that has an order-dependence where a leak check in the coercion code might be useful. However, this is more of a spot-fix for #73154 than a "permanent" fix, since we likely want to go the other way long-term, and allow this pattern without error.

r? @nikomatsakis

jackh726 · 2022-05-20T06:15:51Z

compiler/rustc_typeck/src/check/coercion.rs

+            // want the coerced type to be the actual supertype of these two,
+            // but for now, we want to just error to ensure we don't lock
+            // ourselves into a specific behavior with NLL.
+            self.leak_check(false, snapshot)?;


I will admit that I have no idea if this should be false or true.

The name is not very good. It only affects diagnostics. I think the idea is that you say true if the "placeholders" result from the "actual type" and false if they come from the expected type.

In this instance, B is the expected type, I think, and the placeholders would come from B, so I guess that makes false the proper value. (i.e., the value you are supplying is insufficiently polymorphic to be cast to the type required).

src/test/ui/hrtb/hrtb-exists-forall-fn.stderr

jackh726 · 2022-05-20T06:18:19Z

src/test/ui/regions/region-lifetime-bounds-on-fns-where-clause.rs

@@ -11,23 +11,20 @@ fn b<'a, 'b>(x: &mut &'a isize, y: &mut &'b isize) {
    // Illegal now because there is no `'b:'a` declaration.
    *x = *y;
    //[base]~^ ERROR E0623
-    //[nll]~^^ ERROR lifetime may not live long enough


I think these errors disappear because they are borrowck errors, but we now error and bail in typeck.

jackh726 · 2022-05-20T06:19:00Z

src/test/ui/regions/regions-lifetime-bounds-on-fns.rs

@@ -11,23 +11,20 @@ fn b<'a, 'b>(x: &mut &'a isize, y: &mut &'b isize) {
    // Illegal now because there is no `'b:'a` declaration.
    *x = *y;
    //[base]~^ ERROR lifetime mismatch [E0623]
-    //[nll]~^^ ERROR lifetime may not live long enough


jackh726 · 2022-05-20T06:19:26Z

src/test/ui/regions/region-multiple-lifetime-bounds-on-fns-where-clause.nll.stderr

-              found fn pointer `for<'r, 's, 't0> fn(&'r mut &isize, &'s mut &isize, &'t0 mut &isize)`
+                 found fn item `for<'r, 's, 't0> fn(&'r mut &isize, &'s mut &isize, &'t0 mut &isize) {a::<'_, '_, '_>}`


This is nice.

jackh726 · 2022-05-20T06:19:36Z

src/test/ui/regions/regions-fn-subtyping-return-static-fail.base.stderr

+note: function defined here
+  --> $DIR/regions-fn-subtyping-return-static-fail.rs:24:4
+   |
+LL | fn want_G(f: G) {}
+   |    ^^^^^^ ----


nikomatsakis · 2022-05-20T19:41:20Z

@bors r+

cc @lcnr -- the work we are doing on improving subtyping will eventually make this unnecessary, if I'm not mistaken.

bors · 2022-05-20T19:41:23Z

📌 Commit 719ef0d has been approved by nikomatsakis

Do leak check after function pointer coercion cc rust-lang#73154 I still need to clean diagnostics just a tad, but figured I would put this up anyways. This change is made in order to make match arm coercion order-independent. Basically, any time we do function pointer coercion, we follow it by doing a leak check. This is necessary because the LUB code doesn't handler higher-ranked things correctly, leading us to "coerce", but use the wrong type. A proper fix is to actually fix that code (so the type returned by `unify_and` is a supertype of both `a` and `b` if `Ok`). However, that requires a more in-depth fix, likely heavily overlapping with the new subtyping changes. Here, I've been conservative and error early if we generate unsatisfiable constraints. Note, this should *mostly* only affect NLL, since migrate mode falls back to the LUB implementation (followed by leak check), whereas NLL only does sub. There could be other coercion code that has an order-dependence where a leak check in the coercion code might be useful. However, this is more of a spot-fix for rust-lang#73154 than a "permanent" fix, since we likely want to go the other way long-term, and allow this pattern without error. r? `@nikomatsakis`

bors · 2022-05-21T09:35:47Z

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

jackh726 · 2022-05-21T14:51:26Z

@bors r+

bors · 2022-05-21T14:51:27Z

📌 Commit baf1966b3c75fff6174c31da4a0574793bc2ace8 has been approved by jackh726

lcnr · 2022-05-21T14:58:20Z

@bors r-
@bors r=nikomatsakis

bors · 2022-05-21T14:58:23Z

📌 Commit baf1966b3c75fff6174c31da4a0574793bc2ace8 has been approved by nikomatsakis

jackh726 · 2022-05-21T15:00:01Z

@bors r=nikomatsakis

bors · 2022-05-21T15:00:03Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Apply PGO to libstd on CI #97038

bors · 2022-05-21T15:00:04Z

📌 Commit baf1966b3c75fff6174c31da4a0574793bc2ace8 has been approved by nikomatsakis

jackh726 · 2022-05-21T15:16:12Z

Oh thanks @lcnr :) missed that

bors · 2022-05-22T11:27:03Z

⌛ Testing commit baf1966b3c75fff6174c31da4a0574793bc2ace8 with merge 66e5c5953352dd6ce2a6cbae77dd187880e38510...

bors · 2022-05-22T11:51:20Z

💔 Test failed - checks-actions

jackh726 · 2022-05-22T15:20:26Z

@bors r=nikomatsakis

bors · 2022-05-22T15:20:28Z

📌 Commit 683a9c8 has been approved by nikomatsakis

Rollup of 5 pull requests Successful merges: - rust-lang#97043 (Move some tests to more reasonable directories) - rust-lang#97206 (Do leak check after function pointer coercion) - rust-lang#97275 (small change) - rust-lang#97277 (Avoid accidentally enabling unstable features in compilers (take 2)) - rust-lang#97280 (Quote replace target in bootstrap configure) Failed merges: - rust-lang#97214 (Finish bumping stage0) r? `@ghost` `@rustbot` modify labels: rollup

Move remaining tests with NLL differences to revisions Based on rust-lang#97206 I've already filed issues for any important differences that I've spotted: rust-lang#97252 rust-lang#97253 rust-lang#97256 rust-lang#97267 There is a lot here, but each commit is self-contained as a separate directory. I can split into separate PRs as wanted or needed.

Rustup to rust-lang/rust#97206

rust-highfive assigned nikomatsakis May 20, 2022

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

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

jackh726 commented May 20, 2022

View reviewed changes

jackh726 force-pushed the issue-73154 branch from 1acd663 to 8a48cd0 Compare May 20, 2022 14:22

jackh726 changed the title ~~[WIP] Do leak check after function pointer coercion~~ Do leak check after function pointer coercion May 20, 2022

jackh726 force-pushed the issue-73154 branch 2 times, most recently from 8a1dc74 to a9decbe Compare May 20, 2022 14:30

jackh726 mentioned this pull request May 20, 2022

Remove migrate borrowck mode #95565

Merged

This comment has been minimized.

Sign in to view

jackh726 force-pushed the issue-73154 branch from a9decbe to e002cc2 Compare May 20, 2022 15:03

This comment has been minimized.

Sign in to view

jackh726 force-pushed the issue-73154 branch from e002cc2 to 719ef0d Compare May 20, 2022 15:31

nikomatsakis approved these changes May 20, 2022

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 May 20, 2022

Dylan-DPC mentioned this pull request May 21, 2022

Rollup of 6 pull requests #97243

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 May 21, 2022

jackh726 force-pushed the issue-73154 branch from 719ef0d to baf1966 Compare May 21, 2022 14:51

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 May 21, 2022

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 21, 2022

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 21, 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 21, 2022

jackh726 mentioned this pull request May 21, 2022

Move remaining tests with NLL differences to revisions #97258

Merged

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 22, 2022

Do leak check after function ptr coercion

683a9c8

jackh726 force-pushed the issue-73154 branch from baf1966 to 683a9c8 Compare May 22, 2022 15:19

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 May 22, 2022

jackh726 mentioned this pull request May 22, 2022

Rollup of 5 pull requests #97283

Merged

bors merged commit 4f97de8 into rust-lang:master May 22, 2022

rustbot added this to the 1.63.0 milestone May 22, 2022

jackh726 deleted the issue-73154 branch May 22, 2022 19:22

dario23 added a commit to dario23/rust-semverver that referenced this pull request May 28, 2022

Rustup to rust-lang/rust#97206

ecbade0

bors added a commit to rust-lang/rust-semverver that referenced this pull request May 28, 2022

Auto merge of #310 - dario23:update-nightly, r=JohnTitor

0431504

Rustup to rust-lang/rust#97206

compiler-errors mentioned this pull request Aug 3, 2022

Remove InferCtxt::commit_unconditionally #100099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do leak check after function pointer coercion #97206

Do leak check after function pointer coercion #97206

jackh726 commented May 20, 2022

jackh726 May 20, 2022

nikomatsakis May 20, 2022

jackh726 May 20, 2022

jackh726 May 20, 2022

jackh726 May 20, 2022

jackh726 May 20, 2022

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented May 20, 2022

bors commented May 20, 2022

bors commented May 21, 2022

jackh726 commented May 21, 2022

bors commented May 21, 2022

lcnr commented May 21, 2022

bors commented May 21, 2022

jackh726 commented May 21, 2022

bors commented May 21, 2022

bors commented May 21, 2022

jackh726 commented May 21, 2022

bors commented May 22, 2022

This comment has been minimized.

bors commented May 22, 2022

jackh726 commented May 22, 2022

bors commented May 22, 2022

		found fn pointer `for<'r, 's, 't0> fn(&'r mut &isize, &'s mut &isize, &'t0 mut &isize)`
		found fn item `for<'r, 's, 't0> fn(&'r mut &isize, &'s mut &isize, &'t0 mut &isize) {a::<'_, '_, '_>}`

Do leak check after function pointer coercion #97206

Do leak check after function pointer coercion #97206

Conversation

jackh726 commented May 20, 2022

jackh726 May 20, 2022

Choose a reason for hiding this comment

nikomatsakis May 20, 2022

Choose a reason for hiding this comment

jackh726 May 20, 2022

Choose a reason for hiding this comment

jackh726 May 20, 2022

Choose a reason for hiding this comment

jackh726 May 20, 2022

Choose a reason for hiding this comment

jackh726 May 20, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented May 20, 2022

bors commented May 20, 2022

bors commented May 21, 2022

jackh726 commented May 21, 2022

bors commented May 21, 2022

lcnr commented May 21, 2022

bors commented May 21, 2022

jackh726 commented May 21, 2022

bors commented May 21, 2022

bors commented May 21, 2022

jackh726 commented May 21, 2022

bors commented May 22, 2022

This comment has been minimized.

bors commented May 22, 2022

jackh726 commented May 22, 2022

bors commented May 22, 2022