Regression w.r.t. lazy evaluation of cfg predicates

[danielhenrymantilla]

Code

I tried this code:

#[cfg(any(all(), __super_useful_macro_rules_hack = 42))]
fn main ()
{}

I expected to see this happen: code compiles fine, since the previous cfg resolves to true, short-circuiting the any until it reaches __super_useful_macro_rules_hack = 42

Instead, this happened: on a non-stable Rust compiler, these last attributes are nonetheless evaluated, and the compiler triggers an error for code that otherwise compiles fine.

Version it worked on

It most recently worked on: Rust 1.60.0

Version with regression

rustc --version --verbose:

1.61.0-beta.2
2022-04-09 7c13df853721b60a03e7

Backtrace

Backtrace

<backtrace>

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

---

I suspect this has to do with #94175 (comment) (cc @Urgau)

Why that code

This pattern is an incredibly useful trick for macro_rules! authors, at least until we get something like empty captures or other operators.

When using $( … )* and $( … )? Kleene operators, the metavar guiding that repetition count needs to be inside those parenthesis. But sometimes we have no use for the metavariable.

When the metavariable is of kind :literal, lazily unchecked cfgs are an incredibly convenient place to smuggle those:

https://github.com/getditto/safer_ffi/blob/277f3e7b9c382532a7b2bb5499d296a8d78b6d8e/src/ffi_export.rs#L137-L138

[kpreid]

This sounds like it's more precisely from #94295 (Always evaluate all cfg predicate in all() and any()) or #95109 (Extend --check-cfg tests to all predicate inside all/any) which intentionally check predicates eagerly.

[danielhenrymantilla]

Ah, thanks for the corrected pointer @kpreid.

So in #94295 it was deemed acceptable breakage because the instances witnessed by crater were genuine mistakes, making the error useful. That being said, a lint, even a deny-by-default one, would have been just as useful. And slapping an #[allow()] is something I could have done to fix the safer-ffi code in the meantime.

Now the js-supporting branch of https://docs.rs/safer-ffi won't compile starting from the future 1.61 release, and there won't be an easily applicable fix until ${ignore($literal)} gets stabilized (which would be a more suitable substitute for this pattern).

Would it be possible to backport a revert of #94295 to beta, and reapply it when #95860 gets merged?

[Urgau]

I'm sorry that you're code doesn't compile anymore; I can confirm that the culprit is #94295 and also that it is intentional.

I unfortunately don't know enough macro_rules and their hacks to be able to suggested any workaround, sorry.

So in #94295 it was deemed acceptable breakage because the instances witnessed by crater were genuine mistakes, making the error useful. That being said, a lint, even a deny-by-default one, would have been just as useful. And slapping an #[allow()] is something I could have done to fix the safer-ffi code in the meantime.

There was no need for a lint as the crater run only reported genuine mistakes. We could have definitely added a future-incompatibility lint but there was no need until now and I don't know if we should or not try to have a backport-able patch that transform the previously unevaluated part of the predicate. It would require some non-negligible changes and a far amount of reverts not only in rust-lang/rust but also probably in rust-lang/cargo.

Now the js-supporting branch of https://docs.rs/safer-ffi won't compile starting from the future 1.61 release, and there won't be an easily applicable fix until ${ignore($literal)} gets stabilized (which would be a more suitable substitute for this pattern).
Would it be possible to backport a revert of #94295 to beta, and reapply it when #95860 gets merged?

Reverting only #94295 wouldn't be enough there would also need to revert #95109 and maybe another one in rust-lang/rust.

[Mark-Simulacrum]

It was my impression that cfg_attr should work fine for the use case you describe - and is the intended "I want to shortcircuit". Is that not the case?

[danielhenrymantilla]

I don't think there is a simple replacement until ${ignore(…)} is stabilized, but given that I seem to be the only instance of this problem within the whole observable ecosystem, I'll make the effort to "tank the regression", and come up with a non-trivial workaround for safer-ffi, so as not to burden rust-lang/rust with extra work just for this