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

Allow all literals in attributes (Tracking Issue for RFC #1559) #34981

Closed
nikomatsakis opened this issue Jul 22, 2016 · 35 comments
Closed

Allow all literals in attributes (Tracking Issue for RFC #1559) #34981

nikomatsakis opened this issue Jul 22, 2016 · 35 comments
Labels
B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Tracking issue for rust-lang/rfcs#1559 -- allow all literals in attributes.

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Jul 22, 2016
@eddyb
Copy link
Member

eddyb commented Jul 22, 2016

@cgswords Should this be assigned to you?

@retep998
Copy link
Member

Just checking, will this apply to align and packed (when they are finally implemented) so that I can do #[repr(align(4))] or #[repr(packed(4))]?

@eddyb
Copy link
Member

eddyb commented Jul 27, 2016

@retep998 That is the intention AFAIK, yes.

@SergioBenitez
Copy link
Contributor

I've started implementing this and have made headway. Should have a working implementation soon.

jseyfried added a commit to jseyfried/rust that referenced this issue Aug 28, 2016
Implement RFC#1559: allow all literals in attributes

Implemented rust-lang/rfcs#1559, tracked by rust-lang#34981.
@nrc nrc added the B-RFC-implemented Blocker: Approved by a merged RFC and implemented. label Aug 29, 2016
matklad added a commit to intellij-rust/intellij-rust that referenced this issue Dec 27, 2016
@Keats
Copy link

Keats commented Dec 28, 2016

Any news about stabilisation of that feature?

@Nemo157
Copy link
Member

Nemo157 commented Jan 30, 2017

I would also like to know if there are any thoughts on stabilisation, this would have been great to use with Macros 1.1, but I guess it's too late for that.

(also, for searchability this is the attr_literals feature, would be nice if the feature name was mentioned in the tracking issue once stuff is implemented)

@Nemo157
Copy link
Member

Nemo157 commented Feb 5, 2017

I think I've found a bug (or at least a big limitation) in the current implementation, negative integer and float literals are not supported:

→ rustc - <<END
#[lit = -1]
struct Foo;
END
error: unexpected token: `-`
 --> <anon>:1:7
  |
1 | #[lit = -1]
  |       ^

error: aborting due to previous error

@eddyb
Copy link
Member

eddyb commented Feb 5, 2017

There are no negative literals (only negation expressions).

@Nemo157
Copy link
Member

Nemo157 commented Feb 5, 2017

😞 damn, I sort of expected something like that. It is a limitation for procedural (derive) macros though, any negative numbers will still need to be passed through as a string and parsed manually. I guess the next step would be to allow constant expressions in attributes, but that sounds like a much bigger step than this one.

@eddyb
Copy link
Member

eddyb commented Feb 6, 2017

The plan ha been full token trees, for a while, really, so you'd just parse it as an expression.
Not implemented, though, AFAIK.

@Kixunil
Copy link
Contributor

Kixunil commented Sep 9, 2017

Will this allow type names in attributes?

@withoutboats
Copy link
Contributor

I've been looking at implementing this properly. My belief is that we intend the grammar for attributes to be one of:

[ PATH ]
[ PATH ( EXPR , ... ) ]
[ PATH = EXPR ]

My (ill-informed) read of libsyntax is that we parse all attributes into the Attribute struct. After parsing their contents correctly, everything after the initial path gets unparsed into a TokenStream.

While validating that the attributes are grammatical (as part of constructing an Attribute) we use these types:

enum MetaItemKind {
    Word,
    List(Vec<NestedMetaItem>),
    NameValue(Lit),
}

enum NestedMetaItemKind {
    MetaItem(MetaItem),
    Literal(Lit)
}

It seems to me the correct thing to do is eliminated NestedMetaItem(Kind) and rewrite MetaItemKind as:

pub enum MetaItemKind {
     Word,
     List(Vec<Expr>),
     NameValue(Expr),
}

Then change the code in libsyntax/attr.rs to correctly implement the conversion to/from tokens for MetaItemKind.

Corrections appreciated.

@eddyb
Copy link
Member

eddyb commented Oct 11, 2017

I believe the intent was to have token streams inside at least #[foo(...)], if not #[foo = ...].

cc @jseyfried @nrc

@withoutboats
Copy link
Contributor

Yes, expanding it to an arbitrary token stream seems reasonable.

@Keats
Copy link

Keats commented May 26, 2018

Anything I could do to help stabilise the current literals?

ALCC01 added a commit to ALCC01/sigil that referenced this issue Jul 17, 2018
* Remove reliance on unstable feature int_to_from_bytes
	* Add a fallback method until feature becomes stable in Rust 1.29
	* See rust-lang/rust#51835
* Remove reliance on experimental feature attr_literals
	* Only used for `structopt`, but it provides `raw()`
	* See rust-lang/rust#34981
* Make tests run again
@SergioBenitez
Copy link
Contributor

SergioBenitez commented Aug 3, 2018

Stabilization Report

Overview

The attr_literals feature allows all unsuffixed literals to be used in attributes in all positions where previously only strings were accepted. This includes, but is not limited to, the following:

  • top-level positions:

    #[attr(true)]
    #[attr(100)]
    #[enabled(true)]
    #[attr("hello")]
    
  • nested positions:

    #[attr("string", ident(100))]
    
  • values of key/value pairs:

    #[attr(enabled = true)]
    #[kind(value, number = 4)]
    

The following tests in rustc are demonstrative of this feature's behavior:

Status

The attr_literals feature was implemented in #35850 and merged on August 30, 2016, or 2 years ago. The feature is being used in several crates including Rocket, structopt, and validator. Furthermore, a subset of this feature is already being used in the stabilized repr attribute (#[repr(align(8))]).

To date, no issues or bugs have been opened about the feature despite its fairly widespread use.

Proposal

I propose that we stabilize this feature in its entirety, allowing unsuffixed literals to be used in attributes. This change is backwards compatible. This change is also forwards compatible with the proposed change to make attribute inputs be an arbitrary TokenStream.

To this end, I submit #53044 and rust-lang/reference#388, stabilizing the feature.

@cramertj
Copy link
Member

cramertj commented Aug 3, 2018

@rfcbot fcp merge

See @SergioBenitez's excellent summary above.

@rfcbot
Copy link

rfcbot commented Aug 3, 2018

Team member @cramertj 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 3, 2018
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Aug 12, 2018
@rfcbot
Copy link

rfcbot commented Aug 12, 2018

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 12, 2018
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 22, 2018
@rfcbot
Copy link

rfcbot commented Aug 22, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@nikomatsakis
Copy link
Contributor Author

Have there been any recent changes here that might be responsible for #53298?


lib.rs

macro_rules! helper {
    ($expr:expr) => {
        #[cfg(feature = $expr)]
        fn foo() {}
    }
}

helper!{concat!("thing")}

This should be an error, as concat!("thing") is not allowed in this position. But instead, the cfg attribute in this example is silently ignored, and fn foo gets compiled.

I tested also with #[doc], hence why the title claims that this applies to "attributes with 'name = $expr'" in general.

This is a regression from stable 1.17 to 1.18.

@SergioBenitez
Copy link
Contributor

The feature has been stabilized! Looks like this issue can finally be closed.

@pravic
Copy link
Contributor

pravic commented Oct 22, 2018

Hmm, it's not clear: are all those literals just a syntax sugar and they are converted to strings internally (as it used to be without this RFC) or attribute "consumers" will expect integer literals too?

@eddyb
Copy link
Member

eddyb commented Nov 3, 2018

@pravic The consumers can see the real literal tokens, and in the foo(...) form, any tokens that might be in there (but this is tracked elsewhere).

bors added a commit that referenced this issue Jan 4, 2019
Implement basic input validation for built-in attributes + Stabilize `unrestricted_attribute_tokens`

Based on #57272

---

In accordance with the plan in https://internals.rust-lang.org/t/unrestricted-attribute-tokens-feature-status/8561:
- The correct top-level shape (`#[attr]` vs `#[attr(...)]` vs `#[attr = ...]`) is enforced for built-in attributes.
- For non-built-in non-macro attributes:
    - The key-value form is restricted to bare minimum required to support what we support on stable - unsuffixed literals (#34981).
    - Arbitrary token streams in remaining forms (`#[attr(token_stream)]`, `#[attr{token_stream}]`, `#[attr[token_stream]]` ) are now allowed without a feature gate, like in macro attributes.

This will close #55208 once completed.
Need to go through crater first.
bors added a commit that referenced this issue Feb 25, 2019
Stabilize `unrestricted_attribute_tokens`

In accordance with a plan described in https://internals.rust-lang.org/t/unrestricted-attribute-tokens-feature-status/8561/3.

Delimited non-macro non-builtin attributes now support the same syntax as macro attributes:
```
PATH
PATH `(` TOKEN_STREAM `)`
PATH `[` TOKEN_STREAM `]`
PATH `{` TOKEN_STREAM `}`
```
Such attributes mostly serve as inert proc macro helpers or tool attributes.
To some extent these attributes are de-facto stable due to a hole in feature gate checking (feature gating is done too late - after macro expansion.)
So if macro *removes* such helper attributes during expansion (and it must remove them, unless it's a derive macro), then the code will work on stable.

Key-value non-macro non-builtin attributes are now restricted to bare minimum required to support what we support on stable - unsuffixed literals (#34981).
```
PATH `=` LITERAL
```
(Key-value macro attributes are not supported at all right now.)
Crater run in #57321 found no regressions for this change.
There are multiple possible ways to extend key-value attributes (#57321 (comment)), but I'd expect an RFC for that and it's not a pressing enough issue to block stabilization of delimited attributes.

Built-in attributes are still restricted to the "classic" meta-item syntax, nothing changes here.
#57321 goes further and adds some additional restrictions (more consistent input checking) to built-in attributes.

Closes #55208
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests