New Lint: needless_deref

[lengyijun]

changelog: [needless_deref]
changelog: fixes #234

This pr only lints on function(no generic type) parameters. It only includes two cases:

a:T
foo(&* a) -> foo(&a)

a:&T
foo(&** a)->foo(a)

[rust-highfive]

r? @Manishearth

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

[Manishearth]

Thinking back I think there are lots of cases where this doesn't work, I find myself explicitly derefing pretty often. @rust-lang/clippy what do y'all think about adding this? Maybe only nursery?

[camsteffen]

I think there are two distinct things linted here:

1. let x = &1;
   // type is the same, this ref-deref is a no-op
   let x = &*x;

2. let x = String::new();
   // this deref could be removed and the deref would then be implicit (coerced from type annotation)
   let x: &str = &*x; 

   Note that function arguments are coerced similar to a let binding type annotation.

The first case could be needless_ref_deref or maybe enhance no_effect.

The second case is what we're mainly concerned with here, I think. I would maybe call this needless_explicit_deref or coercable_deref. needless_deref is not right. I have a two concerns with this lint: 1) I don't think that an implicit deref is unilaterally better style. An explicit deref adds clarity. The value of this added clarity varies case by case. 2) An explicit deref may safeguard the code from changing semantics when other code changes. Suppose I have takes_str(&string) and string is implicity derefed from &String to &str. Then suppose take_str is changed to take impl Display instead of &str. The implicit deref silently goes away and now &String is passed to takes_str. If I have an explicit deref, I can be more confident that a &str is (and always will be) passed to the function. So I guess this is a restriction lint.

[lengyijun]

I didn't lint on ref-deref because it failed on assert_eq! macro.

[Manishearth]

r? @camsteffen

(might not have time to go through this rn)

[Jarcho]

I do have a version of this mostly working in any case auto-deref works (except method receivers). Fixing it has been on my list of things to do for a while. There's a version of it in #6837.

[camsteffen]

I didn't lint on ref-deref because it failed on assert_eq! macro.

@lengyijun Can you clarify what you mean by that?

I don't think we should have a lint for coercable derefs without also having a lint for noop derefs. Simpler case first. The implementations should probably be together.

I do have a version of this mostly working in any case auto-deref works (except method receivers). Fixing it has been on my list of things to do for a while. There's a version of it in #6837.

@Jarcho can you try to summarize what is different between this PR and what you have? Then we can decide how to reconcile this. Strictly speaking, since there is no open issue assigned to you (AFAIK), I think it is fair for someone else to open a PR. Perhaps we can move forward with this and you can add enhancements afterwards?

[Jarcho]

The only difference is in the scope of what is checked for. This is limited to specific expressions (&*x and &**x) and only in arguments to functions or function pointers with no generic arguments. I think merging this first is good choice assuming it's ready. I brought it up more as a reference if @lengyijun or anyone found it useful.

An explicit deref may safeguard the code from changing semantics when other code changes. Suppose I have takes_str(&string) and string is implicity derefed from &String to &str. Then suppose take_str is changed to take impl Display instead of &str. The implicit deref silently goes away and now &String is passed to takes_str. If I have an explicit deref, I can be more confident that a &str is (and always will be) passed to the function. So I guess this is a restriction lint.

The Display impl for both str and String does the same thing so this isn't really the best example. I actually can't think of a case where this an impl shared by both a type and is derefed type are different that I've seen. There's a minor code bloat issue here by taking impl Display and using both str and String, but it's not much of a correctness issue.

Be careful with linting &*x where x is a reference. They aren't quite the same thing. e.g.

fn foo(x: &mut &u32) {
    if **x == 0 {
        *x = &1;
    }
}

fn main() {
    let mut x = &0;
    foo(&mut &*x);
    assert_eq!(*x, 0);
    foo(&mut x);
    assert_eq!(*x, 1);
}

Also if ptr::eq is used the result can change.

[camsteffen]

I think merging this first is good choice assuming it's ready. I brought it up more as a reference if @lengyijun or anyone found it useful.

I see, thanks!

There's a minor code bloat issue here by taking impl Display and using both str and String, but it's not much of a correctness issue.

Yes that is what I was alluding to. You're right - it's a minor, subtle, non-correctness issue. But it is something that people (rightly) care about.

[lengyijun]

I didn't lint on ref-deref because it failed on assert_eq! macro.
@lengyijun Can you clarify what you mean by that?

In assert_eq! :

::core::panicking::assert_failed(
    kind,
    &*left_val,    // trigger ref deref
    &*right_val,
    ::core::option::Option::None,
);

So I give up to lint ref-deref

[lengyijun]

The second case is what we're mainly concerned with here, I think. I would maybe call this needless_explicit_deref or coercable_deref. needless_deref is not right. I have a two concerns with this lint: 1) I don't think that an implicit deref is unilaterally better style. An explicit deref adds clarity. The value of this added clarity varies case by case. 2) An explicit deref may safeguard the code from changing semantics when other code changes. Suppose I have takes_str(&string) and string is implicity derefed from &String to &str. Then suppose take_str is changed to take impl Display instead of &str. The implicit deref silently goes away and now &String is passed to takes_str. If I have an explicit deref, I can be more confident that a &str is (and always will be) passed to the function. So I guess this is a restriction lint.

Luckily, I exclude this problem because this pr doesn't deal with function with generic parameters. though I didn't have this case in mind.

[camsteffen]

In assert_eq! :

::core::panicking::assert_failed(
    kind,
    &*left_val,    // trigger ref deref
    &*right_val,
    ::core::option::Option::None,
);

So I give up to lint ref-deref

What is the type of left_val? It should be a reference.

[lengyijun]

Yes, it's a reference.

[camsteffen]

Can you reproduce in a playground? Did you remove both the ref and deref operators?

[lengyijun]

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=0e79aea6cac5bf856eb1ac07be9e714f

[camsteffen]

Oh sorry I'm just now understanding - the lint is triggering on the expanded assert_eq! 🤦 . You can (and should) suppress the lint in macros by checking for expr.span.from_expansion().

[lengyijun]

@camsteffen Thanks.
I plan to close this pr and give another pr on needless_ref_deref first.

My plan to needless_ref_deref(updated):

1. complexity

fn foo(&U){}
// a:&U
foo(&*a) -> foo(a)

// b:&T
foo(&*b) -> foo(&**b)

[camsteffen]

Sounds good. I don't think the function parameters part is necessary. You can just look for a &.. expression in check_expr.

[lengyijun]

fn foo(x: &mut &u32) {
    if **x == 0 {
        *x = &1;
    }
}

fn main() {
    let mut x = &0;
    foo(&mut &*x);
    assert_eq!(*x, 0);
    foo(&mut x);
    assert_eq!(*x, 1);
}

After some experiments, I found I couldn't deal with it unfortunately.