Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
8 changes: 4 additions & 4 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use crate::ClippyConfiguration;
use crate::types::{
DisallowedPath, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename, SourceItemOrdering,
SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings, SourceItemOrderingModuleItemKind,
SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds,
DisallowedPath, DisallowedPathWithoutReplacement, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour,
Rename, SourceItemOrdering, SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings,
SourceItemOrderingModuleItemKind, SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds,
};
use clippy_utils::msrvs::Msrv;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -445,7 +445,7 @@ define_Conf! {
avoid_breaking_exported_api: bool = true,
/// The list of types which may not be held across an await point.
#[lints(await_holding_invalid_type)]
await_holding_invalid_types: Vec<DisallowedPath> = Vec::new(),
await_holding_invalid_types: Vec<DisallowedPathWithoutReplacement> = Vec::new(),
/// DEPRECATED LINT: BLACKLISTED_NAME.
///
/// Use the Disallowed Names lint instead
Expand Down
121 changes: 110 additions & 11 deletions clippy_config/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use clippy_utils::def_path_def_ids;
use rustc_errors::{Applicability, Diag};
use rustc_hir::def_id::DefIdMap;
use rustc_middle::ty::TyCtxt;
use rustc_span::Span;
use serde::de::{self, Deserializer, Visitor};
use serde::{Deserialize, Serialize, ser};
use std::collections::HashMap;
Expand All @@ -12,37 +14,135 @@ pub struct Rename {
pub rename: String,
}

#[derive(Debug, Deserialize)]
#[derive(Debug, Serialize)]
pub struct DisallowedPathWithoutReplacement {
path: String,
reason: Option<String>,
}

#[derive(Clone, Debug, Serialize)]
pub struct DisallowedPath {
path: String,
reason: Option<String>,
replacement: Option<String>,
}

impl<'de> Deserialize<'de> for DisallowedPathWithoutReplacement {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let enum_ = DisallowedPathEnum::deserialize(deserializer)?;
if enum_.replacement().is_some() {
return Err(de::Error::custom("replacement not allowed for this configuration"));
}
Ok(Self {
path: enum_.path().to_owned(),
reason: enum_.reason().map(ToOwned::to_owned),
})
}
}

impl<'de> Deserialize<'de> for DisallowedPath {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let enum_ = DisallowedPathEnum::deserialize(deserializer)?;
Ok(Self {
path: enum_.path().to_owned(),
reason: enum_.reason().map(ToOwned::to_owned),
replacement: enum_.replacement().map(ToOwned::to_owned),
})
}
}

#[derive(Debug, Deserialize, Serialize)]
#[serde(untagged)]
pub enum DisallowedPath {
pub enum DisallowedPathEnum {
Copy link
Member

Choose a reason for hiding this comment

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

I do find this to be an odd implementation detail. I assume this is just a workaround to both derive Deserialize and have our const generic, but imo it adds way too much confusion. It should be clearer that it's not to be used elsewhere and ideally wouldn't be here at all.

Let's make this private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the enum private and I added a comment that it is not meant to be used outside of this file.

I assume this is just a workaround to both derive Deserialize and have our const generic, but imo it adds way too much confusion. It should be clearer that it's not to be used elsewhere and ideally wouldn't be here at all.

I'm not wed to this approach. Can you think of another way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is there something I could do to make the code more clear?

Simple(String),
WithReason { path: String, reason: Option<String> },
WithReason {
path: String,
reason: Option<String>,
replacement: Option<String>,
},
}

impl DisallowedPath {
pub trait AmendDiag {
fn path(&self) -> &str;
fn reason(&self) -> Option<&str>;
fn replacement(&self) -> Option<&str>;
fn amend_diag(&self, span: Span, diag: &mut Diag<'_, ()>) {
if let Some(replacement) = &self.replacement() {
diag.span_suggestion(
span,
self.reason().map_or_else(|| String::from("use"), ToOwned::to_owned),
replacement,
Applicability::MachineApplicable,
);
} else if let Some(reason) = self.reason() {
diag.note(reason.to_owned());
}
}
}

impl AmendDiag for DisallowedPathWithoutReplacement {
fn path(&self) -> &str {
&self.path
}
fn reason(&self) -> Option<&str> {
self.reason.as_deref()
}
fn replacement(&self) -> Option<&str> {
None
}
}

impl AmendDiag for DisallowedPath {
fn path(&self) -> &str {
&self.path
}
fn reason(&self) -> Option<&str> {
self.reason.as_deref()
}
fn replacement(&self) -> Option<&str> {
self.replacement.as_deref()
}
}

impl DisallowedPathEnum {
pub fn path(&self) -> &str {
let (Self::Simple(path) | Self::WithReason { path, .. }) = self;

path
}

pub fn reason(&self) -> Option<&str> {
fn reason(&self) -> Option<&str> {
match &self {
Self::WithReason { reason, .. } => reason.as_deref(),
Self::Simple(_) => None,
}
}

fn replacement(&self) -> Option<&str> {
match &self {
Self::WithReason { replacement, .. } => replacement.as_deref(),
Self::Simple(_) => None,
}
}
}

/// Creates a map of disallowed items to the reason they were disallowed.
pub fn create_disallowed_map(
pub fn create_disallowed_map<T: AmendDiag>(
tcx: TyCtxt<'_>,
disallowed: &'static [DisallowedPath],
) -> DefIdMap<(&'static str, Option<&'static str>)> {
disallowed: &'static [T],
) -> DefIdMap<(&'static str, &'static T)> {
disallowed
.iter()
.map(|x| (x.path(), x.path().split("::").collect::<Vec<_>>(), x.reason()))
.flat_map(|(name, path, reason)| def_path_def_ids(tcx, &path).map(move |id| (id, (name, reason))))
.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()
}

Expand Down Expand Up @@ -436,7 +536,6 @@ macro_rules! unimplemented_serialize {
}

unimplemented_serialize! {
DisallowedPath,
Rename,
MacroMatcher,
}
Expand Down
21 changes: 11 additions & 10 deletions clippy_lints/src/await_holding_invalid.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_config::Conf;
use clippy_config::types::create_disallowed_map;
use clippy_config::types::{AmendDiag, DisallowedPathWithoutReplacement, create_disallowed_map};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::{match_def_path, paths};
use rustc_hir as hir;
Expand Down Expand Up @@ -174,7 +174,7 @@ declare_clippy_lint! {
impl_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF, AWAIT_HOLDING_INVALID_TYPE]);

pub struct AwaitHolding {
def_ids: DefIdMap<(&'static str, Option<&'static str>)>,
def_ids: DefIdMap<(&'static str, &'static DisallowedPathWithoutReplacement)>,
}

impl AwaitHolding {
Expand Down Expand Up @@ -247,25 +247,26 @@ impl AwaitHolding {
);
},
);
} else if let Some(&(path, reason)) = self.def_ids.get(&adt.did()) {
emit_invalid_type(cx, ty_cause.source_info.span, path, reason);
} else if let Some(&(path, disallowed_path)) = self.def_ids.get(&adt.did()) {
emit_invalid_type(cx, ty_cause.source_info.span, path, disallowed_path);
}
}
}
}
}

fn emit_invalid_type(cx: &LateContext<'_>, span: Span, path: &'static str, reason: Option<&'static str>) {
fn emit_invalid_type(
cx: &LateContext<'_>,
span: Span,
path: &'static str,
disallowed_path: &'static DisallowedPathWithoutReplacement,
) {
span_lint_and_then(
cx,
AWAIT_HOLDING_INVALID_TYPE,
span,
format!("holding a disallowed type across an await point `{path}`"),
|diag| {
if let Some(reason) = reason {
diag.note(reason);
}
},
|diag| disallowed_path.amend_diag(span, diag),
);
}

Expand Down
18 changes: 7 additions & 11 deletions clippy_lints/src/disallowed_macros.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use clippy_config::Conf;
use clippy_config::types::create_disallowed_map;
use clippy_config::types::{AmendDiag, 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_errors::Diag;
use rustc_hir::def_id::DefIdMap;
use rustc_hir::{
Expr, ExprKind, ForeignItem, HirId, ImplItem, Item, ItemKind, OwnerId, Pat, Path, Stmt, TraitItem, Ty,
Expand Down Expand Up @@ -60,7 +59,7 @@ declare_clippy_lint! {
}

pub struct DisallowedMacros {
disallowed: DefIdMap<(&'static str, Option<&'static str>)>,
disallowed: DefIdMap<(&'static str, &'static DisallowedPath)>,
seen: FxHashSet<ExpnId>,
// Track the most recently seen node that can have a `derive` attribute.
// Needed to use the correct lint level.
Expand Down Expand Up @@ -91,13 +90,8 @@ impl DisallowedMacros {
return;
}

if let Some(&(path, reason)) = self.disallowed.get(&mac.def_id) {
if let Some(&(path, disallowed_path)) = self.disallowed.get(&mac.def_id) {
let msg = format!("use of a disallowed macro `{path}`");
let add_note = |diag: &mut Diag<'_, _>| {
if let Some(reason) = reason {
diag.note(reason);
}
};
if matches!(mac.kind, MacroKind::Derive)
&& let Some(derive_src) = derive_src
{
Expand All @@ -107,10 +101,12 @@ impl DisallowedMacros {
cx.tcx.local_def_id_to_hir_id(derive_src.def_id),
mac.span,
msg,
add_note,
|diag| disallowed_path.amend_diag(mac.span, diag),
);
} else {
span_lint_and_then(cx, DISALLOWED_MACROS, mac.span, msg, add_note);
span_lint_and_then(cx, DISALLOWED_MACROS, mac.span, msg, |diag| {
disallowed_path.amend_diag(mac.span, diag);
});
}
}
}
Expand Down
14 changes: 6 additions & 8 deletions clippy_lints/src/disallowed_methods.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_config::Conf;
use clippy_config::types::create_disallowed_map;
use clippy_config::types::{AmendDiag, DisallowedPath, create_disallowed_map};
use clippy_utils::diagnostics::span_lint_and_then;
use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::DefIdMap;
Expand Down Expand Up @@ -31,6 +31,8 @@ declare_clippy_lint! {
/// # When using an inline table, can add a `reason` for why the method
/// # is disallowed.
/// { path = "std::vec::Vec::leak", reason = "no leaking memory" },
/// # Can also add a `replacement` that will be offered as a suggestion.
/// { path = "std::sync::Mutex::new", reason = "prefer faster & simpler non-poisonable mutex", replacement = "parking_lot::Mutex::new" },
/// ]
/// ```
///
Expand Down Expand Up @@ -58,7 +60,7 @@ declare_clippy_lint! {
}

pub struct DisallowedMethods {
disallowed: DefIdMap<(&'static str, Option<&'static str>)>,
disallowed: DefIdMap<(&'static str, &'static DisallowedPath)>,
}

impl DisallowedMethods {
Expand All @@ -85,17 +87,13 @@ impl<'tcx> LateLintPass<'tcx> for DisallowedMethods {
},
_ => return,
};
if let Some(&(path, reason)) = self.disallowed.get(&id) {
if let Some(&(path, disallowed_path)) = self.disallowed.get(&id) {
span_lint_and_then(
cx,
DISALLOWED_METHODS,
span,
format!("use of a disallowed method `{path}`"),
|diag| {
if let Some(reason) = reason {
diag.note(reason);
}
},
|diag| disallowed_path.amend_diag(span, diag),
);
}
}
Expand Down
25 changes: 11 additions & 14 deletions clippy_lints/src/disallowed_types.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use clippy_config::Conf;
use clippy_config::types::{AmendDiag, DisallowedPath};
use clippy_utils::diagnostics::span_lint_and_then;
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def::Res;
Expand Down Expand Up @@ -31,7 +32,8 @@ declare_clippy_lint! {
/// # When using an inline table, can add a `reason` for why the type
/// # is disallowed.
/// { path = "std::net::Ipv4Addr", reason = "no IPv4 allowed" },
/// ]
/// # Can also add a `replacement` that will be offered as a suggestion.
/// { path = "std::sync::Mutex", reason = "prefer faster & simpler non-poisonable mutex", replacement = "parking_lot::Mutex" }, /// ]
/// ```
///
/// ```rust,ignore
Expand All @@ -51,24 +53,23 @@ declare_clippy_lint! {
}

pub struct DisallowedTypes {
def_ids: DefIdMap<(&'static str, Option<&'static str>)>,
prim_tys: FxHashMap<PrimTy, (&'static str, Option<&'static str>)>,
def_ids: DefIdMap<(&'static str, &'static DisallowedPath)>,
prim_tys: FxHashMap<PrimTy, (&'static str, &'static DisallowedPath)>,
}

impl DisallowedTypes {
pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {
let mut def_ids = DefIdMap::default();
let mut prim_tys = FxHashMap::default();
for x in &conf.disallowed_types {
let path: Vec<_> = x.path().split("::").collect::<Vec<_>>();
let reason = x.reason();
for disallowed_path in &conf.disallowed_types {
let path: Vec<_> = disallowed_path.path().split("::").collect::<Vec<_>>();
for res in clippy_utils::def_path_res(tcx, &path) {
match res {
Res::Def(_, id) => {
def_ids.insert(id, (x.path(), reason));
def_ids.insert(id, (disallowed_path.path(), disallowed_path));
},
Res::PrimTy(ty) => {
prim_tys.insert(ty, (x.path(), reason));
prim_tys.insert(ty, (disallowed_path.path(), disallowed_path));
},
_ => {},
}
Expand All @@ -78,7 +79,7 @@ impl DisallowedTypes {
}

fn check_res_emit(&self, cx: &LateContext<'_>, res: &Res, span: Span) {
let (path, reason) = match res {
let (path, disallowed_path) = match res {
Res::Def(_, did) if let Some(&x) = self.def_ids.get(did) => x,
Res::PrimTy(prim) if let Some(&x) = self.prim_tys.get(prim) => x,
_ => return,
Expand All @@ -88,11 +89,7 @@ impl DisallowedTypes {
DISALLOWED_TYPES,
span,
format!("use of a disallowed type `{path}`"),
|diag| {
if let Some(reason) = reason {
diag.note(reason);
}
},
|diag| disallowed_path.amend_diag(span, diag),
);
}
}
Expand Down
Loading
Loading