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

extended_key_value_attributes cannot be enabled conditionally #82768

Closed
jyn514 opened this issue Mar 4, 2021 · 9 comments · Fixed by #83366
Closed

extended_key_value_attributes cannot be enabled conditionally #82768

jyn514 opened this issue Mar 4, 2021 · 9 comments · Fixed by #83366
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-stability Area: `#[stable]`, `#[unstable]` etc. F-extended_key_value_attributes `#![feature(extended_key_value_attributes)] T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Mar 4, 2021

I tried this code:

#![cfg_attr(
    any(),
    feature(extended_key_value_attributes),
    doc = include_str!("../README.md")
)]

I expected to see this happen: The compiler gives no error, because doc = include_str! should only be compiled if the predicate is true.

Instead, this happened: The compiler gives an error that extended_key_value_attributes is unstable.

I guess this happens because the compiler has to parse the attribute even if the cfg is false? @petrochenkov is giving an error here intentional?

Meta

rustc --version: 1.52.0-nightly (2021-03-01 4f20caa)

cc #78835
Originally posted by @daxpedda in #82539 (comment)

@jyn514 jyn514 added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-stability Area: `#[stable]`, `#[unstable]` etc. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 4, 2021
@petrochenkov
Copy link
Contributor

fn process_cfg_attr parses all "attribute items" inside cfg_attr first, and only then checks the predicate and possibly throws the attribute items away. (extended_key_value_attributes feature gate is reported during such parsing.)

It's possible to check the predicate first and parse the rest of cfg_attr arguments only if the predicate is true, but in that case cfg_attr will also accept arbitrary garbage like #[cfg_attr(FALSE, a b < 7 . / asjd &)].

@petrochenkov
Copy link
Contributor

The workaround is to add one more layer of cfg_attr:

#![cfg_attr(FALSE, cfg_attr(TRUE, doc = include_str!("../README.md")))] // OK

@jyn514
Copy link
Member Author

jyn514 commented Mar 4, 2021

The workaround is to add one more layer of cfg_attr:

#![cfg_attr(FALSE, cfg_attr(TRUE, doc = include_str!("../README.md")))] // OK

Thanks! Would it be possible to add that as a structured suggestion in the parser? I can take a look at this weekend if you think it's a good idea :)

@petrochenkov
Copy link
Contributor

I don't think it's a reasonable investment of time, especially given that we'll be able to stabilize extended_key_value_attributes pretty soon.

@camelid camelid added the F-extended_key_value_attributes `#![feature(extended_key_value_attributes)] label Mar 6, 2021
@pksunkara
Copy link
Contributor

@daxpedda In what scenario are you checking for the feature existence?

In clap, I do:

#![cfg_attr(feature = "doc", feature(external_doc))]
#![cfg_attr(feature = "doc", doc(include = "../README.md"))]

@jyn514
Copy link
Member Author

jyn514 commented Mar 16, 2021

@pksunkara doc(include) does not use this feature; see the linked thread: #82539 (comment). doc(include) is only relevant in that it should be replaced with this feature.

@pksunkara
Copy link
Contributor

My bad, I meant to paste the updated one as a probable workaround for people who run into the above bug when they don't need to check the feature existence

#![cfg_attr(feature = "doc", feature(extended_key_value_attributes))]
#![cfg_attr(feature = "doc", doc = include_str!("../README.md"))]

@jyn514
Copy link
Member Author

jyn514 commented Mar 22, 2021

@pksunkara that has the same issue, when the doc feature isn't passed it still gives an error that the feature is unstable: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d6f0b2a9d584e889d14fd1688827d510

@Nemo157
Copy link
Member

Nemo157 commented Mar 22, 2021

So, my understanding of the workaround is that you would write it like this:

#![cfg_attr(feature = "doc", feature(extended_key_value_attributes))]
#![cfg_attr(feature = "doc", cfg_attr(feature = "doc", doc = include_str!("../README.md")))]

This compiles all the way back to rustc 1.18.0 (03fc9d622 2017-06-06).

(edit: Given that this is related to the behaviour of existing compilers, I don't see that there is any change that could happen in new compilers to mitigate it, other than changing the syntax, which would sort of defeat the point of just reusing the existing syntax).

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 18, 2021
…=petrochenkov

Stabilize extended_key_value_attributes

Closes rust-lang#44732. Closes rust-lang#78835. Closes rust-lang#82768 (by making it irrelevant).

 # Stabilization report

 ## Summary

This stabilizes using macro expansion in key-value attributes, like so:

 ```rust
 #[doc = include_str!("my_doc.md")]
 struct S;

 #[path = concat!(env!("OUT_DIR"), "/generated.rs")]
 mod m;
 ```

See Petrochenkov's excellent blog post [on internals](https://internals.rust-lang.org/t/macro-expansion-points-in-attributes/11455)
for alternatives that were considered and rejected ("why accept no more and no less?")

This has been available on nightly since 1.50 with no major issues.

## Notes

### Accepted syntax

The parser accepts arbitrary Rust expressions in this position, but any expression other than a macro invocation will ultimately lead to an error because it is not expected by the built-in expression forms (e.g., `#[doc]`).  Note that decorators and the like may be able to observe other expression forms.

### Expansion ordering

Expansion of macro expressions in "inert" attributes occurs after decorators have executed, analogously to macro expressions appearing in the function body or other parts of decorator input.

There is currently no way for decorators to accept macros in key-value position if macro expansion must be performed before the decorator executes (if the macro can simply be copied into the output for later expansion, that can work).

## Test cases

 - https://github.com/rust-lang/rust/blob/master/src/test/ui/attributes/key-value-expansion-on-mac.rs
 - https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/external-doc.rs

The feature has also been dogfooded extensively in the compiler and
standard library:

- rust-lang#83329
- rust-lang#83230
- rust-lang#82641
- rust-lang#80534

## Implementation history

- Initial proposal: rust-lang#55414 (comment)
- Experiment to see how much code it would break: rust-lang#67121
- Preliminary work to restrict expansion that would conflict with this
feature: rust-lang#77271
- Initial implementation: rust-lang#78837
- Fix for an ICE: rust-lang#80563

## Unresolved Questions

~~rust-lang#83366 (comment) listed some concerns, but they have been resolved as of this final report.~~

 ## Additional Information

 There are two workarounds that have a similar effect for `#[doc]`
attributes on nightly. One is to emulate this behavior by using a limited version of this feature that was stabilized for historical reasons:

```rust
macro_rules! forward_inner_docs {
    ($e:expr => $i:item) => {
        #[doc = $e]
        $i
    };
}

forward_inner_docs!(include_str!("lib.rs") => struct S {});
```

This also works for other attributes (like `#[path = concat!(...)]`).
The other is to use `doc(include)`:

```rust
 #![feature(external_doc)]
 #[doc(include = "lib.rs")]
 struct S {}
```

The first works, but is non-trivial for people to discover, and
difficult to read and maintain. The second is a strange special-case for
a particular use of the macro. This generalizes it to work for any use
case, not just including files.

I plan to remove `doc(include)` when this is stabilized
(rust-lang#82539). The `forward_inner_docs`
workaround will still compile without warnings, but I expect it to be
used less once it's no longer necessary.
@bors bors closed this as completed in 3c99dcd May 19, 2021
najamelan added a commit to najamelan/async_executors that referenced this issue Jun 6, 2021
Yep, don't ask me, that seems to be the way to do it:
rust-lang/rust#82768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-stability Area: `#[stable]`, `#[unstable]` etc. F-extended_key_value_attributes `#![feature(extended_key_value_attributes)] T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants