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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for arbitrary token streams in non-macro attributes (unrestricted_attribute_tokens) #55208

Closed
petrochenkov opened this issue Oct 19, 2018 · 8 comments · Fixed by #57367
Closed

Tracking issue for arbitrary token streams in non-macro attributes (unrestricted_attribute_tokens) #55208

petrochenkov opened this issue Oct 19, 2018 · 8 comments · Fixed by #57367
Assignees
@petrochenkov
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 19, 2018 •
edited
Loading

This issue tracks feature unrestricted_attribute_tokens introduced for future-compatibility during stabilization of macros 1.2.

The feature allows using the full macro attribute grammar for non-macro attributes:

PATH
PATH `(` TOKEN_STREAM `)`
PATH `[` TOKEN_STREAM `]`
PATH `{` TOKEN_STREAM `}`

Additionally, the feature extends the grammar of value in key-value attributes (#[key = "value"]) to arbitrary token trees:

PATH = TOKEN_TREE

More information and proposed path to stabilization can be found in https://internals.rust-lang.org/t/unrestricted-attribute-tokens-feature-status/8561.
@petrochenkov petrochenkov self-assigned this Oct 19, 2018
@petrochenkov petrochenkov 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. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Oct 19, 2018
@kennytm kennytm mentioned this issue Nov 6, 2018
Closed
@Centril Centril added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Nov 10, 2018
@Centril
Copy link
Contributor

Centril commented Nov 10, 2018

cc @rust-lang/lang

I'm in favor of doing this change; all of it. But do we need this to go through an RFC first?

@flying-sheep
Copy link

I’m a bit confused: I want to use a macro invocation in the attribute value position, but apparently it doesn’t get expanded: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=14757b61cc384f085a63efea3002b96d

The code compiles, but I have no idea what actually ends up in the attribute.

@eddyb
Copy link
Member

eddyb commented Dec 3, 2018

@flying-sheep That's not supported - serde sees what you gave it, without any expansion, e.g. #[serde(skip_serializing_if = type_is_empty!(bool))].
"Early expansion" has been discussed elsewhere, and for backwards-compatibility reasons it would have to be opt-in anyway (i.e. we can't change what the code you wrote does).

@flying-sheep
Copy link

flying-sheep commented Dec 3, 2018 •
edited
Loading

Do you know where early expansion is discussed? I’m also be happy with #[my_macro!(...)] being possible; is that in the pipes?

@Centril
Copy link
Contributor

Centril commented Dec 3, 2018

@flying-sheep That's not supported - serde sees what you gave it, without any expansion, e.g. #[serde(skip_serializing_if = type_is_empty!(bool))].
"Early expansion" has been discussed elsewhere, and for backwards-compatibility reasons it would have to be opt-in anyway (i.e. we can't change what the code you wrote does).

@eddyb But in this case you can only pass this to serde because an opt-in feature gate has been enabled; can you elaborate on the backwards compat concern?

@eddyb
Copy link
Member

eddyb commented Dec 3, 2018

@Centril I was talking about (early) macro expansion in general, not this specific instance.

But also, I'm pretty sure the intention is to allow free-form attribute input, which would make type_is_empty!($typ) not a macro invocation, unless serde_derive specifically treats it as such.

Meanwhile, opt-in early expansion proposals would work in this case, without attribute parsing or serde_derive knowing anything about a macro even being around.

@petrochenkov petrochenkov mentioned this issue Jan 4, 2019
Merged
bors added a commit that referenced this issue Jan 4, 2019
@bors
Auto merge of #57321 - petrochenkov:atokens, r=<try>
b9139d2 
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.
@petrochenkov
Copy link
Contributor Author

PR implementing the stabilization plan - #57321.

@petrochenkov petrochenkov mentioned this issue Jan 6, 2019
Merged
@shepmaster
Copy link
Member

Followup PR implementing the stabilization plan - #57367

bors added a commit that referenced this issue Feb 25, 2019
@bors
Auto merge of #57367 - petrochenkov:unrestab, r=Centril
55c173c 
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
@ehuss ehuss mentioned this issue Feb 26, 2019
Closed
d-e-s-o added a commit to d-e-s-o/gui that referenced this issue Sep 7, 2019
@d-e-s-o
Use an actual type for the gui(Event = ...) attribute
43c6b47 
The current implementation of gui-derive's Event attribute has the
problem that the user specifies the event type to use in representation
of a string. That is a problem for many reasons, two of which are:
- when an event is used only in an attribute (by virtue of being a
  string there), the compiler will flag it as unused
- type lookup (using the RLS) will simply not work as expected because a
  string looses all the type information

It was not an accident that this functionality was implemented this way
as the unrestricted_attribute_tokens feature, which allows for
non-string tokens to be used as attributes, was unstable at the time
this functionality was implemented, causing compilation errors such as
the following:
  > error: expected unsuffixed literal or identifier, found Event2
  >   --> tests/test_derive.rs:38:7
  >    |
  > 38 | #[gui(Event = Event2)]
  >    |       ^^^^^
  >    |
  >    = help: try enabling `#![feature(unrestricted_attribute_tokens)]`

Now that [Rust issue 55208][issue-55208] has landed and reached the
stable toolchain, this change implements the functionality properly.

[issue-55208]: rust-lang/rust#55208
d-e-s-o added a commit to d-e-s-o/gui that referenced this issue Sep 7, 2019
@d-e-s-o
Use an actual type for the gui(Event = ...) attribute
7d7852b 
The current implementation of gui-derive's Event attribute has the
problem that the user specifies the event type to use in representation
of a string. That is a problem for many reasons, two of which are:
- when an event is used only in an attribute (by virtue of being a
  string there), the compiler will flag it as unused
- type lookup (using the RLS) will simply not work as expected because a
  string looses all the type information

It was not an accident that this functionality was implemented this way
as the unrestricted_attribute_tokens feature, which allows for
non-string tokens to be used as attributes, was unstable at the time
this functionality was implemented, causing compilation errors such as
the following:
  > error: expected unsuffixed literal or identifier, found Event2
  >   --> tests/test_derive.rs:38:7
  >    |
  > 38 | #[gui(Event = Event2)]
  >    |       ^^^^^
  >    |
  >    = help: try enabling `#![feature(unrestricted_attribute_tokens)]`

Now that [Rust issue 55208][issue-55208] has landed and reached the
stable toolchain, this change implements the functionality properly.

[issue-55208]: rust-lang/rust#55208
d-e-s-o added a commit to d-e-s-o/gui that referenced this issue Sep 8, 2019
@d-e-s-o
Use an actual type for the gui(Event = ...) attribute
ebd18c9 
The current implementation of gui-derive's Event attribute has the
problem that the user specifies the event type to use in representation
of a string. That is a problem for many reasons, two of which are:
- when an event is used only in an attribute (by virtue of being a
  string there), the compiler will flag it as unused
- type lookup (using the RLS) will simply not work as expected because a
  string looses all the type information

It was not an accident that this functionality was implemented this way
as the unrestricted_attribute_tokens feature, which allows for
non-string tokens to be used as attributes, was unstable at the time
this functionality was implemented, causing compilation errors such as
the following:
  > error: expected unsuffixed literal or identifier, found Event2
  >   --> tests/test_derive.rs:38:7
  >    |
  > 38 | #[gui(Event = Event2)]
  >    |       ^^^^^
  >    |
  >    = help: try enabling `#![feature(unrestricted_attribute_tokens)]`

Now that [Rust issue 55208][issue-55208] has landed and reached the
stable toolchain, this change implements the functionality properly.

[issue-55208]: rust-lang/rust#55208
d-e-s-o added a commit to d-e-s-o/gui that referenced this issue Sep 8, 2019
@d-e-s-o
Use an actual type for the gui(Event = ...) attribute
f3e0e7a 
The current implementation of gui-derive's Event attribute has the
problem that the user specifies the event type to use in representation
of a string. That is a problem for many reasons, two of which are:
- when an event is used only in an attribute (by virtue of being a
  string there), the compiler will flag it as unused
- type lookup (using the RLS) will simply not work as expected because a
  string looses all the type information

It was not an accident that this functionality was implemented this way
as the unrestricted_attribute_tokens feature, which allows for
non-string tokens to be used as attributes, was unstable at the time
this functionality was implemented, causing compilation errors such as
the following:
  > error: expected unsuffixed literal or identifier, found Event2
  >   --> tests/test_derive.rs:38:7
  >    |
  > 38 | #[gui(Event = Event2)]
  >    |       ^^^^^
  >    |
  >    = help: try enabling `#![feature(unrestricted_attribute_tokens)]`

Now that [Rust issue 55208][issue-55208] has landed and reached the
stable toolchain, this change implements the functionality properly.

[issue-55208]: rust-lang/rust#55208
@CreepySkeleton CreepySkeleton mentioned this issue Dec 30, 2019
Closed
This was referenced Jan 13, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@petrochenkov petrochenkov

Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stabilize unrestricted_attribute_tokens petrochenkov/rust
5 participants
@eddyb @shepmaster @flying-sheep @Centril @petrochenkov