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

Add lint ref_mut_iter_method_chain #7688

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Sep 18, 2021

Mentioned here: #7659 (comment)

changelog: New lint: [ref_mut_iter_method_chain]
#7688

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 18, 2021
@Jarcho Jarcho force-pushed the ref_mut_iter_method_chain branch 2 times, most recently from a8b9e28 to 0813a12 Compare September 18, 2021 21:12
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM overall. Some small questions.

Not sure if this should be warn-by-default.

tests/ui/ref_mut_iter_method_chain.rs Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/ref_mut_iter_method_chain.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 29, 2021
@bors
Copy link
Collaborator

bors commented Sep 30, 2021

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

@Jarcho Jarcho force-pushed the ref_mut_iter_method_chain branch 2 times, most recently from f7230fe to 6b21369 Compare November 12, 2021 16:44
if_chain! {
if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, base_expr) = self_arg.kind;
if !self_arg.span.from_expansion();
if let Some(&iter_trait) = cx.tcx.all_diagnostic_items(()).name_to_id.get(&sym::Iterator);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let Some(&iter_trait) = cx.tcx.all_diagnostic_items(()).name_to_id.get(&sym::Iterator);
if let Some(iter_trait) = cx.tcx.get_diagnostic_item(sym::Iterator);

should work here.

Cow::Owned(mut snip) => {
let ctxt = expr.span.ctxt();

// Attempt to determine if parenthesis are needed base on the target position. The snippet may have
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the same as what sugg::Sugg does? Also I think the enum ExprPosition is pretty similar to the ExprPrecedence enum of rustc.

Would using the Sugg utility also work here? If not, do you think it would be better to expand the Sugg utility, rather than implementing this new utils function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding to the left or right of an operator needs to do different things. 1+1 added to the left of a subtraction doesn't need parenthesis, but added to the right it does. ExprPrecedence doesn't allow that distinction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sugg currently doesn't allow adding parenthesis based on where the will be inserted. I'm in the middle of reworking Sugg now, but it's going to be a larger change to fit this in nicely.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I prefer reusing Sugg here anyway. If there are unnecessary parenthesis, rustc will warn on that afterwards. As long as the suggestion is semantically correct, I don't mind if there are a pair of parenthesis too many.

Especially when you're working on Sugg anyway, I don't want to include a half-baked solution now. (Also big thanks for improving all of our utilities and not only the lints ❤️)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on #7986 which should be a nicer solution. We can hold off merging this until that's done.

Copy link
Member

Choose a reason for hiding this comment

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

We can hold off merging this until that's done.

Yes, let's do this. 👍

@bors
Copy link
Collaborator

bors commented Nov 17, 2021

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

@flip1995 flip1995 added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants