Repository navigation
Document which union patterns are unsafe - #2350
Jules-Bertholet wants to merge 4 commits into
Conversation
|
I think this kind of definition of what patterns "access" a thing could also be useful for implementing a lint for rust-lang/rust#158387 (comment). cc @Nadrieril |
0736dea to
27d2069
Compare
| - [Reference patterns](../patterns.md#reference-patterns) | ||
| - [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]`. |
There was a problem hiding this comment.
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"
| } | ||
| ``` | ||
|
|
||
| `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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This branch has not been deployed
5 participants
Documents rust-lang/rust#161771.