Skip to content
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

Merged
merged 1 commit into from
Apr 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libcore/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ impl<T> Option<T> {
/// ```
///
/// [`Some`]: #variant.Some
#[must_use]
Copy link
Member

@RalfJung RalfJung Apr 15, 2019

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?

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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 matches Ok(_), consider .unwrap() instead
  • Result::is_err: if you intended to assert that this matches Err(_), consider .unwrap_err() instead
  • Option::is_some: if you intended to assert that this matches Some(_), consider .unwrap() instead
  • Option::is_none: if you intended to assert that this matches None, 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...)

Copy link
Contributor

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.

#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn is_some(&self) -> bool {
Expand All @@ -200,6 +201,7 @@ impl<T> Option<T> {
/// ```
///
/// [`None`]: #variant.None
#[must_use]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn is_none(&self) -> bool {
Expand Down
2 changes: 2 additions & 0 deletions src/libcore/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ impl<T, E> Result<T, E> {
/// let x: Result<i32, &str> = Err("Some error message");
/// assert_eq!(x.is_ok(), false);
/// ```
#[must_use]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn is_ok(&self) -> bool {
Expand All @@ -301,6 +302,7 @@ impl<T, E> Result<T, E> {
/// let x: Result<i32, &str> = Err("Some error message");
/// assert_eq!(x.is_err(), true);
/// ```
#[must_use]
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn is_err(&self) -> bool {
Expand Down
12 changes: 6 additions & 6 deletions src/librustc_data_structures/owning_ref/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,7 @@ mod tests {
let x = Box::new(123_i32);
let y: Box<dyn Any> = x;

OwningRef::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_ok();
assert!(OwningRef::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_ok());
}

#[test]
Expand All @@ -1481,7 +1481,7 @@ mod tests {
let x = Box::new(123_i32);
let y: Box<dyn Any> = x;

OwningRef::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_err();
assert!(!OwningRef::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_err());
}
}

Expand Down Expand Up @@ -1868,7 +1868,7 @@ mod tests {
let x = Box::new(123_i32);
let y: Box<dyn Any> = x;

OwningRefMut::new(y).try_map_mut(|x| x.downcast_mut::<i32>().ok_or(())).is_ok();
assert!(OwningRefMut::new(y).try_map_mut(|x| x.downcast_mut::<i32>().ok_or(())).is_ok());
}

#[test]
Expand All @@ -1878,7 +1878,7 @@ mod tests {
let x = Box::new(123_i32);
let y: Box<dyn Any> = x;

OwningRefMut::new(y).try_map_mut(|x| x.downcast_mut::<i32>().ok_or(())).is_err();
assert!(!OwningRefMut::new(y).try_map_mut(|x| x.downcast_mut::<i32>().ok_or(())).is_err());
}

#[test]
Expand All @@ -1888,7 +1888,7 @@ mod tests {
let x = Box::new(123_i32);
let y: Box<dyn Any> = x;

OwningRefMut::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_ok();
assert!(OwningRefMut::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_ok());
}

#[test]
Expand All @@ -1898,7 +1898,7 @@ mod tests {
let x = Box::new(123_i32);
let y: Box<dyn Any> = x;

OwningRefMut::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_err();
assert!(!OwningRefMut::new(y).try_map(|x| x.downcast_ref::<i32>().ok_or(())).is_err());
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/sync/mpsc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,7 @@ impl<T> SyncSender<T> {
/// thread::spawn(move || {
/// // This will return an error and send
/// // no message if the buffer is full
/// sync_sender2.try_send(3).is_err();
/// let _ = sync_sender2.try_send(3);
/// });
///
/// let mut msg;
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-pass/issues/issue-18353.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ struct Str {

fn main() {
let str: Option<&Str> = None;
str.is_some();
let _ = str.is_some();
}