rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

Make `single_range_in_vec_init` ignore type annotations, fn arguments and `ExprField`s

Open matzemathics opened this issue 1 year ago • 6 comments

picking up on #11088.

I will still look at using #11166, as @Alexendoo suggested there.

changelog: Enhancement: [single_range_in_vec_init]: Ignores if it's a local that has type annotations, or is immediately passed to a function or struct initializer

matzemathics avatar Apr 01 '24 16:04 matzemathics

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) some time within the next two weeks.

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

rustbot avatar Apr 01 '24 16:04 rustbot

Just a friendly ping to check in on the progress and if you need any help to move it forward @matzemathics :)

dswij avatar Apr 15 '24 19:04 dswij

@dswij I got (still am) somewhat confused, because the suggested lib::expr_use_ctxt did not manage to find the parents: It returns an ExprUseCtxt { ExprUseNode::Other, ... } if the expression is unused (used as a statement), which is expected. But in all other cases I just got None. I suspect the following check is wrong, and should check the span of child_id instead of parent_id, but then again, that would affect other lints, so why does it work elsewhere?

https://github.com/rust-lang/rust-clippy/blob/7063e3435c0b446e87f64232e6a825787f9db787/clippy_utils/src/lib.rs#L2754-L2756

So if you have any ideas, help would be much appreciated.

EDIT: I tried changing the check in question, and all uitests still pass, so maybe this is a logic error in the expr_use_ctxt function?

matzemathics avatar Apr 16 '24 12:04 matzemathics

Sorry for the delay, it seems like @dswij is busy. Let's get you a new reviewer:

r? clippy

xFrednet avatar Jun 20 '24 20:06 xFrednet

Hey @blyxyas you were chosen by rustbot as the new reviewer, could you take a look at this? If not you can reassign it again :)

xFrednet avatar Jun 20 '24 20:06 xFrednet

Totally! I'll put priority on this (as it's been open for some time, it's our "duty" to the author who took time to make this patch :D)

I should have a review in 1-3 days :)

blyxyas avatar Jun 20 '24 22:06 blyxyas

Hey, this is triage: It looks like @blyxyas is currently busy, let's pick a new reviewer.

r? xFrednet

xFrednet avatar Jul 23 '24 07:07 xFrednet

The linked PR you picked up closes an issue

Closes https://github.com/rust-lang/rust-clippy/issues/11086

Does this PR resolve the same issue?

xFrednet avatar Jul 23 '24 07:07 xFrednet

I've only now seen you question @matzemathics, sorry that it was missed the last time:

I suspect the following check is wrong, and should check the span of child_id instead of parent_id, but then again, that would affect other lints, so why does it work elsewhere?

For some context, the ctxt() checks where the expression comes from. So it is from user written code, macros, or sugaring. I haven't looked at the implementation of that function in detail, but my educated guess is that most lints want to just ignore macros. The check basically breaks if something comes from another context. This lint deals with the vec![] macro. This expands to a different context than its arguments.

EDIT: I tried changing the check in question, and all uitests still pass, so maybe this is a logic error in the expr_use_ctxt function?

Macros are a bit hard to test. We usually just don't lint conservatively. I'm guessing that all ui tests pass because there is no test with complex enough macros.


A side note, since this is your first PR. I think this issue is a harder one, which requires more digging. If you want to fix it, go for it! But if you just want a start to Clippy, there are easier issues out there.

Also sorry for the back and forth, with reviewers. I'm still learning to triage properly :sweat_smile:

xFrednet avatar Jul 23 '24 07:07 xFrednet

Does this PR resolve the same issue?

Yes, however it currently only deals with some special cases and a more general solution seems very much possible.

This lint deals with the vec![] macro. This expands to a different context than its arguments.

I hadn't thought about the possibility, that clippy sees "different code" (i.e. expanded) from what I type. Not sure how this affects the implementation.

A side note, since this is your first PR. I think this issue is a harder one, which requires more digging. If you want to fix it, go for it! But if you just want a start to Clippy, there are easier issues out there.

I still would want to go for it, if I find the time.

Also sorry for the back and forth, with reviewers. I'm still learning to triage properly 😅

No worries, I've been busy myself during the past weeks.

matzemathics avatar Jul 23 '24 10:07 matzemathics

I hadn't thought about the possibility, that clippy sees "different code" (i.e. expanded) from what I type. Not sure how this affects the implementation.

Rust's playground has several tools to investigate the data that rustc and Clippy get. I can recommend looking at the three dots next to the Run button or using the #[clippy::dump] dev attribute.

For this, you can open the playground with the code you want to investigate. Then you can select HIR from the dropdown menu next to the Run button or select Tools > Clippy. :D

xFrednet avatar Jul 23 '24 11:07 xFrednet