rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.66k stars 12.75k forks source link

The compiler suggests the same thing twice #85527

Open YohDeadfall opened 3 years ago

YohDeadfall commented 3 years ago

I tried this code:

use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet;
use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, LangItem};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;

declare_clippy_lint! {
    pub SLICE_FULL_RANGE,
    complexity,
    "useless full ranged slice creation"
}

declare_lint_pass!(SliceFullRange => [SLICE_FULL_RANGE]);

impl<'tcx> LateLintPass<'tcx> for SliceFullRange {
    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
        if_chain! {
            if let ExprKind::Index(var, range) = expr.kind;
            if is_slice_or_str(cx, var);
            if is_full_range(cx, range);
            then {
                span_lint_and_sugg(
                    cx,
                    SLICE_FULL_RANGE,
                    expr.span,
                    "getting a full range should be not necessary",
                    "try",
                    snippet(cx, var.span, ".."),
                    Applicability::MachineApplicable,
                )
            }
        }
    }
}

fn is_full_range(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
    let ty = cx.typeck_results().expr_ty(expr);
    let ty = ty.peel_refs();
    is_type_lang_item(cx, ty, LangItem::RangeFull)
}

fn is_slice_or_str(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
    let ty = cx.typeck_results().expr_ty(expr);
    let ty = ty.peel_refs();

    is_type_diagnostic_item(cx, ty, sym::slice)
    || is_type_diagnostic_item(cx, ty, sym::str)
    || is_type_diagnostic_item(cx, ty, sym::string_type)
    || is_type_diagnostic_item(cx, ty, sym::vec_type)
}

The code above contains an error which is correctly reported by the compiler, but it suggests the same fix twice:

error[E0308]: mismatched types
  --> clippy_lints\src\slice_full_range.rs:43:21
   |
43 |                     snippet(cx, var.span, ".."),
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found enum `Cow`
   |
   = note: expected struct `std::string::String`
                found enum `Cow<'_, str>`
help: try using a conversion method
   |
43 |                     snippet(cx, var.span, "..").to_string(),
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
43 |                     snippet(cx, var.span, "..").to_string(),
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Meta

rustc --version --verbose:

rustc 1.54.0-nightly (bacf770f2 2021-05-05)
binary: rustc
commit-hash: bacf770f2983a52f31e3537db5f0fe1ef2eaa874
commit-date: 2021-05-05
host: x86_64-pc-windows-msvc
release: 1.54.0-nightly
LLVM version: 12.0.0
estebank commented 3 years ago

Looking at the following code:

https://github.com/rust-lang/rust/blob/99e3aef02079e9c10583638520cd0c134dc3a01d/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs#L223-L258

The simplest solution might be to actually .collect() suggestions before it is used at

https://github.com/rust-lang/rust/blob/99e3aef02079e9c10583638520cd0c134dc3a01d/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs#L252-L257

but looking closer, it seems to me that we could change get_conversion_methods to sort() and dedup() before returning, which should fix every instance of this bug.

Rustin170506 commented 3 years ago

Let me try to fix it.

@rustbot claim

Rustin170506 commented 3 years ago

@estebank Could you please help provide some minimal reproduction? I tried but couldn't reproduce it minimally. Thanks!

estebank commented 3 years ago

@hi-rustin My attempt at making a repro case didn't manage to cause the duplicated suggestion. I'm guessing there might be something in the if_chain macro that triggers this, but don't know what it might be.

Rustin170506 commented 3 years ago

@estebank I tried to combine it with if_chain and reproduce it, but it still didn't reproduce.

asquared31415 commented 3 years ago

Crated a minimal reproduction: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=ce21df627aef988eb4d4e67e2ac7ef81

It seems that for some reason having a use item with two identical components (foo::foo or in the original code if_chain::if_chain) causes this behavior.

asquared31415 commented 3 years ago

This may be system dependent, see my comment on the PR opened to try and reproduce and fix this. comment

inquisitivecrystal commented 3 years ago

I'm going to see if I can reproduce locally. If I can, I could hopefully fix it. I might also fiddle around with docker and see if that allows me to generate a reproduction.

inquisitivecrystal commented 3 years ago

Not only can I not reproduce locally, but I can't reproduce even trying to replicate the rust playground setup through docker. sighs

estebank commented 3 years ago

It feels like rustc is gaslighting us, doesn't it? 😩

inquisitivecrystal commented 3 years ago

Not exactly how I'd say it, but once you put it that way... yeah, it really does.

The fact that I tried docker shouldn't dissuade anyone else who would otherwise try it, for a few reasons. First, this is just weird enough that it might occur with docker on your computer but not on mine. Second, I'm not experienced enough to be confident I did everything right.

I'm going to yank the E-easy label. This may be easy to fix, but it's so hard to reproduce that you wouldn't even know whether you'd successfully fixed it.