Always evaluate all cfg predicate in all() and any()

[Urgau]

This pull-request adjust the handling of the all() and any() to always evaluate every cfg predicate because not doing so result in accepting incorrect cfg:

#[cfg(any(unix, foo::bar))] // Should error on foo::bar, but does not on unix platform (but does on non unix platform)
fn foo1() {}

#[cfg(all(foo, foo::bar))] // Should error on foo::bar, but does not
fn foo2() {}

#[cfg(all(foo::bar, foo))] // Correctly error on foo::bar
fn foo3() {}

#[cfg(any(foo::bar, foo))] // Correctly error on foo::bar
fn foo4() {}

This pull-request take the side to directly turn it into a hard error instead of having a future incompatibility lint because the combination to get this incorrect behavior is unusual and highly probable that some code have this without noticing.

A search on Github reveal no such instance nevertheless a Crater run should probably be done before merging this.

This was discover in #94175 when trying to lint on the second predicate. Also note that this seems to have being introduce with Rust 1.27.0: https://rust.godbolt.org/z/KnfqKv15f.

r? @petrochenkov

[bjorn3]

You can currently do something like #[cfg(all(nightly, some_feature_gated_cfg = "..."))], right? After this PR it would become a hard error I think.

[Urgau]

You can currently do something like #[cfg(all(nightly, some_feature_gated_cfg = "..."))], right? After this PR it would become a hard error I think.

Noooo. It indeed seems that it would become an hard error playground. 😿
Is that an official feature ? or a accidental one that we could deprecate and lint on it ?

[bjorn3]

Not sure. I will label this as needs-fcp for the lang team to decide it.

[petrochenkov]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit f57cc8c with merge 506dff8de9f54f528028a7e4497ea925a4f3e19e...

[bors]

☀️ Try build successful - checks-actions
Build commit: 506dff8de9f54f528028a7e4497ea925a4f3e19e (506dff8de9f54f528028a7e4497ea925a4f3e19e)

[rust-timer]

Queued 506dff8de9f54f528028a7e4497ea925a4f3e19e with parent 1204400, future comparison URL.

[petrochenkov]

@craterbot check

[craterbot]

👌 Experiment pr-94295 created and queued.
🤖 Automatically detected try build 506dff8de9f54f528028a7e4497ea925a4f3e19e
🔍 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]

🚧 Experiment pr-94295 is now running

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

[rust-timer]

Finished benchmarking commit (506dff8de9f54f528028a7e4497ea925a4f3e19e): comparison url.

Summary: This benchmark run shows 4 relevant regressions 😿 to instruction counts.

• Average relevant regression: 0.8%
• Largest regression in instruction counts: 0.8% on incr-unchanged builds of externs opt

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[joshtriplett]

Nothing in our documentation explicitly describes all and any as short-circuiting. We do make it clear that cfg_attr is short-circuiting, and putting one cfg inside another would have a similar effect.

There aren't that many nightly-only cfg directives. If crater doesn't turn up anything, we could potentially change this. Which still leaves the question of whether we should.

As one alternative, as far as I can tell, I think we could still detect incorrect cfg directives even if we have to handle the case of nightly-only directives after a gating feature. We could check every argument to all or any for validity, and just only error on feature-gated cfg directives if "reached". (That's assuming a directive is understood but gated, rather than not understood at all, which of course doesn't cover all circumstances.)

[craterbot]

🎉 Experiment pr-94295 is completed!
📊 7 regressed and 3 fixed (214861 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

[joshtriplett]

We discussed this at length in today's @rust-lang/lang meeting.

We're in favor of making this change. We want to see warnings about issues on subsequent arguments of all/any even if not necessary to determine the value. And we're fine with requiring all arguments to use syntax understood by rustc.

We agreed that the issues identified by crater are all bugs, and it's better that we catch those.

This isn't necessarily a long-term commitment that we always have to do full-evaluation, if we can detect and warn about issues in the subsequent arguments without actually doing full evaluation. If we changed back to short-circuiting, that could only make more programs work, not less.

So: @rust-lang/lang, do we have a consensus to evaluate all arguments to all and any?

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[chrysn]

AIU this would also bite on the #[cfg(all(target_arch = "x86", not(panic = "abort")))] I'm using in the riot-wrappers crate (which, due to its build environment requirements, is not covered by a crater run): That crate supports stable on most archs not on x86, so stable builds of non-x86 architectures would start failing with this change.

But it's not a practical issue due to panic=... being on stabilized already, and were that not the case I'd work around it by using two #[cfg()] before the eh_personality item which this is guarding.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[petrochenkov]

@bors r+ rollup=maybe

[bors]

📌 Commit f57cc8c has been approved by petrochenkov