add Arc to `redundant_allocation` #7308

lengyijun · 2021-06-02T08:46:03Z

fixes #7303
changelog: add Arc to redundant_allocation

rust-highfive · 2021-06-02T08:48:01Z

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

xFrednet

Hey lengyijun, welcome and thank you for contributing to Clippy.

The new Arc<> type should also be added to the lint documentation in clippy_lints/src/types/mod.rs. Maybe you can also make the explanation more general and simply add a bullet point list with all checked types. But that is fully up to you.

Could you also move the change log entry in your PR description to be on the same line as "changelog:" ? Take makes writing it a bit easier in the end.

Edit: For the next time, it would also be good if you pick a better commit message. The "But a function is too long" part seems a bit out of place. It also doesn't link to your GitHub account. I'm not sure if there is any policy for it, but it's always better for tracking changes if the account is linked.

The rest of your changes look good to me 👍

r? @flip1995 I would like to take over this PR review if that works for you 🙃.

clippy_lints/src/types/redundant_allocation.rs

xFrednet

Thank you for applying the changes so quickly and relinking your GH account in the first commit 👍

clippy_lints/src/types/redundant_allocation.rs

xFrednet

The refactoring is fantastic. Thank you, for applying them so quickly. This is just a small change I overlooked last time and a question on something I'm unsure about. 🙃

tests/ui/redundant_allocation.rs

clippy_lints/src/types/redundant_allocation.rs

xFrednet

These are the last changes at a quick glance, I'll do a final review when the applicability has been updated. Thank you for applying all the requested changes 🙃

clippy_lints/src/types/redundant_allocation.rs

xFrednet

This version looks excellent to me 🙃 . @flip1995 could you double-check my review?

I think it would be good if the review commits and implementation commits would be squashed in this case, as there are several commits addressing review changes. However, I would leave that decision up to @flip1995, as I'm not totally sure on this yet 😅.

BTW. I also checked with cargo lintcheck and it doesn't report any new lint triggers or ICEs, so that is good as well IMO

flip1995

It also doesn't link to your GitHub account

That doesn't really matter

I think it would be good if the review commits and implementation commits would be squashed

Yes, squashing some commits together would definitely be good.

I have 2 more things:

This lint still suggests to change public API. So it should only trigger iff the avoid_breaking_public_api is set to false, like discussed here: add Arc to redundant_allocation #7308 (comment)
We can't really tell what type the programmer actually wanted. For example Rc<Arc<T>>: We don't really know if the atomic ref counting provided by Arc is important. For Arc/Rc/Box<&T> this doesn't matter and suggesting &T is fine. But if more complex types like Arc, Rc and Box are involved and put together this matters. Right now, always the outer type is suggested as the replacement. I don't think this is a good suggestion. What I would do is to add 2 suggestions to the lint: One that suggests the inner type and one that suggests the outer type as the replacement. But keep in mind that we can't use run-rustfix tests with two suggestions and the test file has to be split up into the simple case Arc/Rc/Box<&T> -> &T and the other cases.

(I'm only 90% sure that my concern in 2. is valid. So tell me if I'm wrong here)

xFrednet · 2021-06-15T08:30:35Z

This lint still suggests to change public API. So it should only trigger iff the avoid_breaking_public_api is set to false, like discussed here: add Arc to redundant_allocation #7308 (comment)

I think that this will require some changes to the type module in general and that other type lints (which also affect the public API, like vec_box) are missing the flag as well. I therefore suggested in that comment to do that all in another PR. I would be happy to do so once this is merged and that's the reason why I didn't press further on the matter. Would that be okay for you two?

flip1995 · 2021-06-15T08:46:16Z

I would be happy to do so once this is merged and that's the reason why I didn't press further on the matter. Would that be okay for you two?

Yes, that should also be fine 👍

lengyijun · 2021-06-18T15:21:26Z

2\. We can't really tell what type the programmer actually wanted. For example `Rc<Arc<T>>`: We don't really know if the atomic ref counting provided by `Arc` is important. For `Arc/Rc/Box<&T>` this doesn't matter and suggesting `&T` is fine. But if more complex types like `Arc`, `Rc` and `Box` are involved and put together this matters. Right now, always the outer type is suggested as the replacement. I don't think this is a good suggestion. What I would do is to add 2 suggestions to the lint: One that suggests the inner type and one that suggests the outer type as the replacement. But keep in mind that we can't use `run-rustfix` tests with two suggestions and the test file has to be split up into the simple case `Arc/Rc/Box<&T> -> &T` and the other cases.

Should we auto fix Box<Box<T>>, Rc<Rc<T>>, Arc<Arc<T>> ?

flip1995 · 2021-06-18T18:00:06Z

Should we auto fix Box<Box<T>>, Rc<Rc<T>>, Arc<Arc<T>> ?

Yes, those should be fine.

clippy_lints/src/types/redundant_allocation.rs

tests/ui/redundant_allocation_fixed.fixed

xFrednet

LGTM

flip1995

Impl looks really good now. The error message still could be slightly improved. Remember that you will have to run cargo dev bless after applying my suggested changes.

clippy_lints/src/types/redundant_allocation.rs

flip1995 · 2021-06-29T09:47:33Z

clippy_lints/src/types/redundant_allocation.rs

+                    "`{}<T>` is already on the heap, `{}<{}<T>>` makes an extra allocation",
+                    inner_sym, outer_sym, inner_sym,


This could be improved by also using the result of let generic_snippet = snippet_with_applicability(cx, inner_span, "..", &mut applicability) here instead of just a generic T. You can do this nicely with format args:

Suggested change

"`{}<T>` is already on the heap, `{}<{}<T>>` makes an extra allocation",

inner_sym, outer_sym, inner_sym,

"`{inner}<{generic}>` is already on the heap, `{outer}<{inner}<{generic}>>` makes an extra allocation",

inner=inner_sym, outer=outer_sym, generic=generic_snippet

flip1995 · 2021-06-29T09:48:20Z

clippy_lints/src/types/redundant_allocation.rs

+            &format!("usage of `{}<{}<T>>`", outer_sym, inner_sym),
+            |diag| {
+                diag.note(&format!(
+                    "`{}<T>` is already on the heap, `{}<{}<T>>` makes an extra allocation",


Same here (and below): use the snippet of the generic arg instead of a generic type param.

flip1995 · 2021-07-14T15:11:34Z

Thanks, LGTM now! Sorry for the long time without a response, GitHub sometimes doesn't give notifications on force pushes.

Can you squash your commits and ping me afterwards, so we can merge this?

lengyijun · 2021-07-14T15:15:39Z

Do I need to squash all commits to one commit?

flip1995 · 2021-07-14T15:18:18Z

Not necessarily. The remaining commits should make sense to keep as separate commits though. E.g. one commit for adding tests, one commit for adding the visitor and one for the actual fix. But you can also just squash everything in one commit.

lengyijun · 2021-07-15T00:21:09Z

Thx.
I prefer to squash them into single commit to make the git tree clean.

flip1995 · 2021-07-15T07:20:31Z

@bors r=xFrednet,flip1995

Thanks!

bors · 2021-07-15T07:20:33Z

📌 Commit e575610 has been approved by xFrednet,flip1995

bors · 2021-07-15T07:20:41Z

⌛ Testing commit e575610 with merge 09f5f15...

bors · 2021-07-15T07:34:47Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet,flip1995
Pushing 09f5f15 to master...

…, r=camsteffen Use `avoid-breaking-exported-api` configuration in types module This PR empowers our lovely `avoid-breaking-exported-api` configuration value to also influence the emission of lints inside the `types` module. (That's pretty much it, not really a change worthy of writing a fairy tale about. Don't get me wrong, I would love to write a short one, but I sadly need to study now). --- Closes: #7489 changelog: The `avoid-breaking-exported-api` configuration now also works for [`box_vec`], [`redundant_allocation`], [`rc_buffer`], [`vec_box`], [`option_option`], [`linkedlist`], [`rc_mutex`] changelog: [`rc_mutex`]: update the lint message to comply with the normal format --- r? `@camsteffen,` as you implemented the configuration value cc: `@flip1995,` as we've discussed this change in #7308

rust-highfive assigned phansch Jun 2, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 2, 2021

xFrednet requested changes Jun 6, 2021

View reviewed changes

clippy_lints/src/types/redundant_allocation.rs Outdated Show resolved Hide resolved

lengyijun force-pushed the redundant_allocation_arc branch from 0bacc4c to ad71930 Compare June 7, 2021 06:45

lengyijun commented Jun 7, 2021

View reviewed changes

clippy_lints/src/types/redundant_allocation.rs Show resolved Hide resolved

xFrednet reviewed Jun 7, 2021

View reviewed changes

clippy_lints/src/types/redundant_allocation.rs Show resolved Hide resolved

clippy_lints/src/types/redundant_allocation.rs Outdated Show resolved Hide resolved

clippy_lints/src/types/redundant_allocation.rs Outdated Show resolved Hide resolved

flip1995 assigned flip1995 and unassigned phansch Jun 8, 2021

xFrednet reviewed Jun 8, 2021

View reviewed changes

tests/ui/redundant_allocation.rs Outdated Show resolved Hide resolved

clippy_lints/src/types/redundant_allocation.rs Show resolved Hide resolved

clippy_lints/src/types/redundant_allocation.rs Outdated Show resolved Hide resolved

xFrednet requested changes Jun 11, 2021

View reviewed changes

clippy_lints/src/types/redundant_allocation.rs Outdated Show resolved Hide resolved

clippy_lints/src/types/redundant_allocation.rs Outdated Show resolved Hide resolved

0969397110 approved these changes Jun 13, 2021

View reviewed changes

xFrednet approved these changes Jun 14, 2021

View reviewed changes

flip1995 requested changes Jun 15, 2021

View reviewed changes

xFrednet reviewed Jun 19, 2021

View reviewed changes

lengyijun force-pushed the redundant_allocation_arc branch 3 times, most recently from c23c274 to cf15c7a Compare June 23, 2021 14:23

xFrednet approved these changes Jun 28, 2021

View reviewed changes

flip1995 reviewed Jun 29, 2021

View reviewed changes

lengyijun force-pushed the redundant_allocation_arc branch from cf15c7a to b6aa612 Compare June 30, 2021 05:28

flip1995 approved these changes Jul 14, 2021

View reviewed changes

flip1995 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 Jul 14, 2021

redundant_allocation: add Arc; some refractoring.

e575610

lengyijun force-pushed the redundant_allocation_arc branch from b6aa612 to e575610 Compare July 14, 2021 23:24

bors merged commit 09f5f15 into rust-lang:master Jul 15, 2021

xFrednet mentioned this pull request Jul 24, 2021

Use avoid_breaking_public_api configuration in types module #7489

Closed

xFrednet mentioned this pull request Aug 12, 2021

Use avoid-breaking-exported-api configuration in types module #7560

Merged

lengyijun deleted the redundant_allocation_arc branch October 27, 2023 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Arc to `redundant_allocation` #7308

add Arc to `redundant_allocation` #7308

lengyijun commented Jun 2, 2021 •

edited

Loading

rust-highfive commented Jun 2, 2021

xFrednet left a comment •

edited

Loading

xFrednet left a comment

xFrednet left a comment

xFrednet left a comment

xFrednet left a comment •

edited

Loading

flip1995 left a comment

xFrednet commented Jun 15, 2021

flip1995 commented Jun 15, 2021

lengyijun commented Jun 18, 2021

flip1995 commented Jun 18, 2021

xFrednet left a comment

flip1995 left a comment

flip1995 Jun 29, 2021

flip1995 Jun 29, 2021 •

edited

Loading

flip1995 commented Jul 14, 2021

lengyijun commented Jul 14, 2021

flip1995 commented Jul 14, 2021 •

edited

Loading

lengyijun commented Jul 15, 2021

flip1995 commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021

		"`{}<T>` is already on the heap, `{}<{}<T>>` makes an extra allocation",
		inner_sym, outer_sym, inner_sym,

add Arc to redundant_allocation #7308

add Arc to redundant_allocation #7308

Conversation

lengyijun commented Jun 2, 2021 • edited Loading

rust-highfive commented Jun 2, 2021

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet left a comment • edited Loading

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

xFrednet commented Jun 15, 2021

flip1995 commented Jun 15, 2021

lengyijun commented Jun 18, 2021

flip1995 commented Jun 18, 2021

xFrednet left a comment

Choose a reason for hiding this comment

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Jun 29, 2021

Choose a reason for hiding this comment

flip1995 Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

flip1995 commented Jul 14, 2021

lengyijun commented Jul 14, 2021

flip1995 commented Jul 14, 2021 • edited Loading

lengyijun commented Jul 15, 2021

flip1995 commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021

bors commented Jul 15, 2021

add Arc to `redundant_allocation` #7308

add Arc to `redundant_allocation` #7308

lengyijun commented Jun 2, 2021 •

edited

Loading

xFrednet left a comment •

edited

Loading

xFrednet left a comment •

edited

Loading

flip1995 Jun 29, 2021 •

edited

Loading

flip1995 commented Jul 14, 2021 •

edited

Loading