Skip to content

Validate paths in disallowed_* configurations #14397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2025
+273 −64
Merged

Validate paths in disallowed_* configurations #14397

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clippy.toml
View file
Original file line number Diff line number Diff line change
@@ -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
90 changes: 74 additions & 16 deletions clippy_config/src/conf.rs
View file
Original file line number Diff line number Diff line change
@@ -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) => {{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have found it clearer if you add started the pattern with something like (disallowed_paths => …), to clearly show that this is something used by this lint family, instead of relying on matching the extra argument.

Copy link
Contributor Author

@smoelius smoelius Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to apply a "trick" that I learned from the default_text macro defined in the same file.

Specifically, on this line, the replacements_allowed argument is included only if the disallowed_path_replacements_allowed attribute was used:

deserialize!(map, $ty, errors, self.0 $(, $replacements_allowed)?);

In this way, the presence/absence of that argument selects between the two arms in the deserialize! macro.

Your suggestion is to use a keyword to choose between the two arms, correct?

I take your point that the present code is subtle and non-obvious. But, to be honest, I cannot immediately see how to make the keyword approach work. Could you offer me some more guidance? Or is there a way to make the present code more clear?

(Sorry if the "trick" explanation was unnecessary.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've noticed that (that was my "instead of relying on matching the extra argument" point). The idea with the keyword would be to make it clearer that this specific branch will be used by the disallowed_* lints.

But since you get the replacements_allowed from the defineConf macro, it is right that it would be more complicated to use such a keyword. You can forget about the suggestion.

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
1 change: 1 addition & 0 deletions clippy_config/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@
rustc::untranslatable_diagnostic
)]

extern crate rustc_data_structures;
extern crate rustc_errors;
extern crate rustc_hir;
extern crate rustc_middle;
114 changes: 103 additions & 11 deletions clippy_config/src/types.rs
View file
Original file line number Diff line number Diff line change
@@ -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)]
11 changes: 8 additions & 3 deletions clippy_lints/src/await_holding_invalid.rs
View file
Original file line number Diff line number Diff line change
@@ -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 }
}
}

10 changes: 9 additions & 1 deletion clippy_lints/src/disallowed_macros.rs
View file
Original file line number Diff line number Diff line change
@@ -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,
Loading