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

Tracking issue for cfg_match #115585

Open
1 of 3 tasks
c410-f3r opened this issue Sep 6, 2023 · 8 comments
Open
1 of 3 tasks

Tracking issue for cfg_match #115585

c410-f3r opened this issue Sep 6, 2023 · 8 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@c410-f3r
Copy link
Contributor

c410-f3r commented Sep 6, 2023

Provides a native way to easily manage multiple conditional flags without having to rewrite each clause multiple times.

Public API

match_cfg! {
    cfg(unix) => {
        fn foo() { /* unix specific functionality */ }
    }
    cfg(target_pointer_width = "32") => {
        fn foo() { /* non-unix, 32-bit functionality */ }
    }
    _ => {
        fn foo() { /* fallback implementation */ }
    }
}

Steps / History

Unresolved Questions

  • What should the final syntax be? A match-like syntax feels more natural in the sense that each macro fragment resembles an arm.
  • Should the macro be supported directly by a language feature?
  • What should the feature name be? cfg_match conflicts with the already existing cfg_match crate.

References

@c410-f3r c410-f3r added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 6, 2023
@madsmtm
Copy link
Contributor

madsmtm commented Sep 12, 2023

I think it needs to be considered how this will interact with doc_auto_cfg?

Also, if this is intended to be match-like, then it seems weird to me that you can omit a fallback implementation? Unless of course all the possible combinations are statically known to be accounted for (which is hard, I know).

@CAD97
Copy link
Contributor

CAD97 commented Sep 25, 2023

Modulo (auto) doc(cfg), this implementation is more complex than it needs to be, since it's only supporting items.

simpler impl
#[macro_export]
macro_rules! cfg_match {
    {
        cfg($cfg:meta) => { $($cfg_items:item)* }
        $(cfg($rest_cfg:meta) => { $($rest_cfg_items:item)* })*
        $(_ => { $($fallthrough_items:item)* })?
    } => {
        #[cfg($cfg)]
        $crate::cfg_match! {
            _ => {
                $($cfg_items)*
                // validate the inactive cfgs as well
                #[cfg(any($($rest_cfg),*))]
                const _: () = ();
            }
        }
        #[cfg(not($cfg))]
        $crate::cfg_match! {
            $(cfg($rest_cfg) => { $($rest_cfg_items)* })*
            $(_ => { $($fallthrough_items)* })?
        }
    };
    {
        $(_ => { $($items:item)* })?
    } => {
        $($($items)*)?
    };
}

No internal matching arms necessary; just a normal inductive definition over the input syntax. The one tricky difference is that without the extra #[cfg] const item in the positive arm, later cfg predicates wouldn't be validated where they would be with the current implementation which always builds the full #[cfg(all(yes, not(any(...))))] matrix out.

This can be rather simply adapted to expression position by matching a block and using break with value.


It's unfortunate though that implemented without compiler support cfg_match! is effectively limited to either supporting item position or expression position. Supporting both needn't block supporting only item position (the more necessary position, since expression context can use if cfg!() else if chains most of the time), but it'd be nice to ensure there's a path forward for a compiler enhanced version which can expand differently between item and expression position, so the macro can be made to just work on both.

@madsmtm
Copy link
Contributor

madsmtm commented Sep 25, 2023

Something else to consider: This should be able to detect invalid cfgs like the following (ideally regardless of whether the cfg is currently enabled or not):

match_cfg! {
    cfg(unix) => {}
    cfg(unix) => {} // Ooops, specified same `cfg` twice
    _ => {}
}

@thomcc
Copy link
Member

thomcc commented Sep 25, 2023

I would prefer if it could support both items and expressions. It being part of the stdlib means that implementing it in the compiler would be completely reasonable, and would also make a lint that warns on redundant cfg arms pretty doable.

@jhpratt
Copy link
Member

jhpratt commented Sep 25, 2023

For detecting duplicates, it's important to note that this is more difficult than it seems at first. rustdoc has been dealing with this for doc(cfg).

@CAD97
Copy link
Contributor

CAD97 commented Sep 30, 2023

I tested the current (1.74.0-nightly (e6aabe8b3 2023-09-26)) behavior of doc_auto_cfg: items defined in cfg_match! do not receive a portability info box. Theoretically cfg dependencies could be tracked through macro expansion, but that's not done currently; cfg dependencies are only recorded on items.

Fixing this is actually fairly simple once you understand why this is: instead of expanding to #[cfg($cfg)] identity! { $($items)* }, splat the cfg directly onto each item with $(#[cfg($cfg)] $items)*. The match_cfg crate does this; the cfg_if crate doesn't. A quick scan through the library directory didn't reveal any places where splatting cfg through cfg_if! would change documentation.

I want to additionally point out that doc_auto_cfg fundamentally can only see what items are defined on the current cfg and mark them as only available on whatever cfg this specific implementation is gated to. If you want to expose a portable API, either define a single portable public version of the API (and use private cfg gates to implement it) or use doc(cfg()) to explicitly tell rustdoc what gate it should display as.

So what I personally think the macro definition wants to be:

in item position
#[macro_export]
macro_rules! cfg_match {
    // base case without wildcard arm
    () => {};

    // base case with wildcard arm (items)
    (_ => { $($wild_item:item)* }) => {
        $($wild_item)*
    };

    // process first arm (items; propagate cfg)
    (
        cfg($this_cfg:meta) => { $($this_item:item)* }
        $(cfg($rest_cfg:meta) => { $($rest_item:item)* })*
        $(_ => { $($wild_item:item)* })?
    ) => {
        $(#[cfg($this_cfg)] $this_item)*
        $crate::cfg_match! {
            $(cfg($rest_cfg) => { $(#[cfg(not($this_cfg))] $rest_item)* })*
            $(_ => { $(#[cfg(not($this_cfg))] $wild_item)* })?
        }
    };
}
in expression position
#[macro_export]
macro_rules! cfg_match {
    // base case without wildcard arm
    () => {};

    // base case with wildcard arm (expression block)
    (_ => $wild:block) => {
        $wild
    };

    // process first arm (expression block)
    (
        cfg($this_cfg:meta) => $this:block
        $(cfg($rest_cfg:meta) => $rest:block)*
        $(_ => $wild:block)?
    ) => {
        {
            #[cfg($this_cfg)]
            $crate::cfg_match! {
                _ => $this
            }
            #[cfg(not($this_cfg))]
            $crate::cfg_match! {
                $(cfg($rest_cfg) => $rest)*
                $(_ => $wild)?
            }
        }
    };
}
in statement position
#[macro_export]
macro_rules! cfg_match {
    // base case without wildcard arm
    () => {};

    // base case with wildcard arm
    (_ => { $($wild:tt)* }) => {
        $($wild)*
    };

    // process first arm
    (
        cfg($this_cfg:meta) => { $($this:tt)* }
        $(cfg($rest_cfg:meta) => { $($rest:tt)* })*
        $(_ => { $($wild:tt)* })?
    ) => {
        #[cfg($this_cfg)]
        $crate::cfg_match! {
            _ => { $($this)* }
        }
        #[cfg(not($this_cfg))]
        $crate::cfg_match! {
            $(cfg($rest_cfg) => { $($rest)* })*
            $(_ => { $($wild)* })?
        }
    };
}

This technically is the most flexible spelling just passing through tt sequences; it works perfectly for items and statements (modulo preserving doc_auto_cfg) and can even be used for expressions if wrapped inside an outer block (e.g. { cfg_match! { ... } }).


There's also the further question of how much syntax checking is expected for inactive arms. The current implementation that matches everything as $($:item)* does full syntax validation, but a naive implementation forwarding tokens context-antagonistically as $($:tt)* can easily cfg out tokens without ever placing them in a syntax-validated position -- in fact the above statement macro form does this.

A far too overly clever implementation:
#[macro_export]
macro_rules! cfg_match {
    // base case (wildcard arm)
    () => {};

    // base case (no wildcard arm)
    (_ => { $($wild:tt)* }) => {
        $($wild)*
    };

    // Note: macro patterns matching $:stmt need to come before those matching
    // $:item, as otherwise statements will match as the $:item pattern instead
    // even when not valid as items (and obviously thus cause a syntax error).

    // apply cfg to wildcard arm (expression position)
    (_ => #[cfg($cfg:meta)] { $wild:expr }) => {
        { #[cfg($cfg)] { $wild } }
    };

    // apply cfg to wildcard arm (statement position)
    (_ => #[cfg($cfg:meta)] { $wild_stmt:stmt; $($wild:tt)* }) => {
        #[cfg($cfg)]
        $crate::cfg_match! {
            _ => { $wild_stmt; $($wild)* }
        }

        // We only parse the first statement in the macro pattern, so we need to
        // emit the captured syntax even when the cfg is inactive such that any
        // syntax errors still get reported. We do the macro capture this way as
        // if we match as statements, minor typos try to parse as items instead.
        #[cfg(any())]
        const _: () = { wild_stmt; $($wild)* };
    };

    // apply cfg to wildcard arm (item position)
    (_ => #[cfg($cfg:meta)] { $($wild:item)* }) => {
        $(#[cfg($cfg)] $wild)*
    };

    // merge multiple cfg into a single cfg(all()) predicate
    (_ => $(#[cfg($cfg:meta)])+ { $($wild:tt)* }) => {
        $crate::cfg_match! {
            _ => #[cfg(all($($cfg),+))] { $($wild)* }
        }
    };

    // split off first arm (empty)
    (
        cfg($this_cfg_arm:meta) => $(#[cfg($this_cfg_pre:meta)])* { /* empty */ }
        $(cfg($rest_cfg_arm:meta) => $(#[cfg($rest_cfg_pre:meta)])* { $($rest:tt)* })*
        $(_ => $(#[cfg($wild_cfg_pre:meta)])* { $($wild:tt)* })?
    ) => {
        $crate::cfg_match! {
            $(cfg($rest_cfg_arm) => $(#[cfg($rest_cfg_pre)])* #[cfg(not($this_cfg_arm))] { $($rest)* })*
            $(_ => $(#[cfg($wild_cfg_pre)])* #[cfg(not($this_cfg_arm))] { $($wild)* })?
        }
    };

    // split off first arm (expression position)
    (
        cfg($this_cfg_arm:meta) => $(#[cfg($this_cfg_pre:meta)])* { $this:expr }
        $(cfg($rest_cfg_arm:meta) => $(#[cfg($rest_cfg_pre:meta)])* { $rest:expr })*
        $(_ => $(#[cfg($wild_cfg_pre:meta)])* { $wild:expr })?
    ) => {{
        $crate::cfg_match! {
            _ => $(#[cfg($this_cfg_pre)])* #[cfg($this_cfg_arm)] { $this }
        }
        $crate::cfg_match! {
            $(cfg($rest_cfg_arm) => $(#[cfg($rest_cfg_pre)])* #[cfg(not($this_cfg_arm))] { $rest })*
            $(_ => $(#[cfg($wild_cfg_pre)])* #[cfg(not($this_cfg_arm))] { $wild })?
        }
    }};

    // split off first arm (statement position)
    (
        cfg($this_cfg_arm:meta) => $(#[cfg($this_cfg_pre:meta)])* { $this_stmt:stmt; $($this:tt)* }
        $(cfg($rest_cfg_arm:meta) => $(#[cfg($rest_cfg_pre:meta)])* { $($rest:tt)* })*
        $(_ => $(#[cfg($wild_cfg_pre:meta)])* { $($wild:tt)* })?
    ) => {
        $crate::cfg_match! {
            _ => $(#[cfg($this_cfg_pre)])* #[cfg($this_cfg_arm)] { $this_stmt; $($this)* }
        }
        // Ensure all arms infer statement position by prefixing a statement. We
        // only match the first arm in the macro pattern because otherwise minor
        // typos unhelpfully cause all arms to parse as item definitions instead
        // and thus reporting an error nowhere near the actual problem, or even
        // more amusingly, accidentally providing automatic semicolon insertion.
        $crate::cfg_match! {
            $(cfg($rest_cfg_arm) => $(#[cfg($rest_cfg_pre)])* #[cfg(not($this_cfg_arm))] { {}; $($rest)* })*
            $(_ => $(#[cfg($wild_cfg_pre)])* #[cfg(not($this_cfg_arm))] { {}; $($wild)* })?
        }
    };

    // split off first arm (item position)
    (
        cfg($this_cfg_arm:meta) => $(#[cfg($this_cfg_pre:meta)])* { $($this:item)* }
        $(cfg($rest_cfg_arm:meta) => $(#[cfg($rest_cfg_pre:meta)])* { $($rest:item)* })*
        $(_ => $(#[cfg($wild_cfg_pre:meta)])* { $($wild:item)* })?
    ) => {
        $crate::cfg_match! {
            _ => $(#[cfg($this_cfg_pre)])* #[cfg($this_cfg_arm)] { $($this)* }
        }
        // Items just match as item in the macro pattern. The error messages
        // would be improved if we could pass through tokens without eagerly
        // matching them as items, but we need to parse the items in the macro
        // so that we can apply the cfg attributes to every item in the arm.
        $crate::cfg_match! {
            $(cfg($rest_cfg_arm) => $(#[cfg($rest_cfg_pre)])* #[cfg(not($this_cfg_arm))] { $($rest)* })*
            $(_ => $(#[cfg($wild_cfg_pre)])* #[cfg(not($this_cfg_arm))] { $($wild)* })?
        }
    };
}

This does almost always behave as desired by using the arm bodies to try to guess how to expand, but is unreasonably complicated by attempting to do so and as a result causes some very poor errors for reasonable syntax errors.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 30, 2023

Well, since #115416 (comment) I have been working in transposing cfg_match to a builtin in order to easily allow single elements in an arm without brackets. Such thing will also permit any other feature that is being discussed here AFAICT.

It is worth noting that the current implementation mimics cfg_if and allows the expansion of token trees because cfgs are applied to macro calls as @CAD97 previously commented.

cfg_match! {
    cfg(unix) => { fn foo() -> i32 { 1 } },
    _ => { fn foo() -> i32 { 2 } },
}

# Expands to ->

#[cfg(all(unix, not(any())))]
cfg_match! { @__identity fn foo() -> i32 { 1 } }

#[cfg(all(not(any(unix))))]
cfg_match! { @__identity fn foo() -> i32 { 2 } }

That said, changing the strategy to apply cfg on each element inside a block will trigger stmt_expr_attributes (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=4cf4facc19b052053cca655b2b42f782) for expressions and conceivably/possibly/maybe/perhaps, a large set of elements along a large set of cfgs could affect compilation times. But, it seems to be the correct approach due to doc_auto_cfg.

@petrochenkov
Copy link
Contributor

FWIW, #[cfg(TRUE)] not being removed from items during expansion is a bug, and reliance of rustdoc on that bug is very unfortunate.

What we need to do is to keep expansion backtraces for cfgs like we do for all other macros.
In that case it won't be important whether a cfg is placed directly on an item, or somewhere higher in the expansion tree (like on a macro producing that item), it will be available from the macro backtrace anyway.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 13, 2023
Initiate the inner usage of `cfg_match` (Compiler)

cc rust-lang#115585

Dogfood to test the implementation and remove dependencies.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 14, 2023
Initiate the inner usage of `cfg_match` (Compiler)

cc rust-lang#115585

Dogfood to test the implementation and remove dependencies.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 21, 2023
Initiate the inner usage of `cfg_match` (Compiler)

cc rust-lang#115585

Dogfood to test the implementation and remove dependencies.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 21, 2023
Rollup merge of rust-lang#116312 - c410-f3r:try, r=Mark-Simulacrum

Initiate the inner usage of `cfg_match` (Compiler)

cc rust-lang#115585

Dogfood to test the implementation and remove dependencies.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 22, 2023
Initiate the inner usage of `cfg_match` (Library)

cc rust-lang#115585

Continuation of rust-lang#116312
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 28, 2023
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 29, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 29, 2023
Rollup merge of rust-lang#117162 - c410-f3r:try, r=workingjubilee

Remove `cfg_match` from the prelude

Fixes rust-lang#117057

cc rust-lang#115585
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 18, 2024
Initiate the inner usage of `cfg_match` (Library)

cc rust-lang#115585

Continuation of rust-lang#116312
CKingX added a commit to CKingX/rust that referenced this issue Feb 18, 2024
Initiate the inner usage of `cfg_match` (Library)

cc rust-lang#115585

Continuation of rust-lang#116312
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 19, 2024
Initiate the inner usage of `cfg_match` (Library)

cc rust-lang#115585

Continuation of rust-lang#116312
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants