From daab21ef9d161cc6b70f9e51f0a70cebd1628dc0 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 19 Jul 2024 21:39:46 +0700 Subject: [PATCH 1/6] Pulicize `clippy_utils::ty::ty_from_hir_ty` And use it in the next commit to avoid ICE. --- clippy_lints/src/zero_sized_map_values.rs | 17 ++--------------- clippy_utils/src/lib.rs | 1 + clippy_utils/src/ty/mod.rs | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/zero_sized_map_values.rs b/clippy_lints/src/zero_sized_map_values.rs index 1221abec1abf..3f64d18e1ad4 100644 --- a/clippy_lints/src/zero_sized_map_values.rs +++ b/clippy_lints/src/zero_sized_map_values.rs @@ -1,10 +1,9 @@ use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::ty::{is_normalizable, is_type_diagnostic_item}; +use clippy_utils::ty::{is_normalizable, is_type_diagnostic_item, ty_from_hir_ty}; use rustc_hir::{self as hir, AmbigArg, HirId, ItemKind, Node}; -use rustc_hir_analysis::lower_ty; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::layout::LayoutOf as _; -use rustc_middle::ty::{self, Ty, TypeVisitableExt}; +use rustc_middle::ty::{self, TypeVisitableExt}; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -82,15 +81,3 @@ fn in_trait_impl(cx: &LateContext<'_>, hir_id: HirId) -> bool { } false } - -fn ty_from_hir_ty<'tcx>(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'tcx>) -> Ty<'tcx> { - cx.maybe_typeck_results() - .and_then(|results| { - if results.hir_owner == hir_ty.hir_id.owner { - results.node_type_opt(hir_ty.hir_id) - } else { - None - } - }) - .unwrap_or_else(|| lower_ty(cx.tcx, hir_ty)) -} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 2f5639b686b8..23bb44dc1f13 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -39,6 +39,7 @@ extern crate rustc_data_structures; extern crate rustc_driver; extern crate rustc_errors; extern crate rustc_hir; +extern crate rustc_hir_analysis; extern crate rustc_hir_typeck; extern crate rustc_index; extern crate rustc_infer; diff --git a/clippy_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index f2bbdda70585..befca19f485e 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -10,6 +10,7 @@ use rustc_hir as hir; use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; use rustc_hir::def_id::DefId; use rustc_hir::{Expr, FnDecl, LangItem, TyKind}; +use rustc_hir_analysis::lower_ty; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::mir::ConstValue; @@ -36,6 +37,19 @@ use crate::{def_path_def_ids, match_def_path, path_res}; mod type_certainty; pub use type_certainty::expr_type_is_certain; +/// Lower a [`hir::Ty`] to a [`rustc_middle::Ty`]. +pub fn ty_from_hir_ty<'tcx>(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'tcx>) -> Ty<'tcx> { + cx.maybe_typeck_results() + .and_then(|results| { + if results.hir_owner == hir_ty.hir_id.owner { + results.node_type_opt(hir_ty.hir_id) + } else { + None + } + }) + .unwrap_or_else(|| lower_ty(cx.tcx, hir_ty)) +} + /// Checks if the given type implements copy. pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { cx.type_is_copy_modulo_regions(ty) From 83b97ae7139933bc17de6b5ccc8b01073e3d4949 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 19 Jul 2024 21:42:54 +0700 Subject: [PATCH 2/6] Use `clippy_utils::ty::ty_from_hir_ty` to avoid ICE --- clippy_lints/src/use_self.rs | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 84b6430294f3..e0efc536e34b 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -2,7 +2,7 @@ use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::is_from_proc_macro; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::ty::same_type_and_consts; +use clippy_utils::ty::{same_type_and_consts, ty_from_hir_ty}; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir::def::{CtorOf, DefKind, Res}; @@ -12,7 +12,6 @@ use rustc_hir::{ self as hir, AmbigArg, Expr, ExprKind, FnRetTy, FnSig, GenericArgsParentheses, GenericParam, GenericParamKind, HirId, Impl, ImplItemKind, Item, ItemKind, Pat, PatKind, Path, QPath, Ty, TyKind, }; -use rustc_hir_analysis::lower_ty; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::Ty as MiddleTy; use rustc_session::impl_lint_pass; @@ -73,7 +72,6 @@ impl UseSelf { enum StackItem { Check { impl_id: LocalDefId, - in_body: u32, types_to_skip: FxHashSet, }, NoCheck, @@ -117,7 +115,6 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { .collect(); StackItem::Check { impl_id: item.owner_id.def_id, - in_body: 0, types_to_skip, } } else { @@ -186,27 +183,12 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { } } - fn check_body(&mut self, _: &LateContext<'_>, _: &hir::Body<'_>) { - // `lower_ty` cannot be called in `Body`s or it will panic (sometimes). But in bodies - // we can use `cx.typeck_results.node_type(..)` to get the `ty::Ty` from a `hir::Ty`. - // However the `node_type()` method can *only* be called in bodies. - if let Some(&mut StackItem::Check { ref mut in_body, .. }) = self.stack.last_mut() { - *in_body = in_body.saturating_add(1); - } - } - - fn check_body_post(&mut self, _: &LateContext<'_>, _: &hir::Body<'_>) { - if let Some(&mut StackItem::Check { ref mut in_body, .. }) = self.stack.last_mut() { - *in_body = in_body.saturating_sub(1); - } - } fn check_ty(&mut self, cx: &LateContext<'tcx>, hir_ty: &Ty<'tcx, AmbigArg>) { if !hir_ty.span.from_expansion() && self.msrv.meets(msrvs::TYPE_ALIAS_ENUM_VARIANTS) && let Some(&StackItem::Check { impl_id, - in_body, ref types_to_skip, }) = self.stack.last() && let TyKind::Path(QPath::Resolved(_, path)) = hir_ty.kind @@ -215,12 +197,7 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { Res::SelfTyParam { .. } | Res::SelfTyAlias { .. } | Res::Def(DefKind::TyParam, _) ) && !types_to_skip.contains(&hir_ty.hir_id) - && let ty = if in_body > 0 { - cx.typeck_results().node_type(hir_ty.hir_id) - } else { - // We don't care about ignoring infer vars here - lower_ty(cx.tcx, hir_ty.as_unambig_ty()) - } + && let ty = ty_from_hir_ty(cx, hir_ty) && let impl_ty = cx.tcx.type_of(impl_id).instantiate_identity() && same_type_and_consts(ty, impl_ty) // Ensure the type we encounter and the one from the impl have the same lifetime parameters. It may be that From 925718d8eb586d143de73e7eb3428cafb25077db Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Sun, 21 Jul 2024 00:24:35 +0700 Subject: [PATCH 3/6] Make the "expensive" comment belong to a branch --- clippy_lints/src/use_self.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index e0efc536e34b..994ca9b66906 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -94,8 +94,8 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { .as_ref() .is_none_or(|params| params.parenthesized == GenericArgsParentheses::No) && !item.span.from_expansion() + // expensive, should be last check && !is_from_proc_macro(cx, item) - // expensive, should be last check { // Self cannot be used inside const generic parameters let types_to_skip = generics From e3e6e6ea4131db92ff23a0e32eb40a353f50f8f6 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Thu, 25 Jul 2024 04:13:15 +0700 Subject: [PATCH 4/6] add bug 13092 --- tests/ui/use_self.fixed | 24 ++++++++++++++++++++++++ tests/ui/use_self.rs | 24 ++++++++++++++++++++++++ tests/ui/use_self.stderr | 22 +++++++++++++++++++++- 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index ffc5b74d7bd6..c6358a90f1e5 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -667,3 +667,27 @@ mod issue_10371 { } } } + +mod issue_13092 { + use std::cell::RefCell; + macro_rules! macro_inner_item { + ($ty:ty) => { + fn foo(_: $ty) { + fn inner(_: $ty) {} + } + }; + } + + #[derive(Default)] + struct MyStruct; + + impl MyStruct { + macro_inner_item!(Self); + } + + impl MyStruct { + thread_local! { + static SPECIAL: RefCell = RefCell::default(); + } + } +} diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index eb9d96168bcd..c900341cadd8 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -667,3 +667,27 @@ mod issue_10371 { } } } + +mod issue_13092 { + use std::cell::RefCell; + macro_rules! macro_inner_item { + ($ty:ty) => { + fn foo(_: $ty) { + fn inner(_: $ty) {} + } + }; + } + + #[derive(Default)] + struct MyStruct; + + impl MyStruct { + macro_inner_item!(MyStruct); + } + + impl MyStruct { + thread_local! { + static SPECIAL: RefCell = RefCell::default(); + } + } +} diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index bd5b685b45d5..06260a598be2 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -259,5 +259,25 @@ error: unnecessary structure name repetition LL | E::A => {}, | ^ help: use the applicable keyword: `Self` -error: aborting due to 43 previous errors +error: unnecessary structure name repetition + --> tests/ui/use_self.rs:685:27 + | +LL | macro_inner_item!(MyStruct); + | ^^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> tests/ui/use_self.rs:690:37 + | +LL | static SPECIAL: RefCell = RefCell::default(); + | ^^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> tests/ui/use_self.rs:690:37 + | +LL | static SPECIAL: RefCell = RefCell::default(); + | ^^^^^^^^ help: use the applicable keyword: `Self` + | + = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` + +error: aborting due to 46 previous errors From bcfd0d1aba7b63dd2b571771885375f9e1f66d1f Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Thu, 5 Sep 2024 00:58:33 +0700 Subject: [PATCH 5/6] Skip `use_self` inside macro expansion of `impl Self` items --- clippy_lints/src/use_self.rs | 14 +++++++++++++- tests/ui/use_self.fixed | 4 ++-- tests/ui/use_self.stderr | 22 +--------------------- 3 files changed, 16 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 994ca9b66906..0d56cf5c2b27 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -128,6 +128,11 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { } fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &hir::ImplItem<'_>) { + // Checking items of `impl Self` blocks in which macro expands into. + if impl_item.span.from_expansion() { + self.stack.push(StackItem::NoCheck); + return; + } // We want to skip types in trait `impl`s that aren't declared as `Self` in the trait // declaration. The collection of those types is all this method implementation does. if let ImplItemKind::Fn(FnSig { decl, .. }, ..) = impl_item.kind @@ -183,6 +188,13 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { } } + fn check_impl_item_post(&mut self, _: &LateContext<'_>, impl_item: &hir::ImplItem<'_>) { + if impl_item.span.from_expansion() + && let Some(StackItem::NoCheck) = self.stack.last() + { + self.stack.pop(); + } + } fn check_ty(&mut self, cx: &LateContext<'tcx>, hir_ty: &Ty<'tcx, AmbigArg>) { if !hir_ty.span.from_expansion() @@ -197,7 +209,7 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { Res::SelfTyParam { .. } | Res::SelfTyAlias { .. } | Res::Def(DefKind::TyParam, _) ) && !types_to_skip.contains(&hir_ty.hir_id) - && let ty = ty_from_hir_ty(cx, hir_ty) + && let ty = ty_from_hir_ty(cx, hir_ty.as_unambig_ty()) && let impl_ty = cx.tcx.type_of(impl_id).instantiate_identity() && same_type_and_consts(ty, impl_ty) // Ensure the type we encounter and the one from the impl have the same lifetime parameters. It may be that diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index c6358a90f1e5..572e919fb2d2 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -682,12 +682,12 @@ mod issue_13092 { struct MyStruct; impl MyStruct { - macro_inner_item!(Self); + macro_inner_item!(MyStruct); } impl MyStruct { thread_local! { - static SPECIAL: RefCell = RefCell::default(); + static SPECIAL: RefCell = RefCell::default(); } } } diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index 06260a598be2..bd5b685b45d5 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -259,25 +259,5 @@ error: unnecessary structure name repetition LL | E::A => {}, | ^ help: use the applicable keyword: `Self` -error: unnecessary structure name repetition - --> tests/ui/use_self.rs:685:27 - | -LL | macro_inner_item!(MyStruct); - | ^^^^^^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> tests/ui/use_self.rs:690:37 - | -LL | static SPECIAL: RefCell = RefCell::default(); - | ^^^^^^^^ help: use the applicable keyword: `Self` - -error: unnecessary structure name repetition - --> tests/ui/use_self.rs:690:37 - | -LL | static SPECIAL: RefCell = RefCell::default(); - | ^^^^^^^^ help: use the applicable keyword: `Self` - | - = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` - -error: aborting due to 46 previous errors +error: aborting due to 43 previous errors From 9ea2b6501e3bd3a6a3d030e1215593f00d8f9ed8 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Sat, 7 Sep 2024 01:15:55 +0700 Subject: [PATCH 6/6] add test to check for popping wrong items co-authored-by: Alex Macleod --- tests/ui/use_self.fixed | 21 +++++++++++++++++++++ tests/ui/use_self.rs | 21 +++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed index 572e919fb2d2..b44840d440b2 100644 --- a/tests/ui/use_self.fixed +++ b/tests/ui/use_self.fixed @@ -691,3 +691,24 @@ mod issue_13092 { } } } + +mod crash_check_13128 { + struct A; + + impl A { + fn a() { + struct B; + + // pushes a NoCheck + impl Iterator for &B { + // Pops the NoCheck + type Item = A; + + // Lints A -> Self + fn next(&mut self) -> Option { + Some(A) + } + } + } + } +} diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index c900341cadd8..342c724c8e48 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -691,3 +691,24 @@ mod issue_13092 { } } } + +mod crash_check_13128 { + struct A; + + impl A { + fn a() { + struct B; + + // pushes a NoCheck + impl Iterator for &B { + // Pops the NoCheck + type Item = A; + + // Lints A -> Self + fn next(&mut self) -> Option { + Some(A) + } + } + } + } +}