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 messages to `Option`'s and `Result`'s `must_use` annotation for `is_*` #62431

Merged
merged 2 commits into from Jul 13, 2019

Conversation

@czipperz
Copy link
Contributor

commented Jul 6, 2019

@czipperz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@@ -201,7 +201,8 @@ impl<T> Option<T> {
/// ```
///
/// [`None`]: #variant.None
#[must_use]
#[must_use = "if you intended to assert that this doesn't have a value, consider \
`.and_then(|| panic!(\"`Option` had a value when expected `None`\"))` instead"]

This comment has been minimized.

Copy link
@czipperz

czipperz Jul 6, 2019

Author Contributor

I think this message is too long. How about the suggestion is .ok_or(()).unwrap_err()?

This comment has been minimized.

Copy link
@scottmcm

scottmcm Jul 6, 2019

Member

I don't have a strong opinion; I just wanted to give a message as nice as the .unwrap() ones do.

Maybe there's a way to combine them? .ok_or("Option::None").unwrap_err() or similar?

This comment has been minimized.

Copy link
@czipperz

czipperz Jul 7, 2019

Author Contributor

Can we just tell the user to wrap it in an assert!()?

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 10, 2019

Member

Really we should add an expect_none.

This comment has been minimized.

Copy link
@cuviper
@scottmcm

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

I agree that unwrap_none is what this wants to mention, but it can't until that stabilizes, so I think these messages are a solid improvement for now. We can always keep improving them in future PRs.

r? @scottmcm

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

📌 Commit 9b0623d has been approved by scottmcm

@rust-highfive rust-highfive assigned scottmcm and unassigned RalfJung Jul 11, 2019

@rust-lang rust-lang deleted a comment from bors Jul 11, 2019

@rust-lang rust-lang deleted a comment from bors Jul 11, 2019

Centril added a commit to Centril/rust that referenced this pull request Jul 12, 2019

Rollup merge of rust-lang#62431 - czipperz:add-messages-to-must-use-i…
…s_-methods, r=scottmcm

Add messages to `Option`'s and `Result`'s `must_use` annotation for `is_*`

r? @RalfJung

bors added a commit that referenced this pull request Jul 12, 2019

Auto merge of #62635 - Centril:rollup-potvfnk, r=Centril
Rollup of 12 pull requests

Successful merges:

 - #61535 (Coherence test when a generic type param has a default value from an associated type)
 - #62274 (rustc_mir: follow FalseUnwind's real_target edge in qualify_consts.)
 - #62431 (Add messages to `Option`'s and `Result`'s `must_use` annotation for `is_*`)
 - #62453 (in which we suggest anonymizing single-use lifetimes in paths )
 - #62568 (Replace unsafe_destructor_blind_to_params with may_dangle)
 - #62578 (Add test for #49919)
 - #62595 (Document that the crate keyword refers to the project root)
 - #62599 (move mem::uninitialized deprecation back by 1 release, to 1.39)
 - #62605 (Emit dropped unemitted errors to aid in ICE debugging)
 - #62607 (Correctly break out of recovery loop)
 - #62608 (`async unsafe fn` tests)
 - #62623 (downgrade indirect_structural_match lint to allow)

Failed merges:

r? @ghost

@bors bors merged commit 9b0623d into rust-lang:master Jul 13, 2019

3 checks passed

pr Build #20190706.10 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.