add lint for recreation of an entire struct

[phi-gamma]

This lint makes Clippy warn about situations where an owned struct is essentially recreated by moving all its fields into a new instance of the struct. The lint is not machine-applicable because the source struct may have been partially moved.

This lint originated in something I spotted during peer review. While working on their branch a colleague ended up with a commit where a function returned a struct that 1:1 replicated one of its owned inputs from its members. Initially I suspected they hadn’t run their code through Clippy but AFAICS there is no lint for this situation yet.

changelog: new lint: [redundant_owned_struct_recreation]

New lint checklist

• [+] Followed [lint naming conventions][lint_naming]
• [+] Added passing UI tests (including committed .stderr file)
• [+] cargo test passes locally
• [+] Executed cargo dev update_lints
• [+] Added lint documentation
• [+] Run cargo dev fmt

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (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

[phi-gamma]

Hi, is there a way to tell in a late pass whether one or more of a struct’s members have been moved? That might enable machine-applicability of this lint.

[y21]

This might be better as an addition to the unnecessary_struct_initialization lint. It has essentially the same idea as this one, except it only emits a warning for Foo { ..foo } (w/o any fields), so it could be improved to also emit a warning if all fields copy from the base.

In particular, this lint also suffers from the same issues that that other lint does, which is that struct reinitialization only requires the fields to be Copy, whereas this lint's suggestion forces a move of the whole struct, which, if the value is later used again doesn't work if the struct is not Copy, and is also the reason why that other lint is in nursery (#10547 is the issue).

[phi-gamma]

Thanks for the pointer, I’ll look into folding it into unnecessary_struct_initialization!

In particular, this lint also suffers from the same issues that that other lint does, which is that struct reinitialization only requires the fields to be Copy, whereas this lint's suggestion forces a move of the whole struct, which doesn't work if the struct is not Copy, and is also the reason why that other lint is in nursery (#10547 is the issue).

Would it be sufficient to restrict the lint to Copy types?

[y21]

I thought about this for a bit and yeah, I think restricting it to only Copy types probably fixes the linked issue, but it might also regress too many "true positives" where a warning would be fine.

Maybe what we could do to avoid false positives while still warning on some useful cases would be to not emit a warning if:

• the struct itself is not Copy
• but any of its fields are

My reasoning is that if the struct is not Copy and none of its fields are, then Foo { bar: foo.bar }; also moves the entire struct effectively just like foo; does and in either case no fields are accessible afterwards, so the suggested code shouldn't result in more errors. If however there is one Copy field and one non-Copy field, then Foo { bar: foo.bar, baz: foo.baz }; would still allow using the Copy field whereas foo; again makes it inaccessible.

---

However I think for the purposes of this PR it would be easier to just not worry about the possible false positives that had already existed and only extend the unnecessary_struct_initialization lint to also catch Foo { bar: foo.bar } (in addition to what it already does).

Fixing those false positives could be done separately and the lint is in the nursery category, so that's fine.

[phi-gamma]

However I think for the purposes of this PR it would be easier to just not worry about the possible false positives that had already existed and only extend the unnecessary_struct_initialization lint to also catch Foo { bar: foo.bar } (in addition to what it already does).

I’ll take a stab at this.

Fixing those false positives could be done separately and the lint is in the nursery category, so that's fine.

Understood, I’ll save that for another PR.

Thanks once more for the thorough response!

[phi-gamma]

Hi again, I believe the PR is in a state that warrants another look now.

As discussed I folded the lint into unnecessary_struct_initialization and integrated some further checks that were present there. It now also handles situations where the new struct is partially created from individual fields and the remainder from a base.

[y21]

I'll change the label here since it looks like you're still working on this (based on red CI and the wip commit), you can change it back with @rustbot ready (or if you're stuck, feel free to ask for help).

@rustbot author

[phi-gamma]

Just ignoring/filtering out shorthand initialization seems like it would introduce false positives

Indeed it does, good catch! I dropped that check.

[phi-gamma]

Can you also add a test case involving ranges?

let r = 0..5;
let r = r.start..r.end;

The desugaring to Range { start: r.start, end: r.end } might emit a warning here, which might actually be good and applicable (although the message might be confusing if it mentions structs)

You’re right, the lint hits:

+error: unnecessary struct building
+  --> tests/ui/unnecessary_struct_initialization.rs:102:14
+   |
+LL |     let r2 = r1.start..r1.end;
+   |              ^^^^^^^^^^^^^^^^ help: replace with: `r1`

Haven’t looked into recognizing the desugared range here, perhaps as a follow-up PR?

[phi-gamma]

•    if is_copy(cx, cx.typeck_results().expr_ty(expr_a)) && path_to_local(expr_b).is_some() {
  

•        // When the type implements `Copy`, a reference to the new struct works on the
  

•        // copy. Using the original would borrow it.
  

•        return false;
  

I know this is pre-existing code (+ a nursery lint) so we can just keep it as is and you can resolve the comment, but this logic doesn't seem entirely correct I think 🤔.

Only the fields need to be Copy for S { ..base }, not the type itself:

struct S1 { a: i32, b: i32 }

let mut s = S1 { a: 0, b: 0 };
(&mut S1 { ..s }).b = 1; // incorrect lint, changing to `&mut s` as suggested fails the assertion
assert_eq!(s.b, 0);

In its current form the lint incorrectly triggers both for Copy fields and Copy structs, as this example shows. However in my tests the clippy code path with the comment above is not actually hit in the example as it is guarded by parent_ty.is_any_ptr(). IMO I’d rather leave that cleanup to another PR.

Thanks a lot for the review, that was very helpful! Just a heads up, I won’t be able to look into this more closely in the coming weeks.

[phi-gamma]

@rustbot ready

[y21]

Sorry for taking so long. This all looks good to me now. And yeah, those other things can be left for follow up PRs.

If you're interested in making a specialized lint message for ranges specifically as a followup, we could call higher::Range::hir and see if that returns Some(_)

Either way, thank you for working on this!

@bors r+

[bors]

:pushpin: Commit 296dcf09fb7e744c8f7b118e2e328dbc1fd6a7a5 has been approved by y21

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 296dcf09fb7e744c8f7b118e2e328dbc1fd6a7a5 with merge 1807580a49d31457ca1fd9d3bcbbb9bf1cfd7a34...

[bors]

:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: y21 Pushing 1807580a49d31457ca1fd9d3bcbbb9bf1cfd7a34 to master...