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.

Sign up for GitHub

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

Jump to bottom

expand: Preserve order of inert attributes during expansion #82419

Merged
merged 2 commits into from
Feb 27, 2021
Merged

expand: Preserve order of inert attributes during expansion #82419

merged 2 commits into from
Feb 27, 2021

Conversation

petrochenkov
Copy link
Contributor

Fixes #67839
Fixes #81871
r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2021
This was referenced Feb 22, 2021
Closed
Closed
@Aaron1011
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2021

📌 Commit fc9d578 has been approved by Aaron1011

@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 Feb 23, 2021
Aaron1011
Aaron1011 reviewed Feb 23, 2021
View reviewed changes
src/test/ui/proc-macro/issue-75930-derive-cfg.stdout
@@ -1,4 +1,4 @@
PRINT-ATTR INPUT (DISPLAY): #[allow(dead_code)] #[derive(Print)] #[print_helper(b)] #[print_helper(a)]
PRINT-ATTR INPUT (DISPLAY): #[print_helper(a)] #[allow(dead_code)] #[derive(Print)] #[print_helper(b)]
Copy link
Member

@Aaron1011 Aaron1011 Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is a subtle detail that I just noticed - since we perform cfg-stripping before invoking custom attributes (e.g. #[my_attr] #[cfg(FALSE)] struct Foo {} does nothing), we also expand #[cfg_attr]. So, an attribute macro sees expanded #[cfg_attr] attributes (e.g. the #[allow(dead_code)]) at the 'top level' of the attribute target, but not anywhere else. For example, you can see #[cfg_attr(not(FALSE), allow(warnings))] in the input below.

Copy link
Contributor Author

@petrochenkov petrochenkov Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

since we perform cfg-stripping before invoking custom attributes

This is something I want to change, btw (and make the expansion order fully left-to-right).
It will certainly needs a crater run, but I think we'll be able to do this in practice, similarly to #79078.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 24, 2021
@Dylan-DPC
Rollup merge of rust-lang#82419 - petrochenkov:inertord, r=Aaron1011
7d65761 
expand: Preserve order of inert attributes during expansion

Fixes rust-lang#67839
Fixes rust-lang#81871
r? `@Aaron1011`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 24, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 24, 2021
@Dylan-DPC
Rollup merge of rust-lang#82419 - petrochenkov:inertord, r=Aaron1011
4f3203f 
expand: Preserve order of inert attributes during expansion

Fixes rust-lang#67839
Fixes rust-lang#81871
r? ``@Aaron1011``
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 24, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 26, 2021
@Dylan-DPC
Rollup merge of rust-lang#82419 - petrochenkov:inertord, r=Aaron1011
2023352 
expand: Preserve order of inert attributes during expansion

Fixes rust-lang#67839
Fixes rust-lang#81871
r? ```@Aaron1011```
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 26, 2021
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
@Dylan-DPC
Rollup merge of rust-lang#82419 - petrochenkov:inertord, r=Aaron1011
05677f8 
expand: Preserve order of inert attributes during expansion

Fixes rust-lang#67839
Fixes rust-lang#81871
r? ````@Aaron1011````
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 27, 2021
Closed
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 27, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2021
@bors
Auto merge of rust-lang#82577 - Dylan-DPC:rollup-c3si8ju, r=Dylan-DPC
9fa580b 
Rollup of 14 pull requests

Successful merges:

 - rust-lang#81794 (update tracking issue for `relaxed_struct_unsize`)
 - rust-lang#82057 (Replace const_cstr with cstr crate)
 - rust-lang#82370 (Improve anonymous lifetime note to indicate the target span)
 - rust-lang#82394 (:arrow_up: rust-analyzer)
 - rust-lang#82396 (Add Future trait for doc_spotlight feature doc)
 - rust-lang#82404 (Test hexagon-enum only when llvm target is present)
 - rust-lang#82419 (expand: Preserve order of inert attributes during expansion)
 - rust-lang#82420 (Enable API documentation for `std::os::wasi`.)
 - rust-lang#82421 (Add a `size()` function to WASI's `MetadataExt`.)
 - rust-lang#82442 (Skip emitting closure diagnostic when closure_kind_origins has no entry)
 - rust-lang#82473 (Use libc::accept4 on Android instead of raw syscall.)
 - rust-lang#82482 (Use small hash set in `mir_inliner_callees`)
 - rust-lang#82490 (Update cargo)
 - rust-lang#82494 (Substitute erased lifetimes on bad placeholder type)

Failed merges:

 - rust-lang#82448 (Combine HasAttrs and HasTokens into AstLike)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 76b40d2 into rust-lang:master Feb 27, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 27, 2021
@kezhuw kezhuw mentioned this pull request May 24, 2024
Open
kezhuw added a commit to kezhuw/rstest that referenced this pull request Jan 16, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
28e20ea 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
@kezhuw kezhuw mentioned this pull request Jan 16, 2025
Merged
kezhuw added a commit to kezhuw/rstest that referenced this pull request Jan 16, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
e3045e4 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/rstest that referenced this pull request Jan 17, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
9937853 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
la10736 pushed a commit to la10736/rstest that referenced this pull request Jan 17, 2025
@kezhuw @la10736
Append generated test macro so next test macros are aware of it
e0b735e 
This way test macros following `#[rstest]` can decide whether or not to
generate test macro to avoid duplicate test runs.

It is an attempt to improve capabilities among test macros. Currently,
following test from [googletest](https://github.com/google/googletest-rust/blob/21f2948684847922a416252b8118e3eada8e29d6/integration_tests/src/google_test_with_rstest.rs#L52-L57)(`main` branch at 2025-01-16) will run twice.

```rust
#[rstest]
#[case(1)]
#[gtest]
fn paramterised_test_should_work_with_rstest_first(#[case] value: u32) -> Result<()> {
    verify_that!(value, eq(value))
}
```

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/googletest-rust that referenced this pull request Jan 20, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
87b2fc2 
`#[gtest]` will benefit from la10736/rstest#291, we could also benefit
other test macros.

It is an attempt to improve capabilities among test macros to avoid
duplicated test runs which is rare to be aware of.

The rationale is simple: procedure of attribute macro see only
attributes following it. Macros next to processing macro will not see
generated macro if it is generated in-place. So, instead of generating
test macro in-place, appending generated test macro will let macros next
to processing macro have chance to react, for example, not generate test
macro if there is already one.

We could deprecate `#[googletest::test]` oneday after rust-lang/rust#82419
released.

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143,
la10736/rstest#291, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/googletest-rust that referenced this pull request Jan 20, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
61d7917 
`#[gtest]` will benefit from la10736/rstest#291, we could also benefit
other test macros.

It is an attempt to improve capabilities among test macros to avoid
duplicated test runs which is rare to be aware of.

The rationale is simple: procedure of attribute macro see only
attributes following it. Macros next to processing macro will not see
generated macro if it is generated in-place. So, instead of generating
test macro in-place, appending generated test macro will let macros next
to processing macro have chance to react, for example, not generate test
macro if there is already one.

We could deprecate `#[googletest::test]` oneday after la10736/rstest#291
released.

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143,
la10736/rstest#291, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
kezhuw added a commit to kezhuw/googletest-rust that referenced this pull request Jan 20, 2025
@kezhuw
Append generated test macro so next test macros are aware of it
dc7620d 
`#[gtest]` will benefit from la10736/rstest#291, we could also benefit
other test macros.

This is an attempt to improve capabilities among test macros to avoid
duplicated test runs which is rare to be aware of.

The rationale is simple: procedure of attribute macro see only
attributes following it. Macros next to processing macro will not see
generated macro if it is generated in-place. So, instead of generating
test macro in-place, appending generated test macro will let macros next
to processing macro have chance to react, for example, not generate test
macro if there is already one.

We could deprecate `#[googletest::test]` oneday after la10736/rstest#291
released.

See: tokio-rs/tokio#6497, d-e-s-o/test-log#46, frondeus/test-case#143,
la10736/rstest#291, kezhuw/stuck#53.
Refs: rust-lang/rust#67839, rust-lang/rust#82419.
@kezhuw kezhuw mentioned this pull request Jan 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011 Aaron1011 left review comments

Assignees

@Aaron1011 Aaron1011

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
5 participants
@petrochenkov @Aaron1011 @bors @rust-highfive @rustbot