get_unwrap is questionable advice

[joshlf]

The get_unwrap lint suggests replacing x.get(y).unwrap() with x[y]. IMO, this is bad advice because the former syntax makes it clear that there's a risk of a panic, while the latter looks innocuous. It's generally good to be aware of where panics can happen. It's especially true when processing untrusted input, where a panic caused by user input is a Denial-of-Service vulnerability.

I propose that we make this lint non-default at the least, and possibly consider removing it.

[Manishearth]

I also feel like this lint is questionable. Putting it in pedantic may be fine, but even then that's making a judgement on what is good code. The nursery or restriction sets may work better?

[oli-obk]

x.get(y).unwrap() makes it clear that there's a risk of a panic
while x[y] looks innocuous

Unwrap is not the sole cause of panics in Rust. I can totally get behind defensive coding, but x[y] looks to me just as panicky as x.get(y).unwrap(). You will actually get a worse panic diagnostic the unwrap ("unwrapped a None"), while the index op gives you both the slice sice and the index in the panic message.

If you write x.get(y).expect("can't happen because of reasons"), the lint will not trigger. Imo using a plain unwrap is the thing at fault here, not the lint.

[joshlf]

I can totally get behind defensive coding, but x[y] looks to me just as panicky as x.get(y).unwrap().

I've gotta say that I disagree with this pretty strongly. Maybe you've developed a better intuition, but in my experience doing code reviews, unwrap and expect leap out to me much more quickly than indexing operations.

If you write x.get(y).expect("can't happen because of reasons"), the lint will not trigger. Imo using a plain unwrap is the thing at fault here, not the lint.

If that's true, then the lint should suggest using expect.

[Manishearth]

We already have a lint that suggests expect, fwiw.

I agree with @joshlf here, while I do think indexing is cleaner, there's a good reason to avoid it for some people in most cases, which isn't great for a lint.

@phansch @flip1995 @llogiq opinions?

[flip1995]

If that's true, then the lint should suggest using expect.

I would suggest both:

help: try indexing
   |
LL |     x[y];
   |     ^^^^
help: or use `expect("..")`
   |
LL |     expect("..");
   |     ^^^^^^^^^^^^

[oli-obk]

there's a good reason to avoid it for some people in most cases, which isn't great for a lint.

So is this a situation where some x[y] are ok, but others are not? Or would it make sense for this to be a sort of configurable thing for your project where you either must always have x.get(y).expect("some explanation here") or always have x[y] (if not already using expect)?

I would have assumed x.get(y).unwrap() to only be the result of refactorings, thus x[y] being cleaner. If that is not the case, I'm fine with just moving the lint to the restriction group.

[Manishearth]

So is this a situation where some x[y] are ok, but others are not?

No, it's a situation where the programmer's preference is to avoid it, and there's a good reason behind that preference that applies to everyone. x[y] vs x.get(y).unwrap() is a more complex tradeoff, with people falling on either side.

I would have assumed x.get(y).unwrap() to only be the result of refactorings

Yeah I can easily see cases where people explicitly prefer it.

Or would it make sense for this to be a sort of configurable thing for your project where you either must always have x.get(y).expect("some explanation here") or always have x[y] (if not already using expect)?

We already have a lint for unwraps so if folks want to do this they can pick a lint 😄

[oli-obk]

Ok, so let's move it to restriction unchanged.

[Manishearth]

On it.

[joshlf]

I would have assumed x.get(y).unwrap() to only be the result of refactorings

Yeah I can easily see cases where people explicitly prefer it.

Yeah, I'm in that camp. I find expect useful in cases where the message will be useful to somebody (e.g., in a library where the library's consumers can trigger the panic, and a message will help them debug), but it's also more verbose since it requires a message. I tend to prefer unwrap in scenarios where the invariant is a local one, so merely knowing the line number at which the panic occurred is enough for debugging.