Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make single_range_in_vec_init ignore type annotations, fn arguments and ExprFields #12611

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matzemathics
Copy link

@matzemathics matzemathics commented Apr 1, 2024

picking up on #11088.

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

changelog: FP [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

@rustbot
Copy link
Collaborator

rustbot commented Apr 1, 2024

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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 1, 2024
@dswij
Copy link
Member

dswij commented Apr 15, 2024

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

@matzemathics
Copy link
Author

matzemathics commented Apr 16, 2024

@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?

if cx.tcx.hir().span(parent_id).ctxt() != ctxt {
return ControlFlow::Break(());
}

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?

@xFrednet
Copy link
Member

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

r? clippy

@rustbot rustbot assigned blyxyas and unassigned dswij Jun 20, 2024
@xFrednet
Copy link
Member

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 :)

@blyxyas
Copy link
Member

blyxyas commented Jun 20, 2024

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 :)

@xFrednet
Copy link
Member

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

r? xFrednet

@rustbot rustbot assigned xFrednet and unassigned blyxyas Jul 23, 2024
@xFrednet
Copy link
Member

The linked PR you picked up closes an issue

Closes #11086

Does this PR resolve the same issue?

@xFrednet
Copy link
Member

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 😅

@matzemathics
Copy link
Author

matzemathics commented Jul 23, 2024

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.

@xFrednet
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants