Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 6 additions & 0 deletions compiler/rustc_builtin_macros/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@fmease fmease Mar 6, 2024

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 with Copy for now, I feel like we should make this more precise / less confusing if possible. Either dropping and `Clone` entirely or saying `Copy` and `Clone` with `Copy` which is a bit awkward I admit. Open to suggestions.

.note = unnamed field

builtin_macros_unnamed_field_derive_clone =
deriving `Clone` on a type with unnamed fields requires also deriving `Copy`
Copy link
Member

@fmease fmease Mar 6, 2024

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

71 changes: 41 additions & 30 deletions compiler/rustc_builtin_macros/src/deriving/clone.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::deriving::generic::ty::*;
use crate::deriving::generic::*;
use crate::deriving::path_std;
use crate::errors;
use rustc_ast::{self as ast, Generics, ItemKind, MetaItem, VariantData};
use rustc_data_structures::fx::FxHashSet;
use rustc_expand::base::{Annotatable, ExtCtxt};
Expand Down Expand Up @@ -32,42 +33,52 @@ pub fn expand_deriving_clone(
let bounds;
let substructure;
let is_simple;
match item {
Annotatable::Item(annitem) => match &annitem.kind {
ItemKind::Struct(_, Generics { params, .. })
| ItemKind::Enum(_, Generics { params, .. }) => {
let container_id = cx.current_expansion.id.expn_data().parent.expect_local();
let has_derive_copy = cx.resolver.has_derive_copy(container_id);
if has_derive_copy
&& !params
.iter()
.any(|param| matches!(param.kind, ast::GenericParamKind::Type { .. }))
{
bounds = vec![];
is_simple = true;
substructure = combine_substructure(Box::new(|c, s, sub| {
cs_clone_simple("Clone", c, s, sub, false)
}));
} else {
bounds = vec![];
is_simple = false;
substructure =
combine_substructure(Box::new(|c, s, sub| cs_clone("Clone", c, s, sub)));
}
}
ItemKind::Union(..) => {
bounds = vec![Path(path_std!(marker::Copy))];
let Annotatable::Item(annitem) = item else {
cx.dcx().span_bug(span, "`#[derive(Clone)]` on trait item or impl item")
};
let has_unnamed = if let ItemKind::Struct(VariantData::Struct { fields, .. }, _) = &annitem.kind
{
fields.iter().any(|f| f.ident.is_some_and(|i| i.name == kw::Underscore))
} else {
false
};
match &annitem.kind {
ItemKind::Struct(_, Generics { params, .. })
| ItemKind::Enum(_, Generics { params, .. }) => {
let container_id = cx.current_expansion.id.expn_data().parent.expect_local();
let has_derive_copy = cx.resolver.has_derive_copy(container_id);
if has_derive_copy
&& !params
.iter()
.any(|param| matches!(param.kind, ast::GenericParamKind::Type { .. }))
{
bounds = vec![];
is_simple = true;
substructure = combine_substructure(Box::new(|c, s, sub| {
cs_clone_simple("Clone", c, s, sub, true)
cs_clone_simple("Clone", c, s, sub, false)
}));
} else {
bounds = vec![];
is_simple = false;
substructure =
combine_substructure(Box::new(|c, s, sub| cs_clone("Clone", c, s, sub)));
}
_ => cx.dcx().span_bug(span, "`#[derive(Clone)]` on wrong item kind"),
},

_ => cx.dcx().span_bug(span, "`#[derive(Clone)]` on trait item or impl item"),
}
ItemKind::Union(..) => {
bounds = vec![Path(path_std!(marker::Copy))];
is_simple = true;
substructure = combine_substructure(Box::new(|c, s, sub| {
cs_clone_simple("Clone", c, s, sub, true)
}));
}
_ => cx.dcx().span_bug(span, "`#[derive(Clone)]` on wrong item kind"),
}

if !is_simple && has_unnamed {
cx.dcx().emit_err(errors::UnnamedFieldDeriveClone { span });
return;
};

let trait_def = TraitDef {
span,
path: path_std!(clone::Clone),
Expand Down
20 changes: 20 additions & 0 deletions compiler/rustc_builtin_macros/src/deriving/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Theoretically we could move the “#[derive(Clone)] requires accompanying #[derive(Copy)]” check here to avoid the duplication, right? Or you do think this “violates the layers of abstraction”? I haven't thought that hard about it.

Ah, but that would lead to us performing more work in the error path since we wouldn't be able to return early in expand_deriving_clone. However, that should be fine as long as we don't trigger any debug asserts.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, one more thing: Could you add a FIXME(unnamed_fields) (here or in mod clone depending on the way we go forward) saying that we should permit derive(Clone) without derive(Copy) once we can support it, i.e., once struct initialization supports unnamed fields.

{
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,
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_builtin_macros/src/deriving/generic/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_>,
Expand All @@ -69,6 +70,10 @@ impl Path {
}
}
}

pub fn components(&self) -> &[Symbol] {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn components(&self) -> &[Symbol] {
pub fn segments(&self) -> &[Symbol] {

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.
Expand Down
16 changes: 16 additions & 0 deletions compiler/rustc_builtin_macros/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,22 @@ pub(crate) struct TestRunnerNargs {
pub(crate) span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_unnamed_field_derive)]
pub(crate) struct UnnamedFieldDerive {
#[primary_span]
pub(crate) span: Span,
#[note]
pub(crate) fields: Vec<Span>,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_unnamed_field_derive_clone)]
pub(crate) struct UnnamedFieldDeriveClone {
#[primary_span]
pub(crate) span: Span,
}

#[derive(Diagnostic)]
#[diag(builtin_macros_expected_register_class_or_explicit_register)]
pub(crate) struct ExpectedRegisterClassOrExplicitRegister {
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_expand/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,9 @@ impl<'a> ExtCtxt<'a> {
is_placeholder: false,
}
}
pub fn expr_err(&self, span: Span) -> P<ast::Expr> {
self.expr(span, ast::ExprKind::Err)
}
pub fn expr_struct(
&self,
span: Span,
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/union/unnamed-fields/derive.rs
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() {}
23 changes: 23 additions & 0 deletions tests/ui/union/unnamed-fields/derive.stderr
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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
note: unnamed field
note: an unnamed field is specified here
Suggested change
note: unnamed field
note: unnamed field found here

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

Loading