Add lint for 'field_reassign_with_default` #568

[hegza]

changelog: Add lint for field_reassign_with_default that checks if mutable object + field modification is used to edit a binding initialized with Default::default() instead of struct constructor.

Fixes #568

Notes:

• Checks for reassignment of one or more fields of a binding initialized with Default::default().
• Implemented using EarlyLintPass, might be future proofed better with LateLintPass.
• Does not trigger if Default::default() is used via another type implementing Default.
• This is a re-open of PR#4761, but I couldn't figure out how to re-open that one so here's a new one with the requested changes :S

[rust-highfive]

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

[Manishearth]

r? @ebroto

might take some time to get to this

[ebroto]

Just a heads-up, I'm half-through the review, should have the time to finish it later.

[ebroto]

Thanks for your pull request! I think this lint can be useful, especially for those not familiar with functional update syntax.

IMO this lint is trickier than it seems... I think the main issue I see with the current iteration is that as we don't recurse into subexpressions, with code like this:

let mut a: A = Default::default();
a.i = 42;
if true {
    a.j = 21;
}

we would:

1. Not catch the assignment to j
2. Worse, suggest making the binding immutable, suggesting code that does not compile.

To make this work, we would probably need a visitor that looks for assignments to the fields of the struct. But what if those assignments are inside a loop? What about function calls that take &mut to one of the fields? We should avoid linting in those cases (see here for some examples of visitors).

I'm inclined to keep things simple and avoid recursing into subexpressions like we do now, but avoid suggesting an immutable binding. That way if there are other assignments we don't suggest code that does not compile, and if there are none rustc's unused_mut will trigger after applying the suggestion.

What do y'all think @flip1995 @Manishearth ?

EDIT: To be clear, for this approach to work we should only take into account the assignments immediately following the call to Default::default

[hegza]

Thanks for finding the edge cases. I tried to reason about it originally and thought that that first one wasn't possible but my mental model was incorrect. There's a lot but I think I should be able to do another try with late lint pass and the offered suggestions. I'll get it through eventually since it's useful.

[bors]

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

[ebroto]

Ping from triage @hegza. Did you have the time to address the review comments? If you are stuck on something we can try to help :)

[hegza]

@ebroto On it. I made a pass on the trivial issues already and signed them off as resolved.

It then took a while to assimilate the rest of what you said into a plan but I feel like I'm progressing and I'm at least currently having a good plan on what to tackle and in which order. I'll get in contact with you if I get stuck.

In detail:

• lint on duplicate assignment to a field should be easy by asserting uniqueness of each element,
• not suggesting mutable is also easy,
• writing my test case on not adding Default::default() to suggestion should clarify how to proceed with that one for me,
• writing my test case on tuples should clarify that one also,
• also test case on needless_update will clarify,
• auto-apply is also easy after the other things are done.

[hegza]

@ebroto now the only things missing are fields initialized as Defaul::default() and auto-apply. The auto-apply seems complicated because I'm not familiar with the concept of FRU. Can you clarify?

Everything else should be done, but I'll leave it here until I can get a comment on the above two issues.

[ebroto]

Thanks for taking the time to apply the suggestions!

Most of my new comments are related to style, but some of them require rethinking the lint a bit. As I said in the last review, I think this lint is trickier than it seems.

[ebroto]

@hegza by FRU I mean "Functional Record Update", so the same thing we are trying to recommend in the suggestion. About the automatic fixes, see my new comment, we can leave them for a follow-up.

[hegza]

Ping from triage @ebroto: I think there was some progress since your last review, but I'm not so sure. Can you take a look?

@hegza If you're waiting for another round of review, don't hesitate to ping the reviewer.

There's progress (albeit slow; other responsibilities cluttering my weekends etc. the usual) but I'm still in the middle of some of the previous changes. I'll ping once I've decided on the next step for each review and recommendation.

The main problem I'm working on is combining the lint with default_trait_access. I think it's easily doable but I'm slightly(!) scared of the refactoring work.

Thanks for another round of review, anyway :D

[ebroto]

There's progress (albeit slow; other responsibilities cluttering my weekends etc. the usual) but I'm still in the middle of some of the previous changes. I'll ping once I've decided on the next step for each review and recommendation.

Of course, no rush, this can wait :)

The main problem I'm working on is combining the lint with default_trait_access. I think it's easily doable but I'm slightly(!) scared of the refactoring work.

Don't hesitate to ping us regarding the refactoring in case you find a blocker.

Again, thanks for sticking with this. I would be very happy to see this merged given the effort you've already put into it.

[ebroto]

The only thing missing are the remarks from the last round of review: #5911 (review)

After that I think we can merge this! :)

[hegza]

@ebroto I think the cases you refer to are already fixed but I hadn't signed them off. I wrote another test-case to verify that as well.

I think we're done (maybe?) If you find more, let me know and I'll get back to it.

[ebroto]

I think the cases you refer to are already fixed

Indeed, we were just missing the test you added.

---

LGTM! Could you squash your commits into one? After that we can merge this!

[hegza]

Squashed @ebroto.

[ebroto]

@bors r+

Thanks for sticking with this during this really long review process ❤️

[bors]

📌 Commit 7b203f3 has been approved by ebroto

[bors]

⌛ Testing commit 7b203f3 with merge 127cdff...

[bors]

💔 Test failed - checks-action_test

[ebroto]

@bors retry

[bors]

⌛ Testing commit 7b203f3 with merge 5b52474...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 5b52474 to master...