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

Stabilize `unrestricted_attribute_tokens` #57367

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 6, 2019

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 6, 2019

@@ -689,6 +687,8 @@ declare_features! (
(accepted, repr_packed, "1.33.0", Some(33158), None),
// Allows calling `const unsafe fn` inside `unsafe` blocks in `const fn` functions.
(accepted, min_const_unsafe_fn, "1.33.0", Some(55607), None),
// support for arbitrary delimited token streams in non-macro attributes
(accepted, unrestricted_attribute_tokens, "1.33.0", Some(55208), None),

This comment has been minimized.

@Centril

Centril Jan 6, 2019

Contributor
Suggested change Beta
(accepted, unrestricted_attribute_tokens, "1.33.0", Some(55208), None),
(accepted, unrestricted_attribute_tokens, "1.34.0", Some(55208), None),

The master=>beta promotion happens on the 15th of January and FCP takes at least 10 days so this won't make it to 1.33. So this will be 1.34 at earliest unless we beta-backport (which we usually don't do for new features...? with uniform_paths being the exception...)

#![feature(custom_attribute, unrestricted_attribute_tokens)]

#[my_attr = !] // OK under feature gate
#[my_attr = !] //~ ERROR unexpected token: `!`

This comment has been minimized.

@Centril

Centril Jan 6, 2019

Contributor

Could you split this into a new separate feature gate instead so that folks can continue their experimentation with this on nightly? suggestion: unrestricted_kv_attribute_tokens

This comment has been minimized.

@petrochenkov

petrochenkov Jan 6, 2019

Contributor

I'm skeptical about this.
The feature was available for almost two years, but crater still found zero uses of it.

The question about key-value attributes is "what subset of #[attr(stuff)] we may want to write as #[attr = stuff]", so getting more experience with #[attr(stuff)] seems enough.

This comment has been minimized.

@petrochenkov

petrochenkov Jan 6, 2019

Contributor

The most requested extension that I've seen is supporting macro calls like #[attr = mac!(args)], but it 1) doesn't fit into the current implementation syntactically, and most importantly 2) it needs eager expansion (or, equivalently, we need to support #[attr(mac!(args))] first).

This comment has been minimized.

@CAD97

CAD97 Jan 7, 2019

Contributor

What I've seen the most of in the wild is #[attr = "path::to::thing"], which could be better written as #[attr = path::to::thing], but that also doesn't fit into the current "unrestricted_kv_attribute_tokens".

@Centril Centril added the relnotes label Jan 6, 2019

@CAD97

This comment has been minimized.

Copy link
Contributor

CAD97 commented Jan 7, 2019

Minor note: it might make sense to frame this not as "stabilizing unrestricted_attribute_tokens" but as stabilizing a non-problematic subset. There's still intent (as far as I've seen) to expand the ability of key-value style attributes, but that's much more complicated than the delimited style.

So my "suggestion" is to make the stabilized subset stable under unrestricted_delimited_attributes or similar, and leave the current capability of unrestricted_attribute_tokens to be discussed in the RFC for expanding the "-value" grammar of key-value attributes.

Also, cc rust-lang-nursery/wg-grammar#13, @eddyb. This grammar is the current grammar in that (permissive) grammar draft, but this probably should be linked.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

☔️ The latest upstream changes (presumably #57379) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit that referenced this pull request Jan 16, 2019

Auto merge of #57321 - petrochenkov:atokens, r=nikomatsakis
Implement basic input validation for built-in attributes

Correct top-level shape (`#[attr]` vs `#[attr(...)]` vs `#[attr = ...]`) is enforced for built-in attributes, built-in attributes must also fit into the "meta-item" syntax (aka the "classic attribute syntax").

For some subset of attributes (found by crater run), errors are lowered to deprecation warnings.

NOTE: This PR previously included #57367 as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment