introduce rc_clone_in_vec_init lint #8769

yonip23 · 2022-04-30T18:18:00Z

changelog: Introduce [rc_clone_in_vec_init] lint

rust-highfive · 2022-04-30T18:18:03Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information.

yonip23 · 2022-04-30T18:24:41Z

Hey there whoever is going to review this 👋🏻

This is my first contribution to this (amazing) project

I'm not sure my lint's name is the best - if you have a better name please lmk!
Also, I'm not sure that my code is the most coherent way to write this - if it doesn't please feel free to leave some references for examples and I'll do my best!

Thanks ahead 🙂

clippy_lints/src/arc_new_in_vec_from_slice.rs

Serial-ATA · 2022-04-30T21:50:47Z

Sorry for the single comments, forgot to start a review 😄

clippy_lints/src/arc_new_in_vec_from_slice.rs

Serial-ATA

Couple more things and it should be good 🙂

clippy_lints/src/arc_new_in_vec_from_elem.rs

yonip23 · 2022-05-01T20:48:30Z

just saw that the commit message is add arc_new_in_vec_from_slice lint and not from_elem - is this ok? or should i re-commit?

Serial-ATA · 2022-05-01T20:55:02Z

Not a big deal, you can git commit --amend if you want.

yonip23 · 2022-05-01T21:01:37Z

Not a big deal, you can git commit --amend if you want.

Just force-pushed again
I think we're good to go now, right?

Serial-ATA · 2022-05-01T21:05:56Z

I'd say so, just have to see if @xFrednet has any other thoughts 🙂

Serial-ATA · 2022-05-02T10:40:21Z

😅 Wasn't even thinking, could you also check for Rc and change the lint message accordingly? (#8719 (comment))

yonip23 · 2022-05-02T11:31:55Z

sweat_smile Wasn't even thinking, could you also check for Rc and change the lint message accordingly? (#8719 (comment))

lol that makes sense, sure thing. Will get to it soon enough

yonip23 · 2022-05-02T17:18:04Z

@Serial-ATA regarding the name, i was thinking implicit_reference_clone or something similar to make it more generic in order to include Rc and Arc - wdyt?

yonip23 · 2022-05-02T17:56:28Z

well i took the liberty to rename - lmk if you're ok with that 😄
i did it in a separate commit so you can review the actual change i made to support Rc more easily - will squash once we're on the same page

Serial-ATA · 2022-05-02T21:35:15Z

Ah naming, the hardest part 😄. How about ref_ptr_in_vec_init? It's not immediately obvious that implicit_reference_clone is only checking vec![].

yonip23 · 2022-05-03T06:57:56Z

Uh, but ref_ptr_in_vec_init doesn't make it clear that the issue is with cloning the ref 😥
I'll probably get to work on it in the evening of my tz (now its morning here) - if we can't come up with a better name by then I'll change to your suggestion 🙂

yonip23 · 2022-05-03T06:59:01Z

Maybe ref_clone_in_vec_init?

Serial-ATA · 2022-05-03T10:34:27Z

Yeah, I think that's better.

yonip23 · 2022-05-03T18:18:02Z

@Serial-ATA renamed 😃

Serial-ATA

Just some typos in the file names, but otherwise LGTM. Thanks for addressing my comments so quickly 🙂.

tests/ui/ref_clone_in_vec_init/rc/custom_arc_strucrt.rs

tests/ui/ref_clone_in_vec_init/arc/custom_arc_strucrt.rs

xFrednet · 2022-05-08T13:02:51Z

Hey, thank you very much for the update! I'll have a look at it over the week. I've also answered you on Zulip 🙃

B.t.w, I think it can be useful to add a dev command to rename a lint - I renamed mine at least 3 times lol something like cargo dev rename_lint --old XXX --new YYY

We have a similar tool for that (cargo dev rename_lint) it might be nice to add a dev mode to it, that doesn't register it as a renamed lint to rustc. And then also document this.

CC: @Jarcho (In case you're interested 🙃 )

yonip23 · 2022-05-08T14:19:47Z

If I'll find some free time in the next few days I'll try to go for the suggestion implementation, but this pr should be ready to merge regardless, right? 🙂
Worst case, i can open another pr with the additional changes

xFrednet

This version looks good to me, the suggestion update can be done in a followup PR as you suggested. I've also used lintcheck, and it found no lint trigger -> not false positive, which is nice. It would be awesome if you could squash the commits, as you offered 🙃

The last thing, I'm unsure about is the lint group. I agree that this is suspicious, but on the other hand, it's not really wrong to use it. Having it warn-by-default might be a bit too strict. @rust-lang/clippy, @Alexendoo, @dswij what are your thought on the group, should this be suspicious or pedantic? 🙃

Jarcho · 2022-05-08T17:03:29Z

I think suspicious is ok. I can't imagine anyone wanting a vec of the same pointer, but I can see someone making the mistake to use vec![..] for this.

Worst case is we change the category afterwards if it's too strict.

yonip23 · 2022-05-08T19:23:41Z

@xFrednet I squashed, please take one last look to make sure i didn't f**k up anything while squashing, and lets merge it 🙂
I'll keep working on the lint suggestion separately as we discussed

dswij · 2022-05-09T08:46:58Z

Don't have hard data on this, but I think that it's much easier to be misusing the vec![..] init rather than actually using it intentionally. This should be in suspicious group, imo.

flip1995

sus as the category SGTM.

I have a few suggestions on how to imrpove the documentation a bit.

Also about the tests: Is there a technical reason why all of those tests have to be in their own file? We usually only separate into different files if the test would otherwise get way too big or if we have auto-fixable and non-auto-fixable suggestions in a lint.

I recommend putting all the tests in one file and put the separate test cases in different functions, that are named like the current file names.

clippy_lints/src/rc_clone_in_vec_init.rs

yonip23 · 2022-05-09T17:11:41Z

@flip1995 I fixed the documentation, thanks for reviewing!

Also about the tests: Is there a technical reason why all of those tests have to be in their own file? We usually only separate into different files if the test would otherwise get way too big or if we have auto-fixable and non-auto-fixable suggestions in a lint.

No technical reason, just makes more sense to me to separate each test to a different file. It's more of a personal preference.
Of course if you insist I can rearrange the tests as you asked 🙂

flip1995 · 2022-05-09T17:20:42Z

I would prefer to keep the tests in line with our other tests. I think it is easier to get an overview of what is tested and compare lint messages and suggestions when they are all in one file (as long as it doesn't get way to big).

I can see why you would prefer the split up test files and in the PR-view this all looks good. But I think later in the file-view, you would have to click/open too many files to see what tests are there and what tests might be missing.

yonip23 · 2022-05-09T20:31:05Z

Got you, np
I'll get to it in the next few days hopefully 🙂

yonip23 · 2022-05-09T21:43:02Z

@flip1995 I got to it quicker than I expected 😄
I took the liberty to separate it into 2 files: rc.rs and arc.rs, instead of just one file. Hope you're ok with it, it made sense to me.
If not, I'll change it of course

flip1995 · 2022-05-10T07:55:02Z

Splitting it into Arc and Rc makes sense. I'm good with tests/documentation now. I didn't review the code, so I leave the final r+ to @xFrednet

xFrednet · 2022-05-10T08:02:05Z

The implementation looks good to me, so everything should be ready since nothing was changed in that regard. Thank you, @flip1995, for the documentation/test review ❤️

Could you squash your fix commits one last time @yonip23? 🙃

yonip23 · 2022-05-10T12:13:04Z

@xFrednet One last check to see everything is squashed correctly please and I think we're good to go 😬

xFrednet · 2022-05-10T12:21:07Z

Everything looks good to me, and the squash is also alright 👍. Thank you for the lint implementation. I hope you also had fun 🙃

@bors r+

bors · 2022-05-10T12:21:08Z

📌 Commit feb6d8c has been approved by xFrednet

bors · 2022-05-10T12:21:14Z

⌛ Testing commit feb6d8c with merge 1a11a49...

bors · 2022-05-10T12:33:48Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 1a11a49 to master...

yonip23 · 2022-05-10T13:39:44Z

It sure was a nice experience, contributing to this project.
Thank you all @xFrednet, @Serial-ATA and everyone involved! 🙏😃

… r=xFrednet add suggestions to rc_clone_in_vec_init A followup to #8769 I also switch the order of the 2 suggestions, since the loop initialization one is probably the common case. `@xFrednet` I'm not letting you guys rest for a minute 😅 changelog: add suggestions to [`rc_clone_in_vec_init`]

rust-highfive assigned xFrednet Apr 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 30, 2022

Serial-ATA reviewed Apr 30, 2022

View reviewed changes

clippy_lints/src/arc_new_in_vec_from_slice.rs Outdated Show resolved Hide resolved

Serial-ATA reviewed Apr 30, 2022

View reviewed changes

clippy_lints/src/arc_new_in_vec_from_slice.rs Outdated Show resolved Hide resolved

Serial-ATA reviewed Apr 30, 2022

View reviewed changes

clippy_lints/src/arc_new_in_vec_from_slice.rs Outdated Show resolved Hide resolved

Serial-ATA suggested changes May 1, 2022

View reviewed changes

clippy_lints/src/arc_new_in_vec_from_slice.rs Outdated Show resolved Hide resolved

clippy_lints/src/arc_new_in_vec_from_slice.rs Outdated Show resolved Hide resolved

clippy_lints/src/arc_new_in_vec_from_slice.rs Outdated Show resolved Hide resolved

Serial-ATA reviewed May 1, 2022

View reviewed changes

clippy_lints/src/arc_new_in_vec_from_slice.rs Outdated Show resolved Hide resolved

Serial-ATA suggested changes May 1, 2022

View reviewed changes

clippy_lints/src/arc_new_in_vec_from_elem.rs Outdated Show resolved Hide resolved

clippy_lints/src/arc_new_in_vec_from_elem.rs Outdated Show resolved Hide resolved

Serial-ATA reviewed May 1, 2022

View reviewed changes

clippy_lints/src/arc_new_in_vec_from_elem.rs Outdated Show resolved Hide resolved

yonip23 force-pushed the 8719 branch from 0cfdb8a to b84fb36 Compare May 1, 2022 20:44

yonip23 force-pushed the 8719 branch from b84fb36 to a9ce6c2 Compare May 1, 2022 21:00

yonip23 changed the title ~~add arc_new_in_vec_from_slice lint~~ add implicit_reference_clone lint May 2, 2022

yonip23 changed the title ~~add implicit_reference_clone lint~~ add ref_clone_in_vec_init lint May 3, 2022

Serial-ATA reviewed May 3, 2022

View reviewed changes

tests/ui/ref_clone_in_vec_init/rc/custom_arc_strucrt.rs Outdated Show resolved Hide resolved

tests/ui/ref_clone_in_vec_init/arc/custom_arc_strucrt.rs Outdated Show resolved Hide resolved

yonip23 mentioned this pull request May 7, 2022

false positive in collapsible-span-lint-calls #8798

Closed

yonip23 changed the title ~~add ref_clone_in_vec_init lint~~ add rc_clone_in_vec_init lint May 7, 2022

yonip23 changed the title ~~add rc_clone_in_vec_init lint~~ introduce rc_clone_in_vec_init lint May 7, 2022

xFrednet approved these changes May 8, 2022

View reviewed changes

yonip23 force-pushed the 8719 branch from 9df9e40 to 7d935ac Compare May 8, 2022 19:20

flip1995 reviewed May 9, 2022

View reviewed changes

clippy_lints/src/rc_clone_in_vec_init.rs Outdated Show resolved Hide resolved

clippy_lints/src/rc_clone_in_vec_init.rs Outdated Show resolved Hide resolved

clippy_lints/src/rc_clone_in_vec_init.rs Outdated Show resolved Hide resolved

introduce rc_clone_in_vec_init lint

feb6d8c

yonip23 force-pushed the 8719 branch from 44de568 to feb6d8c Compare May 10, 2022 12:10

xFrednet approved these changes May 10, 2022

View reviewed changes

bors merged commit 1a11a49 into rust-lang:master May 10, 2022

bors mentioned this pull request May 10, 2022

New lint no_effect_replace #8754

Merged

yonip23 mentioned this pull request May 10, 2022

add suggestions to rc_clone_in_vec_init #8814

Merged

introduce rc_clone_in_vec_init lint #8769

introduce rc_clone_in_vec_init lint #8769

Conversation

yonip23 commented Apr 30, 2022 • edited Loading

rust-highfive commented Apr 30, 2022

yonip23 commented Apr 30, 2022 • edited Loading

Serial-ATA commented Apr 30, 2022

Serial-ATA left a comment

Choose a reason for hiding this comment

yonip23 commented May 1, 2022

Serial-ATA commented May 1, 2022

yonip23 commented May 1, 2022

Serial-ATA commented May 1, 2022

Serial-ATA commented May 2, 2022

yonip23 commented May 2, 2022

yonip23 commented May 2, 2022

yonip23 commented May 2, 2022 • edited Loading

Serial-ATA commented May 2, 2022

yonip23 commented May 3, 2022

yonip23 commented May 3, 2022

Serial-ATA commented May 3, 2022

yonip23 commented May 3, 2022

Serial-ATA left a comment

Choose a reason for hiding this comment

xFrednet commented May 8, 2022

yonip23 commented May 8, 2022

xFrednet left a comment

Choose a reason for hiding this comment

Jarcho commented May 8, 2022

yonip23 commented May 8, 2022 • edited Loading

dswij commented May 9, 2022

flip1995 left a comment

Choose a reason for hiding this comment

yonip23 commented May 9, 2022

flip1995 commented May 9, 2022

yonip23 commented May 9, 2022

yonip23 commented May 9, 2022

flip1995 commented May 10, 2022

xFrednet commented May 10, 2022

yonip23 commented May 10, 2022

xFrednet commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

bors commented May 10, 2022

yonip23 commented May 10, 2022

yonip23 commented Apr 30, 2022 •

edited

Loading

yonip23 commented Apr 30, 2022 •

edited

Loading

yonip23 commented May 2, 2022 •

edited

Loading

yonip23 commented May 8, 2022 •

edited

Loading