Repository navigation
let _ = <access to unsafe field> currently type-checks #54003
Description
The following example passes the unsafe checker, but probably should not:
// #![feature(nll)]
union Foo {
x: u32,
y: u64
}
fn main() {
let foo = Foo { x: 22 };
let _ = foo.x;
}The problem here is that let _ = foo.x is a no-op -- foo.x is a place expression, and the _ pattern just plain ignores it. This means that no MIR is generated. The unsafe checker runs on MIR.
But I think that we expect an error here nonetheless.
This is a regression that was introduced in the move to doing unsafe checking on MIR. I'm not sure the best way to fix: I tend to think we should do unsafe checking on HAIR, but that would be a fairly large effort. We could certainly generate a "dummy access" as we do with matches. (But we want to be careful about borrow checker interactions -- see #53114).
I'm nominating this for lang-team discussion — we do want an unsafe error here, right?
Activity
nikomatsakis commentedon Sep 6, 2018
On the topic of doing unsafe checking on HAIR:
@arielb1 and @eddyb pointed out that, in HAIR, we have to be careful because field access can also occur in patterns. But I think that this is just a matter of looking for "struct patterns" that refer to packed structs, and checking if they have any ref-bindings within? Still, kind of a pain, not as nice as doing the check on MIR.
This may argue for the dummy access.
arielb1 commentedon Sep 6, 2018
I think you are missing a raw pointer there.Nope you are just using a union.eddyb commentedon Sep 6, 2018
IMO this isn't as bad as
match u.void {}not needingunsafe, which is what we've added the dummy accessed for, it actually seems harmless.arielb1 commentedon Sep 9, 2018
Of course it's not as bad - only one of these is unsound.
nikomatsakis commentedon Sep 10, 2018
I agree it is not as bad, but it still seems like a bug to me. Do we want to fix?
cc @rust-lang/lang -- nominating for discussion, but feel free to leave comments, maybe we can settle async before the meeting. =) Should
let _ = a.bgive an error ifbis the field of a union and there is no unsafe block? Currently, it does not, because this compiles to a no-op -- that is, there is no MIR for it. It is basically saying "ignore the placea.b" (in contrast to, say,let _x = a.b, which would be an access).If we do want to fix, and we don't want to move the code to HAIR, I think we want to add some kind of
UnsafeCheck(Place)statement.UnsafeCheckwould:let _ = xis ok ifxis mutably borrowed or uninitialized)nikomatsakis commentedon Sep 10, 2018
The only reason I can see not to fix this is backwards compatibility: this is a somewhat old regression by now. Not sure how old.
aturon commentedon Sep 10, 2018
@nikomatsakis I think we should require an
unsafeblock for consistency. Breakage seems unlikely (though we should check) and very easy to deal with.joshtriplett commentedon Sep 10, 2018
I'd like to see this fixed as well.
Centril commentedon Sep 10, 2018
I agree with @aturon wrt. the goal and the reasoning...
...but I am concerned about complicating MIR (this would be akin to extending Haskell Core with a constructor which they are very reluctant to do for good reasons). Are there other places which would benefit from
UnsafeCheck?eddyb commentedon Sep 11, 2018
FWIW I don't think we need to fix anything, and that these are all equivalent:
joshtriplett commentedon Sep 13, 2018
We discussed this in the @rust-lang/lang meeting, and we were generally in favor of emitting warnings about needing
unsafeeven on the right-hand side oflet _ =.However, we also agreed that we'd like to have a warn-by-default lint for
let _ = expression_with_zero_side_effects;Auto merge of #102256 - cjgillot:let-under, r=lcnr
Auto merge of #102256 - cjgillot:let-under, r=lcnr