de-stabilize bench attribute by RalfJung · Pull Request #134273 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Hm actually I guess this is more of a lang thing, sorry... Cc @rust-lang/lang
Relevant to the language team, which will review and decide on the PR/issue.
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
This comment has been minimized.
This comment has been minimized.
@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).
What you've done looks good to me.
Maybe we should crater this as well, since it does have the potential of breaking stable code.
@bors try
bors added a commit to rust-lang-ci/rust that referenced this pull request
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
☀️ Try build successful - checks-actions
Build commit: 2b41547 (2b41547e41f63ea0ca9cb25417ddd569b19e6a50)
🚧 Experiment pr-134273 is now running
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
@rust-lang/lang 4 months have passed, any thoughts on this? :)
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.
Chausseaumoine pushed a commit to Chausseaumoine/lox-zkp-fork that referenced this pull request
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
We discussed this in the lang call today. Let's propose to do this.
@rfcbot fcp merge
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.
🔔 This is now entering its final comment period, as per the review above. 🔔
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. :)
If one is using nightly bencher with feature "test", will this change require any new features to continue using it?
@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.
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].
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.
Comment on lines 2391 to 2399
| 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(| |
| /// } |
| /// ``` |
| /// |
| /// {{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, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wondering about that. But if we do this it should be a separate PR IMO.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.^^
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.
Labels
This issue / PR is in PFCP or FCP with a disposition to merge it.
The final comment period is finished for this PR / Issue.
Items that are on lang's radar and will need eventual work or consideration.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the language team, which will review and decide on the PR/issue.
Announce this issue on triage meeting