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

assign to part of possibly uninitialized variable is flagged as UB (but only in 2018 edition) #60450

Closed
jethrogb opened this issue May 1, 2019 · 11 comments · Fixed by #63230
Closed
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) NLL-diagnostics Working torwads the "diagnostic parity" goal

Comments

@jethrogb
Copy link
Contributor

jethrogb commented May 1, 2019

Code sample:

fn main() {
    let mut _tmp: (u64, u64);
    
    _tmp.0 = 1;
    _tmp.1 = 1;
}

When compiling this with rustc +1.31.0 --edition 2018, I get:

warning[E0381]: assign to part of possibly uninitialized variable: `_tmp`
 --> t.rs:4:5
  |
4 |     _tmp.0 = 1;
  |     ^^^^^^^^^^ use of possibly uninitialized `_tmp`
  |
  = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
          It represents potential unsoundness in your code.
          This warning will become a hard error in the future.

In later versions, the warning message has been changed somewhat:

   = warning: this error has been downgraded to a warning for backwards compatibility with previous releases
   = warning: this represents potential undefined behavior in your code and this warning will become a hard error in the future

If I compile with --edition 2015 I get no warnings at all. (Edit: this is still silent on 1.34.0 2015, but an warning is emitted in edition 2015 on the latest nightly).

There are several issues as far as I can see:

  • “This error has been downgraded to a warning”: this is the first stable compiler for the 2018 edition, so when was this downgrade supposed to have happened? This code never generated an error, for any edition, on any previous stable compiler.
  • How can there be UB only when I'm compiling for the 2018 edition?
  • What actually is the UB? The compiler is already tracking validity of individual fields just fine (e.g. when moving out of structs).
  • The explanation for E0381 doesn't talk about assignment of uninitialized variables at all.

Edit: I ran into this issue with inline assembly, where you may be required to specify an output register even if it's not used.

@RalfJung
Copy link
Member

RalfJung commented May 1, 2019

Nothing here says there is any UB. This is just about whether the type-checker accepts this program. We reject plenty of non-UB programs.

If I grep for that error in the tests, the hits are all in the "borrowck" directory. That would also explain the edition difference; the error likely comes from NLL.

Cc @matthewjasper

@matthewjasper
Copy link
Contributor

This is just something that NLL chose not to support because the fields can't ever be read (for now). The message isn't particularly great though.

@matthewjasper matthewjasper added A-NLL Area: Non Lexical Lifetimes (NLL) NLL-diagnostics Working torwads the "diagnostic parity" goal labels May 1, 2019
@jethrogb
Copy link
Contributor Author

jethrogb commented May 1, 2019

Nothing here says there is any UB.

It says "potential UB" right in the error message. But if as you say it's not UB, then I don't think we should be breaking existing code for it.

@Centril
Copy link
Contributor

Centril commented May 1, 2019

cc #54987. This was discussed in depth by the language team with decision recorded in #21232 (comment).

Accepting this would be completely useless at the moment as you cannot use the field (as @matthewjasper notes). I think we should therefore convert this into a hard error on both editions and eventually perhaps accept gradual initialization of structs but a sane variant of that (e.g. taking definitive initialization and uninhabitedness into account).

@jethrogb
Copy link
Contributor Author

jethrogb commented May 1, 2019

Accepting this would be completely useless at the moment as you cannot use the field

(as mentioned in my edit of the issue description) It's not "completely useless", I am in fact using this today with inline assembly (which is how I ran into this issue): https://github.com/fortanix/rust-sgx/blob/886d08d/enclave-runner/src/tcs.rs#L99. With inline assembly you sometimes have to specify an output variable even if you're going to do nothing with the result.

with decision recorded

To be clear the decision was to "tentatively decided to attempt, for the short-term ..." which sounds much less definitive.

@Centril
Copy link
Contributor

Centril commented May 1, 2019

@jethrogb Since I'm not familiar with what you are doing... Is there a reason you cannot just let mut _tmp: (u64, u64) = (0, 0);? Or otherwise perhaps use MaybeUninit?

To be clear the decision was to "tentatively decided to attempt, for the short-term ..." which sounds much less definitive.

This refers to the implementation effort (note attempt -- it was attempted successfully).

@jethrogb
Copy link
Contributor Author

jethrogb commented May 1, 2019

The only reason I brought up my example was just to point out that it's not completely useless.

I can work around this by just creating two uninitialized bindings instead, which I didn't do before because it's an additional useless line of code. I'm not particularly bothered about needing to make such a change, especially since it only concerns inline assembly. I'm more bothered by the messaging involved.

@Centril
Copy link
Contributor

Centril commented May 1, 2019

The only reason I brought up my example was just to point out that it's not completely useless.

Fair enough; It's quite a niche use case (unlike gradual initialization in general) and as you say you can do it through other, and in my view clearer, ways.

@RalfJung
Copy link
Member

RalfJung commented May 2, 2019

It says "potential UB" right in the error message.

oops I missed that, sorry.

bors bot added a commit to fortanix/rust-sgx that referenced this issue May 2, 2019
137: Fix warning that appeared in latest nightly r=jseyfried a=jethrogb

See rust-lang/rust#60450

Co-authored-by: Jethro Beekman <jethro@fortanix.com>
@pnkfelix pnkfelix changed the title assign to part of possibly uninitialized variable UB only in 2018 edition assign to part of possibly uninitialized variable is flagged as UB (but only in 2018 edition) May 22, 2019
@tmandry
Copy link
Member

tmandry commented Jul 27, 2019

Since NLL on 2015 edition has now landed, what should the timeline look like for transitioning this to a hard error?

@Centril
Copy link
Contributor

Centril commented Jul 27, 2019

@tmandry If you can separate this from the other NLL error-downgrades I would welcome making it a hard error right away. Otherwise you'll have to wait for all the NLL error-downgrades to be removed.

tmandry added a commit to tmandry/rust that referenced this issue Aug 5, 2019
This is one of the behaviors we no longer allow in NLL. Since it can
lead to undefined behavior, I think it's definitely worth making it a
hard error without waiting to turn off migration mode (rust-lang#58781).

Closes rust-lang#60450.

My ulterior motive here is making it impossible to leave variables
partially initialized across a yield (see discussion at rust-lang#63035), so
tests are included for that.
Centril added a commit to Centril/rust that referenced this issue Aug 6, 2019
…ized, r=Centril

Make use of possibly uninitialized data [E0381] a hard error

This is one of the behaviors we no longer allow in NLL. Since it can
lead to undefined behavior, I think it's definitely worth making it a
hard error without waiting to turn off migration mode (rust-lang#58781).

Closes rust-lang#60450.

My ulterior motive here is making it impossible to leave variables
partially initialized across a yield (see rust-lang#60889, discussion at rust-lang#63035), so
tests are included for that.

cc rust-lang#54987

---

I'm not sure if bypassing the buffer is a good way of doing this. We could also make a `force_errors_buffer` or similar that gets recombined with all the errors as they are emitted. But this is simpler and seems fine to me.

r? @Centril
cc @cramertj @nikomatsakis @pnkfelix @RalfJung
Centril added a commit to Centril/rust that referenced this issue Aug 6, 2019
…ized, r=Centril

Make use of possibly uninitialized data [E0381] a hard error

This is one of the behaviors we no longer allow in NLL. Since it can
lead to undefined behavior, I think it's definitely worth making it a
hard error without waiting to turn off migration mode (rust-lang#58781).

Closes rust-lang#60450.

My ulterior motive here is making it impossible to leave variables
partially initialized across a yield (see rust-lang#60889, discussion at rust-lang#63035), so
tests are included for that.

cc rust-lang#54987

---

I'm not sure if bypassing the buffer is a good way of doing this. We could also make a `force_errors_buffer` or similar that gets recombined with all the errors as they are emitted. But this is simpler and seems fine to me.

r? @Centril
cc @cramertj @nikomatsakis @pnkfelix @RalfJung
Centril added a commit to Centril/rust that referenced this issue Aug 6, 2019
…ized, r=Centril

Make use of possibly uninitialized data [E0381] a hard error

This is one of the behaviors we no longer allow in NLL. Since it can
lead to undefined behavior, I think it's definitely worth making it a
hard error without waiting to turn off migration mode (rust-lang#58781).

Closes rust-lang#60450.

My ulterior motive here is making it impossible to leave variables
partially initialized across a yield (see rust-lang#60889, discussion at rust-lang#63035), so
tests are included for that.

cc rust-lang#54987

---

I'm not sure if bypassing the buffer is a good way of doing this. We could also make a `force_errors_buffer` or similar that gets recombined with all the errors as they are emitted. But this is simpler and seems fine to me.

r? @Centril
cc @cramertj @nikomatsakis @pnkfelix @RalfJung
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-diagnostics Working torwads the "diagnostic parity" goal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@RalfJung @Centril @jethrogb @tmandry @matthewjasper and others