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

syntax: allow setting attributes for deriving impls #20236

Closed
wants to merge 1 commit into from
Closed

syntax: allow setting attributes for deriving impls #20236

wants to merge 1 commit into from

Conversation

emberian
Copy link
Member

The approach this takes is to extend the deriving DSL to accept the form
TRAIT = VALUE, where VALUE is one of the stability names, and the
appropriate attribute will be inserted into the AST of the generated impl.

Closes #18969

@rust-highfive
Copy link
Collaborator

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@emberian
Copy link
Member Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned huonw Dec 26, 2014
@huonw
Copy link
Member

huonw commented Dec 26, 2014

I'm not sure I'm in favour of using TraitName = "...", it seems rather restrictive.

@emberian
Copy link
Member Author

It is restrictive, but it works for what we need now. The approach in #13054 can't work if you want impls to possibly have different stability levels.

@emberian
Copy link
Member Author

In the future we could extend it to have a list of any amount of actual attributes, ie Clone="#[foo] #[bar(baz)]"

@huonw
Copy link
Member

huonw commented Dec 26, 2014

Eh. Not really in favour of strings at all. Also what if we want to add other things?

Name(attributes(stable, cfg(feature = "bar"))) seems mildly better, if a little lispy.

@emberian
Copy link
Member Author

I briefly considered something like that but I consider it ugly.

@emberian
Copy link
Member Author

I'll implement whatever, I just want to knock this off my to-do list.

@aturon
Copy link
Member

aturon commented Dec 27, 2014

@cmr Thanks for doing this!

I tend to agree with @huonw though that we're going to want the ability to set arbitrary attributes, with their own syntax. I'd be fine with doing something more limited for now, as long as there's a clear migration path toward a more general version later.

I don't really know much about the limitations of the syntax here. It'd be awesome, for example, to be able to write:

#[deriving(
    #[stable] 
    #[cfg(test)]
    Clone
)]

@alexcrichton
Copy link
Member

I agree with @aturon and @huonw here that this may not be the final syntax we want, and if so we may want to put it behind a feature gate for now. I believe that @aturon's suggestion should in theory be possible with @nikomatsakis's desire for attributes to be "just token trees", but that may be aways away.

@emberian
Copy link
Member Author

I'll change this to use huon's suggested syntax and put it behind a feature
gate for now, I agree with your aspirations Aaron, but that will be quite a
ways away.
On Dec 27, 2014 10:01 PM, "Alex Crichton" notifications@github.com wrote:

I agree with @aturon https://github.com/aturon and @huonw
https://github.com/huonw here that this may not be the final syntax we
want, and if so we may want to put it behind a feature gate for now. I
believe that @aturon https://github.com/aturon's suggestion should in
theory be possible with @nikomatsakis https://github.com/nikomatsakis's
desire for attributes to be "just token trees", but that may be aways away.


Reply to this email directly or view it on GitHub
#20236 (comment).

@emberian
Copy link
Member Author

emberian commented Jan 1, 2015

r? @huonw @alexcrichton

@rust-highfive rust-highfive assigned huonw and unassigned aturon Jan 1, 2015
This extends the derive DSL from

A := derive '(' N ')'
N := ident | ident ',' N

to

A := derive '(' T ')'
T := N | N ',' N
N := ident | ident '(' attributes '(' meta_seq ') ')

That is, a form like

 #[derive(Clone, Ord(attributes(unstable, cfg(not(windows)))))]

Will apply `#[unstable]` and `#[cfg(not(windows))]` to the derived `Ord` impl.

Closes #18969
@alexcrichton
Copy link
Member

I don't think that this should land without a feature gate for now, this is an extension to a pretty core aspect of the language which hasn't been discussed much here or in rust-lang/rfcs.

@emberian
Copy link
Member Author

emberian commented Jan 2, 2015

Whoops, forgot the feature gate.

On Thu, Jan 1, 2015 at 11:17 PM, Alex Crichton notifications@github.com
wrote:

I don't think that this should land without a feature gate for now, this
is an extension to a pretty core aspect of the language which hasn't been
discussed much here or in rust-lang/rfcs.


Reply to this email directly or view it on GitHub
#20236 (comment).

http://octayn.net/

@emberian emberian closed this Jan 2, 2015
@emberian
Copy link
Member Author

emberian commented Jan 2, 2015

Going to leave this for now to focus on more important changes for the alpha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lints: no way to mark stability of deriving
5 participants