From 1384c1492b9d7d682811dbb562c77285019b6afd Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 9 Sep 2021 10:23:06 -0400 Subject: [PATCH] Fix internal lint checking `match_type` uses * Check for `is_item` instead * Check consts and statics from external crates * Check for lang items * Check for inherent functions which have the same name as a field --- clippy_lints/src/lib.rs | 8 +- clippy_lints/src/utils/internal_lints.rs | 190 ++++++++++++++-------- tests/ui-internal/auxiliary/paths.rs | 2 + tests/ui-internal/is_item_def_path.fixed | 43 +++++ tests/ui-internal/is_item_def_path.rs | 43 +++++ tests/ui-internal/is_item_def_path.stderr | 73 +++++++++ 6 files changed, 289 insertions(+), 70 deletions(-) create mode 100644 tests/ui-internal/auxiliary/paths.rs create mode 100644 tests/ui-internal/is_item_def_path.fixed create mode 100644 tests/ui-internal/is_item_def_path.rs create mode 100644 tests/ui-internal/is_item_def_path.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 92a13be81f41..d5e9cd9b0ffa 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -519,9 +519,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: #[cfg(feature = "internal-lints")] utils::internal_lints::INVALID_PATHS, #[cfg(feature = "internal-lints")] - utils::internal_lints::LINT_WITHOUT_LINT_PASS, + utils::internal_lints::IS_ITEM_DEF_PATH_ON_DIAGNOSTIC_OR_LANG_ITEM, #[cfg(feature = "internal-lints")] - utils::internal_lints::MATCH_TYPE_ON_DIAGNOSTIC_ITEM, + utils::internal_lints::LINT_WITHOUT_LINT_PASS, #[cfg(feature = "internal-lints")] utils::internal_lints::OUTER_EXPN_EXPN_DATA, #[cfg(feature = "internal-lints")] @@ -1170,8 +1170,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(utils::internal_lints::IF_CHAIN_STYLE), LintId::of(utils::internal_lints::INTERNING_DEFINED_SYMBOL), LintId::of(utils::internal_lints::INVALID_PATHS), + LintId::of(utils::internal_lints::IS_ITEM_DEF_PATH_ON_DIAGNOSTIC_OR_LANG_ITEM), LintId::of(utils::internal_lints::LINT_WITHOUT_LINT_PASS), - LintId::of(utils::internal_lints::MATCH_TYPE_ON_DIAGNOSTIC_ITEM), LintId::of(utils::internal_lints::OUTER_EXPN_EXPN_DATA), LintId::of(utils::internal_lints::PRODUCE_ICE), LintId::of(utils::internal_lints::UNNECESSARY_SYMBOL_STR), @@ -1846,7 +1846,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(utils::internal_lints::InvalidPaths)); store.register_late_pass(|| Box::new(utils::internal_lints::InterningDefinedSymbol::default())); store.register_late_pass(|| Box::new(utils::internal_lints::LintWithoutLintPass::default())); - store.register_late_pass(|| Box::new(utils::internal_lints::MatchTypeOnDiagItem)); + store.register_late_pass(|| Box::new(utils::internal_lints::IsItemDefPath)); store.register_late_pass(|| Box::new(utils::internal_lints::OuterExpnDataPass)); } diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index 070a05031b05..9f062863f8c9 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -3,7 +3,8 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_sug use clippy_utils::higher; use clippy_utils::source::snippet; use clippy_utils::{ - is_else_clause, is_expn_of, is_item, is_lint_allowed, method_calls, path_to_res, paths, SpanlessEq, + is_else_clause, is_expn_of, is_item, is_lint_allowed, method_calls, path_to_res, paths, peel_hir_expr_refs, + SpanlessEq, }; use if_chain::if_chain; use rustc_ast::ast::{Crate as AstCrate, ItemKind, LitKind, ModKind, NodeId}; @@ -16,13 +17,13 @@ use rustc_hir::def_id::DefId; use rustc_hir::hir_id::CRATE_HIR_ID; use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; use rustc_hir::{ - BinOpKind, Block, Crate, Expr, ExprKind, HirId, Item, Local, MutTy, Mutability, Node, Path, Stmt, StmtKind, Ty, - TyKind, UnOp, + BinOpKind, Block, Crate, Expr, ExprKind, HirId, Item, Local, MutTy, Mutability, Node, Path, Stmt, StmtKind, TyKind, + UnOp, }; use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; -use rustc_middle::mir::interpret::ConstValue; -use rustc_middle::ty; +use rustc_middle::mir::interpret::{Allocation, ConstValue, GlobalAlloc}; +use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Spanned; use rustc_span::symbol::{Symbol, SymbolStr}; @@ -30,6 +31,7 @@ use rustc_span::{BytePos, Span}; use rustc_typeck::hir_ty_to_ty; use std::borrow::{Borrow, Cow}; +use std::str; #[cfg(feature = "metadata-collector-lint")] pub mod metadata_collector; @@ -226,10 +228,10 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for calls to `clippy_utils::is_item()` taking a path to a diagnostic item. + /// Checks for calls to `clippy_utils::is_item()` taking a path to a diagnostic or lang item. /// /// ### Why is this bad? - /// Using diagnostic items doesn't require hard-coded paths. + /// Diagnostic or lang items don't require hard-coded paths. /// /// ### Example /// Bad: @@ -241,9 +243,9 @@ declare_clippy_lint! { /// ```rust,ignore /// clippy_utils::is_item(cx, ty, sym::vec_type) /// ``` - pub MATCH_TYPE_ON_DIAGNOSTIC_ITEM, + pub IS_ITEM_DEF_PATH_ON_DIAGNOSTIC_OR_LANG_ITEM, internal, - "using `clippy_utils::is_item()` with a path to a diagnostic item" + "using `clippy_utils::is_item()` with a path to a diagnostic or lang item" } declare_clippy_lint! { @@ -436,7 +438,7 @@ impl<'tcx> LateLintPass<'tcx> for LintWithoutLintPass { } } -fn is_lint_ref_type<'tcx>(cx: &LateContext<'tcx>, ty: &Ty<'_>) -> bool { +fn is_lint_ref_type<'tcx>(cx: &LateContext<'tcx>, ty: &hir::Ty<'_>) -> bool { if let TyKind::Rptr( _, MutTy { @@ -752,90 +754,146 @@ fn suggest_note( ); } -declare_lint_pass!(MatchTypeOnDiagItem => [MATCH_TYPE_ON_DIAGNOSTIC_ITEM]); +declare_lint_pass!(IsItemDefPath => [IS_ITEM_DEF_PATH_ON_DIAGNOSTIC_OR_LANG_ITEM]); -impl<'tcx> LateLintPass<'tcx> for MatchTypeOnDiagItem { +impl<'tcx> LateLintPass<'tcx> for IsItemDefPath { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { - if is_lint_allowed(cx, MATCH_TYPE_ON_DIAGNOSTIC_ITEM, expr.hir_id) { + if is_lint_allowed(cx, IS_ITEM_DEF_PATH_ON_DIAGNOSTIC_OR_LANG_ITEM, expr.hir_id) { return; } if_chain! { - // Check if this is a call to utils::is_item() - if let ExprKind::Call(fn_path, [context, ty, ty_path]) = expr.kind; - if is_item(cx, fn_path, &["clippy_utils", "is_item"]); + if let ExprKind::Call(func, [_, _, item_arg]) = expr.kind; + if is_item(cx, func, &["clippy_utils", "is_item"]); // Extract the path to the matched type - if let Some(segments) = path_to_matched_type(cx, ty_path); + if let Some(segments) = path_to_matched_type(cx, item_arg); let segments: Vec<&str> = segments.iter().map(|sym| &**sym).collect(); - if let Some(ty_did) = path_to_res(cx, &segments[..]).opt_def_id(); - // Check if the matched type is a diagnostic item - let diag_items = cx.tcx.diagnostic_items(ty_did.krate); - if let Some(item_name) = diag_items.iter().find_map(|(k, v)| if *v == ty_did { Some(k) } else { None }); + if let Some(def_id) = path_to_res(cx, &segments[..]).opt_def_id(); then { - // TODO: check paths constants from external crates. - let cx_snippet = snippet(cx, context.span, "_"); - let ty_snippet = snippet(cx, ty.span, "_"); + // path_to_res will match field names before anything else, but for this we want to match + // inherent functions first. + let def_id = if cx.tcx.def_kind(def_id) == DefKind::Field { + let method_name = *segments.last().unwrap(); + cx.tcx.def_key(def_id).parent + .and_then(|parent_idx| + cx.tcx.inherent_impls(DefId { index: parent_idx, krate: def_id.krate}).iter() + .flat_map(|impl_id| cx.tcx.item_children(*impl_id).iter()) + .find(|item| item.ident.name.as_str() == method_name) + ) + .and_then(|item| item.res.opt_def_id()) + .unwrap_or(def_id) + } else { + def_id + }; - span_lint_and_sugg( + let diag_items = cx.tcx.diagnostic_items(def_id.krate); + let (msg, replacement) = if let Some(item_name) = diag_items.iter() + .find_map(|(k, v)| if *v == def_id { Some(k) } else { None }) + { + ( + "usage of `clippy_utils::is_item()` with a path to a diagnostic item", + format!("sym::{}", item_name), + ) + } else if let Some(lang_item) = cx.tcx.lang_items().items().iter().position(|id| *id == Some(def_id)) { + let lang_items = path_to_res(cx, &["rustc_hir", "lang_items", "LangItem"]).def_id(); + let item_name = cx.tcx.adt_def(lang_items).variants.iter().nth(lang_item).unwrap() + .ident.name.as_str(); + ( + "usage of `clippy_utils::is_item()` with a path to a lang item", + format!("LangItem::{}", item_name), + ) + } else { + return; + }; + span_lint_and_then( cx, - MATCH_TYPE_ON_DIAGNOSTIC_ITEM, + IS_ITEM_DEF_PATH_ON_DIAGNOSTIC_OR_LANG_ITEM, expr.span, - "usage of `clippy_utils::is_item()` on a path to a diagnostic item", - "try", - format!("clippy_utils::is_item({}, {}, sym::{})", cx_snippet, ty_snippet, item_name), - Applicability::MaybeIncorrect, + msg, + |diag| { + diag.span_suggestion( + item_arg.span, + "try", + replacement, + Applicability::MachineApplicable, + ); + } ); } } } } -fn path_to_matched_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option> { - use rustc_hir::ItemKind; - - match &expr.kind { - ExprKind::AddrOf(.., expr) => return path_to_matched_type(cx, expr), - ExprKind::Path(qpath) => match cx.qpath_res(qpath, expr.hir_id) { +fn path_to_matched_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option> { + match peel_hir_expr_refs(expr).0.kind { + ExprKind::Path(ref qpath) => match cx.qpath_res(qpath, expr.hir_id) { Res::Local(hir_id) => { let parent_id = cx.tcx.hir().get_parent_node(hir_id); - if let Some(Node::Local(local)) = cx.tcx.hir().find(parent_id) { - if let Some(init) = local.init { - return path_to_matched_type(cx, init); - } + if let Some(Node::Local(Local { init: Some(init), .. })) = cx.tcx.hir().find(parent_id) { + path_to_matched_type(cx, init) + } else { + None } }, - Res::Def(DefKind::Const | DefKind::Static, def_id) => { - if let Some(Node::Item(item)) = cx.tcx.hir().get_if_local(def_id) { - if let ItemKind::Const(.., body_id) | ItemKind::Static(.., body_id) = item.kind { - let body = cx.tcx.hir().body(body_id); - return path_to_matched_type(cx, &body.value); - } - } + Res::Def(DefKind::Static, def_id) => { + read_mir_alloc_def_path(cx, cx.tcx.eval_static_initializer(def_id).ok()?, cx.tcx.type_of(def_id)) }, - _ => {}, + Res::Def(DefKind::Const, def_id) => match cx.tcx.const_eval_poly(def_id).ok()? { + ConstValue::ByRef { alloc, offset } if offset.bytes() == 0 => { + read_mir_alloc_def_path(cx, alloc, cx.tcx.type_of(def_id)) + }, + _ => None, + }, + _ => None, }, - ExprKind::Array(exprs) => { - let segments: Vec = exprs - .iter() - .filter_map(|expr| { - if let ExprKind::Lit(lit) = &expr.kind { - if let LitKind::Str(sym, _) = lit.node { - return Some(sym.as_str()); - } + ExprKind::Array(exprs) => exprs + .iter() + .map(|expr| { + if let ExprKind::Lit(lit) = &expr.kind { + if let LitKind::Str(sym, _) = lit.node { + return Some((*sym.as_str()).to_owned()); } + } - None - }) - .collect(); - - if segments.len() == exprs.len() { - return Some(segments); - } - }, - _ => {}, + None + }) + .collect(), + _ => None, } +} - None +fn read_mir_alloc_def_path(cx: &LateContext<'tcx>, alloc: &'tcx Allocation, ty: Ty<'_>) -> Option> { + let (alloc, ty) = if let ty::Ref(_, ty, Mutability::Not) = *ty.kind() { + if let GlobalAlloc::Memory(alloc) = cx.tcx.global_alloc(*alloc.relocations().values().next()?) { + (alloc, ty) + } else { + return None; + } + } else { + (alloc, ty) + }; + + if_chain! { + if let ty::Array(ty, _) | ty::Slice(ty) = *ty.kind(); + if let ty::Ref(_, ty, Mutability::Not) = *ty.kind(); + if ty.is_str(); + then { + alloc + .relocations() + .values() + .map(|alloc| { + if let GlobalAlloc::Memory(alloc) = cx.tcx.global_alloc(*alloc) { + str::from_utf8(alloc.inspect_with_uninit_and_ptr_outside_interpreter(0..alloc.len())) + .ok().map(ToOwned::to_owned) + } else { + None + } + }) + .collect() + } else { + None + } + } } // This is not a complete resolver for paths. It works on all the paths currently used in the paths diff --git a/tests/ui-internal/auxiliary/paths.rs b/tests/ui-internal/auxiliary/paths.rs new file mode 100644 index 000000000000..52fcaec4df32 --- /dev/null +++ b/tests/ui-internal/auxiliary/paths.rs @@ -0,0 +1,2 @@ +pub static OPTION: [&str; 3] = ["core", "option", "Option"]; +pub const RESULT: &[&str] = &["core", "result", "Result"]; diff --git a/tests/ui-internal/is_item_def_path.fixed b/tests/ui-internal/is_item_def_path.fixed new file mode 100644 index 000000000000..ce892e021922 --- /dev/null +++ b/tests/ui-internal/is_item_def_path.fixed @@ -0,0 +1,43 @@ +// run-rustfix +// aux-build:paths.rs +#![deny(clippy::internal)] +#![feature(rustc_private)] + +extern crate clippy_utils; +extern crate paths; +extern crate rustc_hir; +extern crate rustc_lint; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::is_item; +use rustc_lint::LateContext; +use rustc_middle::ty::Ty; + +#[allow(unused)] +use rustc_hir::LangItem; +#[allow(unused)] +use rustc_span::sym; + +#[allow(unused)] +static OPTION: [&str; 3] = ["core", "option", "Option"]; +#[allow(unused)] +const RESULT: &[&str] = &["core", "result", "Result"]; + +fn _f<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) { + let _ = is_item(cx, ty, sym::option_type); + let _ = is_item(cx, ty, sym::result_type); + let _ = is_item(cx, ty, sym::result_type); + + #[allow(unused)] + let rc_path = &["alloc", "rc", "Rc"]; + let _ = clippy_utils::is_item(cx, ty, sym::Rc); + + let _ = is_item(cx, ty, sym::option_type); + let _ = is_item(cx, ty, sym::result_type); + + let _ = is_item(cx, ty, LangItem::OwnedBox); + let _ = is_item(cx, ty, sym::maybe_uninit_uninit); +} + +fn main() {} diff --git a/tests/ui-internal/is_item_def_path.rs b/tests/ui-internal/is_item_def_path.rs new file mode 100644 index 000000000000..323b059c1a79 --- /dev/null +++ b/tests/ui-internal/is_item_def_path.rs @@ -0,0 +1,43 @@ +// run-rustfix +// aux-build:paths.rs +#![deny(clippy::internal)] +#![feature(rustc_private)] + +extern crate clippy_utils; +extern crate paths; +extern crate rustc_hir; +extern crate rustc_lint; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::is_item; +use rustc_lint::LateContext; +use rustc_middle::ty::Ty; + +#[allow(unused)] +use rustc_hir::LangItem; +#[allow(unused)] +use rustc_span::sym; + +#[allow(unused)] +static OPTION: [&str; 3] = ["core", "option", "Option"]; +#[allow(unused)] +const RESULT: &[&str] = &["core", "result", "Result"]; + +fn _f<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) { + let _ = is_item(cx, ty, &OPTION); + let _ = is_item(cx, ty, RESULT); + let _ = is_item(cx, ty, &["core", "result", "Result"]); + + #[allow(unused)] + let rc_path = &["alloc", "rc", "Rc"]; + let _ = clippy_utils::is_item(cx, ty, rc_path); + + let _ = is_item(cx, ty, &paths::OPTION); + let _ = is_item(cx, ty, paths::RESULT); + + let _ = is_item(cx, ty, &["alloc", "boxed", "Box"]); + let _ = is_item(cx, ty, &["core", "mem", "maybe_uninit", "MaybeUninit", "uninit"]); +} + +fn main() {} diff --git a/tests/ui-internal/is_item_def_path.stderr b/tests/ui-internal/is_item_def_path.stderr new file mode 100644 index 000000000000..db600be07687 --- /dev/null +++ b/tests/ui-internal/is_item_def_path.stderr @@ -0,0 +1,73 @@ +error: usage of `clippy_utils::is_item()` with a path to a diagnostic item + --> $DIR/is_item_def_path.rs:28:13 + | +LL | let _ = is_item(cx, ty, &OPTION); + | ^^^^^^^^^^^^^^^^-------^ + | | + | help: try: `sym::option_type` + | +note: the lint level is defined here + --> $DIR/is_item_def_path.rs:3:9 + | +LL | #![deny(clippy::internal)] + | ^^^^^^^^^^^^^^^^ + = note: `#[deny(clippy::is_item_def_path_on_diagnostic_or_lang_item)]` implied by `#[deny(clippy::internal)]` + +error: usage of `clippy_utils::is_item()` with a path to a diagnostic item + --> $DIR/is_item_def_path.rs:29:13 + | +LL | let _ = is_item(cx, ty, RESULT); + | ^^^^^^^^^^^^^^^^------^ + | | + | help: try: `sym::result_type` + +error: usage of `clippy_utils::is_item()` with a path to a diagnostic item + --> $DIR/is_item_def_path.rs:30:13 + | +LL | let _ = is_item(cx, ty, &["core", "result", "Result"]); + | ^^^^^^^^^^^^^^^^-----------------------------^ + | | + | help: try: `sym::result_type` + +error: usage of `clippy_utils::is_item()` with a path to a diagnostic item + --> $DIR/is_item_def_path.rs:34:13 + | +LL | let _ = clippy_utils::is_item(cx, ty, rc_path); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-------^ + | | + | help: try: `sym::Rc` + +error: usage of `clippy_utils::is_item()` with a path to a diagnostic item + --> $DIR/is_item_def_path.rs:36:13 + | +LL | let _ = is_item(cx, ty, &paths::OPTION); + | ^^^^^^^^^^^^^^^^--------------^ + | | + | help: try: `sym::option_type` + +error: usage of `clippy_utils::is_item()` with a path to a diagnostic item + --> $DIR/is_item_def_path.rs:37:13 + | +LL | let _ = is_item(cx, ty, paths::RESULT); + | ^^^^^^^^^^^^^^^^-------------^ + | | + | help: try: `sym::result_type` + +error: usage of `clippy_utils::is_item()` with a path to a lang item + --> $DIR/is_item_def_path.rs:39:13 + | +LL | let _ = is_item(cx, ty, &["alloc", "boxed", "Box"]); + | ^^^^^^^^^^^^^^^^--------------------------^ + | | + | help: try: `LangItem::OwnedBox` + +error: usage of `clippy_utils::is_item()` with a path to a diagnostic item + --> $DIR/is_item_def_path.rs:40:13 + | +LL | let _ = is_item(cx, ty, &["core", "mem", "maybe_uninit", "MaybeUninit", "uninit"]); + | ^^^^^^^^^^^^^^^^---------------------------------------------------------^ + | | + | help: try: `sym::maybe_uninit_uninit` + +error: aborting due to 8 previous errors +