Add unnecessary symbol string lint

camsteffen · camsteffen · commit cc26919b4dcf · 2021-01-08T14:49:59.000-06:00
diff --git 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
@@ -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<u32, DefId>,
 }
 
-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<SymbolStrExpr<'tcx>> {
+        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
@@ -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", "<impl 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
@@ -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
@@ -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
@@ -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
+
