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

Add the cfg_match! macro #115416

Merged
merged 1 commit into from
Sep 24, 2023
Merged

Add the cfg_match! macro #115416

merged 1 commit into from
Sep 24, 2023

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Aug 31, 2023

Movitation

Adds a match-like version of the cfg_if crate without a RFC for the same reasons that caused matches! to be included in the standard library.

  • General-purpose (not domain-specific)
  • Simple (the implementation is short) and useful (things can become difficult with several cfgs)
  • Very popular on crates.io (currently 3th in all-time downloads)
  • The two previous points combined make it number three in left-pad index score
#![feature(cfg_match)]

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

Considerations

A match-like syntax feels more natural in the sense that each macro fragment resembles an arm but I personally don't mind switching to any other desired syntax.

The lack of #[ ... ] is intended to reduce typing, nevertheless, the same reasoning described above can also be applied to this aspect.

Since blocks are intended to only contain items, anything but cfg is not expected to be supported at the current or future time.

Credits goes to @gnzlbg because most of the code was shamelessly copied from https://github.com/gnzlbg/match_cfg.
Credits goes to @alexcrichton because most of the code was shamelessly copied from https://github.com/rust-lang/cfg-if.

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 31, 2023
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Aug 31, 2023

@rustbot labels -T-libs +T-libs-api

r? libs-api

@rustbot rustbot assigned Amanieu and unassigned cuviper Aug 31, 2023
@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 31, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@kadiwa4
Copy link
Contributor

kadiwa4 commented Sep 2, 2023

The implementation is wrong if there are more than 2 non-wildcard match arms.
Example:

match_cfg! {
    cfg(windows) => {
        fn foo() {}
    }
    cfg(unix) => {
        fn foo() {}
    }
    cfg(target_pointer_width = "64") => {
        fn foo() {}
    }
}

Fails to compile on 64-bit unix because foo is defined twice.

Also, the cfg(…) @ cfg(…) syntax used internally is not great because someone might accidentally use that, maybe use something like …, @_but_not, … or a helper macro

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 2, 2023

The implementation is wrong if there are more than 2 non-wildcard match arms. Example:

match_cfg! {
    cfg(windows) => {
        fn foo() {}
    }
    cfg(unix) => {
        fn foo() {}
    }
    cfg(target_pointer_width = "64") => {
        fn foo() {}
    }
}

Fails to compile on 64-bit unix because foo is defined twice.

Also, the cfg(…) @ cfg(…) syntax used internally is not great because someone might accidentally use that, maybe use something like …, @_but_not, … or a helper macro

Well, it is a surprise... To make things more clever, the implementation will now be based on cfg_if.

@c410-f3r c410-f3r force-pushed the match_cfg branch 3 times, most recently from 703b67d to 203728f Compare September 2, 2023 15:16
@rust-log-analyzer

This comment has been minimized.

@Amanieu Amanieu added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Sep 2, 2023
@Amanieu
Copy link
Member

Amanieu commented Sep 5, 2023

We discussed this in the libs-api meeting and are happy to add this as an unstable feature. Please open a tracking issue for this.

There was a desire to have this supported directly by a language feature, but nobody could come up with a convincing design.

Some issues should be address before stabilization, specifically:

  • We should choose between the cfg_if and match_cfg syntax. Both are functionally equivalent.
    • If we choose match_cfg, we should transition our existing uses of cfg_if.
  • We need to ensure rust-analyzer and rustfmt are able to work properly on code contained in the match arms.

@tgross35
Copy link
Contributor

tgross35 commented Sep 5, 2023

A few small notes:

  1. The original post should be updated, the crate is cfg_if (not if_cfg).

  2. With that in mind, the name cfg_match could work here. Maybe better for locating via autocomplete when you type cfg?

  3. Links:

  4. I am thinking that maybe this macro could accept single function items without wrapping in a block. This gives a more compact representation, saving an indentation level for the function body (more on par with cfg_if) plus two lines per match arm

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

@c410-f3r c410-f3r mentioned this pull request Sep 6, 2023
3 tasks
@c410-f3r c410-f3r changed the title Add the match_cfg! macro Add the cfg_match! macro Sep 6, 2023
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 6, 2023

I would like to express my sincere appreciation for the quick feedback, thank you @Amanieu and the participating members of the lib team.

Please open a tracking issue for this.

Done -> #115585

If we choose match_cfg, we should transition our existing uses of cfg_if.

If that will indeed be the case, then I can fulfil the task.

With that in mind, the name cfg_match could work here. Maybe better for locating via autocomplete when you type cfg?

Changed to cfg_match but IIRC, the stabilization of #79524 was halted because of conflicts with the itertools crate.

I am thinking that maybe this macro could accept single function items without wrapping in a block. This gives a more compact representation, saving an indentation level for the function body (more on par with cfg_if) plus two lines per match arm

If possible, I will do it in a following PR.

@tgross35
Copy link
Contributor

tgross35 commented Sep 6, 2023

With that in mind, the name cfg_match could work here. Maybe better for locating via autocomplete when you type cfg?

Changed to cfg_match but IIRC, the stabilization of #79524 was halted because of conflicts with the itertools crate.

Luckily that is different - in that case, the problem is there being no way to automatically resolve a conflict between two traits when they put something with the same name in scope. Not a problem here since crates are preferred over the prelude

$ cat ../println/src/lib.rs
#[macro_export]
macro_rules! println {
    ($x:tt) => { eprintln!("haha! using my crate's println") };
}

$ cat Cargo.toml
[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[dependencies]
println = { path = "../println" }

$ cat src/main.rs
use println::println;

fn main() { println!("using std's println"); }

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/foo`
haha! using my crate's println

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 6, 2023

With that in mind, the name cfg_match could work here. Maybe better for locating via autocomplete when you type cfg?

Changed to cfg_match but IIRC, the stabilization of #79524 was halted because of conflicts with the itertools crate.

Luckily that is different - in that case, the problem is there being no way to automatically resolve a conflict between two traits when they put something with the same name in scope. Not a problem here since crates are preferred over the prelude

$ cat ../println/src/lib.rs
#[macro_export]
macro_rules! println {
    ($x:tt) => { eprintln!("haha! using my crate's println") };
}

$ cat Cargo.toml
[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[dependencies]
println = { path = "../println" }

$ cat src/main.rs
use println::println;

fn main() {  println!("using std's println"); }

$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/foo`
haha! using my crate's println

Glad to hear that. Thanks for investigating this scenario!

@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2023
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 23, 2023

RuntimeError: unreachable

Probably because assert is being invoked due to some mishandled configuration. I will investigate more tomorrow.

@ehuss
Copy link
Contributor

ehuss commented Sep 23, 2023

@bors r-

synchronizing the queue

@bors bors 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 Sep 23, 2023
@c410-f3r
Copy link
Contributor Author

OK, everything should be good now.

@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member

Amanieu commented Sep 23, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 23, 2023

📌 Commit d63959f has been approved by Amanieu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 23, 2023
@bors
Copy link
Contributor

bors commented Sep 24, 2023

⌛ Testing commit d63959f with merge 8a6bae2...

@bors
Copy link
Contributor

bors commented Sep 24, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 8a6bae2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2023
@bors bors merged commit 8a6bae2 into rust-lang:master Sep 24, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 24, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8a6bae2): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.5%, 0.7%] 4
Regressions ❌
(secondary)
0.5% [0.5%, 0.6%] 2
Improvements ✅
(primary)
-1.3% [-1.3%, -1.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-1.3%, 0.7%] 5

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
3.4% [1.7%, 5.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-2.9%, -2.9%] 1
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 8
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 8

Bootstrap: 630.937s -> 629.884s (-0.17%)
Artifact size: 317.15 MiB -> 317.26 MiB (0.03%)

@rustbot rustbot added the perf-regression Performance regression. label Sep 24, 2023
@c410-f3r
Copy link
Contributor Author

Perf is acting stochastically IMO

@rylev
Copy link
Member

rylev commented Sep 26, 2023

Yep this is noise:

@rustbot label: +perf-regression-triaged

@danielhenrymantilla
Copy link
Contributor

For reference, this has broken downstream crates due to name resolution errors, since for some reason the feature-gate does not prevent them:

I will be important in the future that, whenever a new macro is added to the top-level of ::core or ::std, a crater run be involved to detect such issues and warn the developers beforehand.

@c410-f3r
Copy link
Contributor Author

@danielhenrymantilla #117057 (comment) #117162

In my personal opinion, cfg_match should be renamed back to match_cfg to allow prelude support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.