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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

#[cfg_attr] expanding to multiple attributes #2539

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@Havvy
Contributor

Havvy commented Sep 11, 2018

馃柤 Rendered

馃摑Summary

#[cfg_attr(predicate, attr1, attr2, ...)] expands to each attribute instead of only taking one attribute.

馃挅 Thanks

To @Centril for a quick review of the RFC before publishing.

馃棐 I'm working on implementing this right now.

@Centril Centril added the T-lang label Sep 11, 2018

@Centril Centril self-assigned this Sep 11, 2018

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 11, 2018

Contributor

Usually we wait a bit before the debate settles, but in this case this seems to me a very small change with obvious wins to ergonomics, for those it matters to, with few, if any, drawbacks.

@rfcbot merge

Contributor

Centril commented Sep 11, 2018

Usually we wait a bit before the debate settles, but in this case this seems to me a very small change with obvious wins to ergonomics, for those it matters to, with few, if any, drawbacks.

@rfcbot merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 11, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Sep 11, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Havvy

This comment has been minimized.

Show comment
Hide comment
@Havvy

Havvy Sep 11, 2018

Contributor

Oh, cc @petrochenkov who asked me to open an RFC for this.

Contributor

Havvy commented Sep 11, 2018

Oh, cc @petrochenkov who asked me to open an RFC for this.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Sep 11, 2018

Contributor

I kind of wish the predicate was syntactically distinguished but I don't think its possible without introducing a novel syntax pattern that attributes don't have or being a breaking change (or having a weird exception for when there's only 1 attribute).

Contributor

withoutboats commented Sep 11, 2018

I kind of wish the predicate was syntactically distinguished but I don't think its possible without introducing a novel syntax pattern that attributes don't have or being a breaking change (or having a weird exception for when there's only 1 attribute).

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 11, 2018

Contributor

@withoutboats I sorta agree here; but given the status quo, I think there's already an understanding that the second bit is the attribute-to-apply, so it is natural to assume that what follows also are attributes; therefore, there shouldn't be much new to learn.

Contributor

Centril commented Sep 11, 2018

@withoutboats I sorta agree here; but given the status quo, I think there's already an understanding that the second bit is the attribute-to-apply, so it is natural to assume that what follows also are attributes; therefore, there shouldn't be much new to learn.

@cramertj

This comment has been minimized.

Show comment
Hide comment
@cramertj

cramertj Sep 11, 2018

Member

I wondered about #[cfg_attr(predicate, [attr1, attr2, ...])]

Member

cramertj commented Sep 11, 2018

I wondered about #[cfg_attr(predicate, [attr1, attr2, ...])]

@Havvy

This comment has been minimized.

Show comment
Hide comment
@Havvy

Havvy Sep 11, 2018

Contributor

I'd be okay with requiring square brackets there. Though if we do that, is #[cfg_attr(predicate, [])] with no attributes there valid?

Contributor

Havvy commented Sep 11, 2018

I'd be okay with requiring square brackets there. Though if we do that, is #[cfg_attr(predicate, [])] with no attributes there valid?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 11, 2018

Contributor

cc @petrochenkov who asked me to open an RFC for this.

First, I wanted the RFC to specify some kind of attribute syntax before stating that attributes can be put into a comma-separated list, because right now there's no official documentation on what the syntax is and whether it can be put into a list.

Until very recently attributes accepted arbitrary token streams, so #[cfg_attr(predicate_true, attr1, attr2, attr3)] would actually be interpreted as a single attribute with path attr1 and 4 tokens passed to it - , attr2 , attr3.

Right now the implemented syntax is

// Mirrors bang macros `path!(tokens)`/`path![tokens]`/`path!{tokens}` + probably `path!` in the future
PATH `(` TOKEN_STREAM `)`
PATH `[` TOKEN_STREAM `]`
PATH `{` TOKEN_STREAM `}`
PATH
// Additional key-value form
PATH `=` TOKEN_TREE

Everything from that list (with exception of key-value for now) can be passed to attribute proc macros without any feature gates.
Everything from that list can be passed to non-macro attributes (possibly under unrestricted_attribute_tokens feature gate).
cfg_attr supports arbitrary macro and non-macro attributes, so cfg_attr with commas need to support all these syntaxes.

So, this RFC may want to specify what is implemented as supported syntax, or it may stay more conservative and only guarantee that token streams supported by attributes must never contain the comma token , directly.

Contributor

petrochenkov commented Sep 11, 2018

cc @petrochenkov who asked me to open an RFC for this.

First, I wanted the RFC to specify some kind of attribute syntax before stating that attributes can be put into a comma-separated list, because right now there's no official documentation on what the syntax is and whether it can be put into a list.

Until very recently attributes accepted arbitrary token streams, so #[cfg_attr(predicate_true, attr1, attr2, attr3)] would actually be interpreted as a single attribute with path attr1 and 4 tokens passed to it - , attr2 , attr3.

Right now the implemented syntax is

// Mirrors bang macros `path!(tokens)`/`path![tokens]`/`path!{tokens}` + probably `path!` in the future
PATH `(` TOKEN_STREAM `)`
PATH `[` TOKEN_STREAM `]`
PATH `{` TOKEN_STREAM `}`
PATH
// Additional key-value form
PATH `=` TOKEN_TREE

Everything from that list (with exception of key-value for now) can be passed to attribute proc macros without any feature gates.
Everything from that list can be passed to non-macro attributes (possibly under unrestricted_attribute_tokens feature gate).
cfg_attr supports arbitrary macro and non-macro attributes, so cfg_attr with commas need to support all these syntaxes.

So, this RFC may want to specify what is implemented as supported syntax, or it may stay more conservative and only guarantee that token streams supported by attributes must never contain the comma token , directly.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 11, 2018

Contributor

Second, #[cfg_attr(predicate, attr1, attr2, attr3)] is basically the same feature as #[attr1, attr2, attr3] (without cfg_attr), i.e. RFC issue #2516.
So, I think they need to be proposed together.

Contributor

petrochenkov commented Sep 11, 2018

Second, #[cfg_attr(predicate, attr1, attr2, attr3)] is basically the same feature as #[attr1, attr2, attr3] (without cfg_attr), i.e. RFC issue #2516.
So, I think they need to be proposed together.

@ExpHP

This comment has been minimized.

Show comment
Hide comment
@ExpHP

ExpHP Sep 12, 2018

Small or not, I don't get the push for insta-FCP?

There's a question in design space about whether to support 0 attributes. The proposal clearly takes the stance that it should not. I would object to it as well, because it feels like a potential footgun. (you can write the condition, get distracted, come back and forget to add the rest).

However, supporting 0 arguments would be useful for macros and codegen, and would seem to be consistent with many of other rust's decisions:

  • empty <> for both generic parameter lists and generic argument lists
  • trailing + in bounds lists
  • leading | in match
  • cfg(any()) and cfg(all()) evaluating to false and true

ExpHP commented Sep 12, 2018

Small or not, I don't get the push for insta-FCP?

There's a question in design space about whether to support 0 attributes. The proposal clearly takes the stance that it should not. I would object to it as well, because it feels like a potential footgun. (you can write the condition, get distracted, come back and forget to add the rest).

However, supporting 0 arguments would be useful for macros and codegen, and would seem to be consistent with many of other rust's decisions:

  • empty <> for both generic parameter lists and generic argument lists
  • trailing + in bounds lists
  • leading | in match
  • cfg(any()) and cfg(all()) evaluating to false and true
@Havvy

This comment has been minimized.

Show comment
Hide comment
@Havvy

Havvy Sep 12, 2018

Contributor

For the delimited TokenStreams, it's not a problem because the delimiters scope the meaning of the parenthesis.

The problematic case here would be an attribute such as #[foo = "bar", "baz"]. Because naively placing that into cfg_attr like #[cfg_attr(predicate, foo = "bar", "baz")] would make it look like "baz" is an attribute.

To me, there are three ways that can be solved.

  1. Require parenthesis inside cfg_attr or #[] if using a key-value attribute that has a non-delimited comma.
  2. Disallow non-delimited commas.
  3. Never allow key-value attributes for procedural macros and require built-in attributes don't allow non-delimited commas.

Personally, I think the attribute I wrote looks weird, so I'd rather go for option 2.


As for also doing it for attribute syntax in general, that's a bigger change, but you are right that it's basically the same change. Heck, just changing the meaning of #[attr1, attr2] sort of lets multiple attributes fall into place for cfg_attr.

But if we're going to do that, we're going to have to cancel the FCP.

Contributor

Havvy commented Sep 12, 2018

For the delimited TokenStreams, it's not a problem because the delimiters scope the meaning of the parenthesis.

The problematic case here would be an attribute such as #[foo = "bar", "baz"]. Because naively placing that into cfg_attr like #[cfg_attr(predicate, foo = "bar", "baz")] would make it look like "baz" is an attribute.

To me, there are three ways that can be solved.

  1. Require parenthesis inside cfg_attr or #[] if using a key-value attribute that has a non-delimited comma.
  2. Disallow non-delimited commas.
  3. Never allow key-value attributes for procedural macros and require built-in attributes don't allow non-delimited commas.

Personally, I think the attribute I wrote looks weird, so I'd rather go for option 2.


As for also doing it for attribute syntax in general, that's a bigger change, but you are right that it's basically the same change. Heck, just changing the meaning of #[attr1, attr2] sort of lets multiple attributes fall into place for cfg_attr.

But if we're going to do that, we're going to have to cancel the FCP.

@Havvy

This comment has been minimized.

Show comment
Hide comment
@Havvy

Havvy Sep 12, 2018

Contributor

@ExpHP This RFC gives no opinion on it. I considered on that point, and decided to punt. I think we should allow it, but issue a warning if it's found like that in non-macro-generated code. Although this means that #[] becomes a valid attribute if we then also do this for attribute syntax more broadly.

Contributor

Havvy commented Sep 12, 2018

@ExpHP This RFC gives no opinion on it. I considered on that point, and decided to punt. I think we should allow it, but issue a warning if it's found like that in non-macro-generated code. Although this means that #[] becomes a valid attribute if we then also do this for attribute syntax more broadly.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Sep 12, 2018

Contributor

@Havvy
FWIW, #[foo = "bar", "baz"] is not a valid syntax now, TOKEN_TREE in #2539 (comment) means a single token or a delimited group.
I can imagine extending it in the future to support something like #[foo = 1 + 2] (but perhaps #[foo(1 + 2)] is enough to achieve the same effect with fewer issues).

Contributor

petrochenkov commented Sep 12, 2018

@Havvy
FWIW, #[foo = "bar", "baz"] is not a valid syntax now, TOKEN_TREE in #2539 (comment) means a single token or a delimited group.
I can imagine extending it in the future to support something like #[foo = 1 + 2] (but perhaps #[foo(1 + 2)] is enough to achieve the same effect with fewer issues).

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 13, 2018

Member

With the change to more precisely specify attribute syntax, I think this is ready to merge.

Member

joshtriplett commented Sep 13, 2018

With the change to more precisely specify attribute syntax, I think this is ready to merge.

@Havvy

This comment has been minimized.

Show comment
Hide comment
@Havvy

Havvy Sep 13, 2018

Contributor

Recent updates: Two changes have been made to this RFC today. The first outlined the restrictions on future attribute syntax that is a byproduct of this RFC. The second was a material change to allow zero attributes in the list and trailing commas.

I will not be expanding this RFC to also affect general syntax attribute, but rather leave that to its own RFC. It has more drawbacks, and is scope creep.

Contributor

Havvy commented Sep 13, 2018

Recent updates: Two changes have been made to this RFC today. The first outlined the restrictions on future attribute syntax that is a byproduct of this RFC. The second was a material change to allow zero attributes in the list and trailing commas.

I will not be expanding this RFC to also affect general syntax attribute, but rather leave that to its own RFC. It has more drawbacks, and is scope creep.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 25, 2018

Contributor

For the record, I am in favor of the general idea, and supportive of basically any of the various options that been proposed. Happy to let the thread reach a consensus on the minor points.

I would mildly favor permitting zero attributes: it seems like the sort of thing that a code generator might want to be able to do.

It seems like just adding more arguments to #[cfg_attr] is the "obvious" thing to me, given the current syntax, but I also am ok with #[cfg_attr(predicate, [attr1, attr2]).

Contributor

nikomatsakis commented Sep 25, 2018

For the record, I am in favor of the general idea, and supportive of basically any of the various options that been proposed. Happy to let the thread reach a consensus on the minor points.

I would mildly favor permitting zero attributes: it seems like the sort of thing that a code generator might want to be able to do.

It seems like just adding more arguments to #[cfg_attr] is the "obvious" thing to me, given the current syntax, but I also am ok with #[cfg_attr(predicate, [attr1, attr2]).

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 25, 2018

馃敂 This is now entering its final comment period, as per the review above. 馃敂

rfcbot commented Sep 25, 2018

馃敂 This is now entering its final comment period, as per the review above. 馃敂

* `cfg_attr(predicate, attr_1, attr_2)`
* `cfg_attr(predicate, attr,)`
* `cfg_attr(predicate, attr_1, attr_2,)`
* `cfg_attr(predicate)`

This comment has been minimized.

@petrochenkov

petrochenkov Sep 25, 2018

Contributor

Nit: this is better prohibited for uniformity.

Imagine any other syntax for the predicate/attrs separator, e.g. cfg_attr(predicate: attr1, attr2), then empty attribute list would look like cfg_attr(predicate:) with cfg_attr(predicate) being invalid, i.e. the first comma is not like the others.

@petrochenkov

petrochenkov Sep 25, 2018

Contributor

Nit: this is better prohibited for uniformity.

Imagine any other syntax for the predicate/attrs separator, e.g. cfg_attr(predicate: attr1, attr2), then empty attribute list would look like cfg_attr(predicate:) with cfg_attr(predicate) being invalid, i.e. the first comma is not like the others.

This comment has been minimized.

@Centril

Centril Sep 25, 2018

Contributor

We usually go for zero-or-more instead of one-or-more in most places where we allow -or-more? so I'm not sure restricting this to one-or-more is the uniform thing to do here?

@Centril

Centril Sep 25, 2018

Contributor

We usually go for zero-or-more instead of one-or-more in most places where we allow -or-more? so I'm not sure restricting this to one-or-more is the uniform thing to do here?

This comment has been minimized.

@petrochenkov

petrochenkov Sep 26, 2018

Contributor

I mean cfg_attr(predicate, /* no attrs */) - OK, cfg_attr(predicate /* no attrs */) - ERROR, i.e. zero-or-more exactly.

@petrochenkov

petrochenkov Sep 26, 2018

Contributor

I mean cfg_attr(predicate, /* no attrs */) - OK, cfg_attr(predicate /* no attrs */) - ERROR, i.e. zero-or-more exactly.

This comment has been minimized.

@Centril

Centril Sep 26, 2018

Contributor

Oh; I see now :)
I have no strong feelings here.

Are there any future compat hazards with allowing the omission of the , ?
I don't think macros benefit from omitting ,.

@Centril

Centril Sep 26, 2018

Contributor

Oh; I see now :)
I have no strong feelings here.

Are there any future compat hazards with allowing the omission of the , ?
I don't think macros benefit from omitting ,.

This comment has been minimized.

@scottmcm

scottmcm Sep 26, 2018

Member

I think I agree here. Allowing cfg_attr(p) is like allowing both leading and trailing commas; we don't need it, and blocking it simplifies the grammar.

@scottmcm

scottmcm Sep 26, 2018

Member

I think I agree here. Allowing cfg_attr(p) is like allowing both leading and trailing commas; we don't need it, and blocking it simplifies the grammar.

This comment has been minimized.

@Centril

Centril Sep 26, 2018

Contributor

@Havvy can you make the restriction based on @petrochenkov's and @scottmcm's comments?
We can always relax in the future if we feel so inclined but for now we can be more conservative here until we have a good justification to do something else.

@Centril

Centril Sep 26, 2018

Contributor

@Havvy can you make the restriction based on @petrochenkov's and @scottmcm's comments?
We can always relax in the future if we feel so inclined but for now we can be more conservative here until we have a good justification to do something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment