Do leak check after function pointer coercion #97206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
jackh726
commented
May 20, 2022
| // 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)?; |
There was a problem hiding this comment.
I will admit that I have no idea if this should be false or true.
There was a problem hiding this comment.
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).
| @@ -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 | |||
There was a problem hiding this comment.
I think these errors disappear because they are borrowck errors, but we now error and bail in typeck.
| @@ -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 | |||
Comment on lines
-55
to
+8
| 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::<'_, '_, '_>}` |
Comment on lines
+11
to
+15
| note: function defined here | ||
| --> $DIR/regions-fn-subtyping-return-static-fail.rs:24:4 | ||
| | | ||
| LL | fn want_G(f: G) {} | ||
| | ^^^^^^ ---- |
jackh726
changed the title
jackh726
force-pushed
the
issue-73154
branch
2 times, most recently
from
8a1dc74 to
a9decbe
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
approved these changes
May 20, 2022
|
📌 Commit 719ef0d has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
May 21, 2022
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`
|
☔ The latest upstream changes (presumably #97239) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
|
@bors r+ |
|
📌 Commit baf1966b3c75fff6174c31da4a0574793bc2ace8 has been approved by |
bors
added
S-waiting-on-bors
bors
removed
the
S-waiting-on-bors
bors
added
the
S-waiting-on-author
|
📌 Commit baf1966b3c75fff6174c31da4a0574793bc2ace8 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r=nikomatsakis |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit baf1966b3c75fff6174c31da4a0574793bc2ace8 has been approved by |
|
Oh thanks @lcnr :) missed that |
|
⌛ Testing commit baf1966b3c75fff6174c31da4a0574793bc2ace8 with merge 66e5c5953352dd6ce2a6cbae77dd187880e38510... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors r=nikomatsakis |
|
📌 Commit 683a9c8 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 22, 2022
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
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 22, 2022
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.
dario23
added a commit
to dario23/rust-semverver
that referenced
this pull request
May 28, 2022
bors
added a commit
to rust-lang/rust-semverver
that referenced
this pull request
May 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc #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_andis a supertype of bothaandbifOk). 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