Skip to content

make cfg_select a builtin macro #143461

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

Merged
merged 1 commit into from
Jul 13, 2025

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Jul 4, 2025

tracking issue: #115585

This parses mostly the same as the macro cfg_select version, except:

  1. wrapping in double brackets is no longer supported (or needed): cfg_select {{ /* ... */ }} is now rejected.
  2. in an expression context, the rhs is no longer wrapped in a block, so that this now works:
fn main() {
    println!(cfg_select! {
        unix => { "foo" }
        _ => { "bar" }
    });
}
  1. a single wildcard rule is now supported: cfg_select { _ => 1 } now works

I've also added an error if none of the rules evaluate to true, and warnings for any arms that follow the _ wildcard rule.

cc @traviscross if I'm missing any feature that should/should not be included
r? @petrochenkov for the macro logic details

@rustbot rustbot added A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 4, 2025
@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch 2 times, most recently from e3aeb08 to 1dc0034 Compare July 5, 2025 06:03
@folkertdev folkertdev marked this pull request as ready for review July 5, 2025 06:07
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR modifies tests/auxiliary/minicore.rs.

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@traviscross traviscross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior, per the tests and the impl, looks correct to me.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch 2 times, most recently from 3d68353 to 3abcece Compare July 5, 2025 12:52
@folkertdev
Copy link
Contributor Author

I moved some things around (analogously to asm!) so that rustfmt can use the parsed representation of cfg_select!. Now that it is a builtin macro, I'm assuming formatting won't work and becomes a blocker for stabilization. I think the formatting rules are reasonably straightforward though.

Implementation-wise the one thing I'm not sure about is how to get rustfmt to format a TokenStream that represents the rhs of an arm. It gets expanded based on the context (can be an item, expr, stmt, etc.). But hopefully the rustfmt folks know how to deal with that.

@traviscross
Copy link
Contributor

cc @rust-lang/rustfmt @rust-lang/style

@bors

This comment was marked as resolved.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from 3abcece to 53b3b88 Compare July 6, 2025 15:27
@bors

This comment was marked as resolved.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from 53b3b88 to 5d550b1 Compare July 7, 2025 18:46
@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from 4b84ad7 to 01f41cc Compare July 7, 2025 18:58
@folkertdev
Copy link
Contributor Author

Given that formatting now seems straightforward, I think this can be merged without the precise formatting logic. It's probably better (also for review etc.) to add formatting separately in the rustfmt repo. I've played around with it a bit, but I think there is some benefit from refactoring the macro_rules! logic a bit for more code sharing.

So, this PR is complete from my perspective.

@ytmimi
Copy link
Contributor

ytmimi commented Jul 10, 2025

Given that formatting now seems straightforward, I think this can be merged without the precise formatting logic. It's probably better (also for review etc.) to add formatting separately in the rustfmt repo. I've played around with it a bit, but I think there is some benefit from refactoring the macro_rules! logic a bit for more code sharing.

Nice! I agree that a PR to the rustfmt repo would be ideal to add the formatting logic / refactoring for code sharing with macro_rules!

@petrochenkov petrochenkov 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 Jul 10, 2025
@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from 01f41cc to b4afa4a Compare July 10, 2025 14:57
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 12, 2025
fmease added a commit to fmease/rust that referenced this pull request Jul 13, 2025
…o, r=petrochenkov

make `cfg_select` a builtin macro

tracking issue: rust-lang#115585

This parses mostly the same as the `macro cfg_select` version, except:

1. wrapping in double brackets is no longer supported (or needed): `cfg_select {{ /* ... */ }}` is now rejected.
2. in an expression context, the rhs is no longer wrapped in a block, so that this now works:
  ```rust
  fn main() {
      println!(cfg_select! {
          unix => { "foo" }
          _ => { "bar" }
      });
  }
  ```
3. a single wildcard rule is now supported: `cfg_select { _ => 1 }` now works

I've also added an error if none of the rules evaluate to true, and warnings for any arms that follow the `_` wildcard rule.

cc `@traviscross` if I'm missing any feature that should/should not be included
r? `@petrochenkov` for the macro logic details
bors added a commit that referenced this pull request Jul 13, 2025
Rollup of 14 pull requests

Successful merges:

 - #143301 (`tests/ui`: A New Order [26/N])
 - #143461 (make `cfg_select` a builtin macro)
 - #143519 (Check assoc consts and tys later like assoc fns)
 - #143554 (slice: Mark `rotate_left`, `rotate_right` unstably const)
 - #143634 (interpret/allocation: expose init + write_wildcards on a range)
 - #143679 (Preserve the .debug_gdb_scripts section)
 - #143685 (Resolve: merge `source_bindings` and `target_bindings` into `bindings`)
 - #143704 (Be a bit more careful around exotic cycles in in the inliner)
 - #143734 (Refactor resolve resolution bindings)
 - #143774 (constify `From` and `Into`)
 - #143785 (Add --compile-time-deps argument for x check)
 - #143786 (Fix fallback for CI_JOB_NAME)
 - #143825 (clippy: fix test filtering when TESTNAME is empty)
 - #143826 (Fix command trace)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 13, 2025
…o, r=petrochenkov

make `cfg_select` a builtin macro

tracking issue: rust-lang#115585

This parses mostly the same as the `macro cfg_select` version, except:

1. wrapping in double brackets is no longer supported (or needed): `cfg_select {{ /* ... */ }}` is now rejected.
2. in an expression context, the rhs is no longer wrapped in a block, so that this now works:
  ```rust
  fn main() {
      println!(cfg_select! {
          unix => { "foo" }
          _ => { "bar" }
      });
  }
  ```
3. a single wildcard rule is now supported: `cfg_select { _ => 1 }` now works

I've also added an error if none of the rules evaluate to true, and warnings for any arms that follow the `_` wildcard rule.

cc ``@traviscross`` if I'm missing any feature that should/should not be included
r? ``@petrochenkov`` for the macro logic details
bors added a commit that referenced this pull request Jul 13, 2025
Rollup of 13 pull requests

Successful merges:

 - #143301 (`tests/ui`: A New Order [26/N])
 - #143461 (make `cfg_select` a builtin macro)
 - #143519 (Check assoc consts and tys later like assoc fns)
 - #143554 (slice: Mark `rotate_left`, `rotate_right` unstably const)
 - #143634 (interpret/allocation: expose init + write_wildcards on a range)
 - #143679 (Preserve the .debug_gdb_scripts section)
 - #143685 (Resolve: merge `source_bindings` and `target_bindings` into `bindings`)
 - #143734 (Refactor resolve resolution bindings)
 - #143774 (constify `From` and `Into`)
 - #143785 (Add --compile-time-deps argument for x check)
 - #143786 (Fix fallback for CI_JOB_NAME)
 - #143825 (clippy: fix test filtering when TESTNAME is empty)
 - #143826 (Fix command trace)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy
#143876 (comment)

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2025
@matthiaskrgr
Copy link
Member

could also be related to #143519 ? 🤔

@folkertdev
Copy link
Contributor Author

No it's definitely this PR, just finicky targets and a feature that is inherently tied to conditional compilation.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from d683da0 to 3689b80 Compare July 13, 2025 12:35
@folkertdev
Copy link
Contributor Author

@bors2 try jobs=x86_64-msvc-2,test-various

@rust-bors
Copy link

rust-bors bot commented Jul 13, 2025

⌛ Trying commit 3689b80 with merge 0080f64

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jul 13, 2025
make `cfg_select` a builtin macro

tracking issue: #115585

This parses mostly the same as the `macro cfg_select` version, except:

1. wrapping in double brackets is no longer supported (or needed): `cfg_select {{ /* ... */ }}` is now rejected.
2. in an expression context, the rhs is no longer wrapped in a block, so that this now works:
  ```rust
  fn main() {
      println!(cfg_select! {
          unix => { "foo" }
          _ => { "bar" }
      });
  }
  ```
3. a single wildcard rule is now supported: `cfg_select { _ => 1 }` now works

I've also added an error if none of the rules evaluate to true, and warnings for any arms that follow the `_` wildcard rule.

cc `@traviscross` if I'm missing any feature that should/should not be included
r? `@petrochenkov` for the macro logic details
try-job: x86_64-msvc-2
try-job: test-various
@rust-bors
Copy link

rust-bors bot commented Jul 13, 2025

☀️ Try build successful (CI)
Build commit: 0080f64 (0080f6457f88ce3d932fb25a15255696a94a32d9, parent: 0fb279be1d96464c69209053ef0f97ad63088cda)

@folkertdev
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Jul 13, 2025

📌 Commit 3689b80 has been approved by petrochenkov

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 Jul 13, 2025
@bors
Copy link
Collaborator

bors commented Jul 13, 2025

⌛ Testing commit 3689b80 with merge e9182f1...

@bors
Copy link
Collaborator

bors commented Jul 13, 2025

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing e9182f1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 13, 2025
@bors bors merged commit e9182f1 into rust-lang:master Jul 13, 2025
13 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 13, 2025
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 56835d7 (parent) -> e9182f1 (this PR)

Test differences

Show 80 test diffs

Stage 1

  • errors::verify_builtin_macros_cfg_select_no_matches_84: [missing] -> pass (J0)
  • errors::verify_builtin_macros_cfg_select_unreachable_85: [missing] -> pass (J0)
  • [ui] tests/ui/macros/cfg_select.rs: [missing] -> pass (J1)

Stage 2

  • [ui] tests/ui/macros/cfg_select.rs: [missing] -> pass (J2)

Additionally, 76 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard e9182f195b8505c87c4bd055b9f6e114ccda0981 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. x86_64-apple-2: 4845.3s -> 3156.9s (-34.8%)
  2. pr-check-2: 2608.4s -> 2211.0s (-15.2%)
  3. pr-check-1: 1753.5s -> 1516.1s (-13.5%)
  4. aarch64-apple: 4637.4s -> 4095.4s (-11.7%)
  5. dist-apple-various: 6961.7s -> 6167.3s (-11.4%)
  6. x86_64-gnu-llvm-19-1: 3694.0s -> 3305.9s (-10.5%)
  7. x86_64-apple-1: 6096.8s -> 6728.5s (10.4%)
  8. tidy: 74.3s -> 66.8s (-10.2%)
  9. x86_64-gnu-tools: 3568.6s -> 3220.7s (-9.7%)
  10. i686-gnu-2: 6038.0s -> 5526.6s (-8.5%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@folkertdev
Copy link
Contributor Author

@ytmimi now that this has been merged, we can think about the formatting. I have a working implementation locally.

However, the rustfmt repo is using a rust toolchain that is 4 months old. Is there some sort of policy for updating that? (I imagine that might be kind of tricky to do for me, I'm not really familiar with the code base).

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e9182f1): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary 2.6%, secondary 4.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)
4.2% [4.2%, 4.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.6% [2.6%, 2.6%] 1

Cycles

Results (secondary -5.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.2% [-7.5%, -2.9%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 465.532s -> 464.056s (-0.32%)
Artifact size: 374.73 MiB -> 374.70 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants