Skip to content

Add lint to suggest as_chunks over chunks_exact with constant#16002

Open
rommeld wants to merge 2 commits intorust-lang:masterfrom
rommeld:suggest-as-chunks-over-chunks-exact
Open

Add lint to suggest as_chunks over chunks_exact with constant#16002
rommeld wants to merge 2 commits intorust-lang:masterfrom
rommeld:suggest-as-chunks-over-chunks-exact

Conversation

@rommeld
Copy link
Copy Markdown

@rommeld rommeld commented Nov 1, 2025

View all comments

Adds a new lint chunks_exact_to_as_chunks that suggests using as_chunks instead of chunks_exact when the chunk size is a compile-time constant.

changelog: [chunks_exact_to_as_chunks]: Suggest using slice.as_chunks::() when chunks_exact(N) is called with a compile-time constant. This provides better ergonomics and type safety.

fixes #15882

@rustbot rustbot added needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 1, 2025
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Nov 1, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot

This comment has been minimized.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 1, 2025

Lintcheck changes for 04898c1

Lint Added Removed Changed
clippy::chunks_exact_to_as_chunks 9 0 0

This comment will be updated if you push new changes

Copy link
Copy Markdown
Contributor

@ada4a ada4a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start:) Left a couple suggestions

View changes since this review

LL | let mut it = slice.chunks_exact(CHUNK_SIZE);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider using `as_chunks::<4>()` for better ergonomics
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this example shows, the suggestion replace even named constants with their values. To fix that, build the suggestion using a snippet of arg, to preserve its textual representation:

- format!("consider using `{suggestion}::<{chunk_size}>()` for better ergonomics")
+ let arg_str = snippet(cx, arg.span, "_");
+ format!("consider using `{suggestion}::<{arg_str}>()` for better ergonomics")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 610acda

Comment thread tests/ui/chunks_exact_to_as_chunks.rs Outdated

// Should trigger lint - literal constant
let mut it = slice.chunks_exact(4);
//~^ ERROR: using `chunks_exact` with a constant chunk size
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could also use the lint name, which is a bit shorter:

Suggested change
//~^ ERROR: using `chunks_exact` with a constant chunk size
//~^ chunks_exact_to_as_chunks

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 610acda

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no, that's not what I meant.. sorry for not articulating myself clearly.

The error message by itself is great, no need to change it. It's just that in the ui-test file, you can specify the error annotation not only as //~^ ERROR: <error message>, but also as //~^ <lint_name> (in your case, //~^ chunks_exact_with_const_size, which makes the test suite much less verbose overall, and so that's the change I was proposing.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I misunderstood, and I hope this hash is correctly implemented and suits your suggestion bc1d010.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly what I meant:)

/// for chunk in chunks {}
/// ```
#[clippy::version = "1.93.0"]
pub CHUNKS_EXACT_TO_AS_CHUNKS,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint names usually either point out the "bad" pattern used (e.g. ok_expect), or what the pattern could be replaced with (e.g. manual_map). I think yours could use the former scheme, and be called something like chunks_exact_with_const_size

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 610acda

"as_chunks"
};

span_lint_and_help(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be pretty effortlessly switched over to give suggestions, which are quite nice to have. https://doc.rust-lang.org/clippy/development/emitting_lints.html#suggestions-automatic-fixes should help you in that, but don't hesitate to ask if something's unclear

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7082eef

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

Comment on lines +24 to +28
// Check receiver is slice or array type
let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs();
if !recv_ty.is_slice() && !recv_ty.is_array() {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to check for types that can be adjusted to slices, which includes e.g. Vec. For that, you can use TyCtxt::expr_ty_adjusted:

Suggested change
// Check receiver is slice or array type
let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs();
if !recv_ty.is_slice() && !recv_ty.is_array() {
return;
}
// Check if receiver is slice-like
if !cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_slice() {
return;
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1d99b91

Comment on lines +19 to +22
// Check for Rust version
if !msrv.meets(cx, msrvs::AS_CHUNKS) {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is somewhat expensive, so it's best to perform it towards the end, e.g. after the constant check

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2731771

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant after the constant check, but before the lint emission -- currently, the check doesn't actually do anything 😅

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rommeld rommeld force-pushed the suggest-as-chunks-over-chunks-exact branch from 849dbe8 to 610acda Compare November 4, 2025 18:38
@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

Comment thread clippy_lints/src/methods/mod.rs Outdated
);
},
(name @ (sym::chunks_exact | sym::chunks_exact_mut), [arg]) => {
chunks_exact_with_const_size::check(cx, expr, recv, arg, name, self.msrv);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the span that the lint points at is the whole method call expression (expr.span) -- as the last result in Lintcheck (https://github.com/rust-lang/rust-clippy/actions/runs/19080004877#user-content-chunks-exact-with-const-size) shows, that might lead to a somewhat confusing diagnostics:

warning: chunks_exact_with_const_size
   --> target/lintcheck/sources/rustc-demangle-0.1.24/src/v0.rs:299:25
    |
299 |           let mut bytes = self
    |  _________________________^
300 | |             .nibbles
301 | |             .as_bytes()
302 | |             .chunks_exact(2)
    | |____________________________^
    |
    = help: consider using `as_chunks::<2>()` for better ergonomics
    = note: `--force-warn clippy::chunks-exact-with-const-size` implied by `--force-warn clippy::all`

You can instead only highlight the method call (in this case, chunks_exact(2), by using call_span, which comes from the call to method_call(expr) above (line 13) -- see filter_map_bool_then for a (somewhat) simple example

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I understood that correctly. Done in dc9fa9d

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much! Though I think the .0.iter() part was actually correct, you didn't need to remove it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, it would be great if you could add a test case for a multiline call -- to make sure that we're giving the correct diagnostic

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, it would be great if you could add a test case for a multiline call -- to make sure that we're giving the correct diagnostic

I wanted to add a multiline test, but check-fmt.rs comes in the way. And to be honest, I do not know how to handle that issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could either:

  1. Artificially elongate the expression, e.g. by adding more intermediate method calls, so that rustfmt splits it up into multiple lines
  2. Put the #[rustfmt::skip] attribute onto the whole statement, to avoid formatting it entirely -- but I wouldn't use that unless absolutely necessary, e.g. when you want to add tests for really bad formatting, like
    vec   .   chunks_exact(  4)

Copy link
Copy Markdown
Contributor

@ada4a ada4a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress! Here's another couple comments^^

View changes since this review

Comment thread clippy_lints/src/methods/mod.rs Outdated
Comment on lines +5746 to +5748
(name @ (sym::chunks_exact | sym::chunks_exact_mut), [arg]) => {
chunks_exact_with_const_size::check(cx, recv, arg, expr.span, call_span, name, self.msrv);
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I just noticed that this is located in the branch for lints that handle receivers and arguments coming from expansion. I think it would be safer to not do that, at least in the beginning -- working with from-expansion stuff can be quite error-prone -- could you please move this code to the branch under method_call (starting somewhere around line 5050)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 3cfa99a

Comment on lines +19 to +22
// Check for Rust version
if !msrv.meets(cx, msrvs::AS_CHUNKS) {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant after the constant check, but before the lint emission -- currently, the check doesn't actually do anything 😅

Comment thread clippy_lints/src/methods/mod.rs Outdated
);
},
(name @ (sym::chunks_exact | sym::chunks_exact_mut), [arg]) => {
chunks_exact_with_const_size::check(cx, expr, recv, arg, name, self.msrv);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much! Though I think the .0.iter() part was actually correct, you didn't need to remove it

Comment thread clippy_lints/src/methods/mod.rs Outdated
);
},
(name @ (sym::chunks_exact | sym::chunks_exact_mut), [arg]) => {
chunks_exact_with_const_size::check(cx, expr, recv, arg, name, self.msrv);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, it would be great if you could add a test case for a multiline call -- to make sure that we're giving the correct diagnostic

"as_chunks"
};

span_lint_and_help(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

Comment on lines +50 to +55
diag.span_suggestion(
expr_span,
"consider using `as_chunks` instead",
suggestion,
applicability,
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion doesn't need to point at the whole expression either -- you can suggest replacing just the span of chunks_exact(N) (so call_span) with as_chunks::<4>().0.iter() (notice the lack of the starting ., as call_span doesn't include it). This has an additional advantage of not requiring you to get a snippet for the receiver, as you won't need that in the suggestion anymore

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 370d0ae

Comment on lines +45 to +57
// Suggestion replaces just "chunks_exact(N)" with "as_chunks::<N>().0.iter()"
let suggestion = format!("{suggestion_method}::<{arg_str}>().0.iter()");

span_lint_and_sugg(
cx,
CHUNKS_EXACT_WITH_CONST_SIZE,
call_span,
format!("using `{method_name}` with a constant chunk size"),
"consider using `as_chunks` instead",
suggestion,
applicability,
);
}
Copy link
Copy Markdown
Contributor

@ada4a ada4a Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Lintcheck shows in #16002 (comment), here's one bigger remaining1 problem: chunks_exact(_mut) (I'll use the immutable version from here on, for brevity) returns ChunksExact, and apart from being an iterator, that struct also has a remainder method, which returns, well, the remainder. But since we replace the whole chunks_exact call with as_chunks().0.iter(), we lose access to that remainder.

In theory, we could make the lint a lot smarter, so that when it sees that the chunk_exact is stored into a variable, and is then used both as an iterator and to get the remainder, we adjust both calls correctly. So this example from Lintcheck:

let chunk_iter = bytes.chunks_exact(CHUNK_SIZE);
let remainder_chunk = chunk_iter.remainder();
for chunk in chunk_iter {
    /* ... */
}

would be turned into something like this:

let chunk_iter = bytes.as_chunks::<CHUNK_SIZE>();
let remainder_chunk = chunk_iter.1;
for chunk in chunk_iter.0.iter() {
    /* ... */
}

But that'd be pretty complicated, so I suggest we do the following instead:

  1. If we see that the call to .chunks_exact is stored into a variable,
  2. don't offer a suggestion (as it might not be correct)
  3. but instead give a help message, something like:
    let chunk_iter = bytes.chunks_exact(CHUNK_SIZE);
                           ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.as_chunks::<CHUNK_SIZE>() instead`

(1) can be done as follows:

if let Node::LetStmt(_) = cx.tcx.parent_hir_node(expr.hir_id) {
    /* only leave a help message, not a suggestion */
}

Here's how that would probably look in your code:

Suggestion

Suggested change
// Suggestion replaces just "chunks_exact(N)" with "as_chunks::<N>().0.iter()"
let suggestion = format!("{suggestion_method}::<{arg_str}>().0.iter()");
span_lint_and_sugg(
cx,
CHUNKS_EXACT_WITH_CONST_SIZE,
call_span,
format!("using `{method_name}` with a constant chunk size"),
"consider using `as_chunks` instead",
suggestion,
applicability,
);
}
let as_chunks = format_args!("{suggestion_method}::<{arg_str}>()");
span_lint_and_then(
cx,
CHUNKS_EXACT_WITH_CONST_SIZE,
call_span,
format!("using `{method_name}` with a constant chunk size"),
|diag| {
if let Node::LetStmt(_) = cx.tcx.parent_hir_node(expr.hir_id) {
// The `ChunksExact(Mut)` struct is stored for later -- this likely means that the user intends to not only use it as an iterator,
// but also access the remainder using `(into_)remainder`. For now, just give a help message in this case.
// TODO: give a suggestion that replaces this:
// ```
// let chunk_iter = bytes.chunks_exact(CHUNK_SIZE);
// let remainder_chunk = chunk_iter.remainder();
// for chunk in chunk_iter {
// /* ... */
// }
// ```
// with this:
// ```
// let chunk_iter = bytes.as_chunks::<CHUNK_SIZE>();
// let remainder_chunk = chunk_iter.1;
// for chunk in chunk_iter.0.iter() {
// /* ... */
// }
// ```
diag.span_help(
call_span,
format!("consider using `{as_chunks}` instead"),
);
} else {
diag.span_suggestion(
call_span,
"consider using `as_chunks` instead",
// Suggestion replaces just "chunks_exact(N)" with "as_chunks::<N>().0.iter()"
format!("{as_chunks}.0.iter()"),
applicability,
);
}
}
);
}

Footnotes

  1. foreshadowing :P

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might get that totally wrong but isn't that in opposition to your comment above? Because I think it is useful to have the suggestion the possibility to access to the tuple.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I wouldn't want to suggest that in here is that, again, I think that if the user stores the ChunksExact in a variable, then they probably want to access the remainder -- and suggesting .0.iter() would go against that. If they do want to use the variable just as an iterator, they can of course add .0.iter() themselves, but that scenario would be rather rare I think.

In general, the return type of as_chunks is pretty self-explanatory imo -- it's a tuple, and a quick glance at the API docs tells you what each half stands for, so I imagine the user will be able to figure that out...

But if we do want to be extra helpful, we could emit an additional note: given the example code from above, it could look like this:

note: you can access the chunks using `chunk_iter.0.iter()`, and the remainder using `chunk_iter.1`

To do that, you'll need to:

  1. extract the pat from LetStmt
  2. pat is an arbitrary pattern, since the left-hand side of a let-statement could in theory be an arbitrary pattern (e.g. destructuring), but we can assume it's just PatKind::Binding. Therefore, match on that and extract ident -- the identifier (=name) of the variable
  3. construct the note message, and emit it using diag.span_note (after emitting the help message)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!:)

Oh, but there's a caveat I forgot about and you no doubt noticed – since in one of the test cases we now only give a help message and not a suggestion, ui_test (the UI test library we're using) will fail, as even after applying all the suggestions, the test file still raises warnings, precisely because help-only cases haven't actually changed.

Because of this, such test cases are placed into a separate file, usually called <lint_name>_unfixable.rs (there's also this //@no-rustfix thing you'll often see in such files, but you don't need to worry about that for now)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why the tests were failing. Luckily, I do not get frustrated that easily might have taken me a couple of attempts before I would have noticed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 64aae82

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, could you please add the comment from the suggestion (of some version of it, as you please) to your code? Having a TODO will let an interested contributor pick it up in the future

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to use your comment as it is more helpful and provides transparency next possible steps.

format!("using `{method_name}` with a constant chunk size"),
|diag| {
if let Node::LetStmt(let_stmt) = cx.tcx.parent_hir_node(expr.hir_id) {
diag.help(format!("consider using `{as_chunks}` instead"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use span_help with call_span here

Copy link
Copy Markdown
Author

@rommeld rommeld Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 83e0039

let mut applicability = Applicability::MachineApplicable;
let arg_str = snippet_with_applicability(cx, arg.span, "_", &mut applicability);

let as_chunks = format!("{suggestion_method}::<{arg_str}>()");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you should be able to use format_args! here to avoid an intermediary allocation

Copy link
Copy Markdown
Author

@rommeld rommeld Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 83e0039

Comment on lines +22 to +25
// Check if receiver is slice-like
if !cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_slice() {
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually constrain the method correctly. You want to for a single reference to a slice.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 4c5a8f6.

Could you please take a look if I understood you correctly. Took me a while. Thanks.

Comment on lines +27 to +28
let constant_eval = ConstEvalCtxt::new(cx);
if let Some(Constant::Int(_)) = constant_eval.eval(arg) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want is_const_evaluable here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e509232

I hope you were talking about is_const_evaluatable.

diag.span_suggestion(
call_span,
"consider using `as_chunks` instead",
format!("{as_chunks}.0.iter()"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would only suggest .iter() if the parent isn't an into_iter call.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 75e8019

call_span,
format!("using `{method_name}` with a constant chunk size"),
|diag| {
if let Node::LetStmt(let_stmt) = cx.tcx.parent_hir_node(expr.hir_id) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You actually need to be more conservative than this. The only time where the other suggestion is correct is when any iterator would work.

expr_use_ctxt can be used to get how the result is used.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have started here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 219eca3

};

let mut applicability = Applicability::MachineApplicable;
let arg_str = snippet_with_applicability(cx, arg.span, "_", &mut applicability);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be snippet_with_context with expr.span.ctxt() as the target context.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 219eca3

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 17, 2025
@Jarcho
Copy link
Copy Markdown
Contributor

Jarcho commented Nov 17, 2025

@Jarcho Jarcho added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed needs-fcp PRs that add, remove, or rename lints and need an FCP S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) labels Nov 17, 2025
@rustbot rustbot added the needs-fcp PRs that add, remove, or rename lints and need an FCP label Nov 29, 2025
Comment on lines +50 to +106
let use_ctxt = expr_use_ctxt(cx, expr);

let in_for_loop = {
let mut cur_expr = expr;
loop {
if let Some(parent_expr) = get_parent_expr(cx, cur_expr) {
if let Some(for_loop) = ForLoop::hir(parent_expr)
&& for_loop.arg.hir_id == expr.hir_id
{
break true;
}
cur_expr = parent_expr;
} else {
break false;
}
}
};

let is_iterator_method = if let Node::Expr(parent_expr) = use_ctxt.node
&& let ExprKind::MethodCall(_, receiver, _, _) = parent_expr.kind
&& receiver.hir_id == use_ctxt.child_id
&& let Some(method_did) = cx.typeck_results().type_dependent_def_id(parent_expr.hir_id)
&& let Some(trait_did) = cx.tcx.trait_of_assoc(method_did)
{
matches!(
cx.tcx.get_diagnostic_name(trait_did),
Some(sym::Iterator | sym::IntoIterator)
)
} else {
false
};

if in_for_loop {
diag.span_suggestion(
call_span,
"consider using `as_chunks` instead",
format!("{as_chunks}.0"),
applicability,
);
} else if is_iterator_method {
diag.span_suggestion(
call_span,
"consider using `as_chunks` instead",
format!("{as_chunks}.0.iter()"),
applicability,
);
} else {
diag.span_help(call_span, format!("consider using `{as_chunks}` instead"));

if let Node::LetStmt(let_stmt) = use_ctxt.node
&& let PatKind::Binding(_, _, ident, _) = let_stmt.pat.kind
{
diag.note(format!(
"you can access the chunks using `{ident}.0.iter()`, and the remainder using `{ident}.1`"
));
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified quite a bit:

if let Node::Expr(use_expr) = use_ctxt.node {
  match use_expr.kind {
    ExprKind::Call(_, [recv]) | ExprKind::MethodCall(_, recv, [], _)
        if recv.hir_id == use_ctxt.child_id
            && matches!(
                use_expr.span.ctxt().outer_expn_data().kind,
                ExpnKind::Desugaring(DesugaringKind::ForLoop),
            ) =>
    {
        // Suggest `.0``
        return;
    },
    ExprKind::MethodCall(_, recv, ..)
        if recv.hir_id == use_ctxt.child_id
            && use_expr.ty_based_def(cx).assoc_fn_parent(cx).is_diag_item(cx, sym::Iterator) =>
    {
        // Suggest `.0.iter()`
        return;
    },
    _ => {},
  }
}

// Fallback suggestion

You'll also need to avoid linting on use_ctxt.is_ty_unified since that will just be a type error.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in f2508d2

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Dec 12, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rommeld
Copy link
Copy Markdown
Author

rommeld commented Dec 30, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 30, 2025
@rustbot

This comment has been minimized.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Jan 9, 2026

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.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 9, 2026
@rustbot

This comment has been minimized.

@rommeld rommeld force-pushed the suggest-as-chunks-over-chunks-exact branch from ef7bbb6 to de973a9 Compare January 9, 2026 19:00
@rustbot

This comment has been minimized.

@rustbot rustbot removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) has-merge-commits PR has merge commits, merge with caution. labels Jan 9, 2026
@rommeld rommeld force-pushed the suggest-as-chunks-over-chunks-exact branch from de973a9 to 3b15582 Compare January 9, 2026 19:03
@rustbot

This comment has been minimized.

@ada4a
Copy link
Copy Markdown
Contributor

ada4a commented Jan 9, 2026

@rommeld do you need some help with the rebase?... You might find it useful to rollback to the last working version (ad7fc4f as far as I can tell), squash everything together, and retry from there

@rommeld rommeld force-pushed the suggest-as-chunks-over-chunks-exact branch from 3b15582 to 00c0b74 Compare January 9, 2026 19:14
@rustbot

This comment has been minimized.

@rommeld rommeld force-pushed the suggest-as-chunks-over-chunks-exact branch from 00c0b74 to 6b473df Compare January 9, 2026 19:17
This lint suggests using `as_chunks` or `as_chunks_mut` instead of
`chunks_exact` or `chunks_exact_mut` when called with a constant size.

changelog: new lint: [`chunks_exact_to_as_chunks`]
@rommeld rommeld force-pushed the suggest-as-chunks-over-chunks-exact branch from 158f8ac to 10058ea Compare January 9, 2026 20:26
@rommeld rommeld requested a review from Jarcho January 11, 2026 11:01
Copy link
Copy Markdown
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small changes, but otherwise this looks good.

View changes since this review

Comment on lines +52 to +55
if use_ctxt.is_ty_unified {
diag.span_help(call_span, format!("consider using `{as_chunks}` instead"));
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't really be linting at all since it's something like:

if foo { x.chunks_exact(5) }
else { x.chunks_exact(y) }

Here the first call can't be changed since it must have the same type as the second.

Comment thread tests/ui/chunks_exact_to_as_chunks.rs Outdated

// Should trigger lint - literal constant
let mut it = slice.chunks_exact(4);
//~^ ERROR: using `chunks_exact` with a constant chunk size
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the //~ lint_name style of comments rather than the error message.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 12, 2026
@rommeld rommeld requested a review from Jarcho January 12, 2026 19:31
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 12, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 5, 2026

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

call_span,
format!("using `{method_name}` with a constant chunk size"),
|diag| {
let use_ctxt = expr_use_ctxt(cx, expr);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expr_use_ctxt isn't a cheap function. You should be saving the result from the earlier call and reusing it here.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 28, 2026
@mtotbagi
Copy link
Copy Markdown

As rommeld won't have the time to finish this PR, I will finish it. I opened a new PR #16931 where I rebased this one and completed the last requested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suggest as_chunks when people use chunks_exact with a constant length

5 participants