rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Allow `&&`, `||`, and `!` in `cfg`

Open jhpratt opened this issue 8 months ago • 73 comments

About as simple as it gets.

Rendered

jhpratt avatar Mar 30 '25 22:03 jhpratt

I think that a feature != "string" syntax as an alternative to not(feature = "string") would be a nice addition here. !(feature = "string") feels weirder to me than not(feature = "string"), honestly.

clarfonthey avatar Mar 31 '25 19:03 clarfonthey

The problem I see with feature != "foo" is that it could plausibly be interpreted as "any feature other than foo".

jhpratt avatar Mar 31 '25 19:03 jhpratt

That interpretation feels like a stretch to me, but it's also understandable. Perhaps a feature("string") syntax might be a bit more natural with !feature("string"), and also extensible to feature("string", "other string").

clarfonthey avatar Mar 31 '25 20:03 clarfonthey

Updated the following:

  • !feature = "foo" is no longer permitted. Negation can be followed by further negation, parentheses, a single identifier (so #[cfg(!unix)]), and any function-like predicate.
  • &/| added as alternative to &&/||
  • feature != "foo" added as alternative to !(feature = "foo")
  • added table with examples
  • linked to operator precedence for expressions for clarity

jhpratt avatar Mar 31 '25 20:03 jhpratt

I think that a feature != "string" syntax as an alternative to not(feature = "string") would be a nice addition here. !(feature = "string") feels weirder to me than not(feature = "string"), honestly.

The problem here is that = in a cfg expression really means "contains", since feature (and also all other cfg variables) are really sets of strings, not just strings. So != would then mean "does not contain".

= is at least a different symbol from == so there's some hint that the semantics might be different, though it's not a very clear hint at all. != would exactly match expressions so there's not even a hint that the semantics are different.

RalfJung avatar Apr 01 '25 06:04 RalfJung

I read #[cfg(feature = "asdf")] similar to how I read things like #[serde(default = "asdf")] or #[link_name = "asdf"]: The = syntax is used as a key-value pair, like in all attributes.

As mentioned above, feature = "asdf" is not at all like a Rust expression. It doesn't behave like a = expression (it's not an assignment). It doesn't behave like an == expression (it doesn't check equality either). There is no variable/concept named feature that is compared to anything.

It isn't even commutative: #[cfg("asdf" = feature)] doesn't work.

Using key = "value" syntax makes sense as it matches the syntax for all other attributes. But I don't think it makes much sense to take && and || from Rust expressions and trying to force them into attribute syntax. The result feels weirdly inconsistent, with things like !(feature = "asdf") not being the same as feature != "asdf", and so on.

If we do want to make cfg() take something that looks like an expression, we should also change the feature = "asdf" syntax to something that fits a Rust expression. E.g. #[cfg(has_feature("asdf") && has_target_feature("asdf"))].

m-ou-se avatar Apr 01 '25 09:04 m-ou-se

Let's look at a real world example:

#[cfg(any(target_os = "android", target_os = "linux", target_os = "emscripten", target_os = "freebsd"))]

This proposal would allow rewriting that as:

#[cfg(target_os = "android" || target_os = "linux" || target_os = "emscripten" || target_os = "freebsd"))]

Which looks a bit nicer.

But instead of just replacing any() by a series of ||, I think we can do much better without giving up on the key = value syntax with something like:

#[cfg(target_os = "android" | "linux" | "emscripten" | "freebsd")]

I don't want to derail this thread with alternative proposals, but I do think that && and || is not getting us that much. If we go in that direction of making cfg() accept expression-like syntax, then we might be closing ourselves off from improvements to the key = value syntax that might serve the actual use cases better.

m-ou-se avatar Apr 01 '25 09:04 m-ou-se

The result feels weirdly inconsistent, with things like !(feature = "asdf") not being the same as feature != "asdf"

How would they not be the same? The RFC, I think, intents for both of these to mean the same thing: the feature variable does not contain "asdf".

RalfJung avatar Apr 01 '25 09:04 RalfJung

!(feature = "asdf") would work, while feature != "asdf" would be an error.

m-ou-se avatar Apr 01 '25 09:04 m-ou-se

Ah. That seems entirely consistent to me, it's = and not == after all.

I like target_os = "android" | "linux" | "emscripten" | "freebsd" but it doesn't when you need to look at more than one key.

RalfJung avatar Apr 01 '25 10:04 RalfJung

My point is that we need tot look at real world examples of code we're trying to simplify.

For example, quite often, any() is used with the same key, such as when selecting for a list of different operating systems. We should optimize the syntax for patterns that occur in actual code.

Large cfg() attributes can be very confusing. But replacing any() and all() by || and && isn't making it much better. Large boolean expressions will still be confusing, especially if their syntax is a hybrid of expressions and (key=value) attribute syntax with inconsistent precedence and operator meaning.

m-ou-se avatar Apr 01 '25 10:04 m-ou-se

Rust parses feature = "asdf" && unix as feature = ("asdf" && unix), not as (feature = "asdf") && unix.

One might argue that = in attributes is different, but how would we expect an attribute proc macro to parse this today?

#[my_attribute_macro(expr = a && b, expr2 = c || d)]

I would assume it interprets that as two key = value pairs with the values being a && b and c || d. (It's up to the proc macro to decide, this is what you get if you parse it with a Rust expression parser.)

So even in an attribute, I'd argue that && should have higher precedence than the key=value operator. But that's the opposite of what this RFC proposes.

m-ou-se avatar Apr 01 '25 10:04 m-ou-se

maybe embrace that they are sets and allow using "abcd" in feature instead of feature = "abcd"? though then that makes me think we should have named it features.

programmerjake avatar Apr 01 '25 14:04 programmerjake

Large cfg() attributes can be very confusing. But replacing any() and all() by || and && isn't making it much better.

I would go so far as to argue that any() and all() are better for formatting. Binary operators used repeatedly have an awkward indentation problem. Currently, rustfmt produces:

#[cfg(any(
    feature = "foo",
    feature = "bar",
    feature = "baz",
    feature = "quux",
    feature = "magic",
    test
))]

Whereas a similar boolean expression is less regular and longer:

something(cfg(
    feature == "foo"
        || feature == "bar"
        || feature == "baz"
        || feature == "quux"
        || feature == "magic"
        || test,
));

(And there are probably even messier cases to present with && and ! but I don't feel up to producing an illustrative example right now.) I won’t say that I haven’t wished for conventional binary operators in simple cases, but switching to them completely would have significant disadvantages (as would having a mix of both as standard style, simply due to having two things to learn and the choice of which one to use).

On the other hand, supporting feature = "foo" | "bar" has the inherent advantage of making the expression shorter and less repetitive, while still aligning with existing Rust syntax (patterns). It does raise the question of “why can’t I just write cfg(windows | unix)”, which, if also supported, would have a precedence problem — i.e. you have to either allow or prohibit cfg(feature = "foo" | bar), and if allowed, bar could be misinterpreted as a merely-unquoted value string.

kpreid avatar Apr 01 '25 14:04 kpreid

So even in an attribute, I'd argue that && should have higher precedence than the key=value operator. But that's the opposite of what this RFC proposes.

GitHub really needs a :cry: emoji.

To be compatible with this existing syntax, it means all key = "value" cfg has to be wrapped in parenthesis:

#[cfg((target_os="linux") && (target_arch="x86_64") && (target_env="gnu") && !debug_assertions)]

kennytm avatar Apr 01 '25 15:04 kennytm

I would go so far as to argue that any() and all() are better for formatting. Binary operators used repeatedly have an awkward indentation problem.

...

Whereas a similar boolean expression is less regular and longer:

...

(And there are probably even messier cases to present with && and ! but I don't feel up to producing an illustrative example right now.) I won’t say that I haven’t wished for conventional binary operators in simple cases, but switching to them completely would have significant disadvantages (as would having a mix of both as standard style, simply due to having two things to learn and the choice of which one to use).

Yes the formatting is strange, but I'd argue that the binary operator makes the cfg more readable for reviewers because you can be sure it is a chain of any rather than all at the first glance (spot the difference:

Existing syntax
#[cfg(any(
    feature = "foo",
    feature = "bar",
    feature = "baz",
    feature = "quux",
    feature = "magic",
    test
))]
#[cfg(all(
    feature = "foo",
    feature = "bar",
    feature = "baz",
    feature = "quux",
    feature = "magic",
    test
))]
This RFC
#[cfg(
    (feature = "foo")
        || (feature = "bar")
        || (feature = "baz")
        || (feature = "quux")
        || (feature = "magic")
        || test
)]
#[cfg(
    (feature = "foo")
        && (feature = "bar")
        && (feature = "baz")
        && (feature = "quux")
        && (feature = "magic")
        && test
)]
Alternative proposal
#[cfg(any(
    feature = "foo" 
        | "bar" 
        | "baz" 
        | "quux" 
        | "magic",
    test,
))]
#[cfg(all(
    feature = "foo" 
        & "bar" 
        & "baz" 
        & "quux" 
        & "magic",
    test,
))]

kennytm avatar Apr 01 '25 15:04 kennytm

cfg(any()) and cfg(all()) are very useful, often in macros, as always-false and always-true cfg values respectively. This RFC does not need to supplant all existing use cases of all and any, but it does seem weird to replace most things except this one edge case.

SabrinaJewson avatar Apr 01 '25 20:04 SabrinaJewson

cfg(any()) and cfg(all()) are very useful, often in macros, as always-false and always-true cfg values respectively.

https://github.com/rust-lang/rust/issues/131204

Jules-Bertholet avatar Apr 01 '25 20:04 Jules-Bertholet

@m-ou-se wrote:

If we do want to make cfg() take something that looks like an expression, we should also change the feature = "asdf" syntax to something that fits a Rust expression. E.g. #[cfg(has_feature("asdf") && has_target_feature("asdf"))].

Agreed completely. I don't think it would be ambiguous here to reuse "feature" without adding "has_": cfg(feature("asdf")). And this would also be consistent with other parts of cfg syntax.

joshtriplett avatar Apr 01 '25 21:04 joshtriplett

I don't think it would be ambiguous here to reuse "feature" without adding "has_": cfg(feature("asdf")). And this would also be consistent with other parts of cfg syntax.

Technically this change of syntax will conflict with the cfg_version feature but since it is unstable it is fine...? (plus can't use version="x" on stable at all, it somehow triggers cfg_version too)

#![feature(cfg_version)]
fn main() {
	dbg!(cfg!(version("1.69.0")));
	dbg!(cfg!(version("2.0.0")));
	dbg!(cfg!(version = "1.69.0"));
	dbg!(cfg!(version = "2.0.0"));
}
$ rustc +nightly --cfg 'version="2.0.0"' 1.rs
$ ./1
[1.rs:3:2] cfg!(version("1.69.0")) = true
[1.rs:4:2] cfg!(version("2.0.0")) = false
[1.rs:5:2] cfg!(version = "1.69.0") = false
[1.rs:6:2] cfg!(version = "2.0.0") = true

kennytm avatar Apr 02 '25 00:04 kennytm

I feel like many problems raised (by various people) here can be partially mitigated by changing feature = "something" into feature("something") and equivalently for the other key-values such as target_arch("x86_64")

I didn't come up with this, it felt like a slightly nicer way of writing @m-ou-se's has_feature which I like less, and now after having written the rest of this comment and reading back the thread I see that @clarfonthey proposed this quite literally.

Having said that, let this comment be a motivation for why I think this mitigates so many of the concerns. The advantages I see are:

  1. The confusion about the meaning of = (whether it means == or "assignment =" or a third kind of =) becomes irrelevant.
  2. Because we use parentheses, we automatically disambiguate any use of && and ||. In other words, #[my_attribute_macro(expr = a && b, expr2 = c || d)] is instead written as #[my_attribute_macro(expr(a) && b && expr2(c) || d)] Though of course, whether && has precedence over || is still here
  3. Whether to permit feature != "a" is irrelevant. it'd always be !feature("a")
  4. it's consistent with any , all and not which are already parenthesized.
  5. If we choose to permit alternation within an attribute (i.e. feature = "a" | "b" | "c" which with my proposal becomes feature("a" | "b" | "c")) this becomes clearly unambiguous from feature("a") | b | c

jdonszelmann avatar Apr 03 '25 11:04 jdonszelmann

I'll defer to the team here. I'm happy to write up whatever changes are desired, which at this point sounds like it may be feature("foo"), key(val1 | val2 | val3), etc. If this is the case, I have one question: should we permit mixing and matching or only support all-or-nothing?

jhpratt avatar Apr 03 '25 18:04 jhpratt

I like the proposal in https://github.com/rust-lang/rfcs/pull/3796#issuecomment-2775430532, but the feature = "abc" syntax still needs to be supported for backwords compatibility, so we need to define how that interacts with the new syntax. Do we

  1. Disallow the = syntax in combination with && or ||
  2. Require any use of = to be within parentheses, ex. (feature="abc") && (target_os="linux")
  3. As originally described in this RFC
  4. Something else

tmccombs avatar Apr 04 '25 15:04 tmccombs

I propose that we add key(val | val) syntax without adding &&, ||, !. That's where the bigger win is, and it would be less churn.

In a world of boolean logic, the existence of any/all/not functions is indubitably justified, and they deserve our respect.

camsteffen avatar Apr 05 '25 21:04 camsteffen

@camsteffen Is churn the primary (or even sole) concern? I think the :+1:s on this PR show that there's unquestionably the desire for logical operators — it feels obvious imo.

jhpratt avatar Apr 05 '25 23:04 jhpratt

I actually like @camsteffen's proposal a lot. It makes many scenarios simpler right now without changing ther original feature = name syntax. And the existing situation isn't so bad. And, if we feel like adding boolean operators later that still works!

jdonszelmann avatar Apr 06 '25 09:04 jdonszelmann

@jhpratt Not just churn, but the fact of having two ways to express something and a split in the ecosystem of which one is used. The motivation may be to make things simpler by matching other syntax, but I think the reality is that it's increasing complexity with another thing to learn. I would be more open to it if I could snap my fingers and change all the code in the world to the new syntax. But I also agree with @kpreid that any/all is better with regard to formatting.

camsteffen avatar Apr 07 '25 15:04 camsteffen

I would go so far as to argue that any() and all() are better for formatting. Binary operators used repeatedly have an awkward indentation problem…

Yes the formatting is strange, but I'd argue that the binary operator makes the cfg more readable for reviewers…

[…]

Alternative proposal

#[cfg(any(
    feature = "foo" 
        | "bar" 
        | "baz" 
        | "quux" 
        | "magic",
    test,
))]
#[cfg(all(
    feature = "foo" 
        & "bar" 
        & "baz" 
        & "quux" 
        & "magic",
    test,
))]

A little more indentation makes this look much better:

#[cfg(any(
    feature = "foo" 
            | "bar" 
            | "baz" 
            | "quux"
            | "magic",
    test,
))]
#[cfg(all(
    feature = "foo" 
            & "bar" 
            & "baz" 
            & "quux" 
            & "magic",
    test,
))]

Jules-Bertholet avatar Apr 07 '25 15:04 Jules-Bertholet

@Jules-Bertholet the default rustfmt will not use vertical alignment.


@camsteffen Rust isn't afraid of having multiple ways to express the same concept, we have for x in it {} vs while let Some(x) = t.next() {}; async fn f() -> R vs fn f() -> impl Future<Output=R>; fn g(a: impl Read) vs fn g<R: Read>(a: R) vs fn g<R>(a: R) where R: Read+, etc. Even your proposal it going to introduce #[cfg(feature = "name")] vs #[cfg(feature("name"))]. IMO "splitting the ecosystem" is FUD and is no ground to reject a proposal for Rust.

This proposed syntax #[cfg(key("val1" | "val2" | "val3"))] works well when everything involved has the same "key", but won't help when multiple "keys" are involved, e.g.:

Exhibit A

Exhibit B

Existing code

#[cfg(all(
    doc,
    any(
        all(target_arch = "wasm32", not(target_os = "wasi")),
        all(target_vendor = "fortanix", target_env = "sgx")
    )
))]
#[cfg(any(
    unix,
    windows,
    target_os = "psp",
    target_os = "solid_asp3",
    all(target_vendor = "fortanix", target_env = "sgx"),
))]
This RFC
#[cfg(doc && (
    (target_arch = "wasm32") && !(target_os = "wasi") 
        || (target_vendor = "fortanix") && (target_env = "sgx")
))]
#[cfg(unix 
    || windows 
    || (target_os = "psp") 
    || (target_os = "solid_asp3")
    || (target_vendor = "fortanix") && (target_env = "sgx")
)]
key("value") only
#[cfg(all(
    doc,
    any(
        all(target_arch("wasm32"), not(target_os("wasi"))),
        all(target_vendor("fortanix"), target_env("sgx"))
    )
))]
#[cfg(any(
    unix,
    windows,
    target_os("psp" | "solid_asp3"),
    all(target_vendor("fortanix"), target_env("sgx")),
))]
This RFC + key("value")
#[cfg(doc && (
    target_arch("wasm32") && !target_os("wasi") 
        || target_vendor("fortanix") && target_env("sgx")
))]
#[cfg(unix 
    || windows 
    || target_os("psp" | "solid_asp3")
    || target_vendor("fortanix") && target_env("sgx")
)]

kennytm avatar Apr 07 '25 17:04 kennytm

Oddly, my first reaction to those examples that I'd like to use "any" and "&&"... I can't really explain why. But I do feel like the use of infix operators makes it less clear that we are looking at a list of cases.

RalfJung avatar Apr 07 '25 17:04 RalfJung