New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NLL should identify and respect the lifetime annotations that the user wrote #48482

Merged
merged 17 commits into from Mar 24, 2018

Conversation

Projects
None yet
4 participants
@davidtwco
Member

davidtwco commented Feb 23, 2018

Part of #47184.

r? @nikomatsakis

@davidtwco

This comment has been minimized.

Member

davidtwco commented Feb 23, 2018

As of writing, this PR is not complete. It doesn't actually solve the issue yet, it just contains what I've got so far.

It adds the majority of what was included in the mentoring instructions (I have not yet added the special case in the renumbering code as I wasn't sure about that).

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Feb 23, 2018

@davidtwco nice! will look soon

@davidtwco davidtwco closed this Feb 25, 2018

@davidtwco davidtwco deleted the davidtwco:issue-47184 branch Feb 25, 2018

@davidtwco davidtwco restored the davidtwco:issue-47184 branch Feb 27, 2018

@davidtwco davidtwco reopened this Feb 27, 2018

})
}));
} else {
this.visit_bindings(&pattern, &mut |this, _, _, node, span, _| {
this.storage_live_binding(block, node, span);
if let Some(ty) = ty {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

So, this won't work for something like let (a, b): (u32, u32) = (22, 44); -- it will assert that both a and b have type (u32, u32), I believe.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

I'm not sure how most elegantly to handle this case right now -- one way would be to track the ty as we visit the bindings, so that we can destructure it appropriately. Another would be to "build back up" the value from the types of the bindings. I don't like either one, unfortunately :(

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

(I wonder if we can push the problem to typeck, actually, since it already has to do "destructure" the type hint, I think?)

@@ -145,8 +145,18 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
end_block.unit()
}
pub fn user_assert_ty(&mut self, block: BasicBlock, ty: Ty<'tcx>, var: NodeId, span: Span) {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

I've been debating whether my idea to make this a statement was good or not -- but I do see some advantages. For example, it neatly addresses the question of where this subtyping needs to be enforced.

statement: &mut Statement<'tcx>,
location: Location) {
if let StatementKind::UserAssertTy(..) = statement.kind {
statement.kind = StatementKind::Nop;

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

Nit: statement.make_nop();

This comment has been minimized.

@davidtwco

davidtwco Mar 1, 2018

Member

Fixed this.

@@ -1117,6 +1117,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"make unnamed regions display as '# (where # is some non-ident unique id)"),
emit_end_regions: bool = (false, parse_bool, [UNTRACKED],
"emit EndRegion as part of MIR; enable transforms that solely process EndRegion"),
include_user_assert_ty: bool = (false, parse_bool, [UNTRACKED],
"include UserAssertTy as part of MIR dumps"),

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

I think we should just always include it...?

This comment has been minimized.

@davidtwco

davidtwco Feb 27, 2018

Member

Will remove this flag in a future commit.

This comment has been minimized.

@davidtwco

davidtwco Mar 1, 2018

Member

Removed this.

@@ -54,8 +54,10 @@ impl MirPass for CleanupPostBorrowck {
delete.visit_mir(mir);
}
let mut delete = DeleteUserAssertTy;
delete.visit_mir(mir);
if !tcx.sess.opts.debugging_opts.include_user_assert_ty {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

Also, it seems like this flag isn't quite what it suggests -- that is, it doesn't "include it" in the dump, it "stops deleting it". Can't you just use -Ζdump-mir and inspect the results from earlier passes? For example, I generally do -Zdump-mir=nll

This comment has been minimized.

@davidtwco

davidtwco Feb 27, 2018

Member

I completely forgot I could view the MIR from earlier passes, so yeah, I'll remove this flag.

This comment has been minimized.

@davidtwco

davidtwco Mar 1, 2018

Member

Removed this.

@@ -776,6 +776,7 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
| StatementKind::InlineAsm { .. }
| StatementKind::EndRegion(_)
| StatementKind::Validate(..)
| StatementKind::UserAssertTy(..)

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 27, 2018

Contributor

I think we want to do a check here -- something pretty similar to what Assign does. i.e., get the type of the place whose type is being asserted and invoke self.sub_types() (or actually eq_type perhaps).

This comment has been minimized.

@davidtwco

davidtwco Feb 27, 2018

Member

I intended to add a check here in a future commit, I initially included the variant here just so I could see if being included in the MIR without it doing anything.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 28, 2018

Contributor

seems fine

This comment has been minimized.

@davidtwco

davidtwco Mar 1, 2018

Member

Added something that uses eq_type, not sure it's doing 100% of what it needs to though.

let local_ty = mir.local_decls()[*local].ty;
debug!("check_stmt: user_assert_ty ty={:?} local_ty={:?}", ty, local_ty);
if let Err(terr) =
self.eq_types(ty, local_ty, location.at_successor_within_block())

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 1, 2018

Contributor

I think this should be at_self_within_block, though it probably doesn't matter. But this basically means that the type must be equal "on entry" to the statement, which feels "intuitively" right to me.

@davidtwco

This comment has been minimized.

Member

davidtwco commented Mar 2, 2018

So, with everything that is implemented at current, I'm not seeing the error from UserAssertTy being produced.

Currently, the eq_type call is comparing the following:

DEBUG:<unknown>: check_stmt: user_assert_ty ty=std::vec::Vec<&'static std::string::String> local_ty=std::vec::Vec<&'static std::string::String>

I've experimented with ways to support the let (a, b): (u32, u32) = (22, 44); case mentioned in the previous review regarding this, but also couldn't find a particularly elegant way to handle this in the parts of the code that are currently being changed.

I'm uncertain what the next steps for this PR are? @nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Mar 5, 2018

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

@bors

This comment has been minimized.

Contributor

bors commented Mar 8, 2018

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

@nikomatsakis

You are also missing changes to the renumberer. In particular, I think we want to override visit_user_assert_ty in that impl to do nothing, so that we preserve the user's types.

I think this should fix your test case!

Sorry for the super delayed feedback.

@@ -42,15 +42,15 @@ impl MirPass for CleanEndRegions {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
_source: MirSource,
mir: &mut Mir<'tcx>) {
if !tcx.emit_end_regions() { return; }
if tcx.emit_end_regions() {

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 9, 2018

Contributor

any particular reason for this hunk, or just cleanup? (I sort of prefer the "return early" variant)

This comment has been minimized.

@davidtwco

davidtwco Mar 9, 2018

Member

So do I actually, this was just left-over from when I had CleanEndRegions and CleanUserAssertTy combined as a single pass and I didn't want an early return to stop the deletion of the UserAssertTy. I can change it back.

@@ -619,6 +629,11 @@ macro_rules! make_mir_visitor {
}
}
fn super_user_assert_ty(&mut self,
_ty: & $($mutability)* Ty<'tcx>,
_local: & $($mutability)* Local) {

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 9, 2018

Contributor

You are missing some things here:

self.visit_ty(ty);
self.visit_local(local);
@davidtwco

This comment has been minimized.

Member

davidtwco commented Mar 10, 2018

It's now causing an error as we would expect (though, almost certainly not the error we want in the end). If I'm not mistaken, we still need to handle the let (a, b): (u32, u32) = (22, 44); case and where let x: &u32 = ...; needs to be distinguished from let x: &'static u32 = ...; (as per the original instructions).

Normally I'd copy the error from the AST borrowck, but I notice that on nightly with the AST borrowck the error is an ICE while on stable with the AST borrowck it is a normal error. Just noticed this while writing, so until now I wasn't sure what error it should have been - was always seeing the nightly ICE while working locally. Might have been going a bit mad when I wrote this, will be addressed in future commits.

fn main() {
let vec: Vec<&'static String> = vec![&String::new()];
//~^ ERROR

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 12, 2018

Contributor

What error do we get now?

This comment has been minimized.

@davidtwco

davidtwco Mar 12, 2018

Member

As far as I can remember (not able to run this at the moment), we got an ICE produced by this section of the code.

Though, strangely, Travis isn't showing this. Perhaps I got mixed up and saw something else.

}
}
pub struct CleanUserAssertTy;

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 12, 2018

Contributor

why are these two distinct things?

This comment has been minimized.

@davidtwco

davidtwco Mar 12, 2018

Member

If you are referring to the separation of the passes, in my experimentation locally, I found that the passes had to happen at different times otherwise the UserAssertTy statement wouldn't be in the MIR when the NLL type check code gets it (since the type check pass doesn't run when #![feature(nll)] was used - as far as I could tell - ordering it relative to that wasn't viable). Therefore they had to be separate. If this doesn't sound right, I can take another look.

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 13, 2018

Contributor

Hmm. I didn't realize that these passes were running at distinct times. At minimum, could use a comment clarifying what is happening here =)

This comment has been minimized.

@davidtwco

davidtwco Mar 13, 2018

Member

I'll add one with my next push.

This comment has been minimized.

@davidtwco

davidtwco Mar 13, 2018

Member

This discussion should be resolved - I updated the comment on the module to mention this. It doesn't affect these lines specifically so this isn't marked as "outdated".

@@ -118,6 +118,11 @@ impl<'a, 'gcx, 'tcx> MutVisitor<'tcx> for NLLVisitor<'a, 'gcx, 'tcx> {
debug!("visit_closure_substs: substs={:?}", substs);
}
fn visit_user_assert_ty(&mut self, _ty: &mut Ty<'tcx>, _local: &mut Local,
_location: Location) {
debug!("visit_user_assert_ty: skipping renumber");

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 12, 2018

Contributor

It'd be nice to have a comment here. Something like:


User-assert-ty statements represent types that the user added explicitly.
We don't want to erase the regions from these types: rather, we want to
add them as constraints at type-check time.

This comment has been minimized.

@davidtwco

davidtwco Mar 12, 2018

Member

Will add this as soon as I get a chance.

@davidtwco

This comment has been minimized.

Member

davidtwco commented Mar 13, 2018

I've updated the test in this PR with the expected error that we see in the AST borrowck. What was happening is the following:

We were seeing an ICE occuring in the to_region_vid call from this section of the code. This was what I mistakenly thought was the expected error from above - oops!

This is just because it is visiting the type that didn't get renumbered, which we don't want, so I've overridden the visit_user_assert_ty fn here in the newest commits. It might be the case that those statements should have been removed by our pass by the time it got to this point.

I'm not entirely sure where our DeleteUserAssertTy pass should happen, if I put it at the same place the CleanEndRegions pass happens then the statement is removed before the user type assertion check. That's why those two passes are separate and for now, I've just put the DeleteUserAssertTy pass somewhere that I found it worked.

Upon doing this, I expected to see no error be reported - as I previously wasn't seeing the ICE that I thought would be thrown where our user type assertion is performed. Digging in a little, it seems I made a bad assumption regarding all that eq_types actually does (from a cursory glance, I assumed that when it returned Err(..) then the types didn't match and that's where we'd handle error throwing, that evidently is not the case). Given that, it seems like we're actually seeing this do what it is supposed to now (locally the issue-47184 test passes with the expected UI output) - yay!

Looking over initial instructions and previous comments, this still doesn't handle the let (a, b): (u32, u32) = (22, 44); case and the case where let x: &u32 = ...; needs to be distinguished from let x: &'static u32 = ...;.

@davidtwco

This comment has been minimized.

Member

davidtwco commented Mar 13, 2018

After chatting with @nikomatsakis on Gitter, here's a list of everywhere within function bodies where we annotate types. Moved this to the issue.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Mar 13, 2018

@davidtwco turbofish covers more cases than that. Example:

let x = SomeStruct::<'static> { field: u32 };

So really it's most paths. I guess it'd be useful though to try and go over the kinds of places paths can appear that matter:

  • struct names
  • value references (e.g., let x = foo::<...>; where foo is a fn or something)
  • method calls (foo.method::<arg>)
  • fully qualified paths (<T as ..>::method)

(Also worth noting is that these paths can all include inference variables like _ or '_, and we don't want to record those variables, but let's ignore that for now, I have thoughts there but #48411 has to land first)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Mar 13, 2018

that said, can we move this list to the issue, not this PR?

@davidtwco

This comment has been minimized.

Member

davidtwco commented Mar 13, 2018

@nikomatsakis alright, moved that list to the issue. Will add those to the list.

@davidtwco

This comment has been minimized.

Member

davidtwco commented Mar 13, 2018

I've noticed the errors on Travis. Not sure what is different, but I'm struggling to re-produce them. Here's what I'm seeing locally, will keep looking into it. Might just need to do a clean rebuild.

Update: So it seems like the drop-may-dangle.rs and drop-may-not-dangle.rs failures occur with the -Z nll argument. In particular, it fails with one of the calls to to_region_vid in this function. This seems to be when trying to generate constraints for (in the case of drop-may-dangle.rs) bb0[3] which as far as I can gather is a UserAssertTy statement. Not managed to re-produce the issue-47184 test result yet.

Update: Fixed the issue-47184 test on Travis - the result it was showing was correct and was a result of a PR that landed since I branched off - rebasing fixed it. Still looking into the other two.

Update: Pushed the issue-47184 fix. @nikomatsakis has identified the issues causing the other two fails.

@bors

This comment has been minimized.

Contributor

bors commented Mar 13, 2018

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

@davidtwco

This comment has been minimized.

Member

davidtwco commented Mar 15, 2018

I've just pushed a commit that implements the change described in the issue to handle inference variables. This solves the issue that was plauging the drop-may-dangle.rs and drop-may-not-dangle.rs tests, however it no longer passes our original issue-47184 test case.

When we create our canonicalized type and store it in the TypeckTables, we see the following (where o_ty is the original type and c_ty is the canonicalized type):

DEBUG 2018-03-15T18:52:49Z: rustc_typeck::check: visit_local: ty.hir_id=HirId { owner: DefIndex(0:3), local_id: ItemLocalId(2) } o_ty=std::vec::Vec<&'static std::string::String> c_ty=Canonical { variables: Slice([CanonicalVarInfo { kind: Region }]), value: std::vec::Vec<&'_ std::string::String> }

In the above, we notice that after canonicalizing the type, we lose the 'static lifetime. Continuing, when creating the UserAssertTy statement, we see what we would expect, the same canonicalized type being fetched from the TypeckTables for the statement:

DEBUG 2018-03-15T18:52:49Z: rustc_mir::build::matches: user_assert_ty: local_id=ItemLocalId(2)
DEBUG 2018-03-15T18:52:49Z: rustc_mir::build::matches: user_assert_ty: c_ty=Canonical { variables: Slice([CanonicalVarInfo { kind: Region }]), value: std::vec::Vec<&'_ std::string::String> }

Finally, when processing the statement and performing our type check, we see the following:

DEBUG 2018-03-15T18:55:41Z: rustc_mir::borrow_check::nll::type_check: check_stmt: UserAssertTy(Canonical { variables: Slice([CanonicalVarInfo { kind: Region }]), value: std::vec::Vec<&'_ std::string::String> }, _1)
DEBUG 2018-03-15T18:55:41Z: rustc_mir::borrow_check::nll::type_check: check_stmt: user_assert_ty ty=std::vec::Vec<&std::string::String> local_ty=std::vec::Vec<&std::string::String>

We see that the canonical type (Canonical { variables: Slice([CanonicalVarInfo { kind: Region }]), value: std::vec::Vec<&'_ std::string::String> }) is the same as when we created the statement - great! However, we see that when we instantiate the canonical with the fresh inference variables, we get the type std::vec::Vec<&std::string::String> which is being compared with our local std::vec::Vec<&std::string::String> - and as such, no error will be produced.

Previously, this would compare our type std::vec::Vec<&'static std::string::String> with std::vec::Vec<&std::string::String> and that would correctly cause our error to be produced. This seems to be an issue with canonicalize_query (or my usage of it).

davidtwco and others added some commits Mar 13, 2018

UserAssertTy can handle inference variables.
This commit modifies the UserAssertTy statement to take a canonicalized
type rather than a regular type so that we can handle the case where the
user provided type contains a inference variable.

davidtwco added some commits Mar 23, 2018

@davidtwco davidtwco changed the title from WIP: NLL should identify and respect the lifetime annotations that the user wrote to NLL should identify and respect the lifetime annotations that the user wrote Mar 23, 2018

@nikomatsakis

Awesome. Left a few nits but this is basically good to go.

/// let (a, b): (T, U) = y;
///
/// Here we would insert a `UserAssertTy<(T, U)>(y)` instruction to check that the type of `y`
/// is the right thing.

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 23, 2018

Contributor

It is worth adding to the docs that the Canonical is used to capture "inference variables" from user's types.

For example:

let x: Vec<_> = ...;
let y: &u32 = ...;

would result in Vec<?0> and &'?0 u32 respectively (where ?0 is a canonicalized variable).

This comment has been minimized.

@davidtwco

davidtwco Mar 23, 2018

Member

This has been added but it is slightly below the reviewed line so GitHub hasn't noticed.

@@ -344,6 +345,9 @@ pub struct TypeckTables<'tcx> {
/// method calls, including those of overloaded operators.
type_dependent_defs: ItemLocalMap<Def>,
/// Stores the canonicalized types provided by the user.
user_provided_tys: ItemLocalMap<CanonicalTy<'tcx>>,

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 23, 2018

Contributor

Maybe add a "see also: the UserAssertTy statement in MIR" or something like that?

let (ty, _) = self.infcx.instantiate_canonical_with_fresh_inference_vars(
stmt.source_info.span, c_ty);
debug!("check_stmt: user_assert_ty ty={:?} local_ty={:?}", ty, local_ty);
if let Err(terr) = self.eq_types(ty, local_ty, location.at_self()) {

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 23, 2018

Contributor

👍

})
}));
} else {
// FIXME: We currently only insert `UserAssertTy` statements for patterns

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 23, 2018

Contributor

Can we say FIXME(#47184)? We no longer mandate it, but I still prefer to have issue labels where we can.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Mar 23, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Mar 23, 2018

📌 Commit 4161ae7 has been approved by nikomatsakis

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2018

Rollup merge of rust-lang#48482 - davidtwco:issue-47184, r=nikomatsakis
NLL should identify and respect the lifetime annotations that the user wrote

Part of rust-lang#47184.

r? @nikomatsakis
@bors

This comment has been minimized.

Contributor

bors commented Mar 24, 2018

⌛️ Testing commit 4161ae7 with merge ab0ef14...

bors added a commit that referenced this pull request Mar 24, 2018

Auto merge of #48482 - davidtwco:issue-47184, r=nikomatsakis
NLL should identify and respect the lifetime annotations that the user wrote

Part of #47184.

r? @nikomatsakis
@bors

This comment has been minimized.

Contributor

bors commented Mar 24, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing ab0ef14 to master...

@bors bors merged commit 4161ae7 into rust-lang:master Mar 24, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

bors added a commit that referenced this pull request Mar 24, 2018

bors added a commit that referenced this pull request Mar 24, 2018

@davidtwco davidtwco deleted the davidtwco:issue-47184 branch Mar 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment