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

Perform cfg attribute processing during macro expansion and fix bugs #33706

Merged
merged 13 commits into from
May 28, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented May 18, 2016

This PR refactors cfg attribute processing and fixes bugs. More specifically:

  • It merges gated feature checking for stmt/expr attributes, cfg_attr processing, and cfg processing into a single fold.
    • This allows feature gated cfg variables to be used in cfg_attr on unconfigured items. All other feature gated attributes can already be used on unconfigured items.
  • It performs cfg attribute processing during macro expansion instead of after expansion so that macro-expanded items are configured the same as ordinary items. In particular, to match their non-expanded counterparts,
    • macro-expanded unconfigured macro invocations are no longer expanded,
    • macro-expanded unconfigured macro definitions are no longer usable, and
    • feature gated cfg variables on macro-expanded macro definitions/invocations are now errors.

This is a [breaking-change]. For example, the following would break:

macro_rules! m {
    () => {
        #[cfg(attr)]
        macro_rules! foo { () => {} }
        foo!(); // This will be an error

        macro_rules! bar { () => { fn f() {} } }
        #[cfg(attr)] bar!(); // This will no longer be expanded ...
        fn g() { f(); } // ... so that `f` will be unresolved.

        #[cfg(target_thread_local)] // This will be a gated feature error
        macro_rules! baz { () => {} }
    }
}

m!();

EDIT: fixes #22250, fixes #31369.

r? @nrc

@jseyfried jseyfried force-pushed the refactor_cfg branch 2 times, most recently from 61e9e91 to aa36f63 Compare May 18, 2016 10:22
@jseyfried
Copy link
Contributor Author

cc @nikomatsakis

@bors
Copy link
Contributor

bors commented May 19, 2016

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

@jseyfried
Copy link
Contributor Author

Rebased.

@@ -19,13 +19,31 @@ use ptr::P;

use util::small_vector::SmallVector;

pub trait CfgFolder: fold::Folder {
fn in_cfg(&mut self, attrs: &[ast::Attribute]) -> bool;
fn visit_unconfigurable_expr(&mut self, _expr: &ast::Expr) {}
Copy link
Member

Choose a reason for hiding this comment

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

Could you document these methods please?

@nrc
Copy link
Member

nrc commented May 23, 2016

r+ with the comment added

@jseyfried
Copy link
Contributor Author

@nrc I made sure we process cfg_attr attributes on non-optional expressions and added comments.

@jseyfried
Copy link
Contributor Author

@nrc also, some process questions:

  • Should this PR be tagged relnotes? (c.f. #33458)
  • Do you intend "r+ with the comment added" to have the same meaning as "r=me with the comment added"? Currently, I don't approve on behalf of a reviewer unless they say "r=me".

@bors
Copy link
Contributor

bors commented May 26, 2016

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

@jseyfried jseyfried force-pushed the refactor_cfg branch 4 times, most recently from 9d84921 to 3636ce7 Compare May 27, 2016 00:11
@jseyfried
Copy link
Contributor Author

Rebased.

@nrc
Copy link
Member

nrc commented May 27, 2016

@bors: r+

@jseyfried yes and yes

@bors
Copy link
Contributor

bors commented May 27, 2016

📌 Commit 9d84921 has been approved by nrc

@bors
Copy link
Contributor

bors commented May 27, 2016

⌛ Testing commit 9d84921 with merge 4d92f83...

@bors
Copy link
Contributor

bors commented May 27, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@jseyfried
Copy link
Contributor Author

@bors retry

@bors
Copy link
Contributor

bors commented May 27, 2016

⌛ Testing commit 9d84921 with merge 8be1128...

@bors
Copy link
Contributor

bors commented May 27, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented May 27, 2016

📌 Commit 53ab137 has been approved by nrc

@bors
Copy link
Contributor

bors commented May 28, 2016

⌛ Testing commit 53ab137 with merge 8b012ed...

bors added a commit that referenced this pull request May 28, 2016
Perform `cfg` attribute processing during macro expansion and fix bugs

This PR refactors `cfg` attribute processing and fixes bugs. More specifically:
 - It merges gated feature checking for stmt/expr attributes, `cfg_attr` processing, and `cfg` processing into a single fold.
  - This allows feature gated `cfg` variables to be used in `cfg_attr` on unconfigured items. All other feature gated attributes can already be used on unconfigured items.
 - It performs `cfg` attribute processing during macro expansion instead of after expansion so that macro-expanded items are configured the same as ordinary items. In particular, to match their non-expanded counterparts,
  - macro-expanded unconfigured macro invocations are no longer expanded,
  - macro-expanded unconfigured macro definitions are no longer usable, and
  - feature gated `cfg` variables on macro-expanded macro definitions/invocations are now errors.

This is a [breaking-change]. For example, the following would break:
```rust
macro_rules! m {
    () => {
        #[cfg(attr)]
        macro_rules! foo { () => {} }
        foo!(); // This will be an error

        macro_rules! bar { () => { fn f() {} } }
        #[cfg(attr)] bar!(); // This will no longer be expanded ...
        fn g() { f(); } // ... so that `f` will be unresolved.

        #[cfg(target_thread_local)] // This will be a gated feature error
        macro_rules! baz { () => {} }
    }
}

m!();
```

r? @nrc
@bors bors merged commit 53ab137 into rust-lang:master May 28, 2016
@jseyfried
Copy link
Contributor Author

Fixes #22250.

@jseyfried
Copy link
Contributor Author

Fixes #31369.

bors added a commit that referenced this pull request Jun 4, 2016
Fix a regression in the configuration folder

This fixes #34028, a regression caused by #33706 in which unconfigured impl items generated by a macro in an impl item position are not removed.
r? @nrc
@jseyfried jseyfried deleted the refactor_cfg branch February 15, 2017 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cfg attributes appear are improperly applied inside of include! Process cfg_attr during expansion
3 participants