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

RFC: Allow all literals in attributes #1559

Merged
merged 3 commits into from
Jul 22, 2016
Merged

RFC: Allow all literals in attributes #1559

merged 3 commits into from
Jul 22, 2016

Conversation

SergioBenitez
Copy link
Contributor

@SergioBenitez SergioBenitez commented Mar 29, 2016

I'd like to allow all types of literals to appear in attributes at both top-level positions in lists and as values in k/v pairs. This would, for example, make the following syntax valid:

#[attr(ident, ident = 100, "hi", ident = "hello", ident(100))]
#[attr(100)]
#[attr("hello")]
#[repr(C, align = 4)]
#[repr(C, align(4))]

Rendered

@SergioBenitez SergioBenitez changed the title Allow all literals in attributes RFC: Allow all literals in attributes Mar 29, 2016
@eddyb
Copy link
Member

eddyb commented Mar 29, 2016

cc @alexcrichton

@solson
Copy link
Member

solson commented Mar 29, 2016

Unresolved questions:

  • What does "all literals" mean in precise detail? E.g. I see no mention of b"byte strings".
  • What range of integer and float literals are allowed?

@SergioBenitez
Copy link
Contributor Author

@tsion: "All literals" are literals as are accepted by Rust today. That is, literals as defined in the reference and by the AST. This includes byte strings, floating point, etc. The range of these literals shouldn't differ from those in Rust. Parsing literals in attributes shouldn't differ from parsing literals elsewhere.

@brendanzab
Copy link
Member

I'm not very familiar with the current AST, but does 'all literals' include structs, slices, and closures?

@withoutboats
Copy link
Contributor

No.

@SergioBenitez
Copy link
Contributor Author

See the AST definition for what's defined as a literal.

@eddyb
Copy link
Member

eddyb commented Mar 29, 2016

Just for the sake of posterity, the current AST definition of "literal" is this:

pub enum LitKind {
    Str(InternedString, StrStyle),
    ByteStr(Rc<Vec<u8>>),
    Byte(u8),
    Char(char),
    Int(u64, LitIntType),
    Float(InternedString, FloatTy),
    FloatUnsuffixed(InternedString),
    Bool(bool),
}

@alexcrichton
Copy link
Member

In the past I've heard musings that the alternative here, token trees, is a popular choice of how to extend the attribute grammar. I personally feel that so long as we're principled about what we do we should be fine. Some thoughts and questions I have, however, are:

  • If on the command line I specify --cfg foo=32, can that match cfg!(foo = "32") or only cfg!(foo = 32)? It may be worth explaining the interaction between literals and #[cfg] matching here more exhaustively as well?
  • Modifications to the attribute syntax (and hence #[cfg]) now have an implication on Cargo as well. Cargo intentionally mirrors the compiler's syntax in this regard to define target-specific dependencies. All this really means though is that Cargo would need to be updated and a drawback is that any crate using the newer syntax wouldn't build with older Cargos. None of that is really unexpected or new, however, just something to take into account :)

From a "how I personally feel" perspective, I don't think that the motivation is particularly compelling enough to push through a syntactical change like this. I've occasionally felt from time to time it'd be nice to drop the quotes or something like that but never enough to want to actually change the language grammar.

As an aside, this might be an awesome test case for @nikomatsakis's rustypop and put RFC 1331 into action.

@eddyb
Copy link
Member

eddyb commented Mar 29, 2016

@alexcrichton Couldn't cfg keep working on strings, i.e. without changing the interpretation of the command-line arguments? Integer literals would be an opt-in bonus, not a requirement.

@withoutboats
Copy link
Contributor

I would expect --cfg foo=32 to be equivalent to cfg!(foo="32"), since command line arguments are strings.

EDIT: and not equivalent to cfg!(foo=32)

@alexcrichton
Copy link
Member

@eddyb yeah that's one possible route, I may personally prefer to err on the side of being strict, but either way seems ok to me

@nagisa
Copy link
Member

nagisa commented Mar 29, 2016

I don’t see any other way to make “all” literals work in a meaningfully intuitive fashion than coercing those literals into strings (e.g. using literals’ Display impl), or, even better, just taking the literals as they’re written in source.

i.e. #[cfg(foo=32)] would be exactly the same thing to the compiler as #[cfg(foo="32")].

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Mar 29, 2016
@nrc
Copy link
Member

nrc commented Mar 29, 2016

I would prefer that attributes just take token trees, rather than have this key: value stuff going on.

@SergioBenitez
Copy link
Contributor Author

I've made some changes to the RFC. Primarily, I extended the grammar to allow crate attributes (the '!'), defined exactly what "literals" are with references, and included a couple of examples with different kinds of literals. I also added one more alternative: allow only unsuffixed literals.

@alexcrichton

If on the command line I specify --cfg foo=32, can that match cfg!(foo = "32") or only cfg!(foo = 32)? It may be worth explaining the interaction between literals and #[cfg] matching here more exhaustively as well?

The literals allowed by an attribute are totally up to that attribute's author. As other have noted, I would expect that the cfg attribute would only allow strings as values since those value can be passed in through the command line. Thus, the cfg attribute should throw an error if a value of another type was passed in.

@alexcrichton

I don't think that the motivation is particularly compelling enough to push through a syntactical change like this.

I agree that the motivation as stated in the RFC is a bit one-footed. I think this is a case where the motivation for such a change will only become apparent in the future. In particular, I think that as attributes and syntax extensions become more mainstream, the need for flexibility in attributes will become apparent. Without this change, syntax extensions will simply narrow their interfaces to use only what's allowed. It's hard to imagine what will be done with a tool that doesn't exist.

Anecdotally, I'm currently writing a library where attributes serve as one of its core interfaces. I have become increasingly frustrated that I can't write attributes of the form #[attr("string")], #[attr(100)], and #[attr(ident = 100)]. This syntax feels the most natural, but it is simply disallowed. Instead, I have to invent keys for string values, #[attr(key = "string")], and place integers in quotes and occasionally assign strange keys for them: #[attr(key = "100")], #[attr(ident = "100")]. This makes the code harder to read and write, and introduces needless verbiage.

As far as simply allowing any token goes, i.e, the token tree alternative, I'm okay with this. My only concern is that it removes all structure from attributes, which appears to be one of the original goals as alluded to in the reference.

@alexcrichton
Copy link
Member

@SergioBenitez yeah I'd definitely believe that once we start pushing harder on doing "everything with attributes" kinda that we'll want more flexibility in the definition syntax. Thanks for the examples :)

@nrc
Copy link
Member

nrc commented Apr 4, 2016

#1566 proposes (somewhat implicitly) allowing all token trees in attribute args. I intend that libmacro will provide functionallity to make it easy to go from the token tree representation to key/value sets. That will be in a forthcoming RFC, but is sketched in this blog post, see the section 'parsing helper functions`.

Since I would like to put all our energy behind the new procedural macro system, I would prefer not to accept this RFC. I hope that #1566 addresses the use cases here, if not, we should try and make sure it does.

@durka
Copy link
Contributor

durka commented Apr 25, 2016

I'm in favor of this. I think it's super weird that we specify things as strings in attributes when they are clearly numbers, such as target_pointer_width.

@nrc
Copy link
Member

nrc commented Jul 7, 2016

cc @cgswords

Nominating for lang meeting discussion. My feeling is that we want this, but it is better addressed by the more general token trees approach.

@nrc nrc added I-nominated final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed I-nominated labels Jul 7, 2016
@nrc
Copy link
Member

nrc commented Jul 7, 2016

🔔 This RFC is now entering its final comment period 🔔

@nrc
Copy link
Member

nrc commented Jul 7, 2016

A thought from the language team: even if we do want to go with the token tree solution in the long term, if this RFC looks like a benefit (which I personally think it is), then we can accept it as long as it is forwards-compatible (I think it is, but I will put some additional thought into it too).

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jul 18, 2016

As @nrc suggests, I think that while I would long-term prefer token-trees, accepting a wider variety of literals seems like a good idea in the shorter term.

I am mildly nervous about things like suffixed literals and floating point values, but I don't think we should exclude them from what is permitted. I imagine we will want to work out guidelines (as part of follow-up RFCs). For example, while #[align = 4] seems good, do we want to accept #[align = 4_u32]? What about 4_u8? (My opinion: don't accept suffixes unless there is a reason to do so.)

@SergioBenitez
Copy link
Contributor Author

I agree with both of the above comments. While, token trees may be the right solution in the long term (though I'm not sure of this myself), implementing this RFC now would bring greater flexibility in the meantime. Implementing this RFC should be forwards and mostly backwards compatible.

As far as which literals to allow: my personal preference is to allow all unsuffixed literals but not suffixed literals. This includes floating point values. The reason I believe this is the right way to go is because the attribute itself can always constrain the value of literals to that of unsigned, signed, or some width, or range of values, as it prefers.

@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

@nikomatsakis
Copy link
Contributor

Tracking issue: rust-lang/rust#34981

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

@nikomatsakis nikomatsakis merged commit e7c214f into rust-lang:master Jul 22, 2016
nikomatsakis added a commit that referenced this pull request Jul 22, 2016
jseyfried added a commit to jseyfried/rust that referenced this pull request Aug 28, 2016
Implement RFC#1559: allow all literals in attributes

Implemented rust-lang/rfcs#1559, tracked by rust-lang#34981.
critiqjo pushed a commit to critiqjo/rustdoc that referenced this pull request Dec 16, 2016
Implement RFC#1559: allow all literals in attributes

Implemented rust-lang/rfcs#1559, tracked by #34981.
bors added a commit to rust-lang/rust that referenced this pull request Aug 24, 2018
…nkov

Stabilize 'attr_literals' feature.

RFC Issue: rust-lang/rfcs#1559
Tracking Issue: #34981

Reference PR: rust-lang/reference#388.
@Centril Centril added A-syntax Syntax related proposals & ideas A-attributes Proposals relating to attributes labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Proposals relating to attributes A-syntax Syntax related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.