Don't require unsafe for struct and array patterns against union fields

[Jules-Bertholet]

View all comments

These patterns don't access the union directly, only their subpatterns do. So there is no need to require unsafe.

For example, the following now compiles:

union Foo {
    field: [u8; 3],
}

fn bar(foo: Foo) {
    match foo {
        Foo { field: [_, _, _], } => (),
    }
}

Also removes the unsafe requirement for the unstable guard patterns (#129967) in this position.

Reference PR: rust-lang/reference#2350

@rustbot label T-lang needs-fcp A-patterns F-guard_patterns I-lang-nominated

[rustbot]

r? @folkertdev

rustbot has assigned @folkertdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 16 candidates

[folkertdev]

You should actually nominate for T-lang if you want them to look at this.

[traviscross]

Worth noting, for our discussion, that the example in the PR description compiles today, i.e., ahead of this PR landing:

union Foo {
    field: u8,
}

fn bar(foo: Foo) {
    match foo {
        Foo { field: _ } => (), // OK!
    }
}

What fails are things like this:

union Foo {
    field: (u8,),
}

fn bar(foo: Foo) {
    match foo {
        Foo { field: (..) } => (), // ERROR: Needs `unsafe`.
    }
}

[Jules-Bertholet]

Thanks for the correction, I edited the OP

[scottmcm]

Just checking: we still FakeRead/PlaceReference (I don't remember which it should be) the union when doing this, right?

Assuming we do, then I agree that allowing these categories of irrefutable patterns in union patterns makes sense 👍

[scottmcm]

Pondering: [..] is also an irrefutable pattern but isn't updated here (right?)

I don't know if it's possible, but could this whole match change to being about irrefutable pattern instead, or something? If we have to whack-a-mole a whole bunch of things here, that makes me less "oh yeah let's do it" than I was before, since I don't know why people would write this.

(Notably if you're using a pat_param from a macro it'd actually be easier for it to always be unsafe so you don't need to suppress the unneeded-unsafe if they pass something simple.)

Part of why we said that unsafeck is on THIR is that it's more of a lexical check than a flow-sensitive one, so being a bit more unsafe than strictly necessary is generally fine if it's something that the human description of the thing is something that people would say "it's unsafe to do that".

View changes since the review

[Jules-Bertholet]

Pondering: [..] is also an irrefutable pattern but isn't updated here (right?)

Yes it is. There's even a test.

could this whole match change to being about irrefutable pattern instead

No, irrefutability isn't sufficient. x is an irrefutable pattern but still needs to be unsafe; & _ probably should be as well. Nor is it even necessary; the unstable guard patterns are refutable, but shouldn't require unsafe.

What we care about is that the pattern does not perform a read/assert validity.

[tmandry]

We talked about this in today's lang meeting. This change seems fine individually, but along the lines of @scottmcm's comments above, I'd like to do it in a way that preserves (or improves) the overall consistency and simplicity of the language.

One way to evaluate that would be to review a reference PR for the change. @Jules-Bertholet would you be willing to draft one for us to review?

[Jules-Bertholet]

@tmandry Here you go: rust-lang/reference#2350

While drafting, I noticed one issue: this change makes #162213 slightly worse, because now whether a const pattern requires unsafe can depend on what should be private implementation details of the const item. (I don't think that issue should block this change, as the semver hazard exists either way.) Edit: this has been fixed

[Jules-Bertholet]

@theemathas see the newly added test

[Jules-Bertholet]

Whoops, I forgot the important part… 🤦 fixed

[rustbot]

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in match checking

cc @Nadrieril

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Jules-Bertholet]

I've addressed the semver hazard issue as it pertains to this PR. The Reference PR has also been updated.

[Nadrieril]

I'm confused by the logic here. Are you using has_rest/privacy to detect constant patterns? I'd prefer to add a thir pattern node that remembers patterns that were lowered from a constant (this keeps coming up and will be added by #155216 anyway), and use that to know whether we're inside a constant.

View changes since the review

[Nadrieril]

I'm generally confused by what has_rest has to do with this PR, given that it's equivalent to a bunch of wildcard patterns anyway.

[Jules-Bertholet]

Are you using has_rest/privacy to detect constant patterns?

Yes, specifically constant patterns whose equivalent expanded pattern could not have been written directly at the location the pattern is being used. If a struct has non-visible fields or is foreign non_exhaustive, then you need a rest pattern to do a direct pattern match; the absence of such a pattern (constants don't have rest patterns) means the pattern was expanded from a constant in another crate.

I was going for the smallest possible change; if you have a suggestion for a cleaner way to carry though this information, that's fine, will gladly do it your way.

[dianne]

I'd prefer to add a thir pattern node that remembers patterns that were lowered from a constant (this keeps coming up and will be added by #155216 anyway), and use that to know whether we're inside a constant.

We already track patterns lowered from constants, I think. It's not a dedicated node anymore, but the constant's DefId gets stored in PatExtra's expanded_const field. #155216 also stores the valtree evaluation in PatExtra, but hopefully that's not needed here?

[Jules-Bertholet]

the constant's DefId gets stored in PatExtra's expanded_const field

I believe that is only true for the outer pattern, not its subpatterns. We could add another field to UnsafetyVisitor to track it, but that's annoying to get right (e.g. #161771 (comment)). Nadri's suggestion of a dedicated HIR pattern node has the same issue.

[Nadrieril]

the absence of such a pattern (constants don't have rest patterns) means the pattern was expanded from a constant in another crate.

Ok, I agree this works, but that's mixing semantic and syntactic concerns in a way that does not feel robust to me.

We could add another field to UnsafetyVisitor to track it, but that's annoying to get right

The logic imo is much simpler: a constant pattern inside a union pattern is unconditionally unsafe. I'd like to even reserve the right to turn the comparison into PartialEq::eq instead of expanding the pattern, see my comment on the reference PR. The difference should only be observable for ZSTs, as they are the only ones that can have constant that don't access anything.

[Jules-Bertholet]

does not feel robust to me.

Arguably, assuming that constants are the only way to refer to private fields from a pattern is not robust either.

If you would like, I can turn the debug_assert! at https://github.com/rust-lang/rust/pull/161771/changes#diff-db8b06c3d8dde7b9f002e070c4e72fd41fc8487d988324232cd85355c63b2423R313-R321 into a full assert! or proper error.

a constant pattern inside a union pattern is unconditionally unsafe

I don't think we want to be this restrictive. It would be unexpected for e.g. () and UnitStruct {} to be safe patterns, but UnitStruct to be unsafe. If the pattern is an effective no-op, then replacing it with a PartialEq::eq call should always be valid, and also never be necessary.

[Nadrieril]

If the pattern is an effective no-op, then replacing it with a PartialEq::eq call should always be valid, and also never be necessary.

hmm, PartialEq::eq takes a reference to the place but I guess for unit structs that cannot cause extra UB?

It would be unexpected for e.g. () and UnitStruct {} to be safe patterns, but UnitStruct to be unsafe.

UnitStruct is treated as a constant pattern? In that case I agree that would be surprising, but I'm also on team "err on the side of more unsafe because what are you even trying to do here".

[dianne]

It would be unexpected for e.g. () and UnitStruct {} to be safe patterns, but UnitStruct to be unsafe.

UnitStruct shouldn't be treated as a constant if it refers to a unit struct constructor; it'd get lowered with lower_variant_or_leaf, not const-to-pat. only path patterns that refer to named constant items and associated constant items get the expanded_const marker currently, I think.

notably, I think literals don't get any marker currently even though they do go through const-to-pat. this matters with deref_patterns enabled, since I think under this PR, that would let b"" match on a [u8; 0] union field (as it would lower to a PatKind::Array with no subpatterns)

[Jules-Bertholet]

UnitStruct shouldn't be treated as a constant if it refers to a unit struct constructor; it'd get lowered with lower_variant_or_leaf, not const-to-pat. only path patterns that refer to named constant items and associated constant items get the expanded_const marker currently, I think.

Oh, huh. But that sounds like the sort of implementation detail we don't want to enshrine in observable language behavior. The Reference explicitly promises that a unit struct is equivalent to a fieldless braced struct + a constant. So actually, this is strong evidence that we don't want to base this check around expanded_const.

Additionally, fixing #162213 will require doing this same sort of reasoning anyway.

this matters with deref_patterns enabled, since I think under this PR, that would let b"" match on a [u8; 0] union field (as it would lower to a PatKind::Array with no subpatterns)

Huh, didn't know deref_patterns allowed matching a T value with an &T pattern. I see no reason why we shouldn't allow this, though.

[folkertdev]

r? Nadrieril

[rustbot]

Nadrieril is currently at their maximum review capacity.
They may take a while to respond.