Don't report function calls as unnecessary operation if used in array index

[F3real]

Attempts to fix: #7412

changelog: Don't report function calls used in indexing as unnecessary operation. [unnecessary_operation]

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

[flip1995]

Sorry for the late reply, this fell under the radar for some reason.

I don't think this is a bug of the lint. IMO the reasoning why this is a bug is flawed:

The suggested transformation is wrong because the resulting code no longer panics for out-of-bounds indices.

Hiding away and intentional panic in a indexing operation is bad style. If this actually the intention of this code, you can still allow this lint and add a comment, why this lint is allowed. This at least makes the panic more visible.

Clearly this is a semantic change though. But suggesting to remove potentially panicking code is something we allow in Clippy. So what I would do here is to turn the suggestion to Applicability::MaybeIncorrect and add a note to the Known Problems section in the lint documentation noting this behavior.

[flip1995]

r? @flip1995

[F3real]

@flip1995 Well it is a definitely a bad practice, but I wasn't sure if changes that affect behavior are allowed.
I think that your suggestion is good, I've added comment for known issue. As a side note same issue is referenced in #7408

[flip1995]

Sorry for the back and forth in advance...

After thinking about this a bit more, I'm wondering how common this pattern is and if we actually should allow it. On the one hand this fix changes semantics. On the other hand the semantic change removes a possibly unintentional panic from the code and this lint could just be allowed if the panic is actually intentional.

Maybe we could just suggest something different for indexing operations, e.g.

if arr.len() <= func() { panic!(); }

This also optimizes better locally. The question then is if we should keep this in the same lint 🤔

But since I'm not so sure anymore here, @rust-lang/clippy WDYT?

[camsteffen]

Going by the lint name, I think it's appropriate to say "this indexing operation is unnecessary," whether or not the user intended to do an assertion. Technically it could be a different lint, but I don't think a new lint is merited to re-classify this anti-pattern. I think we should either add a second suggestion or modify the current suggestion. A second suggestion with its own help message like "if you need to assert the length, write this" is probably better since both possibilities are plausible.

[F3real]

I agree that adding new lint would be overkill just to cover one edge case.
We could print then a different suggestion in case that a function is used in index or, alternatively, we could just keep comment saying that this is a known issue.
I guess choice depends on how common is this case and do we want to add this extra case to lint or keep the code simpler and more generic.

[camsteffen]

For the suggestion I would do

assert!(fun() < arr.len());

We should prefer fixed issues over known issues. The fix would be <25 lines of code probably so I don't think code complexity is an issue.

[Manishearth]

I would definitely not unconditionally make the suggestion MaybeIncorrect, since at the moment those don't get applied by cargo fix at all. We can perhaps make it so that that only happens when there's an array and function call, if we're not going to fix the suggestion entirely.

[flip1995]

Ok, so I think the right way to go forward is:

1. Don't implement a new lint for it.
2. Adapt the suggestion for this case. What @camsteffen suggested for the suggestion even optimizes almost to the same asm, so that should be a good suggestion
3. Make the suggestion MaybeIncorrect on the condition that it is a Index operation, otherwise keep it MachineApplicable¹
4. No need for adding a Known Problems section then, since we addressed that.

---

¹ This suggestion may fail, if Index is implemented for something (b[a]) that can't be converted to a < b. Implementing a check for this just to get the right applicability seems to be overkill, so I would just set it to MaybeIncorrect.

[flip1995]

LGTM. Leaving the PR open for another day, if someone else wants to take another look. Nothing else for you to do @F3real here. Thanks for dealing with my back and forth!

[flip1995]

Leaving the PR open for another day

I said 15 days ago and then went on vacation. :)

Thanks!

@bors r+

[bors]

📌 Commit ede977c has been approved by flip1995

[bors]

⌛ Testing commit ede977c with merge 3051773...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 3051773 to master...