Skip to content

Commit

Permalink
Auto merge of #7223 - ThibsG:FpUselessConversion7205, r=camsteffen
Browse files Browse the repository at this point in the history
Fix FPs about generic args

Fix 2 false positives in [`use_self`] and [`useless_conversion`] lints, by taking into account generic args and comparing them.

Fixes: #7205
Fixes: #7206

changelog: Fix FPs about generic args in [`use_self`] and [`useless_conversion`] lints
  • Loading branch information
bors committed May 17, 2021
2 parents cec8d95 + 2fb35ce commit 1aad754
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 14 deletions.
10 changes: 5 additions & 5 deletions clippy_lints/src/use_self.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::same_type_and_consts;
use clippy_utils::{in_macro, meets_msrv, msrvs};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def::DefKind;
use rustc_hir::{
def,
self as hir,
def::{self, DefKind},
def_id::LocalDefId,
intravisit::{walk_ty, NestedVisitorMap, Visitor},
Expr, ExprKind, FnRetTy, FnSig, GenericArg, HirId, Impl, ImplItemKind, Item, ItemKind, Node, Path, PathSegment,
QPath, TyKind,
};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::hir::map::Map;
use rustc_middle::ty::{AssocKind, Ty, TyS};
use rustc_middle::ty::{AssocKind, Ty};
use rustc_semver::RustcVersion;
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::{BytePos, Span};
Expand Down Expand Up @@ -459,7 +459,7 @@ fn in_impl(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'_>) -> bool {

fn should_lint_ty(hir_ty: &hir::Ty<'_>, ty: Ty<'_>, self_ty: Ty<'_>) -> bool {
if_chain! {
if TyS::same_type(ty, self_ty);
if same_type_and_consts(ty, self_ty);
if let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind;
then {
!matches!(path.res, def::Res::SelfTy(..))
Expand Down
15 changes: 8 additions & 7 deletions clippy_lints/src/useless_conversion.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
use clippy_utils::source::{snippet, snippet_with_macro_callsite};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::ty::{is_type_diagnostic_item, same_type_and_consts};
use clippy_utils::{get_parent_expr, match_def_path, match_trait_method, paths};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, HirId, MatchSource};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{self, TyS};
use rustc_middle::ty;
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::sym;

Expand Down Expand Up @@ -67,7 +67,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
if match_trait_method(cx, e, &paths::INTO) && &*name.ident.as_str() == "into" {
let a = cx.typeck_results().expr_ty(e);
let b = cx.typeck_results().expr_ty(&args[0]);
if TyS::same_type(a, b) {
if same_type_and_consts(a, b) {
let sugg = snippet_with_macro_callsite(cx, args[0].span, "<expr>").to_string();
span_lint_and_sugg(
cx,
Expand All @@ -90,7 +90,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
}
let a = cx.typeck_results().expr_ty(e);
let b = cx.typeck_results().expr_ty(&args[0]);
if TyS::same_type(a, b) {
if same_type_and_consts(a, b) {
let sugg = snippet(cx, args[0].span, "<expr>").into_owned();
span_lint_and_sugg(
cx,
Expand All @@ -110,7 +110,8 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
if is_type_diagnostic_item(cx, a, sym::result_type);
if let ty::Adt(_, substs) = a.kind();
if let Some(a_type) = substs.types().next();
if TyS::same_type(a_type, b);
if same_type_and_consts(a_type, b);

then {
span_lint_and_help(
cx,
Expand All @@ -137,7 +138,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
if is_type_diagnostic_item(cx, a, sym::result_type);
if let ty::Adt(_, substs) = a.kind();
if let Some(a_type) = substs.types().next();
if TyS::same_type(a_type, b);
if same_type_and_consts(a_type, b);

then {
let hint = format!("consider removing `{}()`", snippet(cx, path.span, "TryFrom::try_from"));
Expand All @@ -154,7 +155,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {

if_chain! {
if match_def_path(cx, def_id, &paths::FROM_FROM);
if TyS::same_type(a, b);
if same_type_and_consts(a, b);

then {
let sugg = Sugg::hir_with_macro_callsite(cx, &args[0], "<expr>").maybe_par();
Expand Down
24 changes: 24 additions & 0 deletions clippy_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,3 +322,27 @@ pub fn walk_ptrs_ty_depth(ty: Ty<'_>) -> (Ty<'_>, usize) {
}
inner(ty, 0)
}

/// Returns `true` if types `a` and `b` are same types having same `Const` generic args,
/// otherwise returns `false`
pub fn same_type_and_consts(a: Ty<'tcx>, b: Ty<'tcx>) -> bool {
match (&a.kind(), &b.kind()) {
(&ty::Adt(did_a, substs_a), &ty::Adt(did_b, substs_b)) => {
if did_a != did_b {
return false;
}

substs_a
.iter()
.zip(substs_b.iter())
.all(|(arg_a, arg_b)| match (arg_a.unpack(), arg_b.unpack()) {
(GenericArgKind::Const(inner_a), GenericArgKind::Const(inner_b)) => inner_a == inner_b,
(GenericArgKind::Type(type_a), GenericArgKind::Type(type_b)) => {
same_type_and_consts(type_a, type_b)
},
_ => true,
})
},
_ => a == b,
}
}
30 changes: 30 additions & 0 deletions tests/ui/use_self.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -462,3 +462,33 @@ mod issue6818 {
a: i32,
}
}

mod issue7206 {
struct MyStruct<const C: char>;
impl From<MyStruct<'a'>> for MyStruct<'b'> {
fn from(_s: MyStruct<'a'>) -> Self {
Self
}
}

// keep linting non-`Const` generic args
struct S<'a> {
inner: &'a str,
}

struct S2<T> {
inner: T,
}

impl<T> S2<T> {
fn new() -> Self {
unimplemented!();
}
}

impl<'a> S2<S<'a>> {
fn new_again() -> Self {
Self::new()
}
}
}
30 changes: 30 additions & 0 deletions tests/ui/use_self.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,3 +462,33 @@ mod issue6818 {
a: i32,
}
}

mod issue7206 {
struct MyStruct<const C: char>;
impl From<MyStruct<'a'>> for MyStruct<'b'> {
fn from(_s: MyStruct<'a'>) -> Self {
Self
}
}

// keep linting non-`Const` generic args
struct S<'a> {
inner: &'a str,
}

struct S2<T> {
inner: T,
}

impl<T> S2<T> {
fn new() -> Self {
unimplemented!();
}
}

impl<'a> S2<S<'a>> {
fn new_again() -> Self {
S2::new()
}
}
}
8 changes: 7 additions & 1 deletion tests/ui/use_self.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -162,5 +162,11 @@ error: unnecessary structure name repetition
LL | A::new::<submod::B>(submod::B {})
| ^ help: use the applicable keyword: `Self`

error: aborting due to 27 previous errors
error: unnecessary structure name repetition
--> $DIR/use_self.rs:491:13
|
LL | S2::new()
| ^^ help: use the applicable keyword: `Self`

error: aborting due to 28 previous errors

19 changes: 19 additions & 0 deletions tests/ui/useless_conversion.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,23 @@ fn main() {
let a: i32 = 1;
let b: i32 = 1;
let _ = (a + b) * 3;

// see #7205
let s: Foo<'a'> = Foo;
let _: Foo<'b'> = s.into();
let s2: Foo<'a'> = Foo;
let _: Foo<'a'> = s2;
let s3: Foo<'a'> = Foo;
let _ = s3;
let s4: Foo<'a'> = Foo;
let _ = vec![s4, s4, s4].into_iter();
}

#[derive(Copy, Clone)]
struct Foo<const C: char>;

impl From<Foo<'a'>> for Foo<'b'> {
fn from(_s: Foo<'a'>) -> Self {
Foo
}
}
19 changes: 19 additions & 0 deletions tests/ui/useless_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,23 @@ fn main() {
let a: i32 = 1;
let b: i32 = 1;
let _ = i32::from(a + b) * 3;

// see #7205
let s: Foo<'a'> = Foo;
let _: Foo<'b'> = s.into();
let s2: Foo<'a'> = Foo;
let _: Foo<'a'> = s2.into();
let s3: Foo<'a'> = Foo;
let _ = Foo::<'a'>::from(s3);
let s4: Foo<'a'> = Foo;
let _ = vec![s4, s4, s4].into_iter().into_iter();
}

#[derive(Copy, Clone)]
struct Foo<const C: char>;

impl From<Foo<'a'>> for Foo<'b'> {
fn from(_s: Foo<'a'>) -> Self {
Foo
}
}
20 changes: 19 additions & 1 deletion tests/ui/useless_conversion.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,23 @@ error: useless conversion to the same type: `i32`
LL | let _ = i32::from(a + b) * 3;
| ^^^^^^^^^^^^^^^^ help: consider removing `i32::from()`: `(a + b)`

error: aborting due to 11 previous errors
error: useless conversion to the same type: `Foo<'a'>`
--> $DIR/useless_conversion.rs:78:23
|
LL | let _: Foo<'a'> = s2.into();
| ^^^^^^^^^ help: consider removing `.into()`: `s2`

error: useless conversion to the same type: `Foo<'a'>`
--> $DIR/useless_conversion.rs:80:13
|
LL | let _ = Foo::<'a'>::from(s3);
| ^^^^^^^^^^^^^^^^^^^^ help: consider removing `Foo::<'a'>::from()`: `s3`

error: useless conversion to the same type: `std::vec::IntoIter<Foo<'a'>>`
--> $DIR/useless_conversion.rs:82:13
|
LL | let _ = vec![s4, s4, s4].into_iter().into_iter();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![s4, s4, s4].into_iter()`

error: aborting due to 14 previous errors

0 comments on commit 1aad754

Please sign in to comment.