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

Stabilize #[cfg_eval] and feature(macro_attributes_in_derive_output) #83824

Closed
wants to merge 1 commit into from
Closed

Stabilize #[cfg_eval] and feature(macro_attributes_in_derive_output) #83824

wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

TODO: Stabilization report.

Closes #82679
Closes #81119
r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2021
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2021
@jyn514 jyn514 added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Apr 3, 2021
@bors
Copy link
Contributor

bors commented Apr 5, 2021

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

pksunkara
pksunkara approved these changes Apr 5, 2021
View reviewed changes
Copy link
Contributor

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 👍

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Apr 11, 2021 •
edited
Loading

I'm going to delay this stabilization a bit and try two more things first:

@pksunkara
Copy link
Contributor

  • Double down on cfg and cfg_attr being expanded out of order, and eagerly expand them for all attributes implicitly, not only for derive and cfg_eval. Need to check backward compatibility and performance.

I am not completely sure about the code and internals, would love if you could help explain a bit of why this is a blocker for stabilising this feature.

  • Produce the original unconfigured item from #[derive(Trait1, Trait2)] while still feeding its configured version to Trait1 and Trait2. Need to check for performance.

Why do we need the original unconfigured item?

@pksunkara
Copy link
Contributor

Ah, for others who are thinking the same, here's related links: #83331, #83354

@petrochenkov
Copy link
Contributor Author

@pksunkara

would love if you could help explain a bit of why this is a blocker for stabilising this feature.

If input is eagerly fully configured for all attributes, then #[cfg_eval] becomes a no-op and no longer has reasons to exist.

Why do we need the original unconfigured item?

So you can place attributes requiring the original unconfigured input after #[derive].

@Aaron1011
Copy link
Member

Double down on cfg and cfg_attr being expanded out of order, and eagerly expand them for all attributes implicitly, not only for derive and cfg_eval. Need to check backward compatibility and performance.

One potential issue with doing this - a macro might want to do addtional processing of #[cfg] attributes. For example, someone could write an attribute #[auto_doc_cfg] which creates #[doc(cfg)] attributes for every #[cfg] attribute in the input (and copies them to any generated items).

@petrochenkov
Copy link
Contributor Author

Yeah, not expanding cfgs eagerly gives proc macro authors a choice, so we'll probably need some kind of opt-out.
I'm considering the variant with eagerly expanding cfgs for everything because in 90+% cases doing that is the preferable choice.

This was referenced Apr 23, 2021
Closed
Closed
@davidhewitt
Copy link
Contributor

As a pyO3 maintainer the evidence downstream (in pyO3 at least) is that having cfg-stripping be opt-out would be slightly more convenient for our users, who are often first-time Rustaceans coming from Python.

That said, once we've got a mechanism in the language (whether opt-in or opt-out) then it's quite plausible with pyO3's current design to conditionally compile python bindings for Rust crates just by slapping #[cfg_attr] on the right parts of the original Rust source.

We would write documentation for this regardless of whether it would need to explain #[cfg_eval] or not, so it's not a big problem either way.

My thanks to you folks for implementing this! 🚀

This was referenced May 5, 2021
Closed
Merged
@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 6, 2021 •
edited
Loading

Blocked on #85073.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2021
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels May 30, 2021
@petrochenkov petrochenkov mentioned this pull request Jun 6, 2021
Closed
@petrochenkov
Copy link
Contributor Author

Blocked on #86057.

@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 6, 2021
@petrochenkov petrochenkov added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 6, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2021
@bors
Auto merge of rust-lang#84995 - petrochenkov:tcollect, r=Aaron1011
86b0baf 
parser: Ensure that all nonterminals have tokens after parsing

`parse_nonterminal` should always result in something with tokens.

This requirement wasn't satisfied in two cases:
- `stmt` nonterminal with expression statements (e.g. `0`, or `{}`, or `path + 1`) because `fn parse_stmt_without_recovery` forgot to propagate `force_collect` in some cases.
- `expr` nonterminal with expressions with built-in attributes (e.g. `#[allow(warnings)] 0`) due to an incorrect optimization in `fn parse_expr_force_collect`, it assumed that all expressions starting with `#` have their tokens collected during parsing, but that's not true if all the attributes on that expression are built-in and inert.

(Discovered when trying to implement eager `cfg` expansion for all attributes rust-lang#83824 (comment).)

r? `@Aaron1011`
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jun 6, 2021
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2021
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 17, 2021
@petrochenkov
Copy link
Contributor Author

Blocked on #87220.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 17, 2021
@petrochenkov
Copy link
Contributor Author

Blocked on #87220.

Actually no, looks like #[cfg_eval] can be stabilized independently from #87220, I've made a PR - #87221.
So this PR can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pksunkara pksunkara pksunkara approved these changes

Assignees

@Aaron1011 Aaron1011

Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
8 participants
@petrochenkov @bors @pksunkara @Aaron1011 @davidhewitt @crlf0710 @rust-highfive @jyn514