-
Notifications
You must be signed in to change notification settings - Fork 13.3k
NLL allows creating variables that are immediately unusable. #53695
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
Comments
I suppose since we already have |
(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??))) |
(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.) |
Weren't those issues with AST borrowck? |
@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... |
@matthewjasper i.e I was thinking of things like this: Update: oops, sorry I meant https://github.com/rust-lang/rust/pull/53407/files?utf8=%E2%9C%93&diff=split#diff-11c4698b7f13fd531c6a5a7b78f27d27R58 |
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. |
Visited by compiler team. Reassigning to T-lang team. |
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. |
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., EDIT: @joshtriplett jinx! |
@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:
But in the case of a trivial pattern like It is in fact this final step -- evaluating directly into 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,)
};
} |
@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. |
I think we basically just want to insert a statement around here: rust/src/librustc_mir/build/matches/mod.rs Line 258 in f17c230
Specifically, a |
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). |
The following code compiles with NLL currently
because it's equivalent to:
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?The text was updated successfully, but these errors were encountered: