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 should respect lifetime annotations from multi-segment paths #54574

Closed
nikomatsakis opened this issue Sep 25, 2018 · 1 comment · Fixed by #55093
Closed

nll should respect lifetime annotations from multi-segment paths #54574

nikomatsakis opened this issue Sep 25, 2018 · 1 comment · Fixed by #55093
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

This example should not compile:

#![feature(nll)]

struct A<'a> { x: &'a u32 }

impl<'a> A<'a> {
    fn new(x: &'a u32) -> Self {
        Self { x }
    }
}

fn foo<'a>() {
    let v = 22;
    let x = A::<'a>::new(&v);
}

fn main() {}

But it does =)

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal labels Sep 25, 2018
@nikomatsakis nikomatsakis added this to the Edition 2018 RC 2 milestone Sep 25, 2018
@mikhail-m1 mikhail-m1 self-assigned this Sep 26, 2018
@nikomatsakis
Copy link
Contributor Author

I did some investigation. I believe this problem is specific to inherent methods. There a bit of a challenge here:

The substitutions on the impl are not directly given by the user. The user only gives the self-type. The instantiate_value_path function handles this via unification:

if let Some((ty::ImplContainer(impl_def_id), self_ty)) = ufcs_associated {
// In the case of `Foo<T>::method` and `<Foo<T>>::method`, if `method`
// is inherent, there is no `Self` parameter, instead, the impl needs
// type parameters, which we can infer by unifying the provided `Self`
// with the substituted impl type.
let ty = self.tcx.type_of(impl_def_id);
let impl_ty = self.instantiate_type_scheme(span, &substs, &ty);
match self.at(&self.misc(span), self.param_env).sup(impl_ty, self_ty) {
Ok(ok) => self.register_infer_ok_obligations(ok),
Err(_) => {
span_bug!(span,
"instantiate_value_path: (UFCS) {:?} was a subtype of {:?} but now is not?",
self_ty,
impl_ty);
}
}
}

which is pretty reasonable, but it does't work for us. The end result is that when we (later) invoke write_user_substs_from_substs the region information has been lost:

self.write_user_substs_from_substs(hir_id, substs);

In fact, it's mildly invalid to call write_user_substs_from_substs at that point -- it's meant to be called before we start doing unifications, actually, although in this case it's sort of ok since the only unification we've done comes pretty directly from user-given types.

I don't think there's a super nice way to fix this. We might have to propagate the "user-given self type" all the way down to MIR, actually: in particular I don't think we can readily translate a "user-given self type" into a set of substitutions, due to annoying things like higher-ranked functions, for which equality is kind of complex.

bors added a commit that referenced this issue Oct 16, 2018
…, r=pnkfelix

nll type annotations in multisegment path

This turned out to be sort of tricky. The problem is that if you have a path like

```
<Foo<&'static u32>>::bar
```

and it comes from an impl like `impl<T> Foo<T>` then the self-type the user gave doesn't *directly* map to the substitutions that the impl wants. To handle this, then, we have to preserve not just the "user-given substs" we used to do, but also a "user-given self-ty", which we have to apply later. This PR makes those changes.

It also removes the code from NLL relate-ops that handled canonical variables and moves to use normal inference variables instead. This simplifies a few things and gives us a bit more flexibility (for example, I predict we are going to have to start normalizing at some point, and it would be easy now).

r? @matthewjasper -- you were just touching this code, do you feel comfortable reviewing this?

Fixes #54574
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 T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants