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

Replace unwrap calls in examples by expect #51668

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@GuillaumeGomez
Member

GuillaumeGomez commented Jun 20, 2018

Because no one should use unwrap!

r? @steveklabnik

@GuillaumeGomez GuillaumeGomez changed the title from Replace unwrap calls in example by expect to Replace unwrap calls in examples by expect Jun 20, 2018

@euclio

This comment has been minimized.

Contributor

euclio commented Jun 20, 2018

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

This comment has been minimized.

Collaborator

rust-highfive commented Jun 20, 2018

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

This comment has been minimized.

Member

GuillaumeGomez commented Jun 21, 2018

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

@euclio

This comment has been minimized.

Contributor

euclio commented Jun 21, 2018

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

This comment has been minimized.

Member

steveklabnik commented Jun 21, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Jun 21, 2018

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

This comment has been minimized.

Contributor

TimNN commented Jul 3, 2018

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

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Jul 4, 2018

Need to update.

@bors

This comment has been minimized.

Contributor

bors commented Jul 19, 2018

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

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Jul 23, 2018

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

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Jul 23, 2018

Yep, don't know when but yep.

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Jul 30, 2018

Frendly weekly ping from triage @GuillaumeGomez! :)

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Jul 31, 2018

I'll update it some day! :D

@Aaronepower

This comment has been minimized.

Contributor

Aaronepower commented Aug 10, 2018

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

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Aug 10, 2018

Soon, it'll take a bit of time.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:remove-unwrap branch from 4a8260a to a3cc2a1 Aug 11, 2018

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:remove-unwrap branch from a3cc2a1 to a747842 Aug 11, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Aug 11, 2018

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

This comment has been minimized.

Contributor

euclio commented Aug 12, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Aug 12, 2018

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

This comment has been minimized.

Member

pietroalbini commented Aug 27, 2018

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

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Aug 27, 2018

Debate is still going on. :)

@TimNN

This comment has been minimized.

Contributor

TimNN commented Sep 4, 2018

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

This comment has been minimized.

Member

GuillaumeGomez commented Sep 4, 2018

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

@steveklabnik

This comment has been minimized.

Member

steveklabnik commented Sep 5, 2018

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

This comment has been minimized.

Member

GuillaumeGomez commented Sep 5, 2018

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

@TimNN

This comment has been minimized.

Contributor

TimNN commented Sep 11, 2018

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

@TimNN TimNN added S-blocked and removed S-waiting-on-author labels Sep 11, 2018

kennytm added a commit to kennytm/rust that referenced this pull request Sep 13, 2018

Rollup merge of rust-lang#53976 - GuillaumeGomez:expect-world, r=stev…
…eklabnik

Replace unwrap calls in example by expect

Part of rust-lang#51668.

r? @steveklabnik

kennytm added a commit to kennytm/rust that referenced this pull request Sep 13, 2018

Rollup merge of rust-lang#53976 - GuillaumeGomez:expect-world, r=stev…
…eklabnik

Replace unwrap calls in example by expect

Part of rust-lang#51668.

r? @steveklabnik
@bors

This comment has been minimized.

Contributor

bors commented Sep 14, 2018

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

@Dylan-DPC

This comment has been minimized.

Member

Dylan-DPC commented Nov 26, 2018

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

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 26, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment