-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add must_use annotations to Result::is_ok and is_err #59648
Conversation
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This seems to have caught several cases in some tests, e.g. https://github.com/rust-lang/rust/blob/master/src/librustc_data_structures/owning_ref/mod.rs#L1474 I think that's missing an |
If this is worth it on (Or I wonder if it would make sense to have a special lint for unused |
@alex want to fix the issues found on CI? It seems reasonable to have similar treatment on |
@alexcrichton Yes, I'd like to! I could use some feedback on the question I asked WRT these tests: https://github.com/rust-lang/rust/blob/master/src/librustc_data_structures/owning_ref/mod.rs#L1467-L1485
|
I don't know myself (didn't write those tests) but it seems harmless to either throw in |
👍 will update to get these tests passing this evening. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@alex did you want to add similar treatment to |
If you'd prefer it as a part of this PR, I'm happy to. |
Yes since they're so similar in purpose let's go ahead and add them in this PR |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Another failing test, again I'm not sure I fully understand what this one is trying to test. I think |
@rfcbot fcp merge |
Team member @alexcrichton 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! 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. 🔔 |
@bors: r+ |
📌 Commit ce5d694 has been approved by |
@@ -178,6 +178,7 @@ impl<T> Option<T> { | |||
/// ``` | |||
/// | |||
/// [`Some`]: #variant.Some | |||
#[must_use] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good idea to add a message here (must_use = "msg"
) explaining that this function has no side-effects, or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! Maybe with a pointer to unwrap
or expect
if people meant it to be an assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this ever get resolved? If not someone should open a pr for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, there's still no message. Can you open an issue or a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would but I honestly have no clue what to put for the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czipperz Well, inspired by https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.collect, here's a proposal for the text:
Result::is_ok
: if you intended to assert that this matchesOk(_)
, consider.unwrap()
insteadResult::is_err
: if you intended to assert that this matchesErr(_)
, consider.unwrap_err()
insteadOption::is_some
: if you intended to assert that this matchesSome(_)
, consider.unwrap()
insteadOption::is_none
: if you intended to assert that this matchesNone
, consider.or_else(|| panic!("called `Option::unwrap()` on a `None` value"))
instead
(That last one makes me think of https://internals.rust-lang.org/t/adding-option-expect-none/10481?u=scottmcm...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a PR based on those messages tonight if someone else doesn't do it first.
⌛ Testing commit ce5d694 with merge 04247b041cdc0a35151d9232d23ced24341bbb95... |
Add must_use annotations to Result::is_ok and is_err Discussed in rust-lang#59610
@bors retry Yielding in favor of r0llup this is included in. |
Rollup of 6 pull requests Successful merges: - #59648 (Add must_use annotations to Result::is_ok and is_err) - #59748 (Add summary and reference to Rust trademark guide) - #59779 (Uplift `get_def_path` from Clippy) - #59955 (bump stdsimd; make intra_doc_link_resolution_failure an error again; make lints more consistent) - #59978 (rustdoc: Remove default keyword from re-exported trait methods) - #59989 (Fix links to Atomic* in RELEASES.md) Failed merges: r? @ghost
Pkgsrc changes: * NetBSD/sparc64 disabling of "packed" removed ("packed" removed upstream) * Adapt src_libstd_build.rs patch, update sed'ing of .cargo-checksum.json Build verified on NetBSD 8.0/amd64. Upstream changes: Version 1.36.0 (2019-07-04) ========================== Language -------- - [Non-Lexical Lifetimes are now enabled on the 2015 edition.][59114] - [The order of traits in trait objects no longer affects the semantics of that object.][59445] e.g. `dyn Send + fmt::Debug` is now equivalent to `dyn fmt::Debug + Send`, where this was previously not the case. Libraries --------- - [`HashMap`'s implementation has been replaced with `hashbrown::HashMap` implem entation.][58623] - [`TryFromSliceError` now implements `From<Infallible>`.][60318] - [`mem::needs_drop` is now available as a const fn.][60364] - [`alloc::Layout::from_size_align_unchecked` is now available as a const fn.][6 0370] - [`String` now implements `BorrowMut<str>`.][60404] - [`io::Cursor` now implements `Default`.][60234] - [Both `NonNull::{dangling, cast}` are now const fns.][60244] - [The `alloc` crate is now stable.][59675] `alloc` allows you to use a subset of `std` (e.g. `Vec`, `Box`, `Arc`) in `#![no_std]` environments if the environment has access to heap memory allocation. - [`String` now implements `From<&String>`.][59825] - [You can now pass multiple arguments to the `dbg!` macro.][59826] `dbg!` will return a tuple of each argument when there is multiple arguments. - [`Result::{is_err, is_ok}` are now `#[must_use]` and will produce a warning if not used.][59648] Stabilized APIs --------------- - [`VecDeque::rotate_left`] - [`VecDeque::rotate_right`] - [`Iterator::copied`] - [`io::IoSlice`] - [`io::IoSliceMut`] - [`Read::read_vectored`] - [`Write::write_vectored`] - [`str::as_mut_ptr`] - [`mem::MaybeUninit`] - [`pointer::align_offset`] - [`future::Future`] - [`task::Context`] - [`task::RawWaker`] - [`task::RawWakerVTable`] - [`task::Waker`] - [`task::Poll`] Cargo ----- - [Cargo will now produce an error if you attempt to use the name of a required dependency as a feature.][cargo/6860] - [You can now pass the `--offline` flag to run cargo without accessing the netw ork.][cargo/6934] You can find further change's in [Cargo's 1.36.0 release notes][cargo-1-36-0]. Clippy ------ There have been numerous additions and fixes to clippy, see [Clippy's 1.36.0 rel ease notes][clippy-1-36-0] for more details. Misc ---- Compatibility Notes ------------------- - [`std::arch::x86::_rdtsc` returns `u64` instead of `i64`][stdsimd/559] - [`std::arch::x86_64::_mm_shuffle_ps` takes an `i32` instead of `u32` for `mask `][stdsimd/522] - With the stabilisation of `mem::MaybeUninit`, `mem::uninitialized` use is no longer recommended, and will be deprecated in 1.38.0. [60318]: rust-lang/rust#60318 [60364]: rust-lang/rust#60364 [60370]: rust-lang/rust#60370 [60404]: rust-lang/rust#60404 [60234]: rust-lang/rust#60234 [60244]: rust-lang/rust#60244 [58623]: rust-lang/rust#58623 [59648]: rust-lang/rust#59648 [59675]: rust-lang/rust#59675 [59825]: rust-lang/rust#59825 [59826]: rust-lang/rust#59826 [59445]: rust-lang/rust#59445 [59114]: rust-lang/rust#59114 [cargo/6860]: rust-lang/cargo#6860 [cargo/6934]: rust-lang/cargo#6934 [`VecDeque::rotate_left`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_left [`VecDeque::rotate_right`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_right [`Iterator::copied`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#tymethod.copied [`io::IoSlice`]: https://doc.rust-lang.org/std/io/struct.IoSlice.html [`io::IoSliceMut`]: https://doc.rust-lang.org/std/io/struct.IoSliceMut.html [`Read::read_vectored`]: https://doc.rust-lang.org/std/io/trait.Read.html#method.read_vectored [`Write::write_vectored`]: https://doc.rust-lang.org/std/io/trait.Write.html#method.write_vectored [`str::as_mut_ptr`]: https://doc.rust-lang.org/std/primitive.str.html#method.as_mut_ptr [`mem::MaybeUninit`]: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html [`pointer::align_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset [`future::Future`]: https://doc.rust-lang.org/std/future/trait.Future.html [`task::Context`]: https://doc.rust-lang.org/beta/std/task/struct.Context.html [`task::RawWaker`]: https://doc.rust-lang.org/beta/std/task/struct.RawWaker.html [`task::RawWakerVTable`]: https://doc.rust-lang.org/beta/std/task/struct.RawWakerVTable.html [`task::Waker`]: https://doc.rust-lang.org/beta/std/task/struct.Waker.html [`task::Poll`]: https://doc.rust-lang.org/beta/std/task/enum.Poll.html [clippy-1-36-0]: https://github.com/rust-lang/rust-clippy/blob/master/CHANGELOG.md#rust-136 [cargo-1-36-0]: https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-136-2019-07-04 [stdsimd/522]: rust-lang/stdarch#522 [stdsimd/559]: rust-lang/stdarch#559
Discussed in #59610