-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Disallow deriving (other than Copy/Clone) on types with unnamed fields #121270
Changes from all commits
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 |
---|---|---|
|
@@ -245,3 +245,9 @@ builtin_macros_unexpected_lit = expected path to a trait, found literal | |
.other = for example, write `#[derive(Debug)]` for `Debug` | ||
|
||
builtin_macros_unnameable_test_items = cannot test inner items | ||
|
||
builtin_macros_unnamed_field_derive = only `Copy` and `Clone` may be derived on structs with unnamed fields | ||
.note = unnamed field | ||
|
||
builtin_macros_unnamed_field_derive_clone = | ||
deriving `Clone` on a type with unnamed fields requires also deriving `Copy` | ||
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. Should we also add the note/label unnamed field like in the case above? 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. iirc we don't have a better span at this point for the field so I omitted that |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -475,6 +475,26 @@ impl<'a> TraitDef<'a> { | |
false | ||
}); | ||
|
||
if let ast::ItemKind::Struct(struct_def, _) | ast::ItemKind::Union(struct_def, _) = | ||
&item.kind | ||
&& let Some(&trait_name) = self.path.components().last() | ||
&& !(trait_name == sym::Copy || trait_name == sym::Clone) | ||
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. Theoretically we could move the “ Ah, but that would lead to us performing more work in the error path since we wouldn't be able to return early in 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. Ah, one more thing: Could you add a |
||
{ | ||
let unnamed = struct_def | ||
.fields() | ||
.iter() | ||
.filter(|f| f.ident.is_some_and(|i| i.name == kw::Underscore)) | ||
.map(|f| f.span) | ||
.collect::<Vec<_>>(); | ||
if !unnamed.is_empty() { | ||
cx.dcx().emit_err(errors::UnnamedFieldDerive { | ||
span: self.span, | ||
fields: unnamed, | ||
}); | ||
return; | ||
} | ||
}; | ||
|
||
let newitem = match &item.kind { | ||
ast::ItemKind::Struct(struct_def, generics) => self.expand_struct_def( | ||
cx, | ||
|
@@ -1578,14 +1598,12 @@ impl<'a> TraitDef<'a> { | |
// `unwrap_or_else` case otherwise the hygiene is wrong and we get | ||
// "field `0` of struct `Point` is private" errors on tuple | ||
// structs. | ||
let mut field_expr = cx.expr( | ||
let mut field_expr = cx.expr_field( | ||
sp, | ||
ast::ExprKind::Field( | ||
selflike_arg.clone(), | ||
struct_field.ident.unwrap_or_else(|| { | ||
Ident::from_str_and_span(&i.to_string(), struct_field.span) | ||
}), | ||
), | ||
selflike_arg.clone(), | ||
struct_field.ident.unwrap_or_else(|| { | ||
Ident::from_str_and_span(&i.to_string(), struct_field.span) | ||
}), | ||
); | ||
if is_packed { | ||
// In general, fields in packed structs are copied via a | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -48,6 +48,7 @@ impl Path { | |||||
) -> P<ast::Ty> { | ||||||
cx.ty_path(self.to_path(cx, span, self_ty, self_generics)) | ||||||
} | ||||||
|
||||||
pub fn to_path( | ||||||
&self, | ||||||
cx: &ExtCtxt<'_>, | ||||||
|
@@ -69,6 +70,10 @@ impl Path { | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
pub fn components(&self) -> &[Symbol] { | ||||||
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.
Suggested change
I think we generally use the terminology path segments in the context of Rust paths contrary to filesystem paths which indeed use path components. |
||||||
&self.path | ||||||
} | ||||||
} | ||||||
|
||||||
/// A type. Supports pointers, Self, and literals. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
#![allow(incomplete_features)] | ||
#![feature(unnamed_fields)] | ||
|
||
#[derive(Clone, Copy, Debug)] | ||
#[repr(C)] | ||
struct Foo { | ||
a: u8, | ||
} | ||
|
||
#[repr(C)] | ||
#[derive(Debug)] //~ ERROR only `Copy` and `Clone` may be derived on structs with unnamed fields | ||
struct TestUnsupported { | ||
_: Foo, | ||
} | ||
|
||
#[repr(C)] | ||
#[derive(Clone, Copy)] | ||
struct Test { | ||
_: Foo, | ||
} | ||
|
||
#[repr(C)] | ||
#[derive(Clone)] //~ ERROR deriving `Clone` on a type with unnamed fields requires also deriving `Copy` | ||
struct TestClone { | ||
_: Foo, | ||
} | ||
|
||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,23 @@ | ||||||||||
error: only `Copy` and `Clone` may be derived on structs with unnamed fields | ||||||||||
--> $DIR/derive.rs:11:10 | ||||||||||
| | ||||||||||
LL | #[derive(Debug)] | ||||||||||
| ^^^^^ | ||||||||||
| | ||||||||||
note: unnamed field | ||||||||||
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 feel like this should be a label instead of note but it's not that important. 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.
Suggested change
Suggested change
Not set in stone but you get my point, something more elaborate :) thanks! |
||||||||||
--> $DIR/derive.rs:13:5 | ||||||||||
| | ||||||||||
LL | _: Foo, | ||||||||||
| ^^^^^^ | ||||||||||
= note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info) | ||||||||||
|
||||||||||
error: deriving `Clone` on a type with unnamed fields requires also deriving `Copy` | ||||||||||
--> $DIR/derive.rs:23:10 | ||||||||||
| | ||||||||||
LL | #[derive(Clone)] | ||||||||||
| ^^^^^ | ||||||||||
| | ||||||||||
= note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info) | ||||||||||
|
||||||||||
error: aborting due to 2 previous errors | ||||||||||
|
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.
We can leave this message as is but since
Clone
can only be derived together withCopy
for now, I feel like we should make this more precise / less confusing if possible. Either droppingand `Clone`
entirely or saying`Copy` and `Clone` with `Copy`
which is a bit awkward I admit. Open to suggestions.