Skip to content
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 ignores lifetimes bounds derived from Sized requirements #50716

Closed
matthewjasper opened this issue May 13, 2018 · 3 comments
Closed

NLL ignores lifetimes bounds derived from Sized requirements #50716

matthewjasper opened this issue May 13, 2018 · 3 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal

Comments

@matthewjasper
Copy link
Contributor

This code incorrectly(?) compiles with NLL. Alternatively we could say that Sized bounds are assumed to be lifetime-independent.

#![feature(nll)]
#![allow(unused)]

trait A {
    type X: ?Sized;
}

fn foo<'a, T: 'static>(s: Box<<&'a T as A>::X>)
where
    for<'b> &'b T: A,
    <&'static T as A>::X: Sized
{
    let x = *s; // requires 'a: 'static
}

fn main() {}
cc #44835 - This example gives a similar error when using AST borrowck. cc #48557 - which will make this issue easier to come across. (
@matthewjasper matthewjasper added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll NLL-sound Working towards the "invalid code does not compile" goal labels May 13, 2018
@nikomatsakis
Copy link
Contributor

Hmm. Probably we need to add code to the MIR type-checker that checks Sized bounds.

@nikomatsakis
Copy link
Contributor

It's sort of semi-debatable if this is a bug, but I suppose that it is. (In practice, Sized impls are always lifetime parametric, but users can construct artificial scenarios like the one above, and I dont know what else we should do but respect those annotations.)

So, the general way to fix this is that in the NLL type check we have to check that all local variables have Sized types. There is an interesting question about when we should make that check — i.e., at what position in the source. For the purposes of our current borrow checker, it actually doesn't much matter, but as we move to more location-sensitive things, it may. We can also use Locations::All to enforce the invariant at all points, but I am in the process of refactoring that concept away (see #50938).

Let's say for now we will do it at each place where the variable is assigned, though one could imagine other places. So e.g. if you have X = ..., then that will require that the type of X is sized. In fact, we probably can just add the general requirement that the LHS of any assignment is Sized for now.

This will be quite similar to the code that (for example) proves that all input types are well-formed when calling a function:

self.prove_predicates(
sig.inputs().iter().map(|ty| ty::Predicate::WellFormed(ty)),
term_location,
);

Except that you may want to use the prove_trait_ref helper — a trait ref is something like T: Trait that includes a trait plus the types that (maybe) implement it:

fn prove_trait_ref(&mut self, trait_ref: ty::TraitRef<'tcx>, location: Location) {
self.prove_predicates(
Some(ty::Predicate::Trait(
trait_ref.to_poly_trait_ref().to_poly_trait_predicate(),
)),
location,
);
}

We would insert these calls when checking StmtAssign types for validity:

StatementKind::Assign(ref place, ref rv) => {
let place_ty = place.ty(mir, tcx).to_ty(tcx);
let rv_ty = rv.ty(mir, tcx);
if let Err(terr) =
self.sub_types(rv_ty, place_ty, location.at_successor_within_block())
{
span_mirbug!(
self,
stmt,
"bad assignment ({:?} = {:?}): {:?}",
place_ty,
rv_ty,
terr
);
}
self.check_rvalue(mir, rv, location);
}

bors added a commit that referenced this issue Jun 27, 2018
Fix NLL issue 50716 and add a regression test.

Fix for NLL issue #50716.

r? @nikomatsakis
@nikomatsakis
Copy link
Contributor

Fixed in #51139, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal
Projects
None yet
Development

No branches or pull requests

2 participants