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 field_reassign when field in closure #10143

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

ericwu17
Copy link
Contributor

@ericwu17 ericwu17 commented Jan 2, 2023

fixes #10136

This change makes the ContainsName struct visit all interior expressions, which means that ContainsName will return true even if name is used in a closure within expr.


changelog: FP: [field_reassign_with_default]: No longer lints cases, where values are initializes from closures capturing struct values
#10143

@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2023

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 2, 2023
@ericwu17 ericwu17 marked this pull request as draft January 2, 2023 02:38
@ericwu17 ericwu17 changed the title Draft: don't lint field_reassign when field in closure don't lint field_reassign when field in closure Jan 2, 2023
This commit makes the ContainsName struct visit all interior
expressions, which means that ContainsName will return true
even if `name` is used in a closure within `expr`.
@ericwu17 ericwu17 force-pushed the field_reassign_with_default-FP branch from 45cd57a to 8de011f Compare January 2, 2023 02:41
@ericwu17
Copy link
Contributor Author

ericwu17 commented Jan 2, 2023

Could someone advise on whether I should be using nested_filter::All or nested_filter::OnlyBodies? I did not understand the documentation very well for these two structs.

@rustbot label: +E-help-wanted

@rustbot rustbot added the E-help-wanted Call for participation: Help is requested to fix this issue. label Jan 2, 2023
pub name: Symbol,
pub result: bool,
}

impl<'tcx> Visitor<'tcx> for ContainsName {
impl<'a, 'tcx> Visitor<'tcx> for ContainsName<'a, 'tcx> {
type NestedFilter = nested_filter::All;
Copy link
Member

Choose a reason for hiding this comment

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

yeah i think it should be OnlyBodies here, All includes things like impls, etc, whereas OnlyBodies just covers closures and consts.

We actually don't want to include consts but that's niche enough and it doesn't hurt to check extra

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Thanks. I made a change to use nested_filter::OnlyBodies instead.

we only need to check closures, so
nestedfilter::All was overkill here.
@Manishearth
Copy link
Member

are you ready for merging, or is this a draft for some other reason too?

@ericwu17
Copy link
Contributor Author

ericwu17 commented Jan 3, 2023

I was ready for merging. Sorry, I forgot to remove the draft marker.

@ericwu17 ericwu17 marked this pull request as ready for review January 3, 2023 00:39
@Manishearth
Copy link
Member

@bors r+

Thanks!!

@bors
Copy link
Collaborator

bors commented Jan 3, 2023

📌 Commit cf4e981 has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 3, 2023

⌛ Testing commit cf4e981 with merge 4334919...

@bors
Copy link
Collaborator

bors commented Jan 3, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing 4334919 to master...

@bors bors merged commit 4334919 into rust-lang:master Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-help-wanted Call for participation: Help is requested to fix this issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

field_reassign_with_default false positive with closure capturing variable that was initialized to default
4 participants