Implement `#[skip]` for builtin derives #121053

clubby789 · 2024-02-13T20:58:14Z

Still needs some work but here's an initial working implementation to get feedback on the strategy.

rustbot · 2024-02-13T20:58:22Z

rustbot has assigned @fmease.
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

juntyr · 2024-02-14T07:48:24Z

That looks like a really useful shorthand!

How does this interact with StructuralPartialEq?

clubby789 · 2024-02-14T10:58:55Z

Hmm, good point. I think the simplest solution is to not emit the SPE impl if any fields are being skipped for PE? That would then match the behaviour of a custom PE impl

clubby789 · 2024-02-16T00:35:56Z

Given that this adds quite a bit of logic to deriving:
@bors try @rust-timer queue

Implement `#[skip]` for builtin derives Implement rust-lang#121050 Still needs some work but here's an initial working implementation to get feedback on the strategy.

bors · 2024-02-16T00:37:05Z

⌛ Trying commit 048a438 with merge 6fc06e2...

bors · 2024-02-16T02:04:25Z

☀️ Try build successful - checks-actions
Build commit: 6fc06e2 (6fc06e29295186c2692d3d62a5c2062e584423f2)

rust-timer · 2024-02-16T03:26:06Z

Finished benchmarking commit (6fc06e2): 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)	3.6%	[0.4%, 6.8%]	2
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)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	2.7%	[2.7%, 2.7%]	1

Cycles

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)	4.6%	[4.6%, 4.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 637.914s -> 640.063s (0.34%)
Artifact size: 306.33 MiB -> 306.36 MiB (0.01%)

fmease · 2024-03-22T16:09:42Z

Sorry for the delay ^^', I will try to review this in a few hours max and I will look into adding hygienic helper attributes. I wonder how hard it would be to implement an MVP. Maybe I can manage to mock a pre-RFC as well. No need to rebase btw, the long review time is entirely on me, sorry about that!

fmease

Some initial thoughts.

fmease · 2024-04-24T00:04:50Z

compiler/rustc_lint_defs/src/builtin.rs

+    /// ### Explanation
+    ///
+    /// The `#[skip]` attribute allows ignoring a field during the derivation
+    /// of specific traits. This is not supported for other traits, e.g. it wouldd


Suggested change

/// of specific traits. This is not supported for other traits, e.g. it wouldd

/// of specific traits. This is not supported for other traits, e.g. it would

fmease · 2024-04-24T00:05:35Z

compiler/rustc_lint_defs/src/builtin.rs

+    ///
+    /// The `#[skip]` attribute allows ignoring a field during the derivation
+    /// of specific traits. This is not supported for other traits, e.g. it wouldd
+    /// not be possible to clone a structure without cloning all fields..


Suggested change

/// not be possible to clone a structure without cloning all fields..

/// not be possible to clone a structure without cloning all fields.

fmease · 2024-04-24T00:09:18Z

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

+                    if !skip_enabled {
+                        rustc_session::parse::feature_err(
+                            &cx.sess,
+                            sym::derive_skip,
+                            attr.span,
+                            "the `#[skip]` attribute is experimental",
+                        )
+                        .emit();
+                    }


Note: Breaking change.

fmease · 2024-04-24T00:10:18Z

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

+                    let Some(skip_attr) = ast::Attribute::meta_kind(attr) else {
+                        unreachable!()
+                    };


That's just .unwrap():

Suggested change

let Some(skip_attr) = ast::Attribute::meta_kind(attr) else {

unreachable!()

};

let skip_attr = ast::Attribute::meta_kind(attr).unwrap();

fmease · 2024-04-24T00:12:20Z

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

+                    };
+
+                    // FIXME: better errors
+                    match skip_attr {


Note: Ignoring the fact that the feature-gate error is already a breaking change, attribute validation of #[skip] emitting hard errors is also a breaking change.

fmease · 2024-04-24T00:19:58Z

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

+}
+
+impl SkippedDerives {
+    pub fn add(&mut self, derive: Symbol) {


simple yet beautiful ❤️

fmease · 2024-04-24T00:24:41Z

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

@@ -1532,11 +1579,76 @@ impl<'a> TraitDef<'a> {
                let mut exprs: Vec<_> = mk_exprs(i, struct_field, sp);
                let self_expr = exprs.remove(0);
                let other_selflike_exprs = exprs;
+                let mut skipped_derives = SkippedDerives::None;


Might want to move a lot of this code into a separate method to avoid rightward drift.

fmease · 2024-04-24T00:30:10Z

tests/ui/deriving/skip/derive-skip-invalid.rs

+
+// FIXME: better error for derives not supporting `skip`
+// #[derive(Clone)]
+// struct SkipClone(#[skip] usize);


If I understand the code correctly, we don't reject #[skip] (SkippedDerives::All) on unsupported traits at all contrary to #[skip(...)] (SkippedDerives::List) and end up generating non-sense code, right?

fmease · 2024-04-24T00:32:28Z

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

+                    // FIXME: better errors
+                    match skip_attr {
+                        ast::MetaItemKind::Word => {
+                            skipped_derives = SkippedDerives::All;


If I understand correctly, this means we're not checking if #[skip] is supported by the current derive macro, right?

fmease · 2024-04-24T00:37:31Z

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

+                                const SUPPORTED_TRAITS: [Symbol; 5] = [
+                                    sym::PartialEq,
+                                    sym::PartialOrd,
+                                    sym::Ord,
+                                    sym::Hash,
+                                    sym::Debug,
+                                ];


Not sure how I feel about hard-coding the list of supported traits. Technically speaking a built-in derive macro supports #[skip] if it declares skip as a helper attribute and conversely if it doesn't declare it then it doesn't support it. I wonder if we could somehow leverage this information (if it's available)?

fmease · 2024-04-24T00:40:47Z

I'm so sorry for letting you hanging like that. The fact that this T-libs-api feature likely requires involved language additions if we want to do it properly, paralyzed me.

fmease · 2024-04-24T00:41:54Z

I'm gonna do a run crater to gather some data before doing anything else.

fmease · 2024-04-24T03:15:26Z

@bors try

bors · 2024-04-24T03:16:36Z

⌛ Trying commit 93300c6 with merge 0d3c0c4...

Implement `#[skip]` for builtin derives Implement rust-lang#121050 Still needs some work but here's an initial working implementation to get feedback on the strategy.

bors · 2024-04-24T04:49:56Z

☀️ Try build successful - checks-actions
Build commit: 0d3c0c4 (0d3c0c4551bd9d6b6a4bd51e251b596e0195adf5)

fmease · 2024-04-24T10:38:53Z

@craterbot check

craterbot · 2024-04-24T10:39:00Z

👌 Experiment pr-121053 created and queued.
🤖 Automatically detected try build 0d3c0c4
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-04-26T02:58:05Z

🚧 Experiment pr-121053 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-04-27T19:46:31Z

🎉 Experiment pr-121053 is completed!
📊 13 regressed and 2 fixed (442198 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

fmease · 2024-06-22T04:18:08Z

rust-lang/libs-team#334 (comment)

@rustbot blocked

joshtriplett · 2024-11-19T18:44:30Z

Given the crater results showing compatibility hazards, it sounds like this is going to need a design and lang RFC for an approach that avoids those. Happy to work with anyone looking to write one. @fmease?

rustbot assigned fmease Feb 13, 2024

rustbot added S-waiting-on-review T-compiler T-libs labels Feb 13, 2024

clubby789 mentioned this pull request Feb 13, 2024

Tracking Issue for derive_skip #121050

Open

4 tasks

rust-cloud-vms bot force-pushed the derive-skip branch from 71d0aa0 to 048a438 Compare February 16, 2024 00:35

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf label Feb 16, 2024

clubby789 mentioned this pull request Feb 16, 2024

Lints while expanding builtin derives are not attributed to the correct AST nodes #121171

Open

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf label Feb 16, 2024

fmease mentioned this pull request Mar 7, 2024

Customizing #[derive(Debug)] rust-lang/libs-team#334

Open

fmease reviewed Apr 24, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

clubby789 added 2 commits April 24, 2024 03:32

Implement #[skip] for builtin derives

e9c3a64

Improve diagnostics for #[skip]

3f50ecc

fmease force-pushed the derive-skip branch from 048a438 to 3f50ecc Compare April 24, 2024 01:49

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

rebase fixes

93300c6

fmease force-pushed the derive-skip branch from 2359013 to 93300c6 Compare April 24, 2024 03:15

craterbot added S-waiting-on-crater and removed S-waiting-on-review labels Apr 24, 2024

craterbot added S-waiting-on-review and removed S-waiting-on-crater labels Apr 27, 2024

rustbot added S-blocked and removed S-waiting-on-review labels Jun 22, 2024

Dylan-DPC added S-waiting-on-ACP and removed S-blocked labels Aug 7, 2024

	/// of specific traits. This is not supported for other traits, e.g. it wouldd
	/// of specific traits. This is not supported for other traits, e.g. it would

	/// not be possible to clone a structure without cloning all fields..
	/// not be possible to clone a structure without cloning all fields.

Implement #[skip] for builtin derives #121053

Are you sure you want to change the base?

Implement #[skip] for builtin derives #121053

Conversation

clubby789 commented Feb 13, 2024 • edited by Dylan-DPC Loading

rustbot commented Feb 13, 2024

juntyr commented Feb 14, 2024

clubby789 commented Feb 14, 2024

clubby789 commented Feb 16, 2024

This comment has been minimized.

bors commented Feb 16, 2024

bors commented Feb 16, 2024

This comment has been minimized.

rust-timer commented Feb 16, 2024

Overall result: ❌ regressions - no action needed

fmease commented Mar 22, 2024 • edited Loading

fmease left a 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

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 Apr 24, 2024 • edited Loading

fmease commented Apr 24, 2024 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

fmease commented Apr 24, 2024

bors commented Apr 24, 2024

bors commented Apr 24, 2024

fmease commented Apr 24, 2024

craterbot commented Apr 24, 2024

craterbot commented Apr 26, 2024

craterbot commented Apr 27, 2024

fmease commented Jun 22, 2024

joshtriplett commented Nov 19, 2024

Implement `#[skip]` for builtin derives #121053

Implement `#[skip]` for builtin derives #121053

clubby789 commented Feb 13, 2024 •

edited by Dylan-DPC

Loading

fmease commented Mar 22, 2024 •

edited

Loading

fmease commented Apr 24, 2024 •

edited

Loading

fmease commented Apr 24, 2024 •

edited

Loading