Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
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
Collaborator

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
Collaborator

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
@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.

@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

Blocked on #87220.

@petrochenkov petrochenkov added S-blocked Status: 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
Collaborator

bors commented May 5, 2021

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

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 21, 2021
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
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``
@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: Blocked on something else such as an RFC or other implementation work. labels Dec 10, 2021
@petrochenkov
Copy link
Contributor Author

Superseded by #92473.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2022
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`
@petrochenkov petrochenkov deleted the ltrattr branch February 22, 2025 19:20
# for free to join this conversation on GitHub. Already have an account? # to comment
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
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