New lint: const_size_windows

[DanBondarenko]

View all comments

See #6580

New lint const_size_windows (style category). Checks for slice::windows with constant window size that can be replaced with slice::array_windows, enabling array destructuring.

Verifies that:

• Call is windows method (slice impl).
• Arg size is usable as generic const param.
• Method call is not from a macro expansion (but receiver and args can be, individually).
• Rust version >=1.94.0 (see array_windows stabilization [1]).

Code suggestions:

• Suggests MachineApplicable MaybeIncorrect code with source-equivalent comparable slice::array_windows call.
• Checks for array_windows having an impl on another trait for the receiver. Suggest UFCS in those cases.
• Checks if size is a complex const requiring surrounding braces.

Also emits a "note":

• Shows an example of a for loop using array destructuring on the iterator items with comparable slice::array_windows call.
• Example accounts for the const-evaluatable size if possible: size = 2 destructures to [left, right], size = 3 to [x, y, z] etc. No generic arg needed.
• If code-example variables are already in use, alternative variable names are used.

Limitations of the lint:

• Does not destructure iterated items as its code suggestion. Instead, the emitted "note" has an example of potential destructuring.
• Does not detect usages of slice::chunks with const size. The chunks method does not have a clean equivalent transformation since as_chunks has a different return type [2].

Open question: Should this be a perf lint or a style lint?

• The original issue New lint: windows_const and chunks_const #6580 suggests this as a perf lint.
• I did not find any benchmarks comparing slice::array_windows to slice::windows.
• The release notes for slice::array_windows mention optimization [3]:

  • If we had used the older .windows(4) iterator, then that argument would be a slice which we would have to index manually, hoping that runtime bounds-checking will be optimized away.

changelog: new lint: [const_size_windows]

---

• Followed [lint naming conventions][lint_naming]: "Deny const-size windows" / "Allow const-size windows"
• Added passing UI tests (including committed .stderr file): 29 positive test cases, 19 negative test cases
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

[github-actions]

No changes for 952ea88

[CommanderStorm]

very well and clearnyl done. I especially like the deep testing

View changes since this review

[Gri-ffin]

This is currently linted

fn collect_windows(slice: &[u8]) {
    let _: Vec<&[u8]> = slice.windows(2).collect();
}

but this

fn collect_windows(slice: &[u8]) {
    let _: Vec<&[u8]> = slice.array_windows::<2>().collect();
}

is wrong. I also don't see any MSRV tests.

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[DanBondarenko]

Thanks to @Gri-ffin for the well-spotted problem. I've concluded that the lint can't be MachineApplicable. I played around with some ideas, but there's no way around the problem that downstream consumers might expect slices instead of arrays. Detecting all such consumers conclusively would be extremely complex as these could be arbitrarily far downstream from the call site.

[Examples of downstream consumers requiring slice]

• [1u8, 2, 3].windows(2).collect::<Vec<&[u8]>>()
• [1u8, 2, 3].windows(2).map(std::str::from_utf8)
• [1u8, 2, 3].windows(2).for_each(|x: &[u8]| println!("{x:?}"))

•  fn collect_to_vec_and_return_type(slice: &[u8]) -> Vec<&[u8]> {
        let mut out = Vec::new();
        for window in slice.windows(2) {
            out.push(window); // <- using `array_windows` would cause `out` to become `Vec<&[u8; 2]>`
        }
        out //  `Vec<&[u8; 2]>` isn't coerced to `Vec<&[u8]>`
    }

Changes made:

• Downgraded suggestion applicability to MaybeIncorrect.
• Fixed a bug where the lint suggested invalid generic const args (e.g. T + 1 where T is another const generic), covered with tests.
• Added MSRV tests.

[On why slice.windows(2).collect()-shaped call sites are still linted]

The lint's ambition is no longer to suggest a replacement that's guaranteed to be equivalent. With the downgraded applicability, the lint concedes that using array_windows instead of windows might require downstream consumers to be updated as well. For collecting specifically, users should consider collecting to a vector of arrays instead of a vector of slices. But the burden of updating such usages falls on the user.

[rustbot]

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