Skip to content

Add needless_conversion_for_trait lint #15451

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.

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 -->
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
10 changes: 10 additions & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
46 changes: 24 additions & 22 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::ClippyConfiguration;
use crate::types::{
DisallowedPath, DisallowedPathWithoutReplacement, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour,
Rename, SourceItemOrdering, SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings,
ConfPath, ConfPathWithoutReplacement, MacroMatcher, MatchLintBehaviour, PubUnderscoreFieldsBehaviour, Rename,
SourceItemOrdering, SourceItemOrderingCategory, SourceItemOrderingModuleItemGroupings,
SourceItemOrderingModuleItemKind, SourceItemOrderingTraitAssocItemKind, SourceItemOrderingTraitAssocItemKinds,
SourceItemOrderingWithinModuleItemGroupings,
};
Expand Down Expand Up @@ -190,17 +190,16 @@ macro_rules! deserialize {
(value, value_span)
}};

($map:expr, $ty:ty, $errors:expr, $file:expr, $replacements_allowed:expr) => {{
($map:expr, $ty:ty, $errors:expr, $file:expr, $replaceable:expr) => {{
let array = $map.next_value::<Vec<toml::Spanned<toml::Value>>>()?;
let mut disallowed_paths_span = Range {
let mut conf_paths_span = Range {
start: usize::MAX,
end: usize::MIN,
};
let mut disallowed_paths = Vec::new();
let mut conf_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())
{
let mut conf_path = match ConfPath::<String, $replaceable>::deserialize(raw_value.into_inner()) {
Err(e) => {
$errors.push(ConfError::spanned(
$file,
Expand All @@ -210,13 +209,13 @@ macro_rules! deserialize {
));
continue;
},
Ok(disallowed_path) => disallowed_path,
Ok(conf_path) => conf_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);
conf_paths_span = union(&conf_paths_span, &value_span);
conf_path.set_span(span_from_toml_range($file, value_span));
conf_paths.push(conf_path);
}
(disallowed_paths, disallowed_paths_span)
(conf_paths, conf_paths_span)
}};
}

Expand All @@ -225,7 +224,7 @@ 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])?
$(#[conf_paths_allow_replacements = $replaceable:expr])?
$(#[lints($($for_lints:ident),* $(,)?)])?
$name:ident: $ty:ty = $default:expr,
)*) => {
Expand Down Expand Up @@ -283,7 +282,7 @@ macro_rules! define_Conf {
// 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 (value, value_span) =
deserialize!(map, $ty, errors, self.0 $(, $replacements_allowed)?);
deserialize!(map, $ty, errors, self.0 $(, $replaceable)?);
// Was this field set previously?
if $name.is_some() {
errors.push(ConfError::spanned(self.0, format!("duplicate field `{}`", name.get_ref()), None, name.span()));
Expand Down Expand Up @@ -529,9 +528,9 @@ 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]
#[conf_paths_allow_replacements = false]
#[lints(await_holding_invalid_type)]
await_holding_invalid_types: Vec<DisallowedPathWithoutReplacement> = Vec::new(),
await_holding_invalid_types: Vec<ConfPathWithoutReplacement> = Vec::new(),
/// DEPRECATED LINT: BLACKLISTED_NAME.
///
/// Use the Disallowed Names lint instead
Expand Down Expand Up @@ -582,9 +581,9 @@ define_Conf! {
/// - `replacement` (optional): suggested alternative macro
/// - `allow-invalid` (optional, `false` by default): when set to `true`, it will ignore this entry
/// if the path doesn't exist, instead of emitting an error
#[disallowed_paths_allow_replacements = true]
#[conf_paths_allow_replacements = true]
#[lints(disallowed_macros)]
disallowed_macros: Vec<DisallowedPath> = Vec::new(),
disallowed_macros: Vec<ConfPath> = Vec::new(),
/// The list of disallowed methods, written as fully qualified paths.
///
/// **Fields:**
Expand All @@ -593,9 +592,9 @@ define_Conf! {
/// - `replacement` (optional): suggested alternative method
/// - `allow-invalid` (optional, `false` by default): when set to `true`, it will ignore this entry
/// if the path doesn't exist, instead of emitting an error
#[disallowed_paths_allow_replacements = true]
#[conf_paths_allow_replacements = true]
#[lints(disallowed_methods)]
disallowed_methods: Vec<DisallowedPath> = Vec::new(),
disallowed_methods: Vec<ConfPath> = Vec::new(),
/// The list of disallowed names to lint about. NB: `bar` is not here since it has legitimate uses. The value
/// `".."` can be used as part of the list to indicate that the configured values should be appended to the
/// default configuration of Clippy. By default, any configuration will replace the default value.
Expand All @@ -609,9 +608,9 @@ define_Conf! {
/// - `replacement` (optional): suggested alternative type
/// - `allow-invalid` (optional, `false` by default): when set to `true`, it will ignore this entry
/// if the path doesn't exist, instead of emitting an error
#[disallowed_paths_allow_replacements = true]
#[conf_paths_allow_replacements = true]
#[lints(disallowed_types)]
disallowed_types: Vec<DisallowedPath> = Vec::new(),
disallowed_types: Vec<ConfPath> = Vec::new(),
/// The list of words this lint should not consider as identifiers needing ticks. The value
/// `".."` can be used as part of the list to indicate, that the configured values should be appended to the
/// default configuration of Clippy. By default, any configuration will replace the default value. For example:
Expand Down Expand Up @@ -878,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.
Expand Down
114 changes: 86 additions & 28 deletions clippy_config/src/types.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand All @@ -18,11 +20,11 @@ pub struct Rename {
pub rename: String,
}

pub type DisallowedPathWithoutReplacement = DisallowedPath<false>;
pub type ConfPathWithoutReplacement = ConfPath<String, false>;

#[derive(Debug, Serialize)]
pub struct DisallowedPath<const REPLACEMENT_ALLOWED: 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
Expand All @@ -31,20 +33,20 @@ pub struct DisallowedPath<const REPLACEMENT_ALLOWED: bool = true> {
/// 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`.
/// The span of the `ConfPath`.
///
/// Used for diagnostics.
#[serde(skip_serializing)]
span: Span,
}

impl<'de, const REPLACEMENT_ALLOWED: bool> Deserialize<'de> for DisallowedPath<REPLACEMENT_ALLOWED> {
impl<'de, const REPLACEABLE: bool> Deserialize<'de> for ConfPath<String, REPLACEABLE> {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let enum_ = DisallowedPathEnum::deserialize(deserializer)?;
if !REPLACEMENT_ALLOWED && enum_.replacement().is_some() {
let enum_ = ConfPathEnum::deserialize(deserializer)?;
if !REPLACEABLE && enum_.replacement().is_some() {
return Err(de::Error::custom("replacement not allowed for this configuration"));
}
Ok(Self {
Expand All @@ -57,11 +59,11 @@ impl<'de, const REPLACEMENT_ALLOWED: bool> Deserialize<'de> for DisallowedPath<R
}
}

// `DisallowedPathEnum` is an implementation detail to enable the `Deserialize` implementation just
// above. `DisallowedPathEnum` is not meant to be used outside of this file.
// `ConfPathEnum` is an implementation detail to enable the `Deserialize` implementation just above.
// `ConfPathEnum` is not meant to be used outside of this file.
#[derive(Debug, Deserialize, Serialize)]
#[serde(untagged, deny_unknown_fields)]
enum DisallowedPathEnum {
enum ConfPathEnum {
Simple(String),
WithReason {
path: String,
Expand All @@ -72,8 +74,8 @@ enum DisallowedPathEnum {
},
}

impl<const REPLACEMENT_ALLOWED: bool> DisallowedPath<REPLACEMENT_ALLOWED> {
pub fn path(&self) -> &str {
impl<T, const REPLACEABLE: bool> ConfPath<T, REPLACEABLE> {
pub fn path(&self) -> &T {
&self.path
}

Expand Down Expand Up @@ -101,7 +103,17 @@ impl<const REPLACEMENT_ALLOWED: bool> DisallowedPath<REPLACEMENT_ALLOWED> {
}
}

impl DisallowedPathEnum {
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;

Expand Down Expand Up @@ -130,25 +142,71 @@ impl DisallowedPathEnum {
}
}

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_disallowed_map<const REPLACEMENT_ALLOWED: bool>(
pub fn create_conf_path_map<T, const REPLACEABLE: bool>(
tcx: TyCtxt<'_>,
disallowed_paths: &'static [DisallowedPath<REPLACEMENT_ALLOWED>],
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 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>)> =
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 disallowed_path in disallowed_paths {
let path = disallowed_path.path();
let sym_path: Vec<Symbol> = path.split("::").map(Symbol::intern).collect();
for conf_path in conf_paths {
let path = conf_path.path();
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)));

Expand All @@ -162,7 +220,7 @@ pub fn create_disallowed_map<const REPLACEMENT_ALLOWED: bool>(

if resolutions.is_empty()
&& prim_ty.is_none()
&& !disallowed_path.allow_invalid
&& !conf_path.allow_invalid
// Don't warn about unloaded crates:
// https://github.com/rust-lang/rust-clippy/pull/14397#issuecomment-2848328221
&& (sym_path.len() < 2 || !find_crates(tcx, sym_path[0]).is_empty())
Expand All @@ -179,16 +237,16 @@ pub fn create_disallowed_map<const REPLACEMENT_ALLOWED: bool>(
};
tcx.sess
.dcx()
.struct_span_warn(disallowed_path.span(), message)
.struct_span_warn(conf_path.span(), message)
.with_help("add `allow-invalid = true` to the entry to suppress this warning")
.emit();
}

for def_id in resolutions {
def_ids.insert(def_id, (path, disallowed_path));
def_ids.insert(def_id, (path, conf_path));
}
if let Some(ty) = prim_ty {
prim_tys.insert(ty, (path, disallowed_path));
prim_tys.insert(ty, (path, conf_path));
}
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_dev/src/new_lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ fn create_lint(lint: &LintData<'_>, enable_msrv: bool) -> io::Result<()> {
} else {
let lint_contents = get_lint_file_contents(lint, enable_msrv);
let lint_path = format!("clippy_lints/src/{}.rs", lint.name);
write_file(&lint_path, lint_contents.as_bytes())?;
write_file(&lint_path, &lint_contents)?;
println!("Generated lint file: `{lint_path}`");

Ok(())
Expand Down Expand Up @@ -433,7 +433,7 @@ fn create_lint_for_ty(lint: &LintData<'_>, enable_msrv: bool, ty: &str) -> io::R
);
}

write_file(lint_file_path.as_path(), lint_file_contents)?;
write_file(&lint_file_path, lint_file_contents)?;
println!("Generated lint file: `clippy_lints/src/{ty}/{}.rs`", lint.name);
println!(
"Be sure to add a call to `{}::check` in `clippy_lints/src/{ty}/mod.rs`!",
Expand Down
Loading
Loading