diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 37a56bc20..f12994c7a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -526,6 +526,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &utils::internal_lints::OUTER_EXPN_EXPN_DATA, #[cfg(feature = "internal-lints")] &utils::internal_lints::PRODUCE_ICE, + #[cfg(feature = "internal-lints")] + &utils::internal_lints::UNNECESSARY_SYMBOL_STR, &approx_const::APPROX_CONSTANT, &arithmetic::FLOAT_ARITHMETIC, &arithmetic::INTEGER_ARITHMETIC, @@ -1372,6 +1374,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: 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), ]); store.register_group(true, "clippy::all", Some("clippy"), vec![ diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index c0b6688fa..59a1852ab 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -13,7 +13,9 @@ use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; use rustc_hir::hir_id::CRATE_HIR_ID; use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; -use rustc_hir::{Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Node, Path, StmtKind, Ty, TyKind}; +use rustc_hir::{ + BinOpKind, Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Node, Path, StmtKind, Ty, TyKind, UnOp, +}; use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_middle::mir::interpret::ConstValue; @@ -273,6 +275,28 @@ declare_clippy_lint! { "interning a symbol that is pre-interned and defined as a constant" } +declare_clippy_lint! { + /// **What it does:** Checks for unnecessary conversion from Symbol to a string. + /// + /// **Why is this bad?** It's faster use symbols directly intead of strings. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// Bad: + /// ```rust,ignore + /// symbol.as_str() == "clippy"; + /// ``` + /// + /// Good: + /// ```rust,ignore + /// symbol == sym::clippy; + /// ``` + pub UNNECESSARY_SYMBOL_STR, + internal, + "unnecessary conversion between Symbol and string" +} + declare_lint_pass!(ClippyLintsInternal => [CLIPPY_LINTS_INTERNAL]); impl EarlyLintPass for ClippyLintsInternal { @@ -873,7 +897,7 @@ pub struct InterningDefinedSymbol { symbol_map: FxHashMap, } -impl_lint_pass!(InterningDefinedSymbol => [INTERNING_DEFINED_SYMBOL]); +impl_lint_pass!(InterningDefinedSymbol => [INTERNING_DEFINED_SYMBOL, UNNECESSARY_SYMBOL_STR]); impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol { fn check_crate(&mut self, cx: &LateContext<'_>, _: &Crate<'_>) { @@ -919,5 +943,130 @@ impl<'tcx> LateLintPass<'tcx> for InterningDefinedSymbol { ); } } + if let ExprKind::Binary(op, left, right) = expr.kind { + if matches!(op.node, BinOpKind::Eq | BinOpKind::Ne) { + let data = [ + (left, self.symbol_str_expr(left, cx)), + (right, self.symbol_str_expr(right, cx)), + ]; + match data { + // both operands are a symbol string + [(_, Some(left)), (_, Some(right))] => { + span_lint_and_sugg( + cx, + UNNECESSARY_SYMBOL_STR, + expr.span, + "unnecessary `Symbol` to string conversion", + "try", + format!( + "{} {} {}", + left.as_symbol_snippet(cx), + op.node.as_str(), + right.as_symbol_snippet(cx), + ), + Applicability::MachineApplicable, + ); + }, + // one of the operands is a symbol string + [(expr, Some(symbol)), _] | [_, (expr, Some(symbol))] => { + // creating an owned string for comparison + if matches!(symbol, SymbolStrExpr::Expr { is_to_owned: true, .. }) { + span_lint_and_sugg( + cx, + UNNECESSARY_SYMBOL_STR, + expr.span, + "unnecessary string allocation", + "try", + format!("{}.as_str()", symbol.as_symbol_snippet(cx)), + Applicability::MachineApplicable, + ); + } + }, + // nothing found + [(_, None), (_, None)] => {}, + } + } + } + } +} + +impl InterningDefinedSymbol { + fn symbol_str_expr<'tcx>(&self, expr: &'tcx Expr<'tcx>, cx: &LateContext<'tcx>) -> Option> { + static IDENT_STR_PATHS: &[&[&str]] = &[&paths::IDENT_AS_STR, &paths::TO_STRING_METHOD]; + static SYMBOL_STR_PATHS: &[&[&str]] = &[ + &paths::SYMBOL_AS_STR, + &paths::SYMBOL_TO_IDENT_STRING, + &paths::TO_STRING_METHOD, + ]; + // SymbolStr might be de-referenced: `&*symbol.as_str()` + let call = if_chain! { + if let ExprKind::AddrOf(_, _, e) = expr.kind; + if let ExprKind::Unary(UnOp::UnDeref, e) = e.kind; + then { e } else { expr } + }; + if_chain! { + // is a method call + if let ExprKind::MethodCall(_, _, [item], _) = call.kind; + if let Some(did) = cx.typeck_results().type_dependent_def_id(call.hir_id); + let ty = cx.typeck_results().expr_ty(item); + // ...on either an Ident or a Symbol + if let Some(is_ident) = if match_type(cx, ty, &paths::SYMBOL) { + Some(false) + } else if match_type(cx, ty, &paths::IDENT) { + Some(true) + } else { + None + }; + // ...which converts it to a string + let paths = if is_ident { IDENT_STR_PATHS } else { SYMBOL_STR_PATHS }; + if let Some(path) = paths.iter().find(|path| match_def_path(cx, did, path)); + then { + let is_to_owned = path.last().unwrap().ends_with("string"); + return Some(SymbolStrExpr::Expr { + item, + is_ident, + is_to_owned, + }); + } + } + // is a string constant + if let Some(Constant::Str(s)) = constant_simple(cx, cx.typeck_results(), expr) { + let value = Symbol::intern(&s).as_u32(); + // ...which matches a symbol constant + if let Some(&def_id) = self.symbol_map.get(&value) { + return Some(SymbolStrExpr::Const(def_id)); + } + } + None + } +} + +enum SymbolStrExpr<'tcx> { + /// a string constant with a corresponding symbol constant + Const(DefId), + /// a "symbol to string" expression like `symbol.as_str()` + Expr { + /// part that evaluates to `Symbol` or `Ident` + item: &'tcx Expr<'tcx>, + is_ident: bool, + /// whether an owned `String` is created like `to_ident_string()` + is_to_owned: bool, + }, +} + +impl<'tcx> SymbolStrExpr<'tcx> { + /// Returns a snippet that evaluates to a `Symbol` and is const if possible + fn as_symbol_snippet(&self, cx: &LateContext<'_>) -> Cow<'tcx, str> { + match *self { + Self::Const(def_id) => cx.tcx.def_path_str(def_id).into(), + Self::Expr { item, is_ident, .. } => { + let mut snip = snippet(cx, item.span.source_callsite(), ".."); + if is_ident { + // get `Ident.name` + snip.to_mut().push_str(".name"); + } + snip + }, + } } } diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 3179be6af..c0b203b53 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -54,6 +54,10 @@ pub const HASH: [&str; 3] = ["core", "hash", "Hash"]; pub const HASHMAP: [&str; 5] = ["std", "collections", "hash", "map", "HashMap"]; pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"]; pub const HASHSET: [&str; 5] = ["std", "collections", "hash", "set", "HashSet"]; +#[cfg(feature = "internal-lints")] +pub const IDENT: [&str; 3] = ["rustc_span", "symbol", "Ident"]; +#[cfg(feature = "internal-lints")] +pub const IDENT_AS_STR: [&str; 4] = ["rustc_span", "symbol", "Ident", "as_str"]; pub const INDEX: [&str; 3] = ["core", "ops", "Index"]; pub const INDEX_MUT: [&str; 3] = ["core", "ops", "IndexMut"]; pub const INSERT_STR: [&str; 4] = ["alloc", "string", "String", "insert_str"]; @@ -150,8 +154,12 @@ pub const STR_STARTS_WITH: [&str; 4] = ["core", "str", "", "starts_wit #[cfg(feature = "internal-lints")] pub const SYMBOL: [&str; 3] = ["rustc_span", "symbol", "Symbol"]; #[cfg(feature = "internal-lints")] +pub const SYMBOL_AS_STR: [&str; 4] = ["rustc_span", "symbol", "Symbol", "as_str"]; +#[cfg(feature = "internal-lints")] pub const SYMBOL_INTERN: [&str; 4] = ["rustc_span", "symbol", "Symbol", "intern"]; #[cfg(feature = "internal-lints")] +pub const SYMBOL_TO_IDENT_STRING: [&str; 4] = ["rustc_span", "symbol", "Symbol", "to_ident_string"]; +#[cfg(feature = "internal-lints")] pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"]; #[cfg(feature = "internal-lints")] pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; diff --git a/tests/ui-internal/unnecessary_symbol_str.fixed b/tests/ui-internal/unnecessary_symbol_str.fixed new file mode 100644 index 000000000..2ec0efe4c --- /dev/null +++ b/tests/ui-internal/unnecessary_symbol_str.fixed @@ -0,0 +1,16 @@ +// run-rustfix +#![feature(rustc_private)] +#![deny(clippy::internal)] +#![allow(clippy::unnecessary_operation, unused_must_use)] + +extern crate rustc_span; + +use rustc_span::symbol::{Ident, Symbol}; + +fn main() { + Symbol::intern("foo") == rustc_span::sym::clippy; + Symbol::intern("foo") == rustc_span::symbol::kw::SelfLower; + Symbol::intern("foo") != rustc_span::symbol::kw::SelfUpper; + Ident::invalid().name == rustc_span::sym::clippy; + rustc_span::sym::clippy == Ident::invalid().name; +} diff --git a/tests/ui-internal/unnecessary_symbol_str.rs b/tests/ui-internal/unnecessary_symbol_str.rs new file mode 100644 index 000000000..87e1b3a2e --- /dev/null +++ b/tests/ui-internal/unnecessary_symbol_str.rs @@ -0,0 +1,16 @@ +// run-rustfix +#![feature(rustc_private)] +#![deny(clippy::internal)] +#![allow(clippy::unnecessary_operation, unused_must_use)] + +extern crate rustc_span; + +use rustc_span::symbol::{Ident, Symbol}; + +fn main() { + Symbol::intern("foo").as_str() == "clippy"; + Symbol::intern("foo").to_string() == "self"; + Symbol::intern("foo").to_ident_string() != "Self"; + &*Ident::invalid().as_str() == "clippy"; + "clippy" == Ident::invalid().to_string(); +} diff --git a/tests/ui-internal/unnecessary_symbol_str.stderr b/tests/ui-internal/unnecessary_symbol_str.stderr new file mode 100644 index 000000000..b1284b7c8 --- /dev/null +++ b/tests/ui-internal/unnecessary_symbol_str.stderr @@ -0,0 +1,39 @@ +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:11:5 + | +LL | Symbol::intern("foo").as_str() == "clippy"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Symbol::intern("foo") == rustc_span::sym::clippy` + | +note: the lint level is defined here + --> $DIR/unnecessary_symbol_str.rs:3:9 + | +LL | #![deny(clippy::internal)] + | ^^^^^^^^^^^^^^^^ + = note: `#[deny(clippy::unnecessary_symbol_str)]` implied by `#[deny(clippy::internal)]` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:12:5 + | +LL | Symbol::intern("foo").to_string() == "self"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Symbol::intern("foo") == rustc_span::symbol::kw::SelfLower` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:13:5 + | +LL | Symbol::intern("foo").to_ident_string() != "Self"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Symbol::intern("foo") != rustc_span::symbol::kw::SelfUpper` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:14:5 + | +LL | &*Ident::invalid().as_str() == "clippy"; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Ident::invalid().name == rustc_span::sym::clippy` + +error: unnecessary `Symbol` to string conversion + --> $DIR/unnecessary_symbol_str.rs:15:5 + | +LL | "clippy" == Ident::invalid().to_string(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `rustc_span::sym::clippy == Ident::invalid().name` + +error: aborting due to 5 previous errors +