Warn when a `#[test]`-like built-in attribute macro is present multiple times. by Ethiraric · Pull Request #91172 · rust-lang/rust

Ethiraric · 2021-11-24T09:24:47Z

GuillaumeGomez · 2021-11-24T09:27:03Z

Milo123459 · 2021-11-24T09:51:49Z

Shouldn't this emit an error, not a warning? I'm trying to think of a use case where having multiple #[test]'s is a good idea and would probably make sense for it to be an error. Just giving my opinion though, it might make sense to have it as a warning.

Ethiraric · 2021-11-24T10:00:24Z

It could very well be an error. I don't see any case either where this would be the expected behavior. Having a hard error on that is maybe too inconvenient on what was probably an oversight from the developer and could break existing codebases.

Milo123459 · 2021-11-24T10:04:16Z

It could very well be an error. I don't see any case either where this would be the expected behavior. Having a hard error on that is maybe too inconvenient on what was probably an oversight from the developer and could break existing codebases.

Implementing a fix for it would be super easy; it just has to remove the other usages of #[test] on that function.

Ethiraric · 2021-11-24T10:06:43Z

How does that weigh in whether this should be an error or a warning?

GuillaumeGomez · 2021-11-24T10:08:07Z

It's up to @rust-lang/compiler team I guess. But in any case, I think we have to first start with a warning saying it'll become a hard error in the future if we decide that it's not supposed to compile.

Milo123459 · 2021-11-24T10:08:23Z

How does that weigh in whether this should be an error or a warning?

Read it wrong. My bad.

I agree - throwing a hard error might be inconsistent.

lcnr · 2021-11-24T10:25:14Z

we currently allow the following without any warnings as well

#[inline]#[inline]fnfoo(){}

I think that ideally we should hard error for duplicate builtin attributes though starting with a future incompat lint is probably the right choice 🤔 imo we should start a crater run for this, you would have to first convert the warning to an error in that case.

nagisa · 2021-11-24T10:27:21Z

Right now the story around duplicate attributes is definitely somewhat erratic. There are some attributes which when duplicated will implement their function twice and sometimes cause an error (though outside of the attribute system). derive and test are an example of this. Some others will behave as if the attribute was specified once (e.g. #[no_mangle]), and some, such as inline will have an overriding behaviour (where only the last applies).

To be fair, I'm not sure if test having the “repeated operation” is the right model for #[test] in the first place – I would've expected it to work more like no_mangle, in which case warning would not be necessary either.

There are also considerations with regards to e.g. macros, where ability to duplicate some attributes may make things significantly easier for them.

GuillaumeGomez · 2021-11-24T10:33:03Z

There are also considerations with regards to e.g. macros, where ability to duplicate some attributes may make things significantly easier for them.

This is mostly why I think a warning should be enough.

Ethiraric · 2021-11-24T13:53:43Z

you would have to first convert the warning to an error in that case.

As implemented in this PR, the diagnostic is reported as a warning.

Should you decide on what the behavior for repeated builtin attributes should be, I am willing, in other PRs, to change their behaviors according to what you think is best.

In any case, let me know what changes you want me to do in this PR.

ehuss · 2021-11-24T16:24:50Z

we currently allow the following without any warnings as well

Duplicate attributes should now warn on the latest nightly.

Ethiraric · 2021-11-24T18:27:31Z

Duplicate attributes should now warn on the latest nightly.

It doesn't as of :

rustc 1.58.0-nightly (65c55bf93 2021-11-23)
binary: rustc
commit-hash: 65c55bf931a55e6b1e5ed14ad8623814a7386424
commit-date: 2021-11-23
host: x86_64-unknown-linux-gnu
release: 1.58.0-nightly
LLVM version: 13.0.0

ehuss · 2021-11-24T18:45:15Z

@Ethiraric Can you say more about what you tried? It should display a warning like this:

warning: unused attribute
--> src/lib.rs:2:1
|
2 | #[inline]
| ^^^^^^^^^ help: remove this attribute
|
= note: `#[warn(unused_attributes)]` on by default
note: attribute also specified here
--> src/lib.rs:1:1
|
1 | #[inline]
| ^^^^^^^^^
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

If you tried with #[test], then that is because the lint intentionally doesn't fire on proc-macros.

Ethiraric · 2021-11-24T19:47:52Z

I tried to compile the file I linked on the issue with the compiler I posted the version of.

Can you try to compile the snippet from #90979 and tell me whether you have a warning?

EDIT: Sorry, hadn't noticed the last line after the snippet.
Why is that that #[test] doesn't show any diagnostic?

ehuss · 2021-11-24T21:03:59Z

Why is that that #[test] doesn't show any diagnostic?

#[test] is treated like a proc-macro, and not a built-in attribute. The lint only fires for built-in attributes.

Ethiraric · 2021-11-24T21:31:17Z

But, with the current behaviour, I don't see any case annotating an item more than once with #[test] would be desired. This is why I thought a warn (not a hard error) would be nice.

I get that because it's a particular macro there currently is no warn. I have implemented this diagnostic here because I think there should be at least something that warns user.

Do you think this would be a bad addition?

GuillaumeGomez · 2021-11-24T21:33:09Z

I think the same as @Ethiraric in here: having a test run as many times as the #[test] attribute is present deserves at least a warning so at least you know why it's happening (even more if it comes from a macro!).

ehuss · 2021-11-24T21:58:26Z

Sorry, I didn't mean to mislead. I wasn't advocating against this change. I was just pointing out in the discussion above that things had changed (for built-in attribute) as of 2 days ago, and people didn't seem to be aware.

GuillaumeGomez · 2021-11-24T22:00:11Z

Funny that @Ethiraric came across this just in the middle of this "change wave". :)

petrochenkov · 2021-11-25T06:15:23Z

The full list of test-like built-in attribute macros is cfg_eval, global_allocator, test_case, test and bench.
They should all behave in the same way with regards to duplicates.
(They can be found by searching for calls to check_builtin_macro_attribute.)

Ethiraric · 2021-11-25T10:25:53Z

Thank you for your review and pointers. Should I add the lint to all built-in attribute macros in this PR or open a new one?

Ethiraric · 2021-12-15T00:28:28Z

Updated the comment for the lint.

I think however the build fails because the snippet of code should be run with --test. I know not how to instruct the compiler to do so.

petrochenkov · 2021-12-15T06:00:22Z

I think however the build fails because the snippet of code should be run with --test. I know not how to instruct the compiler to do so.

You can use

```rust,ignore

for disabling the lint-docs check.

Ethiraric · 2021-12-15T22:33:58Z

Thanks for your review. I should have fixed all your requested changes!

petrochenkov · 2021-12-16T05:59:32Z

@bors r+

bors · 2021-12-16T05:59:34Z

📌 Commit 2be94d4 has been approved by petrochenkov

…rochenkov Warn when a `#[test]`-like built-in attribute macro is present multiple times. Fixesrust-lang#90979.

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#91172 (Warn when a `#[test]`-like built-in attribute macro is present multiple times.) - rust-lang#91796 (Fix since attribute for const_manually_drop feature) - rust-lang#91879 (Remove `in_band_lifetimes` from `rustc_borrowck`) - rust-lang#91947 (Add `io::Error::other`) - rust-lang#91967 (Pull in libdevstat on FreeBSD) - rust-lang#91987 (Add module documentation for rustdoc passes) - rust-lang#92001 (Fix default_method_body_is_const when used across crates) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Pkgsrc changes: * Bump available bootstraps to 1.58.1. * Adjust one patch (and checksum) so that it still applies. Upstream changes: Version 1.59.0 (2022-02-24) ========================== Language -------- - [Stabilize default arguments for const generics][90207] - [Stabilize destructuring assignment][90521] - [Relax private in public lint on generic bounds and where clauses of trait impls][90586] - [Stabilize asm! and global_asm! for x86, x86_64, ARM, Aarch64, and RISC-V][91728] Compiler -------- - [Stabilize new symbol mangling format, leaving it opt-in (-Csymbol-mangling-version=v0)][90128] - [Emit LLVM optimization remarks when enabled with `-Cremark`][90833] - [Fix sparc64 ABI for aggregates with floating point members][91003] - [Warn when a `#[test]`-like built-in attribute macro is present multiple times.][91172] - [Add support for riscv64gc-unknown-freebsd][91284] - [Stabilize `-Z emit-future-incompat` as `--json future-incompat`][91535] - [Soft disable incremental compilation][94124] This release disables incremental compilation, unless the user has explicitly opted in via the newly added RUSTC_FORCE_INCREMENTAL=1 environment variable. This is due to a known and relatively frequently occurring bug in incremental compilation, which causes builds to issue internal compiler errors. This particular bug is already fixed on nightly, but that fix has not yet rolled out to stable and is deemed too risky for a direct stable backport. As always, we encourage users to test with nightly and report bugs so that we can track failures and fix issues earlier. See [94124] for more details. [94124]: rust-lang/rust#94124 Libraries --------- - [Remove unnecessary bounds for some Hash{Map,Set} methods][91593] Stabilized APIs --------------- - [`std::thread::available_parallelism`][available_parallelism] - [`Result::copied`][result-copied] - [`Result::cloned`][result-cloned] - [`arch::asm!`][asm] - [`arch::global_asm!`][global_asm] - [`ops::ControlFlow::is_break`][is_break] - [`ops::ControlFlow::is_continue`][is_continue] - [`TryFrom<char> for u8`][try_from_char_u8] - [`char::TryFromCharError`][try_from_char_err] implementing `Clone`, `Debug`, `Display`, `PartialEq`, `Copy`, `Eq`, `Error` - [`iter::zip`][zip] - [`NonZeroU8::is_power_of_two`][is_power_of_two8] - [`NonZeroU16::is_power_of_two`][is_power_of_two16] - [`NonZeroU32::is_power_of_two`][is_power_of_two32] - [`NonZeroU64::is_power_of_two`][is_power_of_two64] - [`NonZeroU128::is_power_of_two`][is_power_of_two128] - [`DoubleEndedIterator for ToLowercase`][lowercase] - [`DoubleEndedIterator for ToUppercase`][uppercase] - [`TryFrom<&mut [T]> for [T; N]`][tryfrom_ref_arr] - [`UnwindSafe for Once`][unwindsafe_once] - [`RefUnwindSafe for Once`][refunwindsafe_once] - [armv8 neon intrinsics for aarch64][stdarch/1266] Const-stable: - [`mem::MaybeUninit::as_ptr`][muninit_ptr] - [`mem::MaybeUninit::assume_init`][muninit_init] - [`mem::MaybeUninit::assume_init_ref`][muninit_init_ref] - [`ffi::CStr::from_bytes_with_nul_unchecked`][cstr_from_bytes] Cargo ----- - [Stabilize the `strip` profile option][cargo/10088] - [Stabilize future-incompat-report][cargo/10165] - [Support abbreviating `--release` as `-r`][cargo/10133] - [Support `term.quiet` configuration][cargo/10152] - [Remove `--host` from cargo {publish,search,login}][cargo/10145] Compatibility Notes ------------------- - [Refactor weak symbols in std::sys::unix][90846] This may add new, versioned, symbols when building with a newer glibc, as the standard library uses weak linkage rather than dynamically attempting to load certain symbols at runtime. - [Deprecate crate_type and crate_name nested inside `#![cfg_attr]`][83744] This adds a future compatibility lint to supporting the use of cfg_attr wrapping either crate_type or crate_name specification within Rust files; it is recommended that users migrate to setting the equivalent command line flags. - [Remove effect of `#[no_link]` attribute on name resolution][92034] This may expose new names, leading to conflicts with preexisting names in a given namespace and a compilation failure. - [Cargo will document libraries before binaries.][cargo/10172] - [Respect doc=false in dependencies, not just the root crate][cargo/10201] - [Weaken guarantee around advancing underlying iterators in zip][83791] - [Make split_inclusive() on an empty slice yield an empty output][89825] - [Update std::env::temp_dir to use GetTempPath2 on Windows when available.][89999] - [unreachable! was updated to match other formatting macro behavior on Rust 2021][92137] Internal Changes ---------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools. - [Fix many cases of normalization-related ICEs][91255] - [Replace dominators algorithm with simple Lengauer-Tarjan][85013] - [Store liveness in interval sets for region inference][90637] - [Remove `in_band_lifetimes` from the compiler and standard library, in preparation for removing this unstable feature.][91867] [91867]: rust-lang/rust#91867 [83744]: rust-lang/rust#83744 [83791]: rust-lang/rust#83791 [85013]: rust-lang/rust#85013 [89825]: rust-lang/rust#89825 [89999]: rust-lang/rust#89999 [90128]: rust-lang/rust#90128 [90207]: rust-lang/rust#90207 [90521]: rust-lang/rust#90521 [90586]: rust-lang/rust#90586 [90637]: rust-lang/rust#90637 [90833]: rust-lang/rust#90833 [90846]: rust-lang/rust#90846 [91003]: rust-lang/rust#91003 [91172]: rust-lang/rust#91172 [91255]: rust-lang/rust#91255 [91284]: rust-lang/rust#91284 [91535]: rust-lang/rust#91535 [91593]: rust-lang/rust#91593 [91728]: rust-lang/rust#91728 [91878]: rust-lang/rust#91878 [91896]: rust-lang/rust#91896 [91926]: rust-lang/rust#91926 [91984]: rust-lang/rust#91984 [92020]: rust-lang/rust#92020 [92034]: rust-lang/rust#92034 [92483]: rust-lang/rust#92483 [cargo/10088]: rust-lang/cargo#10088 [cargo/10133]: rust-lang/cargo#10133 [cargo/10145]: rust-lang/cargo#10145 [cargo/10152]: rust-lang/cargo#10152 [cargo/10165]: rust-lang/cargo#10165 [cargo/10172]: rust-lang/cargo#10172 [cargo/10201]: rust-lang/cargo#10201 [cargo/10269]: rust-lang/cargo#10269 [cstr_from_bytes]: https://doc.rust-lang.org/stable/std/ffi/struct.CStr.html#method.from_bytes_with_nul_unchecked [muninit_ptr]: https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.as_ptr [muninit_init]: https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.assume_init [muninit_init_ref]: https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#method.assume_init_ref [unwindsafe_once]: https://doc.rust-lang.org/stable/std/sync/struct.Once.html#impl-UnwindSafe [refunwindsafe_once]: https://doc.rust-lang.org/stable/std/sync/struct.Once.html#impl-RefUnwindSafe [tryfrom_ref_arr]: https://doc.rust-lang.org/stable/std/convert/trait.TryFrom.html#impl-TryFrom%3C%26%27_%20mut%20%5BT%5D%3E [lowercase]: https://doc.rust-lang.org/stable/std/char/struct.ToLowercase.html#impl-DoubleEndedIterator [uppercase]: https://doc.rust-lang.org/stable/std/char/struct.ToUppercase.html#impl-DoubleEndedIterator [try_from_char_err]: https://doc.rust-lang.org/stable/std/char/struct.TryFromCharError.html [available_parallelism]: https://doc.rust-lang.org/stable/std/thread/fn.available_parallelism.html [result-copied]: https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.copied [result-cloned]: https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.cloned [asm]: https://doc.rust-lang.org/stable/core/arch/macro.asm.html [global_asm]: https://doc.rust-lang.org/stable/core/arch/macro.global_asm.html [is_break]: https://doc.rust-lang.org/stable/std/ops/enum.ControlFlow.html#method.is_break [is_continue]: https://doc.rust-lang.org/stable/std/ops/enum.ControlFlow.html#method.is_continue [try_from_char_u8]: https://doc.rust-lang.org/stable/std/primitive.char.html#impl-TryFrom%3Cchar%3E [zip]: https://doc.rust-lang.org/stable/std/iter/fn.zip.html [is_power_of_two8]: https://doc.rust-lang.org/stable/core/num/struct.NonZeroU8.html#method.is_power_of_two [is_power_of_two16]: https://doc.rust-lang.org/stable/core/num/struct.NonZeroU16.html#method.is_power_of_two [is_power_of_two32]: https://doc.rust-lang.org/stable/core/num/struct.NonZeroU32.html#method.is_power_of_two [is_power_of_two64]: https://doc.rust-lang.org/stable/core/num/struct.NonZeroU64.html#method.is_power_of_two [is_power_of_two128]: https://doc.rust-lang.org/stable/core/num/struct.NonZeroU128.html#method.is_power_of_two [stdarch/1266]: rust-lang/stdarch#1266

GuillaumeGomez approved these changes Nov 24, 2021
View reviewed changes

rust-highfive assigned petrochenkov Nov 24, 2021

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 24, 2021

jyn514 added A-attributes Area: Attributes (`#[…]`, `#![…]`) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 24, 2021

petrochenkov reviewed Nov 25, 2021
View reviewed changes

Comment threadcompiler/rustc_builtin_macros/src/test.rs Outdated
Comment threadcompiler/rustc_builtin_macros/src/test.rs Outdated

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 Nov 25, 2021

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 15, 2021

petrochenkov reviewed Dec 15, 2021
View reviewed changes

Comment threadcompiler/rustc_builtin_macros/src/util.rs Outdated
Comment threadcompiler/rustc_session/src/session.rs Outdated
Comment threadcompiler/rustc_lint_defs/src/builtin.rs Outdated

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 Dec 15, 2021

This comment has been minimized.
Sign in to view

Add a lint for duplicated attributes.
2be94d4

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2021

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 Dec 16, 2021

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 16, 2021
Rollup merge ofrust-lang#91172- Ethiraric:ethiraric/fix90979, r=pet…
5710b62
…rochenkov Warn when a `#[test]`-like built-in attribute macro is present multiple times. Fixesrust-lang#90979.

matthiaskrgr mentioned this pull request Dec 16, 2021
Rollup of 6 pull requests #91995
Closed

matthiaskrgr mentioned this pull request Dec 16, 2021
Rollup of 7 pull requests #92003
Merged

bors merged commit 7407c4e into rust-lang:masterDec 17, 2021

rustbot added this to the 1.59.0 milestone Dec 17, 2021

coderabbitaiBot mentioned this pull request Jul 22, 2026
perf(xtask): build ripr once and invoke the binary directly in fixtures and dogfood (#2110) EffortlessMetrics/ripr-swarm#2176
Merged

Uh oh!

Uh oh!

Conversation

Ethiraric commented Nov 24, 2021

Uh oh!

GuillaumeGomez commented Nov 24, 2021

Uh oh!

Milo123459 commented Nov 24, 2021

Uh oh!

Ethiraric commented Nov 24, 2021

Uh oh!

Milo123459 commented Nov 24, 2021

Uh oh!

Ethiraric commented Nov 24, 2021

Uh oh!

GuillaumeGomez commented Nov 24, 2021

Uh oh!

Milo123459 commented Nov 24, 2021

Uh oh!

lcnr commented Nov 24, 2021

Uh oh!

nagisa commented Nov 24, 2021

Uh oh!

GuillaumeGomez commented Nov 24, 2021

Uh oh!

Ethiraric commented Nov 24, 2021

Uh oh!

ehuss commented Nov 24, 2021

Uh oh!

Ethiraric commented Nov 24, 2021

Uh oh!

ehuss commented Nov 24, 2021

Uh oh!

Ethiraric commented Nov 24, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehuss commented Nov 24, 2021

Uh oh!

Ethiraric commented Nov 24, 2021

Uh oh!

GuillaumeGomez commented Nov 24, 2021

Uh oh!

ehuss commented Nov 24, 2021

Uh oh!

GuillaumeGomez commented Nov 24, 2021

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Nov 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ethiraric commented Nov 25, 2021

Uh oh!

Ethiraric commented Dec 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Dec 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ethiraric commented Dec 15, 2021

Uh oh!

This comment has been minimized.

petrochenkov commented Dec 16, 2021

Uh oh!

bors commented Dec 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Ethiraric commented Nov 24, 2021•
edited
Loading

petrochenkov commented Nov 25, 2021•
edited
Loading

petrochenkov commented Dec 15, 2021•
edited
Loading