-
-
Notifications
You must be signed in to change notification settings - Fork 15.6k
Don't require unsafe for struct and array patterns against union fields
#161771
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
e347fa6
43da42d
cd1d1aa
db80e46
9b01a2f
80bc8dd
5057088
950d61e
5abfdb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -258,34 +258,55 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> { | |
| // match is conditional on having this value | ||
| | PatKind::Constant { .. } | ||
| | PatKind::Variant { .. } | ||
| | PatKind::Leaf { .. } | ||
| | PatKind::Deref { .. } | ||
| | PatKind::DerefPattern { .. } | ||
| | PatKind::Range { .. } | ||
| | PatKind::Slice { .. } | ||
| | PatKind::Array { .. } | ||
| | PatKind::Guard { .. } | ||
| // Never constitutes a witness of uninhabitedness. | ||
| | PatKind::Never => { | ||
| self.requires_unsafe(pat.span, AccessToUnionField); | ||
| return; // we can return here since this already requires unsafe | ||
| } | ||
| // wildcard doesn't read anything. | ||
| PatKind::Wild | | ||
| PatKind::Wild | ||
| // these just wrap other patterns, which we recurse on below. | ||
| PatKind::Or { .. } | | ||
| PatKind::Error(_) => {} | ||
| | PatKind::Or { .. } | ||
| | PatKind::Leaf { .. } // We do extra checks below for patterns lowered from consts | ||
| | PatKind::Array { .. } | ||
| | PatKind::Guard { .. } | ||
| | PatKind::Error(_) => {} | ||
| } | ||
| }; | ||
|
|
||
| match &pat.kind { | ||
| PatKind::Leaf { subpatterns, .. } => { | ||
| PatKind::Leaf { subpatterns, has_rest } => { | ||
| if let ty::Adt(adt_def, ..) = pat.ty.kind() { | ||
| for pat in subpatterns { | ||
| if adt_def.non_enum_variant().fields[pat.field].safety.is_unsafe() { | ||
| self.requires_unsafe(pat.pattern.span, UseOfUnsafeField); | ||
| let single_variant = adt_def.non_enum_variant(); | ||
|
|
||
| let scope = self.tcx.parent_module(self.hir_context).to_def_id(); | ||
|
|
||
| if self.in_union_destructure | ||
| && !has_rest | ||
| && (single_variant.field_list_has_applicable_non_exhaustive() | ||
| || single_variant | ||
| .fields | ||
| .iter() | ||
| .any(|f| !f.vis.is_accessible_from(scope, self.tcx))) | ||
| { | ||
| // This pattern must have been lowered from a constant. | ||
| // Changes to private implementation details of said constant | ||
| // must not affect whether we require `unsafe`. | ||
| self.requires_unsafe(pat.span, AccessToUnionField); | ||
| return; | ||
| } | ||
|
Comment on lines
+288
to
+301
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused by the logic here. Are you using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm generally confused by what
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We already track patterns lowered from constants, I think. It's not a dedicated node anymore, but the constant's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I believe that is only true for the outer pattern, not its subpatterns. We could add another field to |
||
|
|
||
| for subpat in subpatterns { | ||
| let field = &single_variant.fields[subpat.field]; | ||
| if field.safety.is_unsafe() { | ||
| self.requires_unsafe(subpat.pattern.span, UseOfUnsafeField); | ||
| } | ||
| } | ||
|
|
||
| if adt_def.is_union() { | ||
| let old_in_union_destructure = | ||
| std::mem::replace(&mut self.in_union_destructure, true); | ||
|
|
@@ -334,6 +355,14 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> { | |
| visit::walk_pat(self, pat); | ||
| self.inside_adt = old_inside_adt; | ||
| } | ||
| PatKind::Guard { subpattern, condition } => { | ||
| self.visit_pat(subpattern); | ||
|
|
||
| let old_in_union_destructure = | ||
| std::mem::replace(&mut self.in_union_destructure, false); | ||
| self.visit_expr(&self.thir()[*condition]); | ||
| self.in_union_destructure = old_in_union_destructure; | ||
| } | ||
| _ => { | ||
| visit::walk_pat(self, pat); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| //@ run-pass | ||
| //! Test that guard patterns in union fields don't impose an `unsafe` requirement. | ||
|
|
||
| #![feature(guard_patterns)] | ||
| #![expect(incomplete_features)] | ||
|
|
||
| union Foo { | ||
| field: u8, | ||
| } | ||
|
|
||
| fn main() { | ||
| let foo = Foo { field: 42 }; | ||
| match foo { | ||
| Foo { field: _ if matches!(1, 1) } => (), | ||
| _ => panic!(), //~ WARN unreachable | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| warning: unreachable pattern | ||
| --> $DIR/union.rs:15:9 | ||
| | | ||
| LL | Foo { field: _ if matches!(1, 1) } => (), | ||
| | ---------------------------------- matches all the relevant values | ||
| LL | _ => panic!(), | ||
| | ^ no value can reach this | ||
| | | ||
| = note: `#[warn(unreachable_patterns)]` (part of `#[warn(unused)]`) on by default | ||
|
|
||
| warning: 1 warning emitted | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| #[derive(Clone, Copy, PartialEq)] | ||
| pub struct HasPrivateField { | ||
| not_pub: (), | ||
| } | ||
|
|
||
| pub const HAS_PRIVATE_FIELD: HasPrivateField = HasPrivateField { not_pub: () }; | ||
|
|
||
| #[derive(Clone, Copy, PartialEq)] | ||
| #[non_exhaustive] | ||
| pub struct HasNonExhaustiveFieldList {} | ||
|
|
||
| pub const HAS_NON_EXHAUSTIVE_FIELD_LIST: HasNonExhaustiveFieldList = HasNonExhaustiveFieldList {}; | ||
|
|
||
| #[derive(Clone, Copy, PartialEq)] | ||
| pub struct HasPrivateTupleField(()); | ||
|
|
||
| pub const HAS_PRIVATE_TUPLE_FIELD: HasPrivateTupleField = HasPrivateTupleField(()); | ||
|
|
||
| #[derive(Clone, Copy, PartialEq)] | ||
| #[non_exhaustive] | ||
| pub struct HasNonExhaustiveTupleFieldList(); | ||
|
|
||
| pub const HAS_NON_EXHAUSTIVE_TUPLE_FIELD_LIST: HasNonExhaustiveTupleFieldList = | ||
| HasNonExhaustiveTupleFieldList(); | ||
|
|
||
| pub const NESTED_CONST: (HasPrivateField,) = (HAS_PRIVATE_FIELD,); |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_paramfrom 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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is. There's even a test.
No, irrefutability isn't sufficient.
xis 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 requireunsafe.What we care about is that the pattern does not perform a read/assert validity.