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

Don't lint vec_init_then_push when further extended #8699

Merged
merged 2 commits into from
May 15, 2022

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Apr 14, 2022

fixes #7071

This will still lint when a larger number of pushes are done (four currently). The exact number could be debated, but this is more readable then a sequence of pushes so it shouldn't be too large.

changelog: Don't lint vec_init_then_push when further extended.
changelog: Remove mut binding from vec_init_then_push when possible.

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 14, 2022
@Jarcho Jarcho force-pushed the vec_init_then_push_7071 branch 2 times, most recently from 24974f7 to 4e501fa Compare April 14, 2022 19:32
@bors
Copy link
Contributor

bors commented Apr 15, 2022

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

@xFrednet
Copy link
Member

r? @dswij 🙃

@xFrednet xFrednet self-assigned this Apr 19, 2022
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

Changes LGTM, only a suggestion to document the lint behavior

@xFrednet

clippy_lints/src/vec_init_then_push.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Apr 26, 2022

Hey @Jarcho, could you resolve the conflicts? Then we could see if the CI passes 🙃

@Jarcho Jarcho force-pushed the vec_init_then_push_7071 branch from ff3ba2f to 94ec421 Compare April 27, 2022 04:38
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, a few nits and that it should be ready.

Also thank you for your patience, I forgot this was on my todo list 😅

&& adjusted_mut == Mutability::Mut
&& !adjusted_ty.peel_refs().is_slice() =>
{
return ControlFlow::Break(true);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also set needs_mut = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That case doesn't lint.

Copy link
Member

Choose a reason for hiding this comment

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

But it could, couldn't it? The code later checks:

if res == ControlFlow::Break(true) && self.found <= required_pushes_before_extension {
    return;
}

Which could still be false if self.found is > 4. Or should that be an or check in the if statement?

Copy link
Contributor Author

@Jarcho Jarcho May 6, 2022

Choose a reason for hiding this comment

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

Ignore my previous comment. I don't know what I was looking.

It will already be set by that point, either by explicitly taking a mutable reference, or from an adjustment taking a mutable reference.

I'll add a note about it.

clippy_utils/src/visitors.rs Show resolved Hide resolved
Comment on lines +48 to +60
fn _from_iter(items: impl Iterator<Item = u32>) -> Vec<u32> {
let mut v = Vec::new();
v.push(0);
v.push(1);
v.extend(items);
v
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a second test like this, that reaches the threshold and triggers the lint? From my understanding, it should be as simple as:

fn _from_iter_trigger(items: impl Iterator<Item = u32>) -> Vec<u32> {
    let mut v = Vec::new();
    v.push(0);
    v.push(1);
    v.push(2);
    v.push(3);
    v.extend(items);
    v
}

Also, a test with the capacity would be good, like:

   let mut cap_err = Vec::with_capacity(4);
    cap_err.push(0);
    cap_err.push(1);
    cap_err.push(2);

That shouldn't lint either from my understanding

Copy link
Contributor Author

@Jarcho Jarcho May 6, 2022

Choose a reason for hiding this comment

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

The reason for avoiding the boundary case is to avoid having to change the test if we decide to nudge it a little. It doesn't really matter either way, just that the specific number isn't important.

with_capacity could do with a case right at the edge though.

Copy link
Member

Choose a reason for hiding this comment

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

The reason for avoiding the boundary case is to avoid having to change the test if we decide to nudge it a little.

Okay, then could you add one with some more space, it doesn't have to be at the boundary, but I don't see one for this case 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last test covers it with a much larger number.

Copy link
Member

Choose a reason for hiding this comment

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

The last test covers an if statement. The lint already took if statements into account, that's why I didn't consider this a test to this case. Since you want to add a note about the other comment, could you add a test with extend or some other method?

@xFrednet
Copy link
Member

@Jarcho, this is a ping from triage. Could you address the review comments? (I believe the only real change was a comment that should be added 🙃 )

@rustbot author

@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 May 14, 2022
@Jarcho Jarcho force-pushed the vec_init_then_push_7071 branch from 94ec421 to 0e70f29 Compare May 15, 2022 19:49
@Jarcho Jarcho force-pushed the vec_init_then_push_7071 branch from 0e70f29 to f1574cc Compare May 15, 2022 21:39
@xFrednet
Copy link
Member

@bors r=dswij,xFrednet

@bors
Copy link
Contributor

bors commented May 15, 2022

📌 Commit f1574cc has been approved by dswij,xFrednet

@bors
Copy link
Contributor

bors commented May 15, 2022

⌛ Testing commit f1574cc with merge 6ec7359...

@bors
Copy link
Contributor

bors commented May 15, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij,xFrednet
Pushing 6ec7359 to master...

@bors bors merged commit 6ec7359 into rust-lang:master May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

vec_init_then_push should only trigger if there are no further pushes
7 participants