Set unwrap lints to warn or deny, rather than allow.

[AndrewBrinker]

Right now the following lints are set to allow:

• option_map_unwrap_or
• option_map_unwrap_or_else
• option_unwrap_used
• result_unwrap_used

It seems to me that setting these lints to warn or deny would be a safer default. Having unwraps hidden in library code can cause a variety of problems for downstream users, who are often given no indication that the library they're using may cause a panic. This situation is even more difficult when considering transient dependencies.

While it would be outside the scope of clippy to check the source of all dependencies for potential panics, I think a lot could be done to improve the situation by at least checking the current source for uses of unwrap.

While I would prefer these lints be set to deny, I understand that uses still exist for unwrap, and that warning may be the right middle ground.

Thoughts?

[BurntSushi]

unwrap (or expect) are perfectly sensible things to do in library code, so long as it's acknowledged that an unwrap failing corresponds to a bug in your code.

Linting against unwrap is isomorphic to linting against xs[5], for example.

[BurntSushi]

unwrap (or expect) are perfectly sensible things to do in library code, so long as it's acknowledged that an unwrap failing corresponds to a bug in your code.

Linting against unwrap is isomorphic to linting against xs[5], for example.

[mutantmell]

unwrap can both be a perfectly sensible thing to do and lint to warn by default.

unwrap can panic if you use it incorrectly. You should preferentially use other functions if possible. You should know what you are doing if you are using it. That sounds like a warn state to me.

[mutantmell]

unwrap can both be a perfectly sensible thing to do and lint to warn by default.

unwrap can panic if you use it incorrectly. You should preferentially use other functions if possible. You should know what you are doing if you are using it. That sounds like a warn state to me.

[BurntSushi]

@mutantmell Do you also think xs[5] should be linted against?

unwrap/expect is an explicit acknowledgment of a runtime invariant that you either couldn't or wouldn't move into the type system. This isn't something to be warned against IMO.

[BurntSushi]

@mutantmell Do you also think xs[5] should be linted against?

unwrap/expect is an explicit acknowledgment of a runtime invariant that you either couldn't or wouldn't move into the type system. This isn't something to be warned against IMO.

[mutantmell]

While I agree with you on technical grounds that xs[5] and unwrap are isomorphic issues, for people coming from other languages, this may not be intuitively obvious. Many developers are used to working with arrays but are less familiar with working with options. unwrap/expect have very tempting type signatures (I need a T, but I have an Option<T>. unwrap gets me a T), and have runtime implications that are not immediately obvious.

Again, unwrap can panic if you use it incorrectly. A warning from a linter is not a statement that you shouldn't use it, it's a statement that you should ensure that you know what you are doing.

If you want me to take an opinion on xs[5], then yes, I do believe it should be linted against. That is not what this ticket is about though, and a discussion about adding that as a lint option should be a separate discussion.

[mutantmell]

While I agree with you on technical grounds that xs[5] and unwrap are isomorphic issues, for people coming from other languages, this may not be intuitively obvious. Many developers are used to working with arrays but are less familiar with working with options. unwrap/expect have very tempting type signatures (I need a T, but I have an Option<T>. unwrap gets me a T), and have runtime implications that are not immediately obvious.

Again, unwrap can panic if you use it incorrectly. A warning from a linter is not a statement that you shouldn't use it, it's a statement that you should ensure that you know what you are doing.

If you want me to take an opinion on xs[5], then yes, I do believe it should be linted against. That is not what this ticket is about though, and a discussion about adding that as a lint option should be a separate discussion.

[llogiq]

IIRC I was the one who changed that lints to allow by default because a) their suggestions are not easy to act upon (what would you do instead of panic?) and they tend to drown out other warnings in test code.

So I think those lints are useful, and authors of library code should set them to warn or deny in code where useful error handling is a requirement.

[llogiq]

IIRC I was the one who changed that lints to allow by default because a) their suggestions are not easy to act upon (what would you do instead of panic?) and they tend to drown out other warnings in test code.

So I think those lints are useful, and authors of library code should set them to warn or deny in code where useful error handling is a requirement.

[AndrewBrinker]

Given all the arguments made here, I think warning is the right default. As @mutantmell says, newer Rust developers are likely to default to unwrap() in contexts where it is not a good option, and this is particular problematic for new developers publishing crates. The goal of the lint here is to inform the programmer that unwrap() is a poor default, and encourage them to seek out other options. If unwrap() really is the right option, then they can silence the warnings.

I agree that unwrap() is isomorphic to any other panicking operation, but I don't think that means that linting against unwrap() can only be reasonably done if linting against array indexing is also done. We shouldn't let the perfect be the enemy of the good.

[AndrewBrinker]

Given all the arguments made here, I think warning is the right default. As @mutantmell says, newer Rust developers are likely to default to unwrap() in contexts where it is not a good option, and this is particular problematic for new developers publishing crates. The goal of the lint here is to inform the programmer that unwrap() is a poor default, and encourage them to seek out other options. If unwrap() really is the right option, then they can silence the warnings.

I agree that unwrap() is isomorphic to any other panicking operation, but I don't think that means that linting against unwrap() can only be reasonably done if linting against array indexing is also done. We shouldn't let the perfect be the enemy of the good.

[BurntSushi]

So I think those lints are useful, and authors of library code should set them to warn or deny in code where useful error handling is a requirement.

I don't understand. Useful error handling and panicking when a runtime invariant is violated (i.e., a bug) seem like orthogonal issues to me.

I agree that unwrap() is isomorphic to any other panicking operation, but I don't think that means that linting against unwrap() can only be reasonably done if linting against array indexing is also done. We shouldn't let the perfect be the enemy of the good.

I don't really follow this logic. &xs[5] is the same as xs.get(5).unwrap(). Why should only the latter be linted against? They seem like the exact same footgun that you're trying to protect against.

As @mutantmell says, newer Rust developers are likely to default to unwrap() in contexts where it is not a good option, and this is particular problematic for new developers publishing crates.

The flip side to this is that people don't like disabling lints and we'll end up with errors that describe bugs. For example, if I wrote a panic free library, my error type would probably need to look something like this:

enum Error {
    Io(io::Error),
    SomeLibrarySpecificError(...),
    // Occurs only if there's a bug in my code,
    // but I was told unwrap was bad.
    PoppedFromEmptyStack(...),
}

To me, linting against unwrap is like linting against runtime invariants themselves. It just doesn't seem right to me.

[BurntSushi]

So I think those lints are useful, and authors of library code should set them to warn or deny in code where useful error handling is a requirement.

I don't understand. Useful error handling and panicking when a runtime invariant is violated (i.e., a bug) seem like orthogonal issues to me.

I agree that unwrap() is isomorphic to any other panicking operation, but I don't think that means that linting against unwrap() can only be reasonably done if linting against array indexing is also done. We shouldn't let the perfect be the enemy of the good.

I don't really follow this logic. &xs[5] is the same as xs.get(5).unwrap(). Why should only the latter be linted against? They seem like the exact same footgun that you're trying to protect against.

As @mutantmell says, newer Rust developers are likely to default to unwrap() in contexts where it is not a good option, and this is particular problematic for new developers publishing crates.

The flip side to this is that people don't like disabling lints and we'll end up with errors that describe bugs. For example, if I wrote a panic free library, my error type would probably need to look something like this:

enum Error {
    Io(io::Error),
    SomeLibrarySpecificError(...),
    // Occurs only if there's a bug in my code,
    // but I was told unwrap was bad.
    PoppedFromEmptyStack(...),
}

To me, linting against unwrap is like linting against runtime invariants themselves. It just doesn't seem right to me.

[AndrewBrinker]

@BurntSushi, I agree with you in the case that unwrap is actually being used only for runtime invariants (that is, being used properly). But as I see it, part of the purpose of lints is to defend against the misuse of features, and the misuse of unwrap is a particularly common issue for new Rust programmers, as a lot of example Rust code uses unwrap for expediency. Setting use of unwrap to warn helps educate newer Rust programmers about when unwrap should be used, and can be easily turned off for people that don't need it.

[AndrewBrinker]

@BurntSushi, I agree with you in the case that unwrap is actually being used only for runtime invariants (that is, being used properly). But as I see it, part of the purpose of lints is to defend against the misuse of features, and the misuse of unwrap is a particularly common issue for new Rust programmers, as a lot of example Rust code uses unwrap for expediency. Setting use of unwrap to warn helps educate newer Rust programmers about when unwrap should be used, and can be easily turned off for people that don't need it.

[mulkieran]

I have been favouring changing all unwrap()s to expect()s. The different part is that the expect() string explains why this unwrap action is expected never to fail. That seems to me the only good use of the string argument to expect, so that is how I use it. If an unwrap() is still in my source, this is, by definition, a TODO or FIXME.

Here are some illustrative examples of the use of expect: https://github.com/stratis-storage/stratisd/blob/master/src/engine/structures.rs.

So, the lint could encourage the programmer to change an unwrap() to an expect() with an explanation of the invariant OR to return an error.

[mulkieran]

I have been favouring changing all unwrap()s to expect()s. The different part is that the expect() string explains why this unwrap action is expected never to fail. That seems to me the only good use of the string argument to expect, so that is how I use it. If an unwrap() is still in my source, this is, by definition, a TODO or FIXME.

Here are some illustrative examples of the use of expect: https://github.com/stratis-storage/stratisd/blob/master/src/engine/structures.rs.

So, the lint could encourage the programmer to change an unwrap() to an expect() with an explanation of the invariant OR to return an error.

[mulkieran]

@AndrewBrinker Was this closed because something good is happening regarding the issue or because nothing good is ever going to happen?

[mulkieran]

@AndrewBrinker Was this closed because something good is happening regarding the issue or because nothing good is ever going to happen?

[AndrewBrinker]

It was closed because on further consideration I am convinced by @BurntSushi's arguments, and no longer think this is a good change.

[AndrewBrinker]

It was closed because on further consideration I am convinced by @BurntSushi's arguments, and no longer think this is a good change.

[mulkieran]

Ok. I'll open a new issue.

[mulkieran]

Ok. I'll open a new issue.