fix missing_asserts_for_indexing ignores match for length

[wasd243]

Fixes #17398

missing_asserts_for_indexing previously ignored match expressions, so indexing inside a match on the slice's length was linted even when the match arms already guarantee the indices are in bounds. For example:

match supported.len() { // `match` without assert
    0 => {},
    1 => println!("{}", supported[0]),
    _ => println!("{} or {}", supported[0], supported[1]),
}

Here the 0 and 1 arms rule out all lengths below 2, so the indexing in the wildcard arm cannot fail and the bounds checks are already elided without an assert!.

This PR suppresses the lint for an index inside a match on slice.len() when the non-wildcard arms are integer literals contiguously covering 0..=max (so the wildcard arm implies len > max) and the index is at most max.

The check is deliberately conservative and falls back to linting as before when:

• the arm literals don't contiguously cover 0..=max (e.g. 5 => .., _ => .., where _ can still be 0)
• any arm has a guard
• an arm uses an or-pattern or any non-literal pattern (e.g. 0 | 1 => .. is safe in principle but not analyzed)

One behavioral note: suppression is per-index. If a match guarantees len > 1 but the wildcard arm indexes [0], [1] and [2], only [2] remains unsuppressed, which is equivalent to a single unchecked access and therefore doesn't lint (consistent with the lint never firing on single accesses).

changelog: [missing_asserts_for_indexing]: fix false positive when a match on the slice length guarantees the indices are in bounds

[rustbot]

Thanks for the pull request, and welcome!You should hear from one of our reviewers after this PR is reviewed by at least 2 reviewers from the community

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[github-actions]

Lintcheck changes for cd5f376

Lint	Added	Removed	Changed
clippy::missing_asserts_for_indexing	0	2	1

This comment will be updated if you push new changes

[Gri-ffin]

This assumes that if we have a wildcard anywhere, then the entire match is safe, even if we are accessing indexes we can't safely assume.

fn foo(supported: &[u8]) {
    match supported.len() {
        0 => {},
        1 => {},
        2 => println!("{} {}", supported[0], supported[2]),
        _ => println!("{} {}", supported[0], supported[2]),
    }
}

View changes since the review

[wasd243]

Fixed -- literal arms now require index < n (the arm pins len to exactly n), instead of the shared index <= max check.
Lints now:

match supported.len() {
    0 => {},
    1 => {},
    2 => println!("{} {}", supported[2], supported[3]), // both out of bounds for len == 2 → lints
    _ => {},
}

The example doesn't lint but for a pre-existing reason unrelated to the suppression:

match supported.len() {
    0 => {},
    1 => {},
    2 => println!("{} {}", supported[0], supported[2]), // [0] in bounds → suppressed; [2] NOT suppressed
    _ => println!("{} {}", supported[0], supported[2]), // both in bounds (len >= 3) → suppressed
}

[CommanderStorm]

I think this lint suggestion can be tightened by using the structural pattern matching on slices.

Suggested change

	fn fix_match_case(supported: &[u8]) {
	match supported.len() {
	0 => {},
	1 => println!("{}", supported[0]),
	_ => println!("{} or {}", supported[0], supported[1]),
	}
	fn fix_match_case(supported: &[u8]) {
	match supported {
	[] => {},
	[first] => println!("{}", first),
	[first, second, ...] => println!("{} or {}", first, second),
	}

Much more work though and might be better in another lint.

View changes since the review

[eirnym]

@CommanderStorm pattern matching is not always available: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0afb61b6183cf5bd030edea69c8a52e9

[CommanderStorm]

Yes, you neee to typecheck that it is an slice, if the alternative suggestion makes sense.

[rustbot]

☔ The latest upstream changes (possibly #17607) made this pull request unmergeable. Please resolve the merge conflicts.

[wasd243]

This PR may be out of scope.

It's difficult to reliably reason about potential panics in match/if let expressions.

Although the issue is a valid false positive, addressing it more generally would conflict with the current scope and behavior of the lint, and some of the related work would likely be better suited for a separate lint.

I think it would be better to close this PR temporarily.

[eirnym]

While I agree on generalization issue, is there a possibility to cover at least some cases. I don't even ask about suggestions to replace with, which can land in a documentation rather than be automatic replacements.

[CommanderStorm]

It's difficult to reliably reason about potential panics in match/if let expressions.

Can we boil this down to very simple patterns where the lint is save?
There is also Applicability::MaybeIncorrect as a lint if we are not sure.
Though we obviously only try to add lints where the false positive/negative rate is aceeptable/near zero.

Is there something that we can learn from your effort for this different lint?

• indexing_slicing ignores match for length #17399

[wasd243]

Can we boil this down to very simple patterns where the lint is save?

Maybe we could, but if we only use HIR's patterns and hardcode each specific cases, it would be complex to review the code and fix other cases FP/FN. Even if we only treat this as a false-positive case, the amount of code would be difficult to keep under control. I tried to handle this as a narrow false positive in this PR, but it's 100-200 lines added only for a match, and too many edge case appears such as

fn foo(supported: &[u8]) {
    match supported.len() {
        0 => {},
        1 => {},
        2 => println!("{} {}", supported[0], supported[2]),
        _ => println!("{} {}", supported[0], supported[2]),
    }
}

and y21 already suggested on #17399 an even better solution:

To be fair, maybe a general solution is possible using a dataflow pass on the MIR or something like that, and handling specific patterns is not so unreasonable for a lint that would be warn-by-default

In declare_clippy_lint!, the docs say:

Drawbacks
False positives. It is, in general, very difficult to predict how well
the optimizer will be able to elide bounds checks and it very much depends on
the surrounding code. For example, indexing into the slice yielded by the
slice::chunks_exact
iterator will likely have all of the bounds checks elided even without an assert
if the chunk_size is a constant.

This kind of false positive is already recorded in the lint's docs as a known limitation, and the docs explicitly note that these cases can be difficult to reason about.

For this, my personal answer is no; it may be out of scope.

Is the suggestion maybe incorrect?

Yes.

I tried this code, the same reproducer in issue #17398

fn issue17398(supported: &[u8]) {  
    match supported.len() { // `match` without assert  
        0 => {},  
        1 => println!("{}", supported[0]),  
        _ => println!("{} or {}", supported[0], supported[1]),  
        //~^^ missing_asserts_for_indexing  
    }  
}

error: test got exit code: 1, but expected 0
   --> tests/ui/missing_asserts_for_indexing.fixed:187:33
    |
187 |             1 => println!("{}", supported[0]),
    |                                 ^^^^^^^^^^^^ after rustfix is applied, all errors should be gone, but weren't
    |

full stderr:
error: indexing into a slice multiple times without an `assert`
  --> tests\ui\missing_asserts_for_indexing.fixed:187:33
   |
LL |             1 => println!("{}", supported[0]),
   |                                 ^^^^^^^^^^^^
LL |             _ => println!("{} or {}", supported[0], supported[1]),
   |                                       ^^^^^^^^^^^^  ^^^^^^^^^^^^
   |
   = help: consider asserting the length before indexing: `assert!(supported.len() > 1);`
   = note: asserting the length before indexing will elide bounds checks
   = note: `-D clippy::missing-asserts-for-indexing` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::missing_asserts_for_indexing)]`

error: aborting due to 1 previous error

In this false-positive case, rustfix does not apply the suggestion to the .fixed file, and the UI test fails because the lint is still emitted.

Is the current value being Applicability::MachineApplicable? I'm not sure for it, is there's already some issue/PR related on it?

If not, it would be better to change it into Applicability::MaybeIncorrect because this lint's false-positive is an issue that's already recorded in docs.

[eirnym]

It's worth a while to at least list all false positives you've met and struggled with, so there would be an extensive test scenarios for the future. This also would provide an overview, how problematic would be an implementation for general cases.

As for the fix, without knowledge o an exact type (e.g must be a slice for pattern matching for elements), it's impossible to suggest any fix, but it's worth a while to mention some in documentation, otherwise, this assert could become an annoyance.