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

[WIP] Expand all attributes in left-to-right order #83354

Closed
wants to merge 1 commit into from
Closed

[WIP] Expand all attributes in left-to-right order #83354

wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

Fixes #83331.
r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2021
@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 21, 2021

⌛ Trying commit 6a42bf1e0b4588b67e16e436325a62eecd8e5f78 with merge 99a443d6903f71d521237d8acb5a3016b534d17b...

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2021
@bors
Copy link
Contributor

bors commented Mar 21, 2021

☀️ Try build successful - checks-actions
Build commit: 99a443d6903f71d521237d8acb5a3016b534d17b (99a443d6903f71d521237d8acb5a3016b534d17b)

@petrochenkov
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-83354 created and queued.
🤖 Automatically detected try build 99a443d6903f71d521237d8acb5a3016b534d17b
⚠️ Try build based on commit f826641, but latest commit is 6a42bf1e0b4588b67e16e436325a62eecd8e5f78. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-83354 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Mark-Simulacrum
Copy link
Member

@craterbot retry-report

@craterbot
Copy link
Collaborator

🛠️ Generation of the report for pr-83354 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-83354 is completed!
📊 73 regressed and 16 fixed (152359 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 6, 2021
@Aaron1011 Aaron1011 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2021
@pksunkara pksunkara mentioned this pull request Apr 11, 2021
Closed
@petrochenkov
Copy link
Contributor Author

There's a variety of errors (some of them are strange and I need to check why they happen in more detail), but my impression so far is that we won't be able to do this.
There are too many uses like

#[generate_something]
#[cfg(FALSE)]
ITEM

where "something" is unintentionally generated despite the cfg(FALSE), and is producing errors.

We should be able to report warnings for out-of-order expansions though (similarly to #79202).

I haven't seen regressions from #[cfg(TRUE)]s being removed after expansion (they were previously left in place), but, again, I need to look in more detail. Perhaps we'll be able to land at least this part.

@petrochenkov petrochenkov mentioned this pull request Apr 15, 2021
Closed
@JohnCSimon JohnCSimon 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 Apr 26, 2021
@petrochenkov
Copy link
Contributor Author

Analysis of regressions.

Group 1: Proc macro attribute is written before #[cfg(FALSE)] so it either doesn't resolve due to missing imports or items introduced by the same cfg, or generates some code referring to the ITEM assuming it exists.
Resolved by simple attribute reordering.

#[wasm_bindgen] #[cfg(FALSE)] ITEM ++
#[wasm_bindgen_test] #[cfg(FALSE)] ITEM +
#[bench] #[cfg(FALSE)] ITEM +++++++++++
#[global_allocator] #[cfg(FALSE)] ITEM +++
#[ctor] #[cfg(FALSE)] ITEM +
#[tokio::test] #[cfg(feature = "tokio_support")] ITEM +
#[tokio::test] #[cfg(feature = "test")] ITEM +
#[derive(FromPrimitive)] #[cfg(feature = "serde-serialize")] ITEM +
#[derive(MessageEventContent)] #[cfg(feature = "unstable-pre-spec")] ITEM +
#[pin_project] #[cfg(feature = "discover")] ITEM +

Group 2: #[cfg_attr] disables the legacy derive helper resolution heuristic (#79202) so #[serde] no longer resolves.
Resolved by simple attribute reordering.

#[serde] #[cfg_attr(TRUE, derive(Serialize))] ITEM +

Group 3: #[test_case] (https://crates.io/crates/test-case) processes other #[test_case] attributes in its input specially, and #[cfg_attr]s hide them from it.
Cannot be resolved by simple attribute reordering, needs #[cfg_eval].

#[test_case(args)] #[cfg_attr(TRUE, test_case(args))] ++

Group 4: Some lints use item.attrs.is_empty() as a heuristic, so they start working differently if #[cfg(TRUE)] is removed (#84110).

#[cfg(TRUE)] { EXPR } // unnecessary braces +
#[cfg(TRUE)] extern crate foo; // `extern crate` is not idiomatic ++

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 5, 2021 •
edited
Loading

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 May 5, 2021
@bors
Copy link
Contributor

bors commented May 5, 2021

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

@petrochenkov petrochenkov mentioned this pull request Jun 20, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 21, 2021
@JohnTitor
Rollup merge of rust-lang#86491 - petrochenkov:derefact, r=Aaron1011
d449ce1 
expand: Move some more derive logic to rustc_builtin_macros

And cleanup some `unwrap`s in `cfg_eval`.

Refactorings extracted from rust-lang#83354 and rust-lang#86057.
r? `@Aaron1011`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 21, 2021
@JohnTitor
Rollup merge of rust-lang#86491 - petrochenkov:derefact, r=Aaron1011
8b9e138 
expand: Move some more derive logic to rustc_builtin_macros

And cleanup some `unwrap`s in `cfg_eval`.

Refactorings extracted from rust-lang#83354 and rust-lang#86057.
r? ``@Aaron1011``
@petrochenkov petrochenkov mentioned this pull request Sep 16, 2021
Closed
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 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 Dec 10, 2021
@petrochenkov petrochenkov mentioned this pull request Jan 1, 2022
Merged
@petrochenkov
Copy link
Contributor Author

Superseded by #92473.

@petrochenkov petrochenkov mentioned this pull request Jan 1, 2022
Open
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2022
@bors
Auto merge of rust-lang#92473 - petrochenkov:ltrattr2, r=Aaron1011
fd20513 
expand: Pick `cfg`s and `cfg_attrs` one by one, like other attributes

This is a rebase of rust-lang#83354, but without any language-changing parts ~(except for rust-lang#84110, i.e. the attribute expansion order is the same.

This is a pre-requisite for any other changes making cfg attributes closer to regular macro attributes
- Possibly changing their expansion order (rust-lang#83331)
- Keeping macro backtraces for cfg attributes, or otherwise making them visible after expansion without keeping them in place literally (rust-lang#84110).

Two exceptions to the "one by one" behavior are:
- cfgs eagerly expanded by `derive` and `cfg_eval`, they are still expanded in a batch, that's by design.
- cfgs at the crate root, they are currently expanded not during the main expansion pass, but before that, during `#![feature]` collection. I'll try to disentangle that logic later in a separate PR.

r? `@Aaron1011`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Aaron1011 Aaron1011

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expand #[cfg] and #[cfg_attr] left-to-right for consistency with other attributes
8 participants
@petrochenkov @bors @craterbot @Mark-Simulacrum @Aaron1011 @JohnCSimon @apiraino @rust-highfive