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 allows creating variables that are immediately unusable. #53695

Closed
matthewjasper opened this issue Aug 25, 2018 · 14 comments · Fixed by #54188
Closed

NLL allows creating variables that are immediately unusable. #53695

matthewjasper opened this issue Aug 25, 2018 · 14 comments · Fixed by #54188
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@matthewjasper
Copy link
Contributor

The following code compiles with NLL currently

fn main() {
    let x = {
        let z = 0;
        &z
    };
}

because it's equivalent to:

fn main() {
    let x;
    {
        let z = 0;
        x = &z
    };
}

However, any further use of x will result in a "z does not live long enough" warning. Should there be some read of x after the rest of the let is done to ensure that this doesn't happen?

@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 Aug 25, 2018
@nikomatsakis nikomatsakis added I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 27, 2018
@nikomatsakis nikomatsakis added this to the Rust 2018 RC milestone Aug 27, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Aug 28, 2018

I suppose since we already have ReadForMatch (which is essentially an over-approximation of what a given pattern in a match may do, and thus I think of it as a "fake read"), that provides some precedent for trying to add some sort of fake read in this scenario too... I guess the idea is that we'd put it immediately after each let statement...?

@pnkfelix
Copy link
Member

pnkfelix commented Aug 28, 2018

(nominating in the hopes that the WG-compiler-nll team will discuss this. Though its possible that the decision needs to be with T-compiler. ((or even T-lang??)))

@pnkfelix
Copy link
Member

(I also wonder if adding a fake read for every local will fix some of the issues we've seen with the compiler accepting "surprising" cases where the issue was that a closure was constructed but never called.)

@matthewjasper
Copy link
Contributor Author

Weren't those issues with AST borrowck?

@pnkfelix
Copy link
Member

@matthewjasper well, I was thinking of the cases where I had to add explicit references to a local variable in order to observe a desired unit test failure. Let me find the link...

@pnkfelix pnkfelix modified the milestones: Rust 2018 RC, Edition RC 2 Aug 28, 2018
@pnkfelix
Copy link
Member

After discussion at the WG-compiler-nll meeting, the participants agreed that while we (and possibly T-compiler/T-lang) do need to make some decision here, it does not need to be made in time for the RC milestone.

So I changed the milestone on this ticket to RC2.

@pnkfelix
Copy link
Member

Visited by compiler team. Reassigning to T-lang team.

@pnkfelix pnkfelix added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 30, 2018
@joshtriplett
Copy link
Member

We discussed this in the lang team meeting, and felt that the existing warning lints (such as for unused variables or for a value assigned and never used) seem sufficient to cover this case.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 30, 2018

Discussed in the @rust-lang/lang team meeting. General consensus was that as long as this is not harmful, it seems ok to permit it. There are already lints (e.g., unused_variable) that apply.

https://play.rust-lang.org/?gist=4b1b499e59fdf4d3e377389da016e987&version=nightly&mode=debug&edition=2015

https://play.rust-lang.org/?gist=287d0f0b0eca80ce54f1d40e2ab12975&version=nightly&mode=debug&edition=2015

EDIT: @joshtriplett jinx!

@Centril Centril removed I-needs-decision Issue: In need of a decision. I-nominated labels Aug 30, 2018
@nikomatsakis
Copy link
Contributor

@matthewjasper raised an interesting point on Zulip that I think is worth preserving. In particular, we were discussing how, in MIR, we desugar:

let PAT = EXPR

into something like:

  • Evaluate EXPR into a temporary TMP
  • Destructure TMP into the pattern PAT

But in the case of a trivial pattern like x, we just evaluator EXPR directly into x.

It is in fact this final step -- evaluating directly into x -- that permits this case to type-check, since otherwise there would be an access after EXPR has been fully evaluated (and the block terminated).

I'm not sure if this bothers me, but it's interesting.

As an example, this does not compile, even with NLL (playground):

#![feature(nll)]

fn main() {
    let (x,) = {
        let z = 0;
        (&z,)
    };
}

@nikomatsakis
Copy link
Contributor

@pnkfelix and I discussed on Zulip. Our feeling is that there may be more complications here as well and therefore that we ought to try to make this change as an experiment — if the disruption is minimal, it seems good to try and future proof here, but we might also decide to back off.

@nikomatsakis
Copy link
Contributor

I think we basically just want to insert a statement around here:

self.schedule_drop_for_binding(var, irrefutable_pat.span, OutsideGuard);

Specifically, a ReadForMatch reading the newly created binding.

@nikomatsakis
Copy link
Contributor

Discussed in the lang team meeting. We decided that it makes sense to give an error here; the "official MIR lowering" ought to be that the RHS is evaluated and then moved into the pattern, and hence there is a use that comes after the RHS has been evaluated, and an error is warranted (we happen to optimize this in MIR generation, but that shouldn't necessarily be observable).

bors added a commit that referenced this issue Sep 22, 2018
NLL: disallow creation of immediately unusable variables

Fix #53695

Original description follows

----

This WIP PR is for discussing the impact of fixing #53695 by injecting a fake read in let patterns.

(Travis will fail, at least the `mir-opt` suite is failing in its current state)
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-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants