[WIP] explicit_auto_deref #6837

Jarcho · 2021-03-03T22:26:19Z

fixes: #234
fixes: #1726

A few different things here. I can split this into multiple PR's if that would be better.

needless_borrow was move to the nursery in d4370f8 due to #2740 which was fixed in #3400, so I've moved it back out of the nursery.

All three lints are placed into the same pass so they don't trigger at the same time.

changelog: explicit_deref_methods will lint chained deref calls
changelog: explicit_deref_methods will lint ufcs style calls
changelog: needless_borrow will lint multiple borrows
changelog: needless_borrow moved out of the nursery
changelog: needless_borrow now lints the final expression in a match arm if that expression is in a block
changelog: new lint explicit_auto_deref

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

`needless_borrow` will now check for multiple borrows `needless_borrow` will now consider mutable references

…was fixed over two years ago in d4370f8.

rust-highfive · 2021-03-03T22:26:22Z

r? @flip1995

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

camsteffen · 2021-03-04T15:01:40Z

I can split this into multiple PR's if that would be better.

Yes I think that is generally always better if possible.

camsteffen · 2021-03-04T15:12:54Z

clippy_lints/src/dereference.rs

+// foo!(&*String::new());
+// fn foo<T>(_: &T) {}
+// foo(&*String::new())
+fn is_stable_auto_deref_position(tcx: TyCtxt<'tcx>, typeck: &'tcx TypeckResults<'_>, id: HirId) -> bool {


Hmm. I think it would be better to do this the other way around. That is, in check_expr, look for a "stable auto deref position", and then proceed inward to check for explicit deref. This way you don't have to climb the HIR with parent_iter.

Walking down would require implementing check_local, check_item, check_impl_item and check_trait_item in addition to check_expr. It also requires walking from a block to all the break expressions that target it, which I'm not sure how to do other than visiting everything.

Then there's the interaction with explicit_deref_methods which can occur in other positions. explicit_auto_deref should lint instead when possible, rather than both lints triggering.

Okay I'm convinced. :)

One other idea. Use tcx.hir().opt_span() to check in_macro in one place, perhaps after match parent {}.

Actually I think it's good for both lints to trigger. The user may disagree with one of the lints but agree with the other.

If they disagree with a lint they can just #[allow] it. Then the other lint can trigger instead.

In this case both lints would trigger on the same code with conflicting suggestions, which breaks rustfix.

It is valid to use Clippy without adding #[allow] on undesired lint cases. I don't think it would break rustfix as of rust-lang/cargo#5842.

In this case both the code and the suggestion for explicit_deref_methods (x.deref() -> &*x) trigger explicit_auto_deref (would change both to &x), and this will always be the case. If someone doesn't want to apply explicit_auto_deref they would have to allow it anyways. If they do want to apply it, it's the one that would take priority.

they would have to allow it anyways

not if they are not using rustfix.

I see your point that the suggestions are redundant. But there is a risk that the user will ignore explicit_auto_deref because they want the deref to be explicit, and at the same time, they would be willing to change to *, but that suggestion is suppressed. This risk outweighs the redundancy IMO. You can display the preferred lint first so that the user won't fix it twice. Also displaying both lints is more informative for someone learning Rust since there are two learning opportunities from the same code.

Anyways, that's just my opinion and this is a small corner case.

I guess they could just leave clippy forever triggering explicit_auto_deref. If that's a real use case then both being suggested is reasonable.

That's really the only reason I would push for hiding it (since it doesn't break rustfix, just checked).

bors · 2021-03-07T12:23:47Z

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

`explicit_deref_methods` improvements Breaking up #6837 changelog: `explicit_deref_methods` will lint chained `deref` calls and ufcs style calls

flip1995

Only a quick review. Not sure what the state of this PR is (-> WIP?). I think you can leave everything in one PR, but please avoid squashing commits until the PR is approved.

flip1995 · 2021-03-17T14:52:57Z

clippy_lints/src/dereference.rs

-use if_chain::if_chain;
-use rustc_ast::util::parser::{ExprPrecedence, PREC_POSTFIX, PREC_PREFIX};
+use crate::{
+    needless_borrow::NEEDLESS_BORROW,


I don't think we should trigger lints in two different passes/modules. Why isn't it possible to move the lint completely in this module/pass?

flip1995 · 2021-03-17T15:00:20Z

clippy_lints/src/redundant_deref.rs

@@ -0,0 +1,63 @@
+// use crate::utils::{get_parent_expr, snippet_with_applicability, span_lint_and_sugg};


Why commented out, not removed? Just WIP I guess?

flip1995 · 2021-03-17T15:04:56Z

clippy_lints/src/dereference.rs

+
+        ExprKind::Field(..) | ExprKind::Index(..) | ExprKind::Err => false,
+
+        ExprKind::Box(..)


Even though most of the time, I think being explicit in matches is good, I would just write _ => true here, since this function is more about which expr types are special, that expression types in general.

Jarcho · 2021-03-17T17:17:20Z

Part of it has already been split out in #6865.

flip1995 · 2021-03-18T11:48:11Z

Ah nice, thanks for the update! Do you plan to split out more PRs from this or do you want to rebase and continue working on this PR?

Jarcho · 2021-03-18T11:49:41Z

I'll be splitting them out as I finish parts.

flip1995 · 2021-03-18T11:53:01Z

SGTM. Is there a reason to keep this PR open then?

Jarcho · 2021-03-21T00:05:13Z

I suppose not.

new lint: `borrow_deref_ref` changelog: ``[`borrow_deref_ref`]`` Related pr: #6837 #7577 `@Jarcho` Could you please give a review? `cargo lintcheck` gives no false negative (but tested crates are out-of-date). TODO: 1. Not sure the name. `deref_on_immutable_ref` or some others?

Jarcho added 3 commits March 2, 2021 21:14

Make explicit_deref_methods check for multiple deref calls

d1912cf

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

Merge needless_borrow checks into Dereferencing lint pass

c530608

`needless_borrow` will now check for multiple borrows `needless_borrow` will now consider mutable references

Move needless_borrow from the nursery. The bug that moved it there …

e2e363f

…was fixed over two years ago in d4370f8.

rust-highfive assigned flip1995 Mar 3, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 3, 2021

Jarcho force-pushed the explicit_auto_deref branch from ad46c1e to 31e2918 Compare March 4, 2021 02:25

Add lint explicit_auto_deref

7c2020b

Jarcho force-pushed the explicit_auto_deref branch from 31e2918 to 7c2020b Compare March 4, 2021 02:43

camsteffen reviewed Mar 4, 2021

View reviewed changes

Jarcho mentioned this pull request Mar 8, 2021

explicit_deref_methods improvements #6865

Merged

bors added a commit that referenced this pull request Mar 13, 2021

Auto merge of #6865 - Jarcho:explicit_deref_methods, r=llogiq

28759b2

`explicit_deref_methods` improvements Breaking up #6837 changelog: `explicit_deref_methods` will lint chained `deref` calls and ufcs style calls

flip1995 reviewed Mar 17, 2021

View reviewed changes

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 Mar 17, 2021

Jarcho closed this Mar 21, 2021

Jarcho mentioned this pull request Aug 19, 2021

New Lint: needless_deref #7577

Closed

lengyijun mentioned this pull request Nov 4, 2021

new lint: borrow_deref_ref #7930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] explicit_auto_deref #6837

[WIP] explicit_auto_deref #6837

Jarcho commented Mar 3, 2021 •

edited

Loading

rust-highfive commented Mar 3, 2021

camsteffen commented Mar 4, 2021

camsteffen Mar 4, 2021

Jarcho Mar 4, 2021

camsteffen Mar 4, 2021

camsteffen Mar 4, 2021

Jarcho Mar 4, 2021

camsteffen Mar 4, 2021

Jarcho Mar 4, 2021

camsteffen Mar 5, 2021

Jarcho Mar 5, 2021 •

edited

Loading

bors commented Mar 7, 2021

flip1995 left a comment

flip1995 Mar 17, 2021

flip1995 Mar 17, 2021

flip1995 Mar 17, 2021

Jarcho commented Mar 17, 2021

flip1995 commented Mar 18, 2021

Jarcho commented Mar 18, 2021

flip1995 commented Mar 18, 2021

Jarcho commented Mar 21, 2021

		@@ -0,0 +1,63 @@
		// use crate::utils::{get_parent_expr, snippet_with_applicability, span_lint_and_sugg};


		ExprKind::Field(..) \| ExprKind::Index(..) \| ExprKind::Err => false,

		ExprKind::Box(..)

[WIP] explicit_auto_deref #6837

[WIP] explicit_auto_deref #6837

Conversation

Jarcho commented Mar 3, 2021 • edited Loading

rust-highfive commented Mar 3, 2021

camsteffen commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarcho Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 7, 2021

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarcho commented Mar 17, 2021

flip1995 commented Mar 18, 2021

Jarcho commented Mar 18, 2021

flip1995 commented Mar 18, 2021

Jarcho commented Mar 21, 2021

Jarcho commented Mar 3, 2021 •

edited

Loading

Jarcho Mar 5, 2021 •

edited

Loading