Validate paths in disallowed_* configurations (rust-lang#14397)

This PR resolves rust-lang#11432 by checking that paths resolve in `disallowed_*`
configurations.

It also does some lightweight validation of definition kinds. For
example, only paths that resolve to `DefKind::Macro` definitions are
allowed in `disallowed_macro` configurations.

~The PR is currently two commits. The first is rust-lang#14376 (cc: @Jarcho),
which I submitted just a few days ago. The second commit is everything
else.~

Side note: For me, the most difficult part of this PR was getting the
spans of the individual `DisallowedPath` configurations. There is some
discussion at toml-rs/toml#840, if
interested.

changelog:  validate paths in `disallowed_*` configurations

Author: samueltardieu

clippy.toml

@@ -7,11 +7,14 @@ lint-commented-code = true
 [[disallowed-methods]]
 path = "rustc_lint::context::LintContext::lint"
 reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint*` functions instead"
+allow-invalid = true
 
 [[disallowed-methods]]
 path = "rustc_lint::context::LintContext::span_lint"
 reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint*` functions instead"
+allow-invalid = true
 
 [[disallowed-methods]]
 path = "rustc_middle::ty::context::TyCtxt::node_span_lint"
 reason = "this function does not add a link to our documentation, please use the `clippy_utils::diagnostics::span_lint_hir*` functions instead"
+allow-invalid = true

clippy_config/src/conf.rs

@@ -120,12 +120,7 @@ impl ConfError {
         Self {
             message: message.into(),
             suggestion,
-            span: Span::new(
-                file.start_pos + BytePos::from_usize(span.start),
-                file.start_pos + BytePos::from_usize(span.end),
-                SyntaxContext::root(),
-                None,
-            ),
+            span: span_from_toml_range(file, span),
         }
     }
 }
@@ -176,11 +171,61 @@ macro_rules! default_text {
     };
 }
 
+macro_rules! deserialize {
+    ($map:expr, $ty:ty, $errors:expr, $file:expr) => {{
+        let raw_value = $map.next_value::<toml::Spanned<toml::Value>>()?;
+        let value_span = raw_value.span();
+        let value = match <$ty>::deserialize(raw_value.into_inner()) {
+            Err(e) => {
+                $errors.push(ConfError::spanned(
+                    $file,
+                    e.to_string().replace('\n', " ").trim(),
+                    None,
+                    value_span,
+                ));
+                continue;
+            },
+            Ok(value) => value,
+        };
+        (value, value_span)
+    }};
+
+    ($map:expr, $ty:ty, $errors:expr, $file:expr, $replacements_allowed:expr) => {{
+        let array = $map.next_value::<Vec<toml::Spanned<toml::Value>>>()?;
+        let mut disallowed_paths_span = Range {
+            start: usize::MAX,
+            end: usize::MIN,
+        };
+        let mut disallowed_paths = Vec::new();
+        for raw_value in array {
+            let value_span = raw_value.span();
+            let mut disallowed_path = match DisallowedPath::<$replacements_allowed>::deserialize(raw_value.into_inner())
+            {
+                Err(e) => {
+                    $errors.push(ConfError::spanned(
+                        $file,
+                        e.to_string().replace('\n', " ").trim(),
+                        None,
+                        value_span,
+                    ));
+                    continue;
+                },
+                Ok(disallowed_path) => disallowed_path,
+            };
+            disallowed_paths_span = union(&disallowed_paths_span, &value_span);
+            disallowed_path.set_span(span_from_toml_range($file, value_span));
+            disallowed_paths.push(disallowed_path);
+        }
+        (disallowed_paths, disallowed_paths_span)
+    }};
+}
+
 macro_rules! define_Conf {
     ($(
         $(#[doc = $doc:literal])+
         $(#[conf_deprecated($dep:literal, $new_conf:ident)])?
         $(#[default_text = $default_text:expr])?
+        $(#[disallowed_paths_allow_replacements = $replacements_allowed:expr])?
         $(#[lints($($for_lints:ident),* $(,)?)])?
         $name:ident: $ty:ty = $default:expr,
     )*) => {
@@ -219,7 +264,7 @@ macro_rules! define_Conf {
                 let mut errors = Vec::new();
                 let mut warnings = Vec::new();
 
-                // Declare a local variable for each field field available to a configuration file.
+                // Declare a local variable for each field available to a configuration file.
                 $(let mut $name = None;)*
 
                 // could get `Field` here directly, but get `String` first for diagnostics
@@ -237,15 +282,8 @@ macro_rules! define_Conf {
                         $(Field::$name => {
                             // Is this a deprecated field, i.e., is `$dep` set? If so, push a warning.
                             $(warnings.push(ConfError::spanned(self.0, format!("deprecated field `{}`. {}", name.get_ref(), $dep), None, name.span()));)?
-                            let raw_value = map.next_value::<toml::Spanned<toml::Value>>()?;
-                            let value_span = raw_value.span();
-                            let value = match <$ty>::deserialize(raw_value.into_inner()) {
-                                Err(e) => {
-                                    errors.push(ConfError::spanned(self.0, e.to_string().replace('\n', " ").trim(), None, value_span));
-                                    continue;
-                                }
-                                Ok(value) => value
-                            };
+                            let (value, value_span) =
+                                deserialize!(map, $ty, errors, self.0 $(, $replacements_allowed)?);
                             // Was this field set previously?
                             if $name.is_some() {
                                 errors.push(ConfError::spanned(self.0, format!("duplicate field `{}`", name.get_ref()), None, name.span()));
@@ -286,6 +324,22 @@ macro_rules! define_Conf {
     };
 }
 
+fn union(x: &Range<usize>, y: &Range<usize>) -> Range<usize> {
+    Range {
+        start: cmp::min(x.start, y.start),
+        end: cmp::max(x.end, y.end),
+    }
+}
+
+fn span_from_toml_range(file: &SourceFile, span: Range<usize>) -> Span {
+    Span::new(
+        file.start_pos + BytePos::from_usize(span.start),
+        file.start_pos + BytePos::from_usize(span.end),
+        SyntaxContext::root(),
+        None,
+    )
+}
+
 define_Conf! {
     /// Which crates to allow absolute paths from
     #[lints(absolute_paths)]
@@ -472,6 +526,7 @@ define_Conf! {
     )]
     avoid_breaking_exported_api: bool = true,
     /// The list of types which may not be held across an await point.
+    #[disallowed_paths_allow_replacements = false]
     #[lints(await_holding_invalid_type)]
     await_holding_invalid_types: Vec<DisallowedPathWithoutReplacement> = Vec::new(),
     /// DEPRECATED LINT: BLACKLISTED_NAME.
@@ -517,9 +572,11 @@ define_Conf! {
     #[conf_deprecated("Please use `cognitive-complexity-threshold` instead", cognitive_complexity_threshold)]
     cyclomatic_complexity_threshold: u64 = 25,
     /// The list of disallowed macros, written as fully qualified paths.
+    #[disallowed_paths_allow_replacements = true]
     #[lints(disallowed_macros)]
     disallowed_macros: Vec<DisallowedPath> = Vec::new(),
     /// The list of disallowed methods, written as fully qualified paths.
+    #[disallowed_paths_allow_replacements = true]
     #[lints(disallowed_methods)]
     disallowed_methods: Vec<DisallowedPath> = Vec::new(),
     /// The list of disallowed names to lint about. NB: `bar` is not here since it has legitimate uses. The value
@@ -528,6 +585,7 @@ define_Conf! {
     #[lints(disallowed_names)]
     disallowed_names: Vec<String> = DEFAULT_DISALLOWED_NAMES.iter().map(ToString::to_string).collect(),
     /// The list of disallowed types, written as fully qualified paths.
+    #[disallowed_paths_allow_replacements = true]
     #[lints(disallowed_types)]
     disallowed_types: Vec<DisallowedPath> = Vec::new(),
     /// The list of words this lint should not consider as identifiers needing ticks. The value

clippy_config/src/lib.rs

@@ -13,6 +13,7 @@
     rustc::untranslatable_diagnostic
 )]
 
+extern crate rustc_data_structures;
 extern crate rustc_errors;
 extern crate rustc_hir;
 extern crate rustc_middle;

clippy_config/src/types.rs

@@ -1,5 +1,7 @@
-use clippy_utils::def_path_def_ids;
+use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::{Applicability, Diag};
+use rustc_hir::PrimTy;
+use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefIdMap;
 use rustc_middle::ty::TyCtxt;
 use rustc_span::Span;
@@ -21,6 +23,17 @@ pub struct DisallowedPath<const REPLACEMENT_ALLOWED: bool = true> {
     path: String,
     reason: Option<String>,
     replacement: Option<String>,
+    /// Setting `allow_invalid` to true suppresses a warning if `path` does not refer to an existing
+    /// definition.
+    ///
+    /// This could be useful when conditional compilation is used, or when a clippy.toml file is
+    /// shared among multiple projects.
+    allow_invalid: bool,
+    /// The span of the `DisallowedPath`.
+    ///
+    /// Used for diagnostics.
+    #[serde(skip_serializing)]
+    span: Span,
 }
 
 impl<'de, const REPLACEMENT_ALLOWED: bool> Deserialize<'de> for DisallowedPath<REPLACEMENT_ALLOWED> {
@@ -36,6 +49,8 @@ impl<'de, const REPLACEMENT_ALLOWED: bool> Deserialize<'de> for DisallowedPath<R
             path: enum_.path().to_owned(),
             reason: enum_.reason().map(ToOwned::to_owned),
             replacement: enum_.replacement().map(ToOwned::to_owned),
+            allow_invalid: enum_.allow_invalid(),
+            span: Span::default(),
         })
     }
 }
@@ -50,6 +65,8 @@ enum DisallowedPathEnum {
         path: String,
         reason: Option<String>,
         replacement: Option<String>,
+        #[serde(rename = "allow-invalid")]
+        allow_invalid: Option<bool>,
     },
 }
 
@@ -58,7 +75,7 @@ impl<const REPLACEMENT_ALLOWED: bool> DisallowedPath<REPLACEMENT_ALLOWED> {
         &self.path
     }
 
-    pub fn diag_amendment(&self, span: Span) -> impl FnOnce(&mut Diag<'_, ()>) + use<'_, REPLACEMENT_ALLOWED> {
+    pub fn diag_amendment(&self, span: Span) -> impl FnOnce(&mut Diag<'_, ()>) {
         move |diag| {
             if let Some(replacement) = &self.replacement {
                 diag.span_suggestion(
@@ -72,6 +89,14 @@ impl<const REPLACEMENT_ALLOWED: bool> DisallowedPath<REPLACEMENT_ALLOWED> {
             }
         }
     }
+
+    pub fn span(&self) -> Span {
+        self.span
+    }
+
+    pub fn set_span(&mut self, span: Span) {
+        self.span = span;
+    }
 }
 
 impl DisallowedPathEnum {
@@ -94,20 +119,87 @@ impl DisallowedPathEnum {
             Self::Simple(_) => None,
         }
     }
+
+    fn allow_invalid(&self) -> bool {
+        match &self {
+            Self::WithReason { allow_invalid, .. } => allow_invalid.unwrap_or_default(),
+            Self::Simple(_) => false,
+        }
+    }
 }
 
 /// Creates a map of disallowed items to the reason they were disallowed.
+#[allow(clippy::type_complexity)]
 pub fn create_disallowed_map<const REPLACEMENT_ALLOWED: bool>(
     tcx: TyCtxt<'_>,
-    disallowed: &'static [DisallowedPath<REPLACEMENT_ALLOWED>],
-) -> DefIdMap<(&'static str, &'static DisallowedPath<REPLACEMENT_ALLOWED>)> {
-    disallowed
-        .iter()
-        .map(|x| (x.path(), x.path().split("::").collect::<Vec<_>>(), x))
-        .flat_map(|(name, path, disallowed_path)| {
-            def_path_def_ids(tcx, &path).map(move |id| (id, (name, disallowed_path)))
-        })
-        .collect()
+    disallowed_paths: &'static [DisallowedPath<REPLACEMENT_ALLOWED>],
+    def_kind_predicate: impl Fn(DefKind) -> bool,
+    predicate_description: &str,
+    allow_prim_tys: bool,
+) -> (
+    DefIdMap<(&'static str, &'static DisallowedPath<REPLACEMENT_ALLOWED>)>,
+    FxHashMap<PrimTy, (&'static str, &'static DisallowedPath<REPLACEMENT_ALLOWED>)>,
+) {
+    let mut def_ids: DefIdMap<(&'static str, &'static DisallowedPath<REPLACEMENT_ALLOWED>)> = DefIdMap::default();
+    let mut prim_tys: FxHashMap<PrimTy, (&'static str, &'static DisallowedPath<REPLACEMENT_ALLOWED>)> =
+        FxHashMap::default();
+    for disallowed_path in disallowed_paths {
+        let path = disallowed_path.path();
+        let mut resolutions = clippy_utils::def_path_res(tcx, &path.split("::").collect::<Vec<_>>());
+
+        let mut found_def_id = None;
+        let mut found_prim_ty = false;
+        resolutions.retain(|res| match res {
+            Res::Def(def_kind, def_id) => {
+                found_def_id = Some(*def_id);
+                def_kind_predicate(*def_kind)
+            },
+            Res::PrimTy(_) => {
+                found_prim_ty = true;
+                allow_prim_tys
+            },
+            _ => false,
+        });
+
+        if resolutions.is_empty() {
+            let span = disallowed_path.span();
+
+            if let Some(def_id) = found_def_id {
+                tcx.sess.dcx().span_warn(
+                    span,
+                    format!(
+                        "expected a {predicate_description}, found {} {}",
+                        tcx.def_descr_article(def_id),
+                        tcx.def_descr(def_id)
+                    ),
+                );
+            } else if found_prim_ty {
+                tcx.sess.dcx().span_warn(
+                    span,
+                    format!("expected a {predicate_description}, found a primitive type",),
+                );
+            } else if !disallowed_path.allow_invalid {
+                tcx.sess.dcx().span_warn(
+                    span,
+                    format!("`{path}` does not refer to an existing {predicate_description}"),
+                );
+            }
+        }
+
+        for res in resolutions {
+            match res {
+                Res::Def(_, def_id) => {
+                    def_ids.insert(def_id, (path, disallowed_path));
+                },
+                Res::PrimTy(ty) => {
+                    prim_tys.insert(ty, (path, disallowed_path));
+                },
+                _ => unreachable!(),
+            }
+        }
+    }
+
+    (def_ids, prim_tys)
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Deserialize, Serialize)]

clippy_lints/src/await_holding_invalid.rs

@@ -179,9 +179,14 @@ pub struct AwaitHolding {
 
 impl AwaitHolding {
     pub(crate) fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {
-        Self {
-            def_ids: create_disallowed_map(tcx, &conf.await_holding_invalid_types),
-        }
+        let (def_ids, _) = create_disallowed_map(
+            tcx,
+            &conf.await_holding_invalid_types,
+            crate::disallowed_types::def_kind_predicate,
+            "type",
+            false,
+        );
+        Self { def_ids }
     }
 }
 

clippy_lints/src/disallowed_macros.rs

@@ -3,6 +3,7 @@ use clippy_config::types::{DisallowedPath, create_disallowed_map};
 use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then};
 use clippy_utils::macros::macro_backtrace;
 use rustc_data_structures::fx::FxHashSet;
+use rustc_hir::def::DefKind;
 use rustc_hir::def_id::DefIdMap;
 use rustc_hir::{
     AmbigArg, Expr, ExprKind, ForeignItem, HirId, ImplItem, Item, ItemKind, OwnerId, Pat, Path, Stmt, TraitItem, Ty,
@@ -71,8 +72,15 @@ pub struct DisallowedMacros {
 
 impl DisallowedMacros {
     pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf, earlies: AttrStorage) -> Self {
+        let (disallowed, _) = create_disallowed_map(
+            tcx,
+            &conf.disallowed_macros,
+            |def_kind| matches!(def_kind, DefKind::Macro(_)),
+            "macro",
+            false,
+        );
         Self {
-            disallowed: create_disallowed_map(tcx, &conf.disallowed_macros),
+            disallowed,
             seen: FxHashSet::default(),
             derive_src: None,
             earlies,