[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

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

My comment from our meeting:

nikomatsakis: I eventually think we should have a "raw pointer"-binding mode (which would be safe. --ed) but also I agree with TC that there is a lot of value in being very precise about what is safe and what is unsafe. I've found over time that it helps me to sharpen my mental model and it also means I pay more attention when an unsafe is required, i.e., if I got used to the idea of "oh, but that's not really unsafe", then I'm likely to think less of writing unsafe { ... } without a crisp idea of why my code is valid. I've also just found many times that I appreciate being able to write kinda tricky code and know exactly what I have to vet.

[scottmcm]

dianne mentioned something important here in the meeting, that this can't be "infallible" because of course bindings are infallible and must require unsafe. (Potentially if we had raw pointer bindings those wouldn't need to, but those also don't exist.)

As a lang matter this doesn't need to require unsafe, and being strict about that is really helpful for how we describe things and think about things, so

@rfcbot reviewed

I do hope there's an extractable concept here to allow simplifying things, but that can be left to a lang-docs and compiler-review matter separate from the intent of the language change.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[Nadrieril]

Waiting on rust-lang/reference#2350 to decide which exact patterns we want to allow here.

[Jules-Bertholet]

to decide which exact patterns we want to allow here.

My understanding is that this was decided by the FCP, and the only question left unresolved is how the Reference should phrase it.

[Nadrieril]

The FCP text didn't mention corner cases like how to deal with constants that expand to a pattern that would be safe, which I would personally prefer to keep unsafe, or the case of the empty byte string literal b"" that @dianne uncovered. So I was imagining the details would be up to the Reference PR; at the very least I feel uneasy trying to decide on that myself with my t-compiler hat on.

[Jules-Bertholet]

AFAIUI the whole point of pre-stabilization FCP is to settle these questions. If the lang team didn't mean to make a decision here, I don't see what the point of the exercise was? Either way, the intent being so unclear seems like a process failure to me

[Jules-Bertholet]

@Nadrieril As a t-lang-advisors member, you can raise blocking concerns, so I guess we should proceed as if you had done so.

[Nadrieril]

Hm, I guess I should have raised a concern about these points? I wasn't following closely.

[Jules-Bertholet]

Anyway, my position on the question at issue, "how should we treat constant patterns":

• +1 for the current behavior of this PR
• -0.25 for making them all require unsafe, including unit struct constant patterns (so that UnitStruct {} and UnitStruct would be treated differently)
• -1 on treating unit-struct constants differently from other constants

[nikomatsakis]

Hi all. We discussed this in the lang-team meeting. We would like to better understand @Nadrieril the concerns you are raising both about const patterns and the b"" literals. Our discussion did not cover const patterns in particular, but that was not because we didn't intend for it to do so, so much as that we were unaware there was any particular interaction to be aware of. Therefore we'd like to "re-open" discussion even though the FCP has elapsed since there is new information to consider.

[nikomatsakis]

@rustbot label -finished-final-comment-period

[Nadrieril]

We've been discussing on Zulip: #t-lang > Restart FCP with concern on #161771?

[Jules-Bertholet]

A summary of the discussion:

Currently, this PR treats constants with no private fields (or foreign #[non_exhaustive]) like their expanded pattern. For example, if I have a constant like const ZST: MyStruct = MyStruct {}, then the pattern MyUnion { field: ZST } does not require unsafe.

@Nadrieril objects to this, and would prefer that all patterns expanded from constants be considered unsafe. Though I would prefer the current behavior, I am also willing to accept that.

There's one further subtlety: unit struct patterns. If we consider all constant patterns as unsafe, then with a struct like struct MyStruct;, MyUnion { field: MyStruct } would be an unsafe pattern. @Nadrieril would prefer to special-case unit struct constant patterns in order to make this safe, but is willing to accept not doing so; I feel strongly that we should not have such a special case.

[Nadrieril]

I agree with this summary, with one caveat: I do not wish to special-case unit struct constructors in safety checking, but rather in THIR building for patterns, where they would be considered constructors rather than named constants, which is what I always thought they were (and become anyway, post const_to_pat).

Said differently: instead of saying "this is a named constant" and then in const_to_pat expanding it to the obvious constructor it should be, lower it as a constructor from the start (in THIR building presumably).

[Jules-Bertholet]

where they would be considered constructors rather than named constants, which is what I always thought they were

It is what they are, in the compiler. But the Reference promises they are equivalent to a constant: https://doc.rust-lang.org/reference/items/structs.html#r-items.struct.unit

My objection is that I don't think we should break this promise.¹

Footnotes

1. Technically https://github.com/rust-lang/rust/issues/80890 already breaks it, but that's a bug and we should fix it. ↩