Make #[derive(Anything)] into sugar for #[derive_Anything] #23137

kmcallister · 2015-03-07T03:20:28Z

This is a hack, but I don't think we can do much better as long as derive is running at the syntax expansion phase.

If the custom_derive feature gate is enabled, this works with user-defined traits and syntax extensions. Without the gate, you can't use e.g. #[derive_Clone] directly, so this does not change the stable language.

To make this effective, we now check gated attributes both before and after macro expansion. This uncovered a number of tests that were missing feature gates.

This PR also cleans up the deriving code somewhat, and forbids some previously-meaningless attribute syntax. For this reason it's technically a

[breaking-change]

r? @sfackler

This is important because attributes can affect expansion.

This is a hack, but I don't think we can do much better as long as `derive` is running at the syntax expansion phase. If the custom_derive feature gate is enabled, this works with user-defined traits and syntax extensions. Without the gate, you can't use e.g. #[derive_Clone] directly, so this does not change the stable language. This commit also cleans up the deriving code somewhat, and forbids some previously-meaningless attribute syntax. For this reason it's technically a [breaking-change]

sfackler · 2015-03-07T17:57:39Z

src/libsyntax/ext/deriving/mod.rs

+            };
+
+            if !(is_builtin_trait(tname) || cx.ecfg.enable_custom_derive()) {
+                feature_gate::emit_feature_err(&cx.parse_sess.span_diagnostic,


If we're treating this change as some nice sugar for libraries that are already opting into the unstable plugin infrastructure, do we need a separate feature for this? It should be covered under #![feature(plugin)] and I'm not sure if there's much benefit to be gained from having a separate feature for custom derives.

Without this check, a misspelling like

#[derive(Eqq)] struct Foo;

will produce a warning or error about unknown attribute derive_Eqq. And that may not happen if compilation fails first due to a missing trait impl. The gate produces a better error message.

It also controls whether you can use e.g. #[derive_Eq] directly. We could probably forbid this outright, but I don't see a need.

sfackler · 2015-03-07T18:00:57Z

We talked about this a bit on IRC. It would be possible to do this by explicitly registering derive plugins that wouldn't involve desugaring. Either solution would need changes made before we can stabilize plugins so I'm fine with this in the interim.

sfackler · 2015-03-07T18:39:13Z

@bors r+ 491054f

@sfackler

This is a hack, but I don't think we can do much better as long as `derive` is running at the syntax expansion phase. If the `custom_derive` feature gate is enabled, this works with user-defined traits and syntax extensions. Without the gate, you can't use e.g. `#[derive_Clone]` directly, so this does not change the stable language. To make this effective, we now check gated attributes both before and after macro expansion. This uncovered a number of tests that were missing feature gates. This PR also cleans up the deriving code somewhat, and forbids some previously-meaningless attribute syntax. For this reason it's technically a [breaking-change] r? @sfackler

bors · 2015-03-07T18:39:18Z

⌛ Testing commit 491054f with merge 668c647...

bors · 2015-03-07T21:52:18Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

huonw · 2015-03-07T22:12:59Z

I would appreciate if I can be cc'd on nontrivial #[derive] changes like this. I know @nikomatsakis had some thoughts too.

liigo · 2015-03-08T01:25:26Z

I don't like sugars like this. What's the value to have it? And I don't like we have two very similar styles of derive things.

liigo · 2015-03-08T01:35:23Z

And maybe need an RFC before implementing language changes. (The PR did
change language reference!)
On Mar 8, 2015 9:25 AM, "Liigo Zhuang" com.liigo@gmail.com wrote:

I don't like sugars like this. What's the value to have it? And I don't
like we have two very similar styles of derive things.

seanmonstar · 2015-03-08T06:40:17Z

I was surprised to see this without an RFC?

sfackler · 2015-03-08T06:53:42Z

It doesn't seem to me like it would need an RFC. It shouldn't cause any user facing behavior to change at all unless they're opting into an unstable feature, and even then the breakage is minor.

kmcallister · 2015-03-08T16:47:07Z

And maybe need an RFC before implementing language changes. (The PR did change language reference!)

There's lots of stuff in the language reference that's unstable, compiler-specific, or just plain wrong.

kmcallister · 2015-03-08T16:52:37Z

I don't like sugars like this. What's the value to have it? And I don't like we have two very similar styles of derive things.

But one is sugar for the other; there's no duplication of meaning. The value is that we can easily register new derive_Foo extensions, inside the compiler, or outside, in plugins.

We could add a special mechanism for hooking into derive(...) directly, but that's a lot more complexity for a solution that isn't really any safer or more hygienic, because they're just string names either way. It's complexity for a purely aesthetic benefit, that doesn't surface in the stable language.

@sfackler and I did talk about long-term directions for this. We probably want a uniform way to refer to syntax-phase stuff (macros, derive extensions, etc.) with a crate-qualified name. Using real paths would be much harder, as syntax expansion happens before name resolution (this was discussed ad nauseam on the macro reform RFC).

kmcallister · 2015-03-08T16:54:49Z

@huonw: Sorry about that, I will cc you in the future. I did talk to Niko on IRC and he said this is how he always wanted to do derive.

steveklabnik · 2015-03-08T17:44:55Z

I was really surprised with this too.

seanmonstar · 2015-03-09T00:07:13Z

It doesn't seem to me like it would need an RFC. It shouldn't cause any user facing behavior to change at all unless they're opting into an unstable feature, and even then the breakage is minor.

@sfackler Breaking changes aren't what require RFCs. Changes to the language or standard library do.

kmcallister · 2015-03-09T01:15:37Z

It's a change that doesn't affect the stable language at all, and only slightly changes the unstable language. If every small change to the unstable language / libraries requires going through the RFC process, then I'll probably stop working on these minor enhancements, because I don't have the patience to push on every tiny fix for weeks and weeks.

Unsubscribing, but please let me know if there is a change to the written RFC policy.

sfackler · 2015-03-09T03:11:32Z

@seanmonstar here are several PRs that altered the syntax extension framework in the last couple of months:

#22899
#22875
#22383
#22300
#22116
#21052

Should all of those have gone through the RFC process? If any tweak to the syntax extension infrastructure requires an RFC, it will completely cripple our ability to iterate on the interface and stabilize it by increasing the time it takes to land a change from a couple of days to 1-2 months.

seanmonstar · 2015-03-09T03:29:11Z

Several of those seem to be bug fixes, or altering the internals of the rustc implementation. But whichever.

That this caused others to be surprised as well seems to suggests many thought a change like this would have used an RFC. 🐋

liigo · 2015-03-10T05:31:53Z

@sfackler

here are several PRs that altered the syntax extension framework in the last couple of months: ...
Should all of those have gone through the RFC process?

There is difference: this PR (#23137) try to modify the well-known #[derive()] syntax, or try to replace it with a different one. #[derive()] is a stable language feature, right? Maybe @brson knows well whether we need an RFC here or not (because he commited the RFC process).

richo · 2015-03-10T05:42:36Z

I can't believe I'm letting myself get dragged into this, but:

This doesn't change the derive syntax at all. It adds a gate that causes it to desugar, which in turn allows plugin authors to catch and handle those derive tags.

Unless I've missed something fairly drastic, this includes no changes to the stable language, and is hidden entirely behind a feature gate.

huonw · 2015-03-10T06:00:51Z

@richo is correct. This change affets #[derive] in a way that users of the stable language can not possibly notice, since (a) the only way to register a custom attribute is via the unstable plugin system, and (b) this desugaring only occurs if a specific feature gate is enabled. To get a custom derive with the main syntax one has have at least #![feature(plugin, custom_derive)] at the top of the crate.

I don't think this sort of change requires an RFC: we regularly break parts of the plugin API, and any future changes to the custom derive functionality will be in the same vein.

nikomatsakis · 2015-03-10T10:57:33Z

On Mon, Mar 09, 2015 at 11:00:57PM -0700, Huon Wilson wrote:

I don't think this sort of change requires an RFC: we regularly
break parts of the plugin API, and any future changes to the custom
derive functionality will be in the same vein.

I think this is a borderline case. I think that if/when the plugin API
(or, more likely, some revamp of it) goes through the RFC process,
this proposal (or, more likely, whatever it evolves into) would be
included in that RFC, but it's ok to iterate in the meantime.

(In general, we try to balance the desire to have everything up front
in an RFC with the desire to be able to iterate in tree. Iterating in
tree carries the risk of winding up de facto committed to a path due
to the existence of code relying on it. The plugin architecture is
probably the best example of that kind of risk at the moment; there is
widespread feeling that it exposes too much of rustc's internals and
that the design should be revisited at some point in the future. I
don't see though that this change digs us any deeper into that hole in
particular.)

As far as the actual design choice goes, personally I agree with kmc
that this is a practical step in the short-term. (I am not sure
whether I like his proposed future alternatives (per-crate names, etc)
because I think I would like to integrate expansion and name
resolution more deeply, but I haven't worked through that all the way
and it may be madness.) I personally prefer a simple sugar like this
to a whole bunch of fancy registries (convention over configuration
and all that).

(I'm not entirely sure what's the best venue for discussing a change
like this; I know there was IRC discussion, but that is transient; RFC
may be too heavyweight; discuss perhaps, but it's a bit broad.)

liigo · 2015-03-11T05:13:09Z

@nikomatsakis

(I'm not entirely sure what's the best venue for discussing a change
like this; I know there was IRC discussion, but that is transient; RFC
may be too heavyweight; discuss perhaps, but it's a bit broad.)

Just discuss in a (WIP) PR? (without a quickly r+.)

Keegan McAllister added 3 commits March 6, 2015 14:12

Consolidate ExpansionConfig feature tests

63ee3fe

Check gated attributes before and after macro expansion

e60e6f0

This is important because attributes can affect expansion.

rust-highfive assigned sfackler Mar 7, 2015

sfackler reviewed Mar 7, 2015
View reviewed changes

bors merged commit 491054f into rust-lang:master Mar 7, 2015

arielb1 mentioned this pull request Jun 21, 2015

Compiler panic: "Path not fully resolved" when passing reference to impl function #22933

Closed

eefriedman mentioned this pull request Mar 31, 2016

Tracking issue for custom_derive feature #29644

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make #[derive(Anything)] into sugar for #[derive_Anything] #23137

Make #[derive(Anything)] into sugar for #[derive_Anything] #23137

kmcallister commented Mar 7, 2015

sfackler Mar 7, 2015

kmcallister Mar 7, 2015

sfackler Mar 7, 2015

sfackler commented Mar 7, 2015

sfackler commented Mar 7, 2015

bors commented Mar 7, 2015

bors commented Mar 7, 2015

huonw commented Mar 7, 2015

liigo commented Mar 8, 2015 via email

liigo commented Mar 8, 2015

seanmonstar commented Mar 8, 2015

sfackler commented Mar 8, 2015

kmcallister commented Mar 8, 2015

kmcallister commented Mar 8, 2015

kmcallister commented Mar 8, 2015

steveklabnik commented Mar 8, 2015

seanmonstar commented Mar 9, 2015

kmcallister commented Mar 9, 2015

sfackler commented Mar 9, 2015

seanmonstar commented Mar 9, 2015

liigo commented Mar 10, 2015

richo commented Mar 10, 2015

huonw commented Mar 10, 2015

nikomatsakis commented Mar 10, 2015

liigo commented Mar 11, 2015

Make #[derive(Anything)] into sugar for #[derive_Anything] #23137

Make #[derive(Anything)] into sugar for #[derive_Anything] #23137

Conversation

kmcallister commented Mar 7, 2015

sfackler Mar 7, 2015

Choose a reason for hiding this comment

kmcallister Mar 7, 2015

Choose a reason for hiding this comment

sfackler Mar 7, 2015

Choose a reason for hiding this comment

sfackler commented Mar 7, 2015

sfackler commented Mar 7, 2015

bors commented Mar 7, 2015

bors commented Mar 7, 2015

huonw commented Mar 7, 2015

liigo commented Mar 8, 2015 via email

liigo commented Mar 8, 2015

seanmonstar commented Mar 8, 2015

sfackler commented Mar 8, 2015

kmcallister commented Mar 8, 2015

kmcallister commented Mar 8, 2015

kmcallister commented Mar 8, 2015

steveklabnik commented Mar 8, 2015

seanmonstar commented Mar 9, 2015

kmcallister commented Mar 9, 2015

sfackler commented Mar 9, 2015

seanmonstar commented Mar 9, 2015

liigo commented Mar 10, 2015

richo commented Mar 10, 2015

huonw commented Mar 10, 2015

nikomatsakis commented Mar 10, 2015

liigo commented Mar 11, 2015