Skip to content

Document which union patterns are unsafe #2350

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.

Sign up for GitHub

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

Draft
Jules-Bertholet wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Document which union patterns are unsafe #2350

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c5b65d
Document which union patterns are unsafe
Jules-Bertholet Sep 2, 2026
c903ed6
Account for constants with private fields
Jules-Bertholet Sep 7, 2026
27d2069
Account for match ergonomics
Jules-Bertholet Sep 10, 2026
d663ce8
Mention slice patterns
Jules-Bertholet Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/items/unions.md
View file
Open in desktop
Comment thread
Jules-Bertholet marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,19 @@ fn f(u: MyUnion) {
}
```

`unsafe` is only required if the union field is accessed, in whole or in part, by the pattern, including any of its sub-patterns. For the purpose of this requirement, the following patterns are considered to perform an access:

@RalfJung RalfJung Oct 2, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we add a new pattern kind and forget to add it here, then we're implicitly specifying that as being allowed? That seems like a dangerous default.

View changes since the review

@Jules-Bertholet Jules-Bertholet Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, but on the other hand:

  • Explaining this in the negative would be a lot less clear/understandable.
  • You can't add a new pattern kind to the compiler without touching the code that implements this check.

I think we should just add a comment to visit_pat in rustc_mir_build/check_unsafety reminding people to update this list.

@RalfJung RalfJung Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explaining this in the negative would be a lot less clear/understandable.

Why? isn't it something like, a pattern matching on a union field is safe only if it recursively consists exclusively of

  • array patterns
  • struct patterns
  • _ patterns
  • (add a few more maybe?)

@Jules-Bertholet Jules-Bertholet Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think reasoning in terms of what code doesn't do is inherently more fraught than reasoning about what it does. That's the approach we take in https://doc.rust-lang.org/reference/unsafety.html: we list all the unsafe operations, not all the operations that aren't unsafe.

@RalfJung RalfJung Oct 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the opposite is true, it's much more reliable to enumerate what is safe than what isn't.

For Rust generally this isn't a great default because most operations are safe, but when we are talking about unions it is exactly the right default IMO.


- [Literal patterns](../patterns.md#literal-patterns)
- [Identifier patterns](../patterns.md#identifier-patterns)
- [Range patterns](../patterns.md#range-patterns)
- [Reference patterns](../patterns.md#reference-patterns)
Comment thread
Jules-Bertholet marked this conversation as resolved.
- [Non-reference patterns](../patterns.md#r-patterns.ident.binding.non-reference) matching reference values
- [Struct](../patterns.md#struct-patterns) and [tuple struct](../patterns.md#tuple-struct-patterns) patterns which correspond to an enum variant
- [Path patterns](../patterns.md#path-patterns), if the result of expanding the constant into a pattern contains one of the above, or if the expanded pattern could not have been written directly at the location where it is used due to field privacy or `#[non_exhaustive]`.

@Nadrieril Nadrieril Sep 21, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to add constant patterns there, because it should be allowed to turn a constant patter nmatch into a call to PartialEq::eq for performance. This conflicts with https://doc.rust-lang.org/reference/patterns.html?highlight=patterns#r-patterns.const.translation a bit so we conceptually are saying "unsafety checking is done before we expand constant patterns"

View changes since the review


> [!NOTE]
> [Slice patterns](../patterns.md#slice-patterns) other than `[..]`, when matching against slices of dynamic size, could also be considered to access the union. However, union fields must implement `Sized`, so the rules regarding patterns matching reference values already account for this case.

> [!WARNING]
> The order in which the subpatterns of a pattern are tested is not specified. A union field named in a pattern may be read even when the pattern as a whole does not match. Reading a union field is undefined behavior unless it holds a valid value of its type (see [items.union.fields.validity]). Nothing else in the pattern can be relied on to prevent the read.
>
Expand Down
Loading