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.

Sign up for GitHub

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

Jump to bottom

suspicious_map: accept mutation in map().count() #5375

Closed
wants to merge 1 commit into from
Closed

suspicious_map: accept mutation in map().count() #5375

wants to merge 1 commit into from

Conversation

mathstuf
Copy link
Contributor

@mathstuf mathstuf commented Mar 26, 2020 •
edited
Loading

If the closure in the map call ends up mutating a variable, the call
is assumed to no longer be suspicious.

Given just a function name or path, there's no way to detect that there
is interiour mutability, so the lint still fires in that case. However,
it is now documented as a known problem.

Closes: #5253

Cc: @jpospychala @flip1995 @matthiaskrgr

changelog:

* `suspicious_map` no longer lints closures in `map()` which modify a variable

@mathstuf mathstuf mentioned this pull request Mar 26, 2020
Closed
@mathstuf mathstuf force-pushed the map-count-mut-detection branch 2 times, most recently from 30b2400 to 02eec56 Compare March 26, 2020 01:20
@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Mar 26, 2020
@mathstuf mathstuf force-pushed the map-count-mut-detection branch from 02eec56 to fbca9f5 Compare March 26, 2020 19:17
@mathstuf
suspicious_map: accept mutation in map().count()
77d47e1 
If the closure in the `map` call ends up mutating a variable, the call
is assumed to no longer be suspicious.

Given just a function name or path, there's no way to detect that there
is interiour mutability, so the lint still fires in that case. However,
it is now documented as a known problem.

Fixes rust-lang#5253
@mathstuf mathstuf force-pushed the map-count-mut-detection branch from fbca9f5 to 77d47e1 Compare March 26, 2020 19:39
flip1995
flip1995 reviewed Mar 27, 2020
View reviewed changes
tests/ui/suspicious_map.stderr
error: this call to `map()` won't have an effect on the call to `count()`
--> $DIR/suspicious_map.rs:14:13
|
LL | let _ = (0..3).map(ext_closure).count();
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this case still gets linted...

Copy link
Contributor Author

@mathstuf mathstuf Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

All we get is the name of the function. Should we do name lookup and look into the implementation to check for mutated (non-local) variables?

Which makes me think…I suppose mutable local variables will false-negative this as well :/ . Hrm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or can we do a name lookup and somehow ask if it is FnMut?

Copy link
Member

Choose a reason for hiding this comment

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

I was hoping, that maybe_body_owned_by will get the BodyId of the closure and that it would Just Work™ with mutable_variables. Can you check if it even does get a BodyId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does get a BodyId, but doesn't detect any mutated variables in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So none of the examples in the test are Upvar instances when going through mutated_variables. I'm not sure what's going on here right now, but I suspect the Copy-ness is confusing things here?

@flip1995 flip1995 self-requested a review March 30, 2020 19:10
@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 31, 2020
@flip1995
Copy link
Member

@mathstuf Sorry for taking so long for the review. I don't have an excuse, just wasn't really motivated to review PRs recently. Just wanted to let you know, that I didn't forgot about this and I'll try to find a solution for the remaining issue in the coming days.

@mathstuf
Copy link
Contributor Author

No problem :) . Thanks for the update.

@flip1995 flip1995 self-assigned this May 25, 2020
@flip1995 flip1995 added the E-help-wanted Call for participation: Help is requested to fix this issue. label Jul 3, 2020
@bors
Copy link
Contributor

bors commented Jul 3, 2020

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

@camsteffen camsteffen mentioned this pull request Mar 2, 2021
Merged
bors added a commit that referenced this pull request Mar 15, 2021
@bors
Auto merge of #6831 - camsteffen:suspicious-map, r=flip1995
bfad923 
Fix suspicious_map false positives

changelog: Fix suspicious_map false positives

Fixes #5253
Replaces #5375
bors added a commit that referenced this pull request Mar 15, 2021
@bors
Auto merge of #6831 - camsteffen:suspicious-map, r=Y-Nak,flip1995
a611b11 
Fix suspicious_map false positives

changelog: Fix suspicious_map false positives

Fixes #5253
Replaces #5375
bors added a commit that referenced this pull request Mar 15, 2021
@bors
Auto merge of #6831 - camsteffen:suspicious-map, r=Y-Nak,flip1995
0e042d2 
Fix suspicious_map false positives

changelog: Fix suspicious_map false positives

Fixes #5253
Replaces #5375
@ThibsG
Copy link
Contributor

ThibsG commented Mar 22, 2021

I guess we can close this PR, as #6831 (based on this one) was merged recently

@flip1995 flip1995 closed this Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 Awaiting requested review from flip1995

Assignees

@flip1995 flip1995

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

suspicious_map oddity
4 participants
@mathstuf @flip1995 @bors @ThibsG