add option_as_ref_deref lint

[basil-cow]

changelog: add a new lint that lints option.as_ref().map(Deref::deref) (and similar calls), which could be expressed more succinctly as option.as_deref[_mut](). Closes #4918.

[ghost]

I found a case where the lint gives a bad suggestion.

fn main() {
    let s: String = String::from("STRING");

    let _: Option<&str> = Some(&s).as_ref().map(|x| x.as_str());
    let _: Option<&String> = Some(&s).as_deref();
}

warning: called `.as_ref().map(Deref::deref)` (or with one of deref aliases) on an Option value. This can be done more directly by calling `Some(&s).as_deref()` instead
 --> src/main.rs:6:27
  |
6 |     let _: Option<&str> = Some(&s).as_ref().map(|x| x.as_str());
  |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using as_deref instead: `Some(&s).as_deref()`

This suggestion won't work because the type of Some(&s).as_ref().map(|x| x.as_str()) isn't the same type as Some(&s).as_deref().

[basil-cow]

I knew deref coercion would screw me over somewhere, should've created a test for that myself, thanks for noticing. I thought coercion is already explicit in hir but I guess it is not?

[flip1995]

Maybe we can check the type of x.as_ref().map(..) calls and check if expr_ty(x) has Deref::Target of the type of the map(..) call and only lint in this case.

[basil-cow]

@flip1995 I am a little puzzled with getting <T as Deref>::Target type, maybe you can point me in the right direction?

[ghost]

I think you can just check if x (i.e. the as_str() receiver) has any adjustments applied to it.

Here is my reasoning...

o.as_def() is o.as_ref().map(|x| x.deref()). So replacing o.as_ref().map(|x| x.as_str()); with o.as_deref() boils downs to whether you can replace x.as_str() with x.deref(). IMO, this should be the safe as long as there are no adjustments (autorefs, autoderefs) applied to x in x.as_str().

[basil-cow]

@mikerite That was my reasoning as well when I was implementing it. Currently I am checking whether the first argument to as_str is a QPath that resolves to the input parameter of the closure. I thought that would be enough, bur apparently autoderef information is stored somewhere else (and not in the coercion table, because that one is empty in tests) and is not explicitly converted to hir (x.as_str() is still x.as_str() in hir and not (*x).as_str()). I think I'll just check that x's type is the same as as_str's input parameter type (but I would welcome a more straightforward solution)

[bors]

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

[ghost]

Try the is_adjusted method in utils.

[flip1995]

I am a little puzzled with getting <T as Deref>::Target type, maybe you can point me in the right direction?

I'm not sure myself. Somehow get the Item of the Deref impl of the type of x and the check the associated item Target. For types not implementing Deref explicitly is as easy as calling builtin_deref. But maybe this is too hard, idk.

[basil-cow]

I presumably fixed it, can anybody think of any more tests to add?

[flip1995]

A macro test would be good

[bors]

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

[pickfire]

Maybe as_ref().map(|c| &**c) to as_deref() too? Got it from @lzutao

[bors]

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

[flip1995]

I just realized, that I reviewed this PR weeks ago, but never pressed "Submit review". Sorry for the long wait time...

[flip1995]

Why not just span_lint_and_sugg? Everything else LGTM.

[basil-cow]

Lets merge this for now, I'll probably add |x| &**x later

[flip1995]

@bors r+

[bors]

📌 Commit 796958c has been approved by flip1995

[bors]

⌛ Testing commit 796958c with merge 6763447...

[bors]

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

[shepmaster]

Lets merge this for now, I'll probably add |x| &**x later

Is this being tracked anywhere? That's how I tend to write this case.

[flip1995]

Good catch! It is now: #5367