Add must_use annotations to Result::is_ok and is_err #59648
Merged
Conversation
This file contains 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
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
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 |
alexcrichton
added
the
T-libs-api
|
@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. |
rfcbot
added
the
proposed-final-comment-period
rfcbot
added
the
disposition-merge
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
added
final-comment-period
|
@bors: r+ |
|
📌 Commit ce5d694 has been approved by |
bors
added
S-waiting-on-bors
RalfJung
reviewed
Apr 15, 2019
| @@ -178,6 +178,7 @@ impl<T> Option<T> { | |||
| /// ``` | |||
| /// | |||
| /// [`Some`]: #variant.Some | |||
| #[must_use] | |||
There was a problem hiding this comment.
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.
Agreed! Maybe with a pointer to unwrap or expect if people meant it to be an assertion?
There was a problem hiding this comment.
Did this ever get resolved? If not someone should open a pr for it.
There was a problem hiding this comment.
Nope, there's still no message. Can you open an issue or a PR?
There was a problem hiding this comment.
I would but I honestly have no clue what to put for the message.
There was a problem hiding this comment.
@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.
I'll open a PR based on those messages tonight if someone else doesn't do it first.
|
⌛ Testing commit ce5d694 with merge 04247b041cdc0a35151d9232d23ced24341bbb95... |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Apr 15, 2019
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. |
bors
added a commit
that referenced
this pull request
Apr 15, 2019
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
rfcbot
added
finished-final-comment-period
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Jul 5, 2019
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discussed in #59610