de-stabilize bench attribute #134273

RalfJung · 2024-12-13T17:33:03Z

This has been soft-unstable since forever (#64066), and shown in future-compat reports since Rust 1.77 (#116274).

The feature covering bench itself is tracked in #50297, which has been closed despite still having active feature gates referencing it.

Cc @rust-lang/libs-api

rustbot · 2024-12-13T17:33:13Z

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 · 2024-12-13T17:50:49Z

Hm actually I guess this is more of a lang thing, sorry... Cc @rust-lang/lang

RalfJung · 2024-12-13T19:10:29Z

@ehuss what is the right thing to do with a lint that is not emitted any more by the compiler, but is kept around since it might be emitted again in the future? I don't think it makes sense to have an example in this lint, since we'll keep changing when the lint is emitted (as some soft-unstable items become real-unstable, and other stable items become soft-unstable).

ehuss · 2024-12-14T01:13:17Z

What you've done looks good to me.

RalfJung · 2024-12-20T10:06:28Z

Maybe we should crater this as well, since it does have the potential of breaking stable code.
@bors try

de-stabilize bench attribute This has been soft-unstable since forever (rust-lang#64066), and shown in future-compat reports since Rust 1.77 (rust-lang#116274). The feature covering `bench` itself is tracked in rust-lang#50297, which has been closed despite still having active feature gates referencing it. Cc `@rust-lang/libs-api`

bors · 2024-12-20T10:07:43Z

⌛ Trying commit f5c63b9 with merge 2b41547...

bors · 2024-12-20T11:53:45Z

☀️ Try build successful - checks-actions
Build commit: 2b41547 (2b41547e41f63ea0ca9cb25417ddd569b19e6a50)

RalfJung · 2025-01-04T21:35:13Z

@craterbot check

craterbot · 2025-01-04T21:35:30Z

👌 Experiment pr-134273 created and queued.
🤖 Automatically detected try build 2b41547
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-01-05T15:27:22Z

🚧 Experiment pr-134273 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-01-06T12:04:01Z

🎉 Experiment pr-134273 is completed!
📊 161 regressed and 28 fixed (563652 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

RalfJung · 2025-03-19T15:57:13Z

@rust-lang/lang 4 months have passed, any thoughts on this? :)

traviscross · 2025-03-19T19:48:51Z

Thanks for the ping. Our design meetings for the next four weeks are all focused on bringing down the queue, so we should get to this.

This repo originally used the #[bench] test framework which is unstable and has started throwing clippy warnings which will soon become a hard error: rust-lang/rust#134273 This MR moves to Criterion for benchmarking, similar to dalek-ed25519 for the dleq benchmarking but removes benchmarking for the define_proof macro. This should be revisited and is noted in lox-zkp#2

traviscross · 2025-04-23T17:51:23Z

We discussed this in the lang call today. Let's propose to do this.

@rfcbot fcp merge

rfcbot · 2025-04-23T17:51:25Z

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

rfcbot · 2025-04-23T17:57:02Z

🔔 This is now entering its final comment period, as per the review above. 🔔

joshtriplett · 2025-04-23T17:57:36Z

Relatedly, for anyone who is looking at this later on: We should consider having good a first-party benchmarking framework, for the same reason that we have a first-party test framework. Our first-party test framework has made testing extremely widespread in the Rust ecosystem, and benchmarking would have much the same effect. Right now, benchmarking requires third-party frameworks like divan or criterion.

Thoughts for folks working on libtest, and/or the folks working on benchmarking frameworks. :)

sammysheep · 2025-05-02T02:26:36Z

If one is using nightly bencher with feature "test", will this change require any new features to continue using it?

sammysheep · 2025-05-02T02:39:35Z

@joshtriplett We have used all three (and more) and while each has their strong point, we find ourselves using the modest but still very good nightly bencher. The ease and lightweight nature of first party tooling makes you want to use it again and again. It provides rapid feedback and an immediate direction.

For final decisions we examine assembly on Godbolt and/or run an integrated benchmark with real world data using hyperfine to gain clarity.

First party tooling is not the end; it is an important beginning.

RalfJung · 2025-05-02T06:29:08Z

If one is using nightly bencher with feature "test", will this change require any new features to continue using it?

No, feature(test) is how you enable #[bench].

rfcbot · 2025-05-03T17:57:28Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

RalfJung · 2025-05-04T10:35:40Z

@ibraheemdev the team has approved, this is waiting for review now. :)
@rustbot ready

traviscross · 2025-05-05T04:09:11Z

compiler/rustc_lint_defs/src/builtin.rs

 declare_lint! {
-    /// The `soft_unstable` lint detects unstable features that were
-    /// unintentionally allowed on stable.
-    ///
-    /// ### Example
-    ///
-    /// ```rust,compile_fail
-    /// #[cfg(test)]
-    /// extern crate test;
-    ///
-    /// #[bench]
-    /// fn name(b: &mut test::Bencher) {
-    ///     b.iter(|| 123)
-    /// }
-    /// ```
-    ///
-    /// {{produces}}
-    ///
-    /// ### Explanation
-    ///
-    /// The [`bench` attribute] was accidentally allowed to be specified on
-    /// the [stable release channel]. Turning this to a hard error would have
-    /// broken some projects. This lint allows those projects to continue to
-    /// build correctly when [`--cap-lints`] is used, but otherwise signal an
-    /// error that `#[bench]` should not be used on the stable channel. This
-    /// is a [future-incompatible] lint to transition this to a hard error in
-    /// the future. See [issue #64266] for more details.
+    /// The `soft_unstable` lint detects unstable features that were unintentionally allowed on
+    /// stable. This is a [future-incompatible] lint to transition this to a hard error in the
+    /// future. See [issue #64266] for more details.
    ///
    /// [issue #64266]: https://github.com/rust-lang/rust/issues/64266
-    /// [`bench` attribute]: https://doc.rust-lang.org/nightly/unstable-book/library-features/test.html
-    /// [stable release channel]: https://doc.rust-lang.org/book/appendix-07-nightly-rust.html
-    /// [`--cap-lints`]: https://doc.rust-lang.org/rustc/lints/levels.html#capping-lints
    /// [future-incompatible]: ../index.md#future-incompatible-lints
    pub SOFT_UNSTABLE,
    Deny,


Should we just be removing this lint and the infrastructure for it entirely? Over in #64266 (comment) you had said:

IMO if we want to de-stabilize something in the future, we should just have a new lint for each such case. I don't think it is a good idea to have one lint used as a grab-bag for a bunch of independent things; each de-stabilization needs to be handled on its own timeline anyway and might need its own guidance for how to adjust the code to make it still work.

Yeah I was wondering about that. But if we do this it should be a separate PR IMO.

traviscross

Modulo the question about whether we should just be removing the soft_unstable lint entirely, which I'm OK with either way, this impl looks OK to me, so r=me along with the review from @ibraheemdev.

RalfJung · 2025-05-05T06:28:08Z

so r=me along with the review from @ibraheemdev.

There's no (existing) review by them that I can find? So I don't understand how to interpret this statement. Usually "r=me" means "feel free to land this with me as approver", but then the "along with the review" part makes it sound like you don't want me to land this with your review yet -- so then what does this "r=me" mean if we are either way still waiting for full review by @ibraheemdev ? Seems like ether you reviewed this and we can land it based on this or not, but your message fits neither of those cases.^^

traviscross · 2025-05-05T11:01:46Z

I mean that @ibraheemdev can include me in the r= along with his review, or that after he also gives his r=me, that I can be included in the final r= by whoever does it. I.e., count me as an approval, but wait for his review as well.

rustbot assigned ibraheemdev Dec 13, 2024

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 Dec 13, 2024

RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 13, 2024

This comment has been minimized.

Sign in to view

de-stabilize bench attribute

8e599b0

RalfJung force-pushed the de-stabilize-bench branch from a35de38 to 8e599b0 Compare December 13, 2024 18:16

This comment has been minimized.

Sign in to view

the soft_unstable lint cannot have an example that will keep working

f5c63b9

RalfJung added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 14, 2024

RalfJung mentioned this pull request Dec 14, 2024

Tracking issue for future-incompatibility lint soft_unstable #64266

Open

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 6, 2025

ibraheemdev added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 31, 2025

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 23, 2025

traviscross added I-lang-radar Items that are on lang's radar and will need eventual work or consideration. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels Apr 23, 2025

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 23, 2025

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 3, 2025

rustbot mentioned this pull request May 3, 2025

Tracking issue for release notes of #134273: de-stabilize bench attribute #140629

Open

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2025

RalfJung removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 4, 2025

traviscross reviewed May 5, 2025

View reviewed changes

traviscross approved these changes May 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

de-stabilize bench attribute #134273

de-stabilize bench attribute #134273

RalfJung commented Dec 13, 2024

rustbot commented Dec 13, 2024

RalfJung commented Dec 13, 2024

This comment has been minimized.

This comment has been minimized.

RalfJung commented Dec 13, 2024

ehuss commented Dec 14, 2024

RalfJung commented Dec 20, 2024

bors commented Dec 20, 2024

bors commented Dec 20, 2024

RalfJung commented Jan 4, 2025

craterbot commented Jan 4, 2025

craterbot commented Jan 5, 2025

craterbot commented Jan 6, 2025

RalfJung commented Mar 19, 2025

traviscross commented Mar 19, 2025

traviscross commented Apr 23, 2025

rfcbot commented Apr 23, 2025 •

edited by joshtriplett

Loading

rfcbot commented Apr 23, 2025

joshtriplett commented Apr 23, 2025

sammysheep commented May 2, 2025

sammysheep commented May 2, 2025

RalfJung commented May 2, 2025

rfcbot commented May 3, 2025

RalfJung commented May 4, 2025

traviscross May 5, 2025 •

edited

Loading

RalfJung May 5, 2025

traviscross left a comment

RalfJung commented May 5, 2025 •

edited

Loading

traviscross commented May 5, 2025 •

edited

Loading

de-stabilize bench attribute #134273

Are you sure you want to change the base?

de-stabilize bench attribute #134273

Conversation

RalfJung commented Dec 13, 2024

rustbot commented Dec 13, 2024

RalfJung commented Dec 13, 2024

This comment has been minimized.

This comment has been minimized.

RalfJung commented Dec 13, 2024

ehuss commented Dec 14, 2024

RalfJung commented Dec 20, 2024

bors commented Dec 20, 2024

bors commented Dec 20, 2024

RalfJung commented Jan 4, 2025

craterbot commented Jan 4, 2025

craterbot commented Jan 5, 2025

craterbot commented Jan 6, 2025

RalfJung commented Mar 19, 2025

traviscross commented Mar 19, 2025

traviscross commented Apr 23, 2025

rfcbot commented Apr 23, 2025 • edited by joshtriplett Loading

rfcbot commented Apr 23, 2025

joshtriplett commented Apr 23, 2025

sammysheep commented May 2, 2025

sammysheep commented May 2, 2025

RalfJung commented May 2, 2025

rfcbot commented May 3, 2025

RalfJung commented May 4, 2025

traviscross May 5, 2025 • edited Loading

Choose a reason for hiding this comment

RalfJung May 5, 2025

Choose a reason for hiding this comment

traviscross left a comment

Choose a reason for hiding this comment

RalfJung commented May 5, 2025 • edited Loading

traviscross commented May 5, 2025 • edited Loading

rfcbot commented Apr 23, 2025 •

edited by joshtriplett

Loading

traviscross May 5, 2025 •

edited

Loading

RalfJung commented May 5, 2025 •

edited

Loading

traviscross commented May 5, 2025 •

edited

Loading