Add clippy lints to catch more panics

[sffc]

We should enable the following clippy lints:

• indexing_slicing https://rust-lang.github.io/rust-clippy/master/#indexing_slicing
• expect_used https://rust-lang.github.io/rust-clippy/master/#expect_used
• unwrap_used https://rust-lang.github.io/rust-clippy/master/#unwrap_used
• panic https://rust-lang.github.io/rust-clippy/master/#panic

Ideally we can limit this to library code only. We don't want these in test code or offline tooling code.

As usual, in cases where the usage of these operators is valid, an #[allow(clippy::indexing_slicing)] can be used, with a comment explaining why it is OK.

The goal here is to better enforce this section of the style guide: https://github.com/unicode-org/icu4x/blob/main/docs/process/style_guide.md#best-practice

I can't talk about panics without also referencing #1183

• @Manishearth
• @dminor
• @iainireland

[robertbastian]

I don't agree with expect over unwrap. If either are justified because it's obvious that the value is Ok/Some, using expect puts a pointless string in the binary.

[Manishearth]

It's not really pointless because it tells you more about where in the code the problem happened. Fortunately, unwrap/expect now use #[track_caller] so they're not

---

I'm over all positive about adding these lints; I think they'll work out well! I feel like the main situation where we'll want to keep them in is near unsafe code: it's better to panic than trigger UB (which is essentially, allowing for a broader set of undesirable behavior including a panic elsewhere).

We can also improve the lints upstream to handle more cases if there are panics that are clearly safe at compile time.

[sffc]

I don't agree with expect over unwrap. If either are justified because it's obvious that the value is Ok/Some, using expect puts a pointless string in the binary.

I want to forbid both.

[dminor]

This makes sense to me.

[ozghimire]

I can add these clippy lints. Is anyone else working on this at the moment?

[gregtatum]

@sffc is that list above up to date?

[sffc]

Yes it is. Nothing has changed since I filed the issue.

[sffc]

Looking at @ozghimire's PR, I'm wondering if we should restrict unwrap and [], but allow expect, since the point is to have an affirmative comment that the panic is safe, and expect already gives that to us.

[robertbastian]

since the point is to have an affirmative comment that the panic is safe

We're not saying the panic is "safe" (which has a special meaning in Rust), we're saying it's impossible. We shouldn't include a message in the binary that will never be used.

I also think we should be consistent and either always expect or always unwrap.

[gregtatum]

We shouldn't include a message in the binary that will never be used.

But logic errors do come up, and the assumption that it'll never be shown can be wrong.

[sffc]

Fixed in #1650