Skip to content

explicit_auto_deref could suggest variale with borrows #9109

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

Closed
matthiaskrgr opened this issue Jul 3, 2022 · 2 comments · Fixed by #9126
Closed

explicit_auto_deref could suggest variale with borrows #9109

matthiaskrgr opened this issue Jul 3, 2022 · 2 comments · Fixed by #9126
Labels
L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@matthiaskrgr
Copy link
Member

Description

The op_ref lint already does this:

warning: needlessly taken reference of left operand
   --> crates/rust-analyzer/src/reload.rs:582:51
    |
582 |             if dummy_replace.iter().any(|replace| &**replace == name) {
    |                                                   ----------^^^^^^^^
    |                                                   |
    |                                                   help: use the left value directly: `**replace`
    |
    = note: `#[warn(clippy::op_ref)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#op_ref

IMO its easier to understand if the span and the suggestion includes the borrows

so

warning: deref which would be done by auto-deref
   --> crates/rust-analyzer/src/handlers.rs:981:41
    |
981 |         snap.analysis.rename(position, &*params.new_name)?.map_err(to_proto::rename_error)?;
    |                                         ^^^^^^^^^^^^^^^^ help: try this: `params.new_name`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref

could become

warning: deref which would be done by auto-deref
   --> crates/rust-analyzer/src/handlers.rs:981:41
    |
981 |         snap.analysis.rename(position, &*params.new_name)?.map_err(to_proto::rename_error)?;
    |                                        ^^^^^^^^^^^^^^^^^ help: try this: `&params.new_name`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref

Version

clippy 0.1.64 (f2d9393 2022-07-02)

Additional Labels

No response

@matthiaskrgr matthiaskrgr added the L-suggestion Lint: Improving, adding or fixing lint suggestions label Jul 3, 2022
@msfjarvis
Copy link

I think I have a similar case (let me know if it should be a separate issue), but I'm not sure if it's actually a bad diagnostic or just me holding it wrong. Given this code:

use once_cell::sync::Lazy;

static BASE_URL: Lazy<String> =
    Lazy::new(|| std::env::var("BASE_URL").expect("BASE_URL must be defined"));

fn main() {
    function(&**BASE_URL);
}

fn function(url: &str) {
    let _ = url;
}

The emitted diagnostic is this:

warning: deref which would be done by auto-deref
 --> src/repro.rs:7:15
  |
7 |     function(&**BASE_URL);
  |               ^^^^^^^^^^ help: try this: `BASE_URL`

At a first glance one might change this invocation to function(BASE_URL) which obviously fails, because when you do a double take you'll see what is really being suggested is function(&BASE_URL). This is probably intentionally so, but it is a bit confusing nonetheless. It'd be great if the diagnostic could look something like this instead:

warning: deref which would be done by auto-deref
 --> src/repro.rs:7:15
  |
7 |     function(&**BASE_URL);
  |              ^^^^^^^^^^^ help: try this: `&BASE_URL`

@Jarcho
Copy link
Contributor

Jarcho commented Jul 5, 2022

I originally had it that way and I don't remember why I switched it. It had something to do with how the lint pass was implemented.

Related question, how does the suggestion look with &mut *x just suggesting x. e.g.

function(&mut **x)
              ^^^ help: try this: `x`

This one looks fine to me unlike a immutable borrow.

Edit: I guess #9127 answers that question.

bors added a commit that referenced this issue Aug 8, 2022
`explicit_auto_deref` changes

fixes #9123
fixes #9109
fixes #9143
fixes #9101

This avoid suggesting code which hits a rustc bug. Basically `&{x}` won't use auto-deref if the target type is `Sized`.

changelog: Don't suggest using auto deref for block expressions when the target type is `Sized`
changelog: Include the borrow in the suggestion for `explicit_auto_deref`
changelog: Don't lint `explicit_auto_deref` on `dyn Trait` return
changelog: Don't lint `explicit_auto_deref` when other adjustments are required
@bors bors closed this as completed in 4912c0e Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants