Add needless_conversion_for_trait lint; get uitest to pass

Author: smoelius

CHANGELOG.md

@@ -6271,6 +6271,7 @@ Released 2018-09-13
 [`needless_character_iteration`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_character_iteration
 [`needless_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
 [`needless_continue`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue
+[`needless_conversion_for_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_conversion_for_trait
 [`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main
 [`needless_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_else
 [`needless_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_for_each
@@ -6758,4 +6759,5 @@ Released 2018-09-13
 [`verbose-bit-mask-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#verbose-bit-mask-threshold
 [`warn-on-all-wildcard-imports`]: https://doc.rust-lang.org/clippy/lint_configuration.html#warn-on-all-wildcard-imports
 [`warn-unsafe-macro-metavars-in-private-macros`]: https://doc.rust-lang.org/clippy/lint_configuration.html#warn-unsafe-macro-metavars-in-private-macros
+[`watched-functions`]: https://doc.rust-lang.org/clippy/lint_configuration.html#watched-functions
 <!-- end autogenerated links to configuration documentation -->

Cargo.toml

@@ -38,6 +38,7 @@ ui_test = "0.30.2"
 regex = "1.5.5"
 serde = { version = "1.0.145", features = ["derive"] }
 serde_json = "1.0.122"
+tempfile = "3.20"
 toml = "0.7.3"
 walkdir = "2.3"
 filetime = "0.2.9"

book/src/lint_configuration.md

@@ -1135,3 +1135,13 @@ Whether to also emit warnings for unsafe blocks with metavariable expansions in
 ---
 **Affected lints:**
 * [`macro_metavars_in_unsafe`](https://rust-lang.github.io/rust-clippy/master/index.html#macro_metavars_in_unsafe)
+
+
+## `watched-functions`
+Non-standard functions `needless_conversion_for_trait` should warn about.
+
+**Default Value:** `[]`
+
+---
+**Affected lints:**
+* [`needless_conversion_for_trait`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_conversion_for_trait)

clippy_config/src/conf.rs

@@ -199,7 +199,7 @@ macro_rules! deserialize {
         let mut conf_paths = Vec::new();
         for raw_value in array {
             let value_span = raw_value.span();
-            let mut conf_path = match ConfPath::<$replaceable>::deserialize(raw_value.into_inner()) {
+            let mut conf_path = match ConfPath::<String, $replaceable>::deserialize(raw_value.into_inner()) {
                 Err(e) => {
                     $errors.push(ConfError::spanned(
                         $file,
@@ -877,6 +877,9 @@ define_Conf! {
     /// Whether to also emit warnings for unsafe blocks with metavariable expansions in **private** macros.
     #[lints(macro_metavars_in_unsafe)]
     warn_unsafe_macro_metavars_in_private_macros: bool = false,
+    /// Non-standard functions `needless_conversion_for_trait` should warn about.
+    #[lints(needless_conversion_for_trait)]
+    watched_functions: Vec<ConfPathWithoutReplacement> = Vec::new(),
 }
 
 /// Search for the configuration file.

clippy_config/src/types.rs

@@ -1,15 +1,17 @@
 use clippy_utils::paths::{PathNS, find_crates, lookup_path};
+use itertools::Itertools;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::{Applicability, Diag};
 use rustc_hir::PrimTy;
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::DefIdMap;
 use rustc_middle::ty::TyCtxt;
-use rustc_span::{Span, Symbol};
+use rustc_span::{DUMMY_SP, Span, Symbol};
 use serde::de::{self, Deserializer, Visitor};
 use serde::{Deserialize, Serialize, ser};
 use std::collections::HashMap;
 use std::fmt;
+use std::ops::Deref;
 
 #[derive(Debug, Deserialize)]
 #[serde(deny_unknown_fields)]
@@ -18,11 +20,11 @@ pub struct Rename {
     pub rename: String,
 }
 
-pub type ConfPathWithoutReplacement = ConfPath<false>;
+pub type ConfPathWithoutReplacement = ConfPath<String, false>;
 
 #[derive(Debug, Serialize)]
-pub struct ConfPath<const REPLACEABLE: bool = true> {
-    path: String,
+pub struct ConfPath<T = String, const REPLACEABLE: bool = true> {
+    path: T,
     reason: Option<String>,
     replacement: Option<String>,
     /// Setting `allow_invalid` to true suppresses a warning if `path` does not refer to an existing
@@ -38,7 +40,7 @@ pub struct ConfPath<const REPLACEABLE: bool = true> {
     span: Span,
 }
 
-impl<'de, const REPLACEABLE: bool> Deserialize<'de> for ConfPath<REPLACEABLE> {
+impl<'de, const REPLACEABLE: bool> Deserialize<'de> for ConfPath<String, REPLACEABLE> {
     fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
     where
         D: Deserializer<'de>,
@@ -72,8 +74,8 @@ enum ConfPathEnum {
     },
 }
 
-impl<const REPLACEABLE: bool> ConfPath<REPLACEABLE> {
-    pub fn path(&self) -> &str {
+impl<T, const REPLACEABLE: bool> ConfPath<T, REPLACEABLE> {
+    pub fn path(&self) -> &T {
         &self.path
     }
 
@@ -101,6 +103,16 @@ impl<const REPLACEABLE: bool> ConfPath<REPLACEABLE> {
     }
 }
 
+impl<T, const REPLACEABLE: bool> ConfPath<T, REPLACEABLE>
+where
+    T: Deref,
+    <T as Deref>::Target: ToSymPath,
+{
+    pub fn to_sym_path(&self) -> Vec<Symbol> {
+        self.path().to_sym_path()
+    }
+}
+
 impl ConfPathEnum {
     pub fn path(&self) -> &str {
         let (Self::Simple(path) | Self::WithReason { path, .. }) = self;
@@ -130,24 +142,71 @@ impl ConfPathEnum {
     }
 }
 
+pub trait ToSymPath {
+    fn to_sym_path(&self) -> Vec<Symbol>;
+}
+
+impl ToSymPath for str {
+    fn to_sym_path(&self) -> Vec<Symbol> {
+        self.split("::").map(Symbol::intern).collect()
+    }
+}
+
+#[derive(Clone, Copy, Debug)]
+pub struct SymPath<'a>(pub &'a [Symbol]);
+
+impl Deref for SymPath<'_> {
+    type Target = [Symbol];
+    fn deref(&self) -> &Self::Target {
+        self.0
+    }
+}
+
+impl fmt::Display for SymPath<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.0.iter().map(Symbol::to_string).join("::"))
+    }
+}
+
+impl ToSymPath for [Symbol] {
+    fn to_sym_path(&self) -> Vec<Symbol> {
+        self.to_owned()
+    }
+}
+
+pub const fn conf_path_from_sym_path(sym_path: &'static [Symbol]) -> ConfPath<SymPath<'static>, false> {
+    ConfPath {
+        path: SymPath(sym_path),
+        reason: None,
+        replacement: None,
+        allow_invalid: false,
+        span: DUMMY_SP,
+    }
+}
+
 /// Creates a map of disallowed items to the reason they were disallowed.
 #[allow(clippy::type_complexity)]
-pub fn create_conf_path_map<const REPLACEABLE: bool>(
+pub fn create_conf_path_map<T, const REPLACEABLE: bool>(
     tcx: TyCtxt<'_>,
-    conf_paths: &'static [ConfPath<REPLACEABLE>],
+    conf_paths: &'static [ConfPath<T, REPLACEABLE>],
     ns: PathNS,
     def_kind_predicate: impl Fn(DefKind) -> bool,
     predicate_description: &str,
     allow_prim_tys: bool,
 ) -> (
-    DefIdMap<(&'static str, &'static ConfPath<REPLACEABLE>)>,
-    FxHashMap<PrimTy, (&'static str, &'static ConfPath<REPLACEABLE>)>,
-) {
-    let mut def_ids: DefIdMap<(&'static str, &ConfPath<REPLACEABLE>)> = DefIdMap::default();
-    let mut prim_tys: FxHashMap<PrimTy, (&'static str, &ConfPath<REPLACEABLE>)> = FxHashMap::default();
+    DefIdMap<(&'static <T as Deref>::Target, &'static ConfPath<T, REPLACEABLE>)>,
+    FxHashMap<PrimTy, (&'static <T as Deref>::Target, &'static ConfPath<T, REPLACEABLE>)>,
+)
+where
+    T: Deref + fmt::Display,
+    <T as Deref>::Target: ToSymPath,
+{
+    let mut def_ids: DefIdMap<(&'static <T as Deref>::Target, &ConfPath<T, REPLACEABLE>)> = DefIdMap::default();
+    let mut prim_tys: FxHashMap<PrimTy, (&'static <T as Deref>::Target, &ConfPath<T, REPLACEABLE>)> =
+        FxHashMap::default();
     for conf_path in conf_paths {
         let path = conf_path.path();
-        let sym_path: Vec<Symbol> = path.split("::").map(Symbol::intern).collect();
+        let sym_path = path.to_sym_path();
         let mut resolutions = lookup_path(tcx, ns, &sym_path);
         resolutions.retain(|&def_id| def_kind_predicate(tcx.def_kind(def_id)));
 

clippy_lints/src/declared_lints.rs

@@ -544,6 +544,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE_INFO,
     crate::needless_borrows_for_generic_args::NEEDLESS_BORROWS_FOR_GENERIC_ARGS_INFO,
     crate::needless_continue::NEEDLESS_CONTINUE_INFO,
+    crate::needless_conversion_for_trait::NEEDLESS_CONVERSION_FOR_TRAIT_INFO,
     crate::needless_else::NEEDLESS_ELSE_INFO,
     crate::needless_for_each::NEEDLESS_FOR_EACH_INFO,
     crate::needless_if::NEEDLESS_IF_INFO,

clippy_lints/src/lib.rs

@@ -259,6 +259,7 @@ mod needless_bool;
 mod needless_borrowed_ref;
 mod needless_borrows_for_generic_args;
 mod needless_continue;
+mod needless_conversion_for_trait;
 mod needless_else;
 mod needless_for_each;
 mod needless_if;
@@ -828,5 +829,10 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf)));
     store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom));
     store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
+    store.register_late_pass(move |tcx| {
+        Box::new(needless_conversion_for_trait::NeedlessConversionForTrait::new(
+            tcx, conf,
+        ))
+    });
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/methods/unnecessary_to_owned.rs

@@ -401,6 +401,11 @@ fn check_other_call_arg<'tcx>(
         && let Some(as_ref_trait_id) = cx.tcx.get_diagnostic_item(sym::AsRef)
         && (trait_predicate.def_id() == deref_trait_id || trait_predicate.def_id() == as_ref_trait_id)
         && let receiver_ty = cx.typeck_results().expr_ty(receiver)
+        // Verify the output type contains the input type to be replaced.
+        // `needless_conversion_for_trait` cannot change the output type (see
+        // `clippy_utils::ty::replace_types`). Hence, this check ensures that `unnecessary_to_owned`
+        // and `needless_conversion_for_trait` do not flag the same code.
+        && fn_sig.output().contains(input)
         // We can't add an `&` when the trait is `Deref` because `Target = &T` won't match
         // `Target = T`.
         && let Some((n_refs, receiver_ty)) = if n_refs > 0 || is_copy(cx, receiver_ty) {