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

Implement "suspicious_map" lint #4394

Merged
merged 4 commits into from
Aug 18, 2019
Merged

Conversation

jeremystucki
Copy link
Contributor

Resolves #4010

changelog: New lint suspicious_map.

cx,
SUSPICIOUS_MAP,
expr.span,
"Make sure you did not confuse `map` with `filter`.",
Copy link
Member

Choose a reason for hiding this comment

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

Someone should document the rules of rust error messages and suggestions somewhere somtimes. 😄

Suggested change
"Make sure you did not confuse `map` with `filter`.",
"make sure you did not confuse `map` with `filter`",

IMO this should be a span_help_and_lint:

  • message: "this call to map() won't have an effect on the call to count()"
  • help: "make sure you did not confuse map with fliter"

/// let _ = (0..3).map(|x| x + 2).count();
/// ```
pub SUSPICIOUS_MAP,
pedantic,
Copy link
Member

Choose a reason for hiding this comment

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

I think this could even be a style (Using map for side effects should be rewritten) or complexity (Using map for side effects might be confusing and can be written less complex) lint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I just thought it was too invasive at first

Copy link
Member

Choose a reason for hiding this comment

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

Let's try it. If people complain, we can still move it to pedantic.

@flip1995
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 18, 2019

📌 Commit 9c39c02 has been approved by flip1995

bors added a commit that referenced this pull request Aug 18, 2019
Implement "suspicious_map" lint

Resolves #4010

changelog: New lint `suspicious_map`.
@bors
Copy link
Collaborator

bors commented Aug 18, 2019

⌛ Testing commit 9c39c02 with merge b233685...

@bors
Copy link
Collaborator

bors commented Aug 18, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing b233685 to master...

@bors bors merged commit 9c39c02 into rust-lang:master Aug 18, 2019
@jeremystucki jeremystucki deleted the suspicious_map branch August 18, 2019 16:26
@sdht0
Copy link

sdht0 commented Oct 11, 2019

This lint does not seem to be checking if the map().count() is on an Iterator or not. I'm using differential-dataflow where a map().count() actually makes sense.

Do you think this lint should take other maps and counts into consideration, or is an allow config the only way out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new lint: map().count() on iterator
4 participants