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

explicit_deref_methods improvements #6865

Merged
merged 4 commits into from
Mar 13, 2021
Merged

explicit_deref_methods improvements #6865

merged 4 commits into from
Mar 13, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 8, 2021

Breaking up #6837

changelog: explicit_deref_methods will lint chained deref calls and ufcs style calls

@rust-highfive
Copy link

r? @llogiq

(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 Mar 8, 2021
@Jarcho Jarcho force-pushed the explicit_deref_methods branch 3 times, most recently from a8e4a5e to 84dbde4 Compare March 8, 2021 05:50
@llogiq
Copy link
Contributor

llogiq commented Mar 8, 2021

I think you need to re-bless some UI test results.

@Jarcho Jarcho force-pushed the explicit_deref_methods branch 2 times, most recently from 09995d1 to ee1d796 Compare March 8, 2021 14:06
llogiq
llogiq reviewed Mar 9, 2021
View reviewed changes
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

In general this seems like mostly a solid improvement! It just needs a small bit of work before we merge it.

clippy_lints/src/dereference.rs Show resolved Hide resolved
clippy_lints/src/dereference.rs
Some(Node::Expr(e)) => e,
_ => return true,
};
match parent.kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have a utils function for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not to go from a node to an expression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe this should be moved to utils, we might want to use it elsewhere. But that's not a blocker.

clippy_lints/src/redundant_deref.rs Outdated Show resolved Hide resolved
tests/ui/explicit_deref_methods.stderr Outdated Show resolved Hide resolved
@Jarcho Jarcho force-pushed the explicit_deref_methods branch 3 times, most recently from 585742d to 351f3b5 Compare March 9, 2021 17:13
@bors
Copy link
Contributor

bors commented Mar 11, 2021

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

@llogiq
Copy link
Contributor

llogiq commented Mar 11, 2021

Sorry for taking so long. r=me after a rebase.

Jarcho added 4 commits March 13, 2021 08:39
@Jarcho
Make explicit_deref_methods check for multiple deref calls
a261bc5 
Fix suggestion for `explicit_deref_methods`. Sometimes `&**` is needed, sometimes nothing is needed.
Allow `explicit_deref_methods` to trigger in a few new contexts.
`explicit_deref_methods` will now consider ufcs calls
@Jarcho
Remove unneeded code.
1666e43
@Jarcho
Keep track of whether deref or deref_mut was called
704f7a8 
Remove more unnecessary code
@Jarcho
Properly lint macro arguments for explicit_deref_methods
2713ad4
@Jarcho Jarcho force-pushed the explicit_deref_methods branch from 351f3b5 to 2713ad4 Compare March 13, 2021 13:40
@llogiq
Copy link
Contributor

llogiq commented Mar 13, 2021

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Mar 13, 2021

📌 Commit 2713ad4 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Mar 13, 2021

⌛ Testing commit 2713ad4 with merge 28759b2...

@bors
Copy link
Contributor

bors commented Mar 13, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 28759b2 to master...

@bors bors merged commit 28759b2 into rust-lang:master Mar 13, 2021
@bors bors mentioned this pull request Mar 13, 2021
Merged
@Jarcho Jarcho mentioned this pull request Mar 17, 2021
Closed
@Jarcho Jarcho deleted the explicit_deref_methods branch April 6, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llogiq llogiq llogiq left review comments

Assignees

@llogiq llogiq

Labels
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.
4 participants
@Jarcho @rust-highfive @llogiq @bors