Skip to content

Conversation

RalfJung
Copy link
Member

#144648 broke the attributes that ignore the tests on Miri; this patch should fix that.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2025
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 11, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@RalfJung
Copy link
Member Author

RalfJung commented Sep 11, 2025

This just fixes the Miri cases; there are more cfg_attr(..., ignore) attached to nonpoison_and_poison_unwrap_test! and none of those actually work (the attribute ends up attached to the first test only).

Cc @connortsui20 ; would be good if you could fix the rest of them.

@tgross35
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 11, 2025

📌 Commit 0928026 has been approved by tgross35

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2025
@connortsui20
Copy link
Contributor

@RalfJung sure! Could you tell me why the cfg_attr is different from cfg(not()) here?

@RalfJung
Copy link
Member Author

RalfJung commented Sep 11, 2025

Note that it's cfg_attr($cond, ignore) vs cfg(not($cond)). See the cfg_attr docs for details.

But it is true that I just assumed that the cfg would be expanded early and disable the the entire macro invocation, which may or may not be true... I'll have to test that.

@connortsui20
Copy link
Contributor

Ah I see, the #[ignore] only gets passed down to the first test.

https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg_attr-attribute
https://doc.rust-lang.org/reference/attributes/testing.html#the-ignore-attribute

Is it worth refactoring the macro so that it can apply any attribute macros to both? That's almost certainly going to cause confusion in the future. Though I don't really know how I would implement that to be honest...

@RalfJung
Copy link
Member Author

cfg indeed does get applied before the macro is expanded apparently, so both tests get disabled.

Is it worth refactoring the macro so that it can apply any attribute macros to both? That's almost certainly going to cause confusion in the future. Though I don't really know how I would implement that to be honest...

Yeah you could have something like

nonpoison_and_poison_unwrap_test!(
    name: frob,
    test_body:
    #[attr1]
    #[attr2]
    {

Using the meta macro matcher, that should not be too hard to implement.

Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2025
rwlock tests: fix miri macos test regression

rust-lang#144648 broke the attributes that ignore the tests on Miri; this patch should fix that.
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2025
…r=RalfJung

fix cfg for poison test macro

Fixes test regression in rust-lang#144648
Continuation of rust-lang#146433

I think this is right? Not really sure how to test this myself to be honest.

r? `@RalfJung`

I'll also leave the improvement to the test macro for a separate PR (described [here](rust-lang#146433 (comment))) since I've never done something like that before. Though since this fixes all of the tests, it might not be necessary since anyone in the future will see the `cfg()` and not `cfg_attr()`?
bors added a commit that referenced this pull request Sep 12, 2025
Rollup of 16 pull requests

Successful merges:

 - #145660 (initial implementation of the darwin_objc unstable feature)
 - #145895 (thread parking: fix docs and examples)
 - #146308 (support integer literals in `${concat()}`)
 - #146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - #146332 (tidy: make behavior of extra-checks more uniform)
 - #146338 (Extends AArch64 branch protection support to include GCS)
 - #146374 (Update `browser-ui-test` version to `0.22.2`)
 - #146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - #146426 (Bump miow to 0.60.1)
 - #146432 (Implement `Socket::take_error` for Hermit)
 - #146433 (rwlock tests: fix miri macos test regression)
 - #146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - #146439 (fix cfg for poison test macro)
 - #146448 ([rustdoc] Correctly handle literal search on paths)
 - #146449 (Fix `libgccjit` symlink when we build GCC locally)
 - #146455 (test: remove an outdated normalization for rustc versions)

Failed merges:

 - #146389 (Convert `no_std` and `no_core` to the new attribute infrastructure)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2025
rwlock tests: fix miri macos test regression

rust-lang#144648 broke the attributes that ignore the tests on Miri; this patch should fix that.
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2025
…r=RalfJung

fix cfg for poison test macro

Fixes test regression in rust-lang#144648
Continuation of rust-lang#146433

I think this is right? Not really sure how to test this myself to be honest.

r? ``@RalfJung``

I'll also leave the improvement to the test macro for a separate PR (described [here](rust-lang#146433 (comment))) since I've never done something like that before. Though since this fixes all of the tests, it might not be necessary since anyone in the future will see the `cfg()` and not `cfg_attr()`?
bors added a commit that referenced this pull request Sep 12, 2025
Rollup of 16 pull requests

Successful merges:

 - #144549 (match clang's `va_arg` assembly on arm targets)
 - #145660 (initial implementation of the darwin_objc unstable feature)
 - #145895 (thread parking: fix docs and examples)
 - #146308 (support integer literals in `${concat()}`)
 - #146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - #146332 (tidy: make behavior of extra-checks more uniform)
 - #146374 (Update `browser-ui-test` version to `0.22.2`)
 - #146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - #146426 (Bump miow to 0.60.1)
 - #146432 (Implement `Socket::take_error` for Hermit)
 - #146433 (rwlock tests: fix miri macos test regression)
 - #146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - #146439 (fix cfg for poison test macro)
 - #146448 ([rustdoc] Correctly handle literal search on paths)
 - #146449 (Fix `libgccjit` symlink when we build GCC locally)
 - #146455 (test: remove an outdated normalization for rustc versions)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2025
…r=RalfJung

fix cfg for poison test macro

Fixes test regression in rust-lang#144648
Continuation of rust-lang#146433

I think this is right? Not really sure how to test this myself to be honest.

r? ```@RalfJung```

I'll also leave the improvement to the test macro for a separate PR (described [here](rust-lang#146433 (comment))) since I've never done something like that before. Though since this fixes all of the tests, it might not be necessary since anyone in the future will see the `cfg()` and not `cfg_attr()`?
bors added a commit that referenced this pull request Sep 12, 2025
Rollup of 15 pull requests

Successful merges:

 - #144549 (match clang's `va_arg` assembly on arm targets)
 - #145895 (thread parking: fix docs and examples)
 - #146308 (support integer literals in `${concat()}`)
 - #146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - #146332 (tidy: make behavior of extra-checks more uniform)
 - #146374 (Update `browser-ui-test` version to `0.22.2`)
 - #146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - #146426 (Bump miow to 0.60.1)
 - #146432 (Implement `Socket::take_error` for Hermit)
 - #146433 (rwlock tests: fix miri macos test regression)
 - #146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - #146439 (fix cfg for poison test macro)
 - #146448 ([rustdoc] Correctly handle literal search on paths)
 - #146449 (Fix `libgccjit` symlink when we build GCC locally)
 - #146455 (test: remove an outdated normalization for rustc versions)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 312e15f into rust-lang:master Sep 12, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Sep 12, 2025
rust-timer added a commit that referenced this pull request Sep 12, 2025
Rollup merge of #146439 - connortsui20:fix-sync-macro-attr, r=RalfJung

fix cfg for poison test macro

Fixes test regression in #144648
Continuation of #146433

I think this is right? Not really sure how to test this myself to be honest.

r? ```@RalfJung```

I'll also leave the improvement to the test macro for a separate PR (described [here](#146433 (comment))) since I've never done something like that before. Though since this fixes all of the tests, it might not be necessary since anyone in the future will see the `cfg()` and not `cfg_attr()`?
rust-timer added a commit that referenced this pull request Sep 12, 2025
Rollup merge of #146433 - RalfJung:rwlock-miri, r=tgross35

rwlock tests: fix miri macos test regression

#144648 broke the attributes that ignore the tests on Miri; this patch should fix that.
@RalfJung RalfJung deleted the rwlock-miri branch September 12, 2025 15:14
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 13, 2025
fix cfg for poison test macro

Fixes test regression in rust-lang/rust#144648
Continuation of rust-lang/rust#146433

I think this is right? Not really sure how to test this myself to be honest.

r? ```@RalfJung```

I'll also leave the improvement to the test macro for a separate PR (described [here](rust-lang/rust#146433 (comment))) since I've never done something like that before. Though since this fixes all of the tests, it might not be necessary since anyone in the future will see the `cfg()` and not `cfg_attr()`?
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 13, 2025
Rollup of 15 pull requests

Successful merges:

 - rust-lang/rust#144549 (match clang's `va_arg` assembly on arm targets)
 - rust-lang/rust#145895 (thread parking: fix docs and examples)
 - rust-lang/rust#146308 (support integer literals in `${concat()}`)
 - rust-lang/rust#146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - rust-lang/rust#146332 (tidy: make behavior of extra-checks more uniform)
 - rust-lang/rust#146374 (Update `browser-ui-test` version to `0.22.2`)
 - rust-lang/rust#146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - rust-lang/rust#146426 (Bump miow to 0.60.1)
 - rust-lang/rust#146432 (Implement `Socket::take_error` for Hermit)
 - rust-lang/rust#146433 (rwlock tests: fix miri macos test regression)
 - rust-lang/rust#146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - rust-lang/rust#146439 (fix cfg for poison test macro)
 - rust-lang/rust#146448 ([rustdoc] Correctly handle literal search on paths)
 - rust-lang/rust#146449 (Fix `libgccjit` symlink when we build GCC locally)
 - rust-lang/rust#146455 (test: remove an outdated normalization for rustc versions)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants