let _ = <access to unsafe field> currently type-checks

[nikomatsakis]

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?

[nikomatsakis]

On the topic of doing unsafe checking on HAIR:

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.

@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]

I think you are missing a raw pointer there. Nope you are just using a union.

[eddyb]

IMO this isn't as bad as match u.void {} not needing unsafe, which is what we've added the dummy accessed for, it actually seems harmless.

[arielb1]

IMO this isn't as bad as match u.void {} not needing unsafe, which is what we've added the dummy accessed for, it actually seems harmless.

Of course it's not as bad - only one of these is unsound.

[nikomatsakis]

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.b give an error if b is 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 place a.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.

UnsafeCheck would:

• be stripped out once static checking is done
• be used by unsafe code to report errors
• be ignored by the borrow checker, which is only concerned with actual accesses
  • (in particular, let _ = x is ok if x is mutably borrowed or uninitialized)

[nikomatsakis]

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]

@nikomatsakis I think we should require an unsafe block for consistency. Breakage seems unlikely (though we should check) and very easy to deal with.

[joshtriplett]

I'd like to see this fixed as well.

[Centril]

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]

FWIW I don't think we need to fix anything, and that these are all equivalent:

let _ = a.b;
let Union { b: _ } = a;
let Union { .. } = a;

[joshtriplett]

We discussed this in the @rust-lang/lang meeting, and we were generally in favor of emitting warnings about needing unsafe even on the right-hand side of let _ =.

However, we also agreed that we'd like to have a warn-by-default lint for let _ = expression_with_zero_side_effects;