Fix and improve internal lint checking for match_type usages
* Check for `const`s and `static`s from external crates
* Check for `LangItem`s
* Handle inherent functions which have the same name as a field
* Also check the following functions:
* `match_trait_method`
* `match_def_path`
* `is_expr_path_def_path`
* `is_qpath_def_path`
* Handle checking for a constructor to a diagnostic item or `LangItem`
This commit is contained in:
Jason Newcomb
parent
8e7af6b429
commit
162aa19793
26 changed files with 539 additions and 181 deletions
|
|
@ -2,11 +2,11 @@ use crate::utils::internal_lints::metadata_collector::is_deprecated_lint;
|
|||
use clippy_utils::consts::{constant_simple, Constant};
|
||||
use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
|
||||
use clippy_utils::macros::root_macro_call_first_node;
|
||||
use clippy_utils::source::snippet;
|
||||
use clippy_utils::source::{snippet, snippet_with_applicability};
|
||||
use clippy_utils::ty::match_type;
|
||||
use clippy_utils::{
|
||||
def_path_res, higher, is_else_clause, is_expn_of, is_expr_path_def_path, is_lint_allowed, match_def_path,
|
||||
method_calls, paths, peel_blocks_with_stmt, SpanlessEq,
|
||||
def_path_res, higher, is_else_clause, is_expn_of, is_expr_path_def_path, is_lint_allowed, match_any_def_paths,
|
||||
match_def_path, method_calls, paths, peel_blocks_with_stmt, peel_hir_expr_refs, SpanlessEq,
|
||||
};
|
||||
use if_chain::if_chain;
|
||||
use rustc_ast as ast;
|
||||
|
|
@ -20,21 +20,24 @@ use rustc_hir::def_id::DefId;
|
|||
use rustc_hir::hir_id::CRATE_HIR_ID;
|
||||
use rustc_hir::intravisit::Visitor;
|
||||
use rustc_hir::{
|
||||
BinOpKind, Block, Closure, Expr, ExprKind, HirId, Item, Local, MutTy, Mutability, Node, Path, Stmt, StmtKind, Ty,
|
||||
BinOpKind, Block, Closure, Expr, ExprKind, HirId, Item, Local, MutTy, Mutability, Node, Path, Stmt, StmtKind,
|
||||
TyKind, UnOp,
|
||||
};
|
||||
use rustc_hir_analysis::hir_ty_to_ty;
|
||||
use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
|
||||
use rustc_middle::hir::nested_filter;
|
||||
use rustc_middle::mir::interpret::ConstValue;
|
||||
use rustc_middle::ty::{self, fast_reject::SimplifiedTypeGen, subst::GenericArgKind, FloatTy};
|
||||
use rustc_middle::mir::interpret::{Allocation, ConstValue, GlobalAlloc};
|
||||
use rustc_middle::ty::{
|
||||
self, fast_reject::SimplifiedTypeGen, subst::GenericArgKind, AssocKind, DefIdTree, FloatTy, Ty,
|
||||
};
|
||||
use rustc_semver::RustcVersion;
|
||||
use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass};
|
||||
use rustc_span::source_map::Spanned;
|
||||
use rustc_span::symbol::Symbol;
|
||||
use rustc_span::symbol::{Ident, Symbol};
|
||||
use rustc_span::{sym, BytePos, Span};
|
||||
|
||||
use std::borrow::{Borrow, Cow};
|
||||
use std::str;
|
||||
|
||||
#[cfg(feature = "internal")]
|
||||
pub mod metadata_collector;
|
||||
|
|
@ -226,11 +229,11 @@ declare_clippy_lint! {
|
|||
|
||||
declare_clippy_lint! {
|
||||
/// ### What it does
|
||||
/// Checks for calls to `utils::match_type()` on a type diagnostic item
|
||||
/// and suggests to use `utils::is_type_diagnostic_item()` instead.
|
||||
/// Checks for usages of def paths when a diagnostic item or a `LangItem` could be used.
|
||||
///
|
||||
/// ### Why is this bad?
|
||||
/// `utils::is_type_diagnostic_item()` does not require hardcoded paths.
|
||||
/// The path for an item is subject to change and is less efficient to look up than a
|
||||
/// diagnostic item or a `LangItem`.
|
||||
///
|
||||
/// ### Example
|
||||
/// ```rust,ignore
|
||||
|
|
@ -241,9 +244,9 @@ declare_clippy_lint! {
|
|||
/// ```rust,ignore
|
||||
/// utils::is_type_diagnostic_item(cx, ty, sym::Vec)
|
||||
/// ```
|
||||
pub MATCH_TYPE_ON_DIAGNOSTIC_ITEM,
|
||||
pub UNNECESSARY_DEF_PATH,
|
||||
internal,
|
||||
"using `utils::match_type()` instead of `utils::is_type_diagnostic_item()`"
|
||||
"using a def path when a diagnostic item or a `LangItem` is available"
|
||||
}
|
||||
|
||||
declare_clippy_lint! {
|
||||
|
|
@ -537,7 +540,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 {
|
||||
|
|
@ -888,89 +891,225 @@ fn suggest_note(
|
|||
);
|
||||
}
|
||||
|
||||
declare_lint_pass!(MatchTypeOnDiagItem => [MATCH_TYPE_ON_DIAGNOSTIC_ITEM]);
|
||||
declare_lint_pass!(UnnecessaryDefPath => [UNNECESSARY_DEF_PATH]);
|
||||
|
||||
impl<'tcx> LateLintPass<'tcx> for MatchTypeOnDiagItem {
|
||||
#[allow(clippy::too_many_lines)]
|
||||
impl<'tcx> LateLintPass<'tcx> for UnnecessaryDefPath {
|
||||
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) {
|
||||
enum Item {
|
||||
LangItem(Symbol),
|
||||
DiagnosticItem(Symbol),
|
||||
}
|
||||
static PATHS: &[&[&str]] = &[
|
||||
&["clippy_utils", "match_def_path"],
|
||||
&["clippy_utils", "match_trait_method"],
|
||||
&["clippy_utils", "ty", "match_type"],
|
||||
&["clippy_utils", "is_expr_path_def_path"],
|
||||
];
|
||||
|
||||
if is_lint_allowed(cx, UNNECESSARY_DEF_PATH, expr.hir_id) {
|
||||
return;
|
||||
}
|
||||
|
||||
if_chain! {
|
||||
// Check if this is a call to utils::match_type()
|
||||
if let ExprKind::Call(fn_path, [context, ty, ty_path]) = expr.kind;
|
||||
if is_expr_path_def_path(cx, fn_path, &["clippy_utils", "ty", "match_type"]);
|
||||
if let ExprKind::Call(func, [cx_arg, def_arg, args@..]) = expr.kind;
|
||||
if let ExprKind::Path(path) = &func.kind;
|
||||
if let Some(id) = cx.qpath_res(path, func.hir_id).opt_def_id();
|
||||
if let Some(which_path) = match_any_def_paths(cx, id, PATHS);
|
||||
let item_arg = if which_path == 4 { &args[1] } else { &args[0] };
|
||||
// Extract the path to the matched type
|
||||
if let Some(segments) = path_to_matched_type(cx, ty_path);
|
||||
let segments: Vec<&str> = segments.iter().map(Symbol::as_str).collect();
|
||||
if let Some(ty_did) = def_path_res(cx, &segments[..]).opt_def_id();
|
||||
// Check if the matched type is a diagnostic item
|
||||
if let Some(item_name) = cx.tcx.get_diagnostic_name(ty_did);
|
||||
if let Some(segments) = path_to_matched_type(cx, item_arg);
|
||||
let segments: Vec<&str> = segments.iter().map(|sym| &**sym).collect();
|
||||
if let Some(def_id) = def_path_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, "_");
|
||||
// def_path_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()
|
||||
.find_map(|impl_id| cx.tcx.associated_items(*impl_id)
|
||||
.find_by_name_and_kind(
|
||||
cx.tcx,
|
||||
Ident::from_str(method_name),
|
||||
AssocKind::Fn,
|
||||
*impl_id,
|
||||
)
|
||||
)
|
||||
)
|
||||
.map_or(def_id, |item| item.def_id)
|
||||
} else {
|
||||
def_id
|
||||
};
|
||||
|
||||
span_lint_and_sugg(
|
||||
// Check if the target item is a diagnostic item or LangItem.
|
||||
let (msg, item) = if let Some(item_name)
|
||||
= cx.tcx.diagnostic_items(def_id.krate).id_to_name.get(&def_id)
|
||||
{
|
||||
(
|
||||
"use of a def path to a diagnostic item",
|
||||
Item::DiagnosticItem(*item_name),
|
||||
)
|
||||
} else if let Some(lang_item) = cx.tcx.lang_items().items().iter().position(|id| *id == Some(def_id)) {
|
||||
let lang_items = def_path_res(cx, &["rustc_hir", "lang_items", "LangItem"]).def_id();
|
||||
let item_name = cx.tcx.adt_def(lang_items).variants().iter().nth(lang_item).unwrap().name;
|
||||
(
|
||||
"use of a def path to a `LangItem`",
|
||||
Item::LangItem(item_name),
|
||||
)
|
||||
} else {
|
||||
return;
|
||||
};
|
||||
|
||||
let has_ctor = match cx.tcx.def_kind(def_id) {
|
||||
DefKind::Struct => {
|
||||
let variant = cx.tcx.adt_def(def_id).non_enum_variant();
|
||||
variant.ctor_def_id.is_some() && variant.fields.iter().all(|f| f.vis.is_public())
|
||||
}
|
||||
DefKind::Variant => {
|
||||
let variant = cx.tcx.adt_def(cx.tcx.parent(def_id)).variant_with_id(def_id);
|
||||
variant.ctor_def_id.is_some() && variant.fields.iter().all(|f| f.vis.is_public())
|
||||
}
|
||||
_ => false,
|
||||
};
|
||||
|
||||
let mut app = Applicability::MachineApplicable;
|
||||
let cx_snip = snippet_with_applicability(cx, cx_arg.span, "..", &mut app);
|
||||
let def_snip = snippet_with_applicability(cx, def_arg.span, "..", &mut app);
|
||||
let (sugg, with_note) = match (which_path, item) {
|
||||
// match_def_path
|
||||
(0, Item::DiagnosticItem(item)) =>
|
||||
(format!("{cx_snip}.tcx.is_diagnostic_item(sym::{item}, {def_snip})"), has_ctor),
|
||||
(0, Item::LangItem(item)) => (
|
||||
format!("{cx_snip}.tcx.lang_items().require(LangItem::{item}).ok() == Some({def_snip})"),
|
||||
has_ctor
|
||||
),
|
||||
// match_trait_method
|
||||
(1, Item::DiagnosticItem(item)) =>
|
||||
(format!("is_trait_method({cx_snip}, {def_snip}, sym::{item})"), false),
|
||||
// match_type
|
||||
(2, Item::DiagnosticItem(item)) =>
|
||||
(format!("is_type_diagnostic_item({cx_snip}, {def_snip}, sym::{item})"), false),
|
||||
(2, Item::LangItem(item)) =>
|
||||
(format!("is_type_lang_item({cx_snip}, {def_snip}, LangItem::{item})"), false),
|
||||
// is_expr_path_def_path
|
||||
(3, Item::DiagnosticItem(item)) if has_ctor => (
|
||||
format!(
|
||||
"is_res_diag_ctor({cx_snip}, path_res({cx_snip}, {def_snip}), sym::{item})",
|
||||
),
|
||||
false,
|
||||
),
|
||||
(3, Item::LangItem(item)) if has_ctor => (
|
||||
format!(
|
||||
"is_res_lang_ctor({cx_snip}, path_res({cx_snip}, {def_snip}), LangItem::{item})",
|
||||
),
|
||||
false,
|
||||
),
|
||||
(3, Item::DiagnosticItem(item)) =>
|
||||
(format!("is_path_diagnostic_item({cx_snip}, {def_snip}, sym::{item})"), false),
|
||||
(3, Item::LangItem(item)) => (
|
||||
format!(
|
||||
"path_res({cx_snip}, {def_snip}).opt_def_id()\
|
||||
.map_or(false, |id| {cx_snip}.tcx.lang_items().require(LangItem::{item}).ok() == Some(id))",
|
||||
),
|
||||
false,
|
||||
),
|
||||
_ => return,
|
||||
};
|
||||
|
||||
span_lint_and_then(
|
||||
cx,
|
||||
MATCH_TYPE_ON_DIAGNOSTIC_ITEM,
|
||||
UNNECESSARY_DEF_PATH,
|
||||
expr.span,
|
||||
"usage of `clippy_utils::ty::match_type()` on a type diagnostic item",
|
||||
"try",
|
||||
format!("clippy_utils::ty::is_type_diagnostic_item({cx_snippet}, {ty_snippet}, sym::{item_name})"),
|
||||
Applicability::MaybeIncorrect,
|
||||
msg,
|
||||
|diag| {
|
||||
diag.span_suggestion(expr.span, "try", sugg, app);
|
||||
if with_note {
|
||||
diag.help(
|
||||
"if this `DefId` came from a constructor expression or pattern then the \
|
||||
parent `DefId` should be used instead"
|
||||
);
|
||||
}
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn path_to_matched_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<Vec<Symbol>> {
|
||||
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<Vec<String>> {
|
||||
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);
|
||||
}
|
||||
}
|
||||
},
|
||||
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);
|
||||
}
|
||||
}
|
||||
},
|
||||
_ => {},
|
||||
},
|
||||
ExprKind::Array(exprs) => {
|
||||
let segments: Vec<Symbol> = exprs
|
||||
.iter()
|
||||
.filter_map(|expr| {
|
||||
if let ExprKind::Lit(lit) = &expr.kind {
|
||||
if let LitKind::Str(sym, _) = lit.node {
|
||||
return Some(sym);
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(Node::Local(Local { init: Some(init), .. })) = cx.tcx.hir().find(parent_id) {
|
||||
path_to_matched_type(cx, init)
|
||||
} else {
|
||||
None
|
||||
})
|
||||
.collect();
|
||||
|
||||
if segments.len() == exprs.len() {
|
||||
return Some(segments);
|
||||
}
|
||||
}
|
||||
},
|
||||
Res::Def(DefKind::Static(_), def_id) => read_mir_alloc_def_path(
|
||||
cx,
|
||||
cx.tcx.eval_static_initializer(def_id).ok()?.inner(),
|
||||
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.inner(), cx.tcx.type_of(def_id))
|
||||
},
|
||||
_ => None,
|
||||
},
|
||||
_ => None,
|
||||
},
|
||||
_ => {},
|
||||
}
|
||||
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
|
||||
None
|
||||
})
|
||||
.collect(),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
fn read_mir_alloc_def_path<'tcx>(cx: &LateContext<'tcx>, alloc: &'tcx Allocation, ty: Ty<'_>) -> Option<Vec<String>> {
|
||||
let (alloc, ty) = if let ty::Ref(_, ty, Mutability::Not) = *ty.kind() {
|
||||
let &alloc = alloc.provenance().values().next()?;
|
||||
if let GlobalAlloc::Memory(alloc) = cx.tcx.global_alloc(alloc) {
|
||||
(alloc.inner(), ty)
|
||||
} else {
|
||||
return None;
|
||||
}
|
||||
} else {
|
||||
(alloc, ty)
|
||||
};
|
||||
|
||||
if let ty::Array(ty, _) | ty::Slice(ty) = *ty.kind()
|
||||
&& let ty::Ref(_, ty, Mutability::Not) = *ty.kind()
|
||||
&& ty.is_str()
|
||||
{
|
||||
alloc
|
||||
.provenance()
|
||||
.values()
|
||||
.map(|&alloc| {
|
||||
if let GlobalAlloc::Memory(alloc) = cx.tcx.global_alloc(alloc) {
|
||||
let alloc = alloc.inner();
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue