support ascription for patterns in NLL #53873
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
rust-highfive
added
the
S-waiting-on-review
nikomatsakis
force-pushed
the
nll-universe-subtyping-and-pattern-ascription
branch
from
9bff8b5
to
93cd6a5
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nikomatsakis
force-pushed
the
nll-universe-subtyping-and-pattern-ascription
branch
from
a70eb79
to
c543e2c
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Hmm, I also see this |
|
Changing the code from let mut y: Box<D> = rusti::init();to let mut y = rusti::init::<Box<D>>();averts the crash. The problem seems to be that we are adding a instruction, where |
|
Presumably this has something to do with this special case: rust/src/librustc_mir/build/matches/mod.rs Lines 245 to 264 in d8af8b6 |
|
This is basically the only way that the final code differs: presumably something here is illegal -- I guess the |
|
I just realized while working through this bug that we also have to make sure that NLL enforces let ref mut x: T = <expr>We historically had some soundness issues there, because we only enforced that That said, I don't think that would affect NLL, but perhaps for a sort of confusing reason. Under this PR anyway, we will enforce that Perhaps that is OK, though. That is, we still have to handle cases like |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
force-pushed
the
nll-universe-subtyping-and-pattern-ascription
branch
from
836855e
to
4a4099c
Compare
|
☔ The latest upstream changes (presumably #53575) made this pull request unmergeable. Please resolve the merge conflicts. |
pnkfelix
reviewed
Sep 7, 2018
| assert!(!k.has_escaping_regions()); | ||
|
|
||
| k | ||
| // These always correspond to an `_` or `'_` written by |
There was a problem hiding this comment.
I just want to make sure I'm understanding this comment: You are drawing this conclusion that the type (or region) is always either _ or '_ ... because that is the only scenario where canonical_var_values[var] is allowed to return None?
pnkfelix
reviewed
Sep 7, 2018
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
||
| // Test that the NLL `relate_tys` code correctly deduces that a |
There was a problem hiding this comment.
This comment appears to be a cut-and-paste mistake left over from hr-fn-aba-as-aaa.rs
pnkfelix
reviewed
Sep 7, 2018
| let b = 44; | ||
|
|
||
| // Here we get an error because `DoubleCell<_>` requires the same type | ||
| // on both parts of the `Cell`, and we can't have that. |
There was a problem hiding this comment.
it probably wouldn't hurt, if you get the chance, to add a parenthetical reminding the reader that the type Cell<X> is invariant with respect to X (which is why, IIUC, the return type of make_cell here cannot be "simply upcast" from Cell(&'static _, &'b _) up to Cell(&'b _, &'b _).
There was a problem hiding this comment.
(basically I stopped for a beat when I read "... and we can't have that", because I needed to reason through the steps of why the two parts could not be unified to one type.)
pnkfelix
reviewed
Sep 7, 2018
| // Reset the ambient variance to covariant. This is needed | ||
| // to correctly handle cases like | ||
| // | ||
| // for<'a> fn(&'a u32, &'a u3) == for<'b, 'c> fn(&'b u32, &'c u32) |
pnkfelix
reviewed
Sep 7, 2018
| @@ -16,9 +16,7 @@ | |||
| // another -- effectively, the single lifetime `'a` is just inferred | |||
There was a problem hiding this comment.
Github will not let me leave a comment directly on the relevant line, because it is not part of the diff here, but starting on line 11 there is a parenthetical that says "(though the current code actually gets it wrong, see below)"; you should remove that parenthetical as part of this commit.
pnkfelix
reviewed
Sep 7, 2018
| | | ||
| LL | fn bar<'a, 'b, I : for<'x> Foo<&'x isize>>( | ||
| | -- -- lifetime `'b` defined here | ||
| | | | ||
| | lifetime `'a` defined here | ||
| ... | ||
| LL | let z: I::A = if cond { x } else { y }; | ||
| | ^ assignment requires that `'a` must outlive `'b` | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^ requires that `'a` must outlive `'b` |
There was a problem hiding this comment.
oops apparently that 🎉 was premature. 😄 Luckily this change was not the point per se of the PR; I just (subjectively) thought it might have been a happy accident.
pnkfelix
reviewed
Sep 7, 2018
| @@ -35,7 +35,7 @@ error[E0596]: cannot borrow `*ptr_x` as mutable, as it is behind a `*const` poin | |||
| --> $DIR/borrowck-access-permissions.rs:56:23 | |||
| | | |||
| LL | let ptr_x : *const _ = &x; | |||
| | -- help: consider changing this to be a mutable pointer: `&mut x` | |||
| | ----- help: consider changing this to be a mutable pointer: `*mut i32` | |||
There was a problem hiding this comment.
this is interesting; hypothetically we probably would prefer, if possible to highlight the ascribed type*const _ here, right?
pnkfelix
reviewed
Sep 7, 2018
| error[E0597]: borrowed value does not live long enough | ||
| --> $DIR/dont_promote_unstable_const_fn.rs:28:28 | ||
| | | ||
| LL | let _: &'static u32 = &foo(); //~ ERROR does not live long enough |
pnkfelix
reviewed
Sep 7, 2018
| LL | let x: Box<Foo + 'static> = Box::new(v); | ||
| | ^^^^^^^^^^^ lifetime `'static` required | ||
| ... | ||
| LL | x |
There was a problem hiding this comment.
This change in diagnostic seems unfortunate.
After all, the user wrote down the type they wanted x to have, and that type exactly matches the return type of the function, so having the diagnostic blame the point to where x is returned, rather than the point where x was initialized (i.e. the prior behavior), seems subpar to me.
Let me try to guess: Is it something where we do not use the ascribed type from let x: Box<Foo + 'static> as the type for x itself (and instead are using the lifetime that resulted from region inference in our internal type for x), and thus we subsequently error when we try to return x as the result in a context expecting Box<Foo + 'static>?
Or is there something else going on?
In any case, I won't block landing this PR based on this issue. But we may want to file a follow-up issue about it, since it seems like something we should try to address eventually, if possible.
pnkfelix
reviewed
Sep 7, 2018
src/test/ui/slice-mut-2.nll.stderr
Outdated
| @@ -2,7 +2,7 @@ error[E0596]: cannot borrow `*x` as mutable, as it is behind a `&` reference | |||
| --> $DIR/slice-mut-2.rs:17:18 | |||
| | | |||
| LL | let x: &[isize] = &[1, 2, 3, 4, 5]; | |||
| | ---------------- help: consider changing this to be a mutable reference: `&mut [1, 2, 3, 4, 5]` | |||
| | - help: consider changing this to be a mutable reference: `&mut [isize]` | |||
There was a problem hiding this comment.
hmm, I'm again surprised that we highlight x here, and not the ascribed type. I thought we had code in place that tried to select the span of the explicitly provided type in scenarios like this.
|
Do you have a unit test illustrating the effect of the commit "generalize |
|
Very interesting that the commit "optimize The big one I still want an answer about is this one: #53873 (review) Just to elaborate on that linked comment, here is the test code: The oddity: The NLL diagnostics, at least temporarily, were highlighting the return of In particular, I want to know if there are other potential scenarios where we produce a diagnostic that is similarly "bad", in the sense that it may frustrate a user who thinks intuitively that if they wrote |
|
This seems fine. I have nits, and there are known bugs in the PR. But its an improvement on our current status, which may mean we might want to land this PR even in its current state... Having said that, I'll not issue any bors commands until niko gets a chance to finish resolving the remaining issues on this PR, or at least weigh in on whether to land it as is. |
pnkfelix
reviewed
Sep 10, 2018
| INDENT, | ||
| indent, | ||
| mut_str, | ||
| local, | ||
| var.ty | ||
| ); | ||
| if let Some(user_ty) = var.user_ty { | ||
| write!(indented_var, " as {:?}", user_ty).unwrap(); |
There was a problem hiding this comment.
hmm. I'd have to think about this syntax and whether it makes sense. (But I won't let that block landing this PR; the printed MIR output format is unstable, afterall...)
There was a problem hiding this comment.
yeah it...probably doesn't make sense =)
pnkfelix
reviewed
Sep 10, 2018
| let x = 22; | ||
| let y: &'static u32; | ||
| y = &x; //~ ERROR |
|
@bors r+ |
|
📌 Commit 2b6f966 has been approved by |
bors
added
S-waiting-on-bors
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors r- Travis failure. I think I know what the problem is, I thought that interaction seemed suspicious. ( |
bors
added
S-waiting-on-author
nikomatsakis
force-pushed
the
nll-universe-subtyping-and-pattern-ascription
branch
from
30c5572
to
df37678
Compare
|
@bors r=pnkfelix |
|
📌 Commit df37678 has been approved by |
bors
added
S-waiting-on-bors
27 tasks
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
We are now carrying the user-given type through MIR, so it makes sense that this would change the hash.
|
@bors r=pnkfelix |
|
📌 Commit f95f23f has been approved by |
|
@bors p=1 Edition blocker |
bors
added a commit
that referenced
this pull request
Sep 11, 2018
…n-ascription, r=pnkfelix support ascription for patterns in NLL This implements the strategy outlined in [this comment](#47184 (comment)): - We first extend the NLL subtyping code so it can handle inference variables and subtyping. - Then we extend HAIR patterns with type ascription. - Then we treat the type `T` in `let pat: T = ...` as an ascription. Before landing, a few things: - [x] Fix the WF rule bug (filed a FIXME #54105) - [x] Fix an ICE I encountered locally around bound regions, or else file a follow-up - [x] More tests probably =) r? @pnkfelix
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Sep 11, 2018
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.
This implements the strategy outlined in this comment:
Tinlet pat: T = ...as an ascription.Before landing, a few things:
r? @pnkfelix