Replace unwrap calls in examples by expect

[GuillaumeGomez]

Because no one should use unwrap!

r? @steveklabnik

[euclio]

I think it's OK to use unwrap for things that trivially cannot fail, like CString::new() on a literal, because the expect doesn't add any additional information beyond "it failed". I think expect should be used in cases where additional explanation of why it cannot fail is needed, like expect("element was inserted above") instead of expect("get() failed").

[rust-highfive]

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/46/ad/28647c5e1f4bb4094af886e203cfde5543fafd6a5bf830a85909d2058f9f/awscli-1.15.42-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 10.7MB/s eta 0:00:01
    1% |▌                               | 20kB 1.9MB/s eta 0:00:01
    2% |▉                               | 30kB 2.2MB/s eta 0:00:01
    3% |█                               | 40kB 2.0MB/s eta 0:00:01
---
[01:27:15] 
[01:27:15] running 930 tests
[01:27:52] iii.................................................................................................
[01:28:12] ................................................................................................iii.
[01:28:25] .....i......i...i.F....i............................................................................
[01:28:53] ....................iiii........ii..................................................................
[01:29:02] ....................................................................................................
[01:29:20] ...................................................................iiii.............................
[01:29:47] ....................................................................................................
[01:29:47] ....................................................................................................
[01:29:58] ...............................................................................iiii.................
[01:30:03] ..............................
[01:30:03] failures:
[01:30:03] 
[01:30:03] ---- ffi/c_str.rs - ffi::c_str::CString::into_boxed_c_str (line 592) stdout ----
[01:30:03] error: expected one of `.`, `;`, `?`, or an operator, found `::`
[01:30:03]  --> ffi/c_str.rs:595:50
[01:30:03]   |
[01:30:03] 6 | let c_string = CString::new(b"foo".to_vec()).CStr::from_bytes_with_nul;
[01:30:03]   |                                                  ^^ expected one of `.`, `;`, `?`, or an operator here
[01:30:03] 
[01:30:03] error: unused imports: `CStr`, `CString`
[01:30:03]  --> ffi/c_str.rs:593:16
[01:30:03]   |
[01:30:03] 4 | use std::ffi::{CString, CStr};
[01:30:03]   |                ^^^^^^^  ^^^^
[01:30:03] note: lint level defined here
[01:30:03]  --> ffi/c_str.rs:590:9
[01:30:03]   |
[01:30:03] 1 | #![deny(warnings)]
---
[01:30:03] 
[01:30:03] error: test failed, to rerun pass '--doc'
[01:30:03] 
[01:30:03] 
[01:30:03] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:30:03] 
[01:30:03] 
[01:30:03] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:30:03] Build completed unsuccessfully in 0:40:13
[01:30:03] Build completed unsuccessfully in 0:40:13
[01:30:03] make: *** [check] Error 1
[01:30:03] Makefile:58: recipe for target 'check' failed
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2e1959a6
travis_time:start:2e1959a6
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:11c859b4
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

[GuillaumeGomez]

@euclio: Beyond than just use expect when needed, we should ensure that people use unwrap as little as possible.

[euclio]

Right, I understand that we should be discouraging unwrap. But, I think that unwrap has its uses, and a blanket ban in std documentation seems too heavy-handed to me.

The reason we want to discourage unwrap is twofold. First, you don't want to unwrap when you should be propagating or handling the error instead. std documentation already does this throughout, so it's not relevant to this PR. Second, unwrap doesn't provide any information about why we thought it was OK to unwrap in the case of failure. That's why expect exists: so we can display that reasoning. Using expect with a message like expect("func() failed") isn't much better than unwrap, because the only information it provides is that the method failed, which is what unwrap gives us. Instead, expect should contain the reasoning behind the expect, like I said above.

My second point is that if you're writing expect messages like that, then an expression like CString::new("foo").expect("0 byte found in literal") is redundant: obviously a 0-byte does not occur in the literal. At that point, the expect is just line noise, and I'd prefer unwrap.

[steveklabnik]

Yup, agreed with Andy here.

…

On Jun 21, 2018, at 9:18 AM, Andy Russell ***@***.***> wrote: Right, I understand that we should be discouraging unwrap. But, I think that unwrap has its uses, and a blanket ban in std documentation seems too heavy-handed to me. The reason we want to discourage unwrap is twofold. First, you don't want to unwrap when you should be propagating or handling the error instead. std documentation already does this throughout, so it's not relevant to this PR. Second, unwrap doesn't provide any information about why we thought it was OK to unwrap in the case of failure. That's why expect exists: so we can display that reasoning. Using expect with a message like expect("func() failed") isn't much better than unwrap, because the only information it provides is that the method failed, which is what unwrap gives us. Instead, expect should contain the reasoning behind the expect, like I said above. My second point is that if you're writing expect messages like that, then an expression like CString::new("foo").expect("0 byte found in literal") is redundant: obviously a 0-byte does not occur in the literal. At that point, the expect is just line noise, and I'd prefer unwrap. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

[GuillaumeGomez]

We agree on the fact that messages I wrote should be improved. My point was that, in any case, docs shouldn't provide examples with unwrap.

[TimNN]

Ping from triage, @GuillaumeGomez: what's the status of this?

[GuillaumeGomez]

Need to update.

[bors]

☔ The latest upstream changes (presumably #52486) made this pull request unmergeable. Please resolve the merge conflicts.

[pietroalbini]

Ping from triage @GuillaumeGomez! It's been a while since we heard from you, will you have time to work on this again?

[GuillaumeGomez]

Yep, don't know when but yep.

[pietroalbini]

Frendly weekly ping from triage @GuillaumeGomez! :)

[GuillaumeGomez]

I'll update it some day! :D

[XAMPPRocky]

Triage: @GuillaumeGomez Have you been able to get back to this?

[GuillaumeGomez]

Soon, it'll take a bit of time.

[GuillaumeGomez]

So, made few improvements on some wordings. However:

Right, I understand that we should be discouraging unwrap. But, I think that unwrap has its uses, and a blanket ban in std documentation seems too heavy-handed to me.

I don't agree with that statement for example: expect does exactly the same but in addition provides a message which is information. The more information the better, even more on a code written in a language which intends to have a reputation of never crashing (wether it's true or not).

Using expect with a message like expect("func() failed") isn't much better than unwrap, because the only information it provides is that the method failed, which is what unwrap gives us

Yes but it is more clear (and you can add additional information).

Again, this is all about giving good habits to users. Even a message like "fuck it" would be super useful to find where the error comes from (unless you have a few of them of course).

In any case, I tried to improve messages a bit but I don't know how I could make them better at this point so any suggestion is very welcome if you think they could be improved. :)

[euclio]

Again, this is all about giving good habits to users. Even a message like "fuck it" would be super useful to find where the error comes from (unless you have a few of them of course).

This is what the stack trace provides, no? Also, I would argue that using `expect` with a uninformative message is actually a bad habit.

…

On Sun, Aug 12, 2018, 3:04 AM Guillaume Gomez ***@***.***> wrote: So, made few improvements on some wordings. However: Right, I understand that we should be discouraging unwrap. But, I think that unwrap has its uses, and a blanket ban in std documentation seems too heavy-handed to me. I don't agree with that statement for example: expect does exactly the same but in addition provides a message which is *information*. The more information the better, even more on a code written in a language which intends to have a reputation of never crashing (wether it's true or not). Using expect with a message like expect("func() failed") isn't much better than unwrap, because the only information it provides is that the method failed, which is what unwrap gives us Yes but it is more clear (and you can add additional information). Again, this is all about giving good habits to users. Even a message like "fuck it" would be super useful to find where the error comes from (unless you have a few of them of course). In any case, I tried to improve messages a bit but I don't know how I could make them better at this point so any suggestion is very welcome if you think they could be improved. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51668 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTxFrGjK9I8UKcoA5lQFdU0s9SDoih5ks5uPxyxgaJpZM4UwF2R> .

[GuillaumeGomez]

This is what the stack trace provides, no?

Yes and no. Stack trace only indicates functions most of the time (it's already a good start but still lacking).

Also, I would argue that using expect with a uninformative message is actually a bad habit.

I think this is more a question of opinion in here. I want an error message to be informative whereas I want a panic message to be easy to be grepped so I can fix it because a panic should never happen on the opposite of an error message.

[pietroalbini]

Ping from triage @GuillaumeGomez! It's been a while since we heard from you, will you have time to work on this again?

[GuillaumeGomez]

Debate is still going on. :)

[TimNN]

Ping from triage @GuillaumeGomez / @steveklabnik. What is the status of this PR?

Is this blocked on some external issue?
Is this blocked on discussion in this PR?
Would it maybe help to split this PR into multiple smaller ones?
Should we nominate this for some team to discuss to resolve the discussion using "realtime communication" (rather than via comments every now and then)?

[GuillaumeGomez]

I don't know actually. The debate is "should expect sentences be more useful than easy to grep" basically.

[steveklabnik]

Would it maybe help to split this PR into multiple smaller ones?

I think so. From the first comment:

I think it's OK to use unwrap for things that trivially cannot fail, like CString::new() on a literal, because the expect doesn't add any additional information beyond "it failed". I think expect should be used in cases where additional explanation of why it cannot fail is needed, like expect("element was inserted above") instead of expect("get() failed").

If this were only the ones where things can fail, we could merge, it's the others that are controversial.

[GuillaumeGomez]

@steveklabnik Ok, I'll make a first PR for the mergeable one and open another one with the rest.

[TimNN]

Ping from triage! I'm marking this as "blocked" on the individual, smaller PRs.

[bors]

☔ The latest upstream changes (presumably #54168) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC-zz]

Ping from triage @GuillaumeGomez what's the update on this?

[GuillaumeGomez]

This PR is a bit behind. I need to cut it into small pieces to be merged separately.

[steveklabnik]

@GuillaumeGomez it's been a few months; since it's gonna get cut into pieces anyway, I'm going to close this to clear out my queue. thanks.

[GuillaumeGomez]

Sure, completely forgot about it!