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

expand: Leave traces when expanding cfg attributes #138844

Merged
merged 1 commit into from
Mar 27, 2025

Conversation

petrochenkov
Copy link
Contributor

This is the same as #138515, but for cfg(true) instead of cfg_attr.

The difference is that cfg(true)s already left "traces" after themselves - the cfg attributes themselves, with expanded_inert_attrs set to true, with full tokens, available to proc macros.
This is not a reasonably expected behavior, but it could not be removed without a replacement, because a major rustdoc feature and a number of clippy lints rely on it. This PR implements a replacement.

This needs a crater run, because it changes observable behavior (in an intended way) - proc macros can no longer see expanded cfg(true) attributes.

(Some minor unnecessary special casing for sym::cfg_attr is also removed in this PR.)

r? @nnethercote

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 22, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

@petrochenkov
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 22, 2025

⌛ Trying commit 750814d with merge 411f3ca...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2025
expand: Leave traces when expanding `cfg` attributes

This is the same as rust-lang#138515, but for `cfg(true)` instead of `cfg_attr`.

The difference is that `cfg(true)`s already left "traces" after themselves - the `cfg` attributes themselves, with `expanded_inert_attrs` set to true, with full tokens, available to proc macros.
This is not a reasonably expected behavior, but it could not be removed without a replacement, because a [major rustdoc feature](rust-lang/rfcs#3631) and a number of clippy lints rely on it. This PR implements a replacement.

This needs a crater run, because it changes observable behavior (in an intended way) - proc macros can no longer see expanded `cfg(true)` attributes.

(Some minor unnecessary special casing for `sym::cfg_attr` is also removed in this PR.)

r? `@nnethercote`
@bors
Copy link
Contributor

bors commented Mar 23, 2025

☀️ Try build successful - checks-actions
Build commit: 411f3ca (411f3ca0d8086d5a27e2aaa79b9ff41f53ab2f51)

@petrochenkov
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-138844 created and queued.
🤖 Automatically detected try build 411f3ca
🔍 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 craterbot 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 23, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-138844 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-138844 is completed!
📊 6 regressed and 7 fixed (602721 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ 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 Mar 24, 2025
Copy link
Member

Choose a reason for hiding this comment

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

Starting a thread on a random file: Just noticed those changes and had a question:

In Clippy we have this utils function:

/// Checks if the given span contains a `#[cfg(..)]` attribute
pub fn span_contains_cfg(cx: &LateContext<'_>, s: Span) -> bool {
s.check_source_text(cx, |src| {
let mut iter = tokenize_with_text(src);
// Search for the token sequence [`#`, `[`, `cfg`]
while iter.any(|(t, ..)| matches!(t, TokenKind::Pound)) {
let mut iter = iter.by_ref().skip_while(|(t, ..)| {
matches!(
t,
TokenKind::Whitespace | TokenKind::LineComment { .. } | TokenKind::BlockComment { .. }
)
});
if matches!(iter.next(), Some((TokenKind::OpenBracket, ..)))
&& matches!(iter.next(), Some((TokenKind::Ident, "cfg", _)))
{
return true;
}
}
false
})
}

This function is there to check after-the-fact if there was a cfg involved in the code that we're currently checking. For example when we're looking at a match:

match x {
    #[cfg(foo)]
    optionalArm => {},
    alwaysArm => {},
}

And then gating linting on it.

IIUC this can help us getting rid of this utils function? If so, I'd open a issue on Clippy for it. Definitely not for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cfg(true) already stays in code after expansion, and some other logic in clippy checks for it, yet this function still exists.

Maybe it takes a span of some larger node and expects to see #[cfg(false)]s in it as well? In that case this PR won't help, configured out nodes still disappear without a trace.

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2025
@petrochenkov
Copy link
Contributor Author

🎉 Experiment pr-138844 is completed! 📊 6 regressed and 7 fixed (602721 total) 📰 Open the full report.

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

Mostly spurious linker errors.
There are two more suspicious failures (maxcountryman.tower-sessions.81105af2b68dccaf1213d73726dd7bc9cf6ec624 and coolcatcoder.retrieval.5d088231d2467cc1c03fde3fcda8b694a2584a29), but they do not reproduce for me locally.

I think this is good to go now.
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2025
@nnethercote
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 27, 2025

📌 Commit 92d802e has been approved by nnethercote

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2025
Rollup of 7 pull requests

Successful merges:

 - rust-lang#138844 (expand: Leave traces when expanding `cfg` attributes)
 - rust-lang#138926 (Remove `kw::Empty` uses from `rustc_middle`.)
 - rust-lang#138989 (Clean up a few things in rustc_hir_analysis::check::region)
 - rust-lang#138999 (Report compiletest pass mode if forced)
 - rust-lang#139014 (Improve suggest construct with literal syntax instead of calling)
 - rust-lang#139015 (Remove unneeded LLVM CI test assertions)
 - rust-lang#139016 (Add job duration changes to post-merge analysis report)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3a8621d into rust-lang:master Mar 27, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 27, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2025
Rollup merge of rust-lang#138844 - petrochenkov:cfgtrace2, r=nnethercote

expand: Leave traces when expanding `cfg` attributes

This is the same as rust-lang#138515, but for `cfg(true)` instead of `cfg_attr`.

The difference is that `cfg(true)`s already left "traces" after themselves - the `cfg` attributes themselves, with `expanded_inert_attrs` set to true, with full tokens, available to proc macros.
This is not a reasonably expected behavior, but it could not be removed without a replacement, because a [major rustdoc feature](rust-lang/rfcs#3631) and a number of clippy lints rely on it. This PR implements a replacement.

This needs a crater run, because it changes observable behavior (in an intended way) - proc macros can no longer see expanded `cfg(true)` attributes.

(Some minor unnecessary special casing for `sym::cfg_attr` is also removed in this PR.)

r? `@nnethercote`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants