Disallow deriving (other than Copy/Clone) on types with unnamed fields #121270

clubby789 · 2024-02-18T15:55:26Z

Fixes #121263
Fixes #121299
Fixes #121161

rustbot · 2024-02-18T15:55:34Z

rustbot has assigned @michaelwoerister.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

compiler/rustc_hir_analysis/src/collect.rs

…ng `_`

…unnamed fields

clubby789 · 2024-02-19T04:21:10Z

Added a debug assert for underscore in rustc_expand (and use it)
Add checks during generic derives that we don't have unnamed fields on non Copy/Clone derives
In Clone derive, if we have unnamed fields, check that we're also deriving Copy

clubby789 · 2024-02-19T04:22:18Z

Adds some unfortunate duplicated checks to derives so
@bors try @rust-timer queue

…, r=<try> Check for accessing unnamed field in `find_field` Fixes rust-lang#121263 I'm not entirely sure this is the right solution, but it seems to work 😓

bors · 2024-02-19T04:23:28Z

⌛ Trying commit c863574 with merge cc60b4c...

bors · 2024-02-19T05:51:19Z

☀️ Try build successful - checks-actions
Build commit: cc60b4c (cc60b4c88e308bba60d7680958fc23f65638a57d)

rust-timer · 2024-02-19T09:53:49Z

Finished benchmarking commit (cc60b4c): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.8%	[-5.8%, -5.8%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 640.336s -> 641.792s (0.23%)
Artifact size: 308.82 MiB -> 308.81 MiB (-0.00%)

fmease

I have some nitpicks and a suggestion for how we might be able to deduplicate things (iterating through the fields twice to check for unnamed fields).

Perf looks spurious.

Sorry for taking soo long! Thanks for your patience :)

fmease · 2024-03-06T16:28:37Z

compiler/rustc_builtin_macros/messages.ftl

@@ -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


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.

fmease · 2024-03-06T16:29:28Z

compiler/rustc_builtin_macros/messages.ftl

+    .note = unnamed field
+
+builtin_macros_unnamed_field_derive_clone =
+    deriving `Clone` on a type with unnamed fields requires also deriving `Copy`


Should we also add the note/label unnamed field like in the case above?

iirc we don't have a better span at this point for the field so I omitted that

fmease · 2024-03-07T10:47:40Z

compiler/rustc_builtin_macros/src/deriving/generic/ty.rs

@@ -69,6 +70,10 @@ impl Path {
            }
        }
    }
+
+    pub fn components(&self) -> &[Symbol] {


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.

fmease · 2024-03-07T10:50:19Z

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

+                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)


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.

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.

fmease · 2024-03-07T10:58:36Z

tests/ui/union/unnamed-fields/derive.stderr

+LL | #[derive(Debug)]
+   |          ^^^^^
+   |
+note: unnamed field


I feel like this should be a label instead of note but it's not that important.

fmease · 2024-03-07T11:02:36Z

tests/ui/union/unnamed-fields/derive.stderr

+LL | #[derive(Debug)]
+   |          ^^^^^
+   |
+note: unnamed field


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!

fmease · 2024-03-20T17:34:38Z

Marking this is as blocked / waiting on team. Feature unnamed_fields will soon go into FCP-close.

apiraino · 2024-03-28T09:48:45Z

Marking this is as blocked / waiting on team. Feature unnamed_fields will soon go into FCP-close.

@fmease can you please point me to the FCP? Did a quick search and couldn't find it :)

fmease · 2024-03-28T16:12:02Z

There's no FCP yet, my comment was a bit imprecise. T-lang has decided to propose an FCP with disposition close in the triage meeting 2024-03-20 (see this section of the minutes, scroll down to Consensus) once @compiler-errors has written up the necessary context for the other T-lang members (arguments for & against unnamed_fields).

pnkfelix · 2024-06-06T14:51:53Z

@rustbot label: +I-lang-nominated

This PR that addresses some ICEs arising for the unstable feature(unnamed_fields), by conservatively mapping the ICE'ing cases to static errors instead.

The T-compiler team wants to know the opinion of T-lang of whether feature(unnamed_fields) is sufficiently likely, in the near future, to be removed (or significantly reworked) to such a degree that it would make more sense to close this PR rather than have contributors spend further time on it.

(See also the context established by https://hackmd.io/7r0i-EWyR8yO6po2LnS2rA#Tracking-issue-for-RFC-2102-Unnamed-fields-of-struct-and-union-type-rust49804 (where I think there was supposed to be an eventual writeup of the concerns people had with feature(unnamed_fields)) and #49804 (comment) )

traviscross · 2024-07-03T04:29:26Z

We discussed this in the meeting on 2024-06-12 without consensus.

Some felt that this was still needed, others first wanted to look for a more minimal approach.

We left this for further discussion.

rustbot assigned michaelwoerister Feb 18, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 18, 2024

fmease reviewed Feb 18, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/collect.rs Outdated Show resolved Hide resolved

fmease reviewed Feb 19, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/collect.rs Outdated Show resolved Hide resolved

fmease assigned fmease and unassigned michaelwoerister Feb 19, 2024

clubby789 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024

clubby789 added 2 commits February 19, 2024 01:53

Use expr_field in builtin derives and assert that we aren't accessi…

985f756

…ng `_`

Prevent builtin derive macro (except Copy+Clone) usage on types with …

c863574

…unnamed fields

rust-cloud-vms bot force-pushed the more-unnamed-fields-checks branch from 48d9772 to c863574 Compare February 19, 2024 04:18

clubby789 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2024

clubby789 changed the title ~~Check for accessing unnamed field in find_field~~ Disallow deriving (other than Copy/Clone) on types with anonymous fields Feb 22, 2024

clubby789 changed the title ~~Disallow deriving (other than Copy/Clone) on types with anonymous fields~~ Disallow deriving (other than Copy/Clone) on types with unnamed fields Feb 22, 2024

This was referenced Feb 22, 2024

ICE: Anonymous structs or unions cannot be type parameters #121161

Open

ICE: Anonymous structs or unions cannot be type parameters #121299

Open

fmease approved these changes Mar 7, 2024

View reviewed changes

fmease reviewed Mar 7, 2024

View reviewed changes

fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. F-unnamed_fields `#![feature(unnamed_fields)]` and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2024

Dylan-DPC added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Apr 26, 2024

fmease mentioned this pull request May 12, 2024

Tracking issue for RFC 2102, "Unnamed fields of struct and union type" #49804

Open

4 tasks

rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow deriving (other than Copy/Clone) on types with unnamed fields #121270

Disallow deriving (other than Copy/Clone) on types with unnamed fields #121270

clubby789 commented Feb 18, 2024 •

edited

Loading

rustbot commented Feb 18, 2024

clubby789 commented Feb 19, 2024

clubby789 commented Feb 19, 2024

This comment has been minimized.

bors commented Feb 19, 2024

bors commented Feb 19, 2024

This comment has been minimized.

rust-timer commented Feb 19, 2024

fmease left a comment •

edited

Loading

fmease Mar 6, 2024 •

edited

Loading

fmease Mar 6, 2024 •

edited

Loading

clubby789 Mar 7, 2024

fmease Mar 7, 2024

fmease Mar 7, 2024

fmease Mar 7, 2024

fmease Mar 7, 2024

fmease Mar 7, 2024

fmease commented Mar 20, 2024

apiraino commented Mar 28, 2024

fmease commented Mar 28, 2024 •

edited

Loading

pnkfelix commented Jun 6, 2024

traviscross commented Jul 3, 2024

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

Disallow deriving (other than Copy/Clone) on types with unnamed fields #121270

Are you sure you want to change the base?

Disallow deriving (other than Copy/Clone) on types with unnamed fields #121270

Conversation

clubby789 commented Feb 18, 2024 • edited Loading

rustbot commented Feb 18, 2024

clubby789 commented Feb 19, 2024

clubby789 commented Feb 19, 2024

This comment has been minimized.

bors commented Feb 19, 2024

bors commented Feb 19, 2024

This comment has been minimized.

rust-timer commented Feb 19, 2024

Overall result: ❌ regressions - no action needed

fmease left a comment • edited Loading

Choose a reason for hiding this comment

fmease Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

fmease Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease commented Mar 20, 2024

apiraino commented Mar 28, 2024

fmease commented Mar 28, 2024 • edited Loading

pnkfelix commented Jun 6, 2024

traviscross commented Jul 3, 2024

clubby789 commented Feb 18, 2024 •

edited

Loading

fmease left a comment •

edited

Loading

fmease Mar 6, 2024 •

edited

Loading

fmease Mar 6, 2024 •

edited

Loading

fmease commented Mar 28, 2024 •

edited

Loading