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: unconstrained lifetimes can be returned via associated types #47470

Closed
arielb1 opened this issue Jan 15, 2018 · 6 comments
Closed

NLL: unconstrained lifetimes can be returned via associated types #47470

arielb1 opened this issue Jan 15, 2018 · 6 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Jan 15, 2018

This compiles, runs, and is UB (with NLL) - it errors correctly the return type is specified explicitly.

#![feature(nll)]

struct Foo<'a>(&'a ());
trait Bar {
    type Assoc;
    fn get(self) -> Self::Assoc;
}

impl<'a> Bar for Foo<'a> {
    type Assoc = &'a u32;
    // using `-> &'a u32` here results in a correct error
    fn get(self) -> Self::Assoc {
        let local = 42;
        &local
    }
}

fn main() {
    let f = Foo(&()).get();
    println!("{}", f);
}

cc @nikomatsakis

@arielb1 arielb1 added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Jan 15, 2018
@nikomatsakis nikomatsakis added this to the NLL Future Compat Warnings milestone Jan 16, 2018
@nikomatsakis
Copy link
Contributor

I'm guessing the problem has to do with the MIR type checker and how it relates the return type found in the MIR to the declared types. I don't quite see what's going wrong, but here are some pointers to the relevant code. This function is responsible for relating things properly:

pub(super) fn equate_inputs_and_outputs(

You can see that for input types, it normalizes the input type declared in the function signature (unnormalized_input_ty) to yield input_ty and then equates that with the type found in the MIR (mir.local_decls[local].ty):

let input_ty = self.normalize(&unnormalized_input_ty, start_position);
let mir_input_ty = mir.local_decls[local].ty;
self.equate_normalized_input_or_output(start_position, input_ty, mir_input_ty);

It seems like we do the same thing for the return type here:

obligations.add(infcx
.at(&cx.misc(cx.last_span), cx.param_env)
.eq(output_ty, mir_output_ty)?);

But something is going wrong.

@nikomatsakis nikomatsakis modified the milestones: NLL run-pass without ICEs, NLL soundness violations Jan 19, 2018
@nikomatsakis
Copy link
Contributor

OK, digging a bit further, I see the problem, but I'm not sure yet how to fix it. It has to do with the projection cache. In particular, the first time we normalize <Foo<'1> as Bar>::Assoc, we do so at a particular location (bb0[1]) and we produce a result involving '3, where '1 == 3 @ bb0[1]. So far so good. But then we do the same projection from another place (bb0[0]) and we hit the cache: but this time, we don't produce the requirements that '1 == '3 @ bb0[0].

The "long term" fix I had planned for this was to refactor how projection and normalization work into queries. These queries would produce region constraints as part of their result that would get replayed. This feels like a good way to do it, but it rests on the idea of converting trait operations into queries, which I haven't had time to pursue, and I'd rather not have as a blocker for NLL.

I'm not sure what the better option is, though. I can of course disable the projection cache but that seems unwise. Will ponder a bit.

@arielb1
Copy link
Contributor Author

arielb1 commented Jan 22, 2018

@nikomatsakis

Clearing the projection cache when moving between locations should at least keep the exponential cases within control. I think it's probably good enough for the NLL POC.

@arielb1
Copy link
Contributor Author

arielb1 commented Jan 22, 2018

IIRC the main motivations for the projection cache were:

  1. exponential performance worst-cases, which a per-location cache avoids.
  2. working with deeply nested types during translation, which is not a concern during NLL inference.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 23, 2018 via email

@jkordish jkordish added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Feb 1, 2018
@nikomatsakis
Copy link
Contributor

This is fixed by #48411

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) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants