Skip to content
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

New lint [min_ident_chars] #10916

Merged
merged 8 commits into from
Jun 12, 2023
Merged
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4958,6 +4958,7 @@ Released 2018-09-13
[`mem_replace_option_with_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_none
[`mem_replace_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default
[`mem_replace_with_uninit`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_uninit
[`min_ident_chars`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars
[`min_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_max
[`misaligned_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#misaligned_transmute
[`mismatched_target_os`]: https://rust-lang.github.io/rust-clippy/master/index.html#mismatched_target_os
Expand Down
22 changes: 22 additions & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -663,3 +663,25 @@ Whether to allow module inception if it's not public.
* [`module_inception`](https://rust-lang.github.io/rust-clippy/master/index.html#module_inception)


## `allowed-idents-below-min-chars`
Allowed names below the minimum allowed characters. 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.

**Default Value:** `{"j", "z", "i", "y", "n", "x", "w"}` (`rustc_data_structures::fx::FxHashSet<String>`)

---
**Affected lints:**
* [`min_ident_chars`](https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars)


## `min-ident-chars-threshold`
Minimum chars an ident can have, anything below or equal to this will be linted.

**Default Value:** `1` (`u64`)

---
**Affected lints:**
* [`min_ident_chars`](https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars)


1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::VERBOSE_FILE_READS_INFO,
crate::methods::WRONG_SELF_CONVENTION_INFO,
crate::methods::ZST_OFFSET_INFO,
crate::min_ident_chars::MIN_IDENT_CHARS_INFO,
crate::minmax::MIN_MAX_INFO,
crate::misc::SHORT_CIRCUIT_STATEMENT_INFO,
crate::misc::TOPLEVEL_REF_ARG_INFO,
Expand Down
9 changes: 9 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ mod matches;
mod mem_forget;
mod mem_replace;
mod methods;
mod min_ident_chars;
mod minmax;
mod misc;
mod misc_early;
Expand Down Expand Up @@ -1033,6 +1034,14 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|_| Box::new(redundant_type_annotations::RedundantTypeAnnotations));
store.register_late_pass(|_| Box::new(arc_with_non_send_sync::ArcWithNonSendSync));
store.register_late_pass(|_| Box::new(needless_if::NeedlessIf));
let allowed_idents_below_min_chars = conf.allowed_idents_below_min_chars.clone();
let min_ident_chars_threshold = conf.min_ident_chars_threshold;
store.register_late_pass(move |_| {
Box::new(min_ident_chars::MinIdentChars {
allowed_idents_below_min_chars: allowed_idents_below_min_chars.clone(),
min_ident_chars_threshold,
})
});
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
153 changes: 153 additions & 0 deletions clippy_lints/src/min_ident_chars.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
use clippy_utils::{diagnostics::span_lint, is_from_proc_macro};
use rustc_data_structures::fx::FxHashSet;
use rustc_hir::{
def::{DefKind, Res},
intravisit::{walk_item, Visitor},
GenericParamKind, HirId, Item, ItemKind, ItemLocalId, Node, Pat, PatKind,
};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::Span;
use std::borrow::Cow;

declare_clippy_lint! {
/// ### What it does
/// Checks for idents which comprise of a single letter.
///
/// Note: This lint can be very noisy when enabled; it may be desirable to only enable it
/// temporarily.
///
/// ### Why is this bad?
/// In many cases it's not, but at times it can severely hinder readability. Some codebases may
/// wish to disallow this to improve readability.
///
/// ### Example
/// ```rust,ignore
/// for m in movies {
/// let title = m.t;
/// }
/// ```
/// Use instead:
/// ```rust,ignore
/// for movie in movies {
/// let title = movie.title;
/// }
/// ```
#[clippy::version = "1.72.0"]
pub MIN_IDENT_CHARS,
restriction,
"disallows idents that are too short"
}
impl_lint_pass!(MinIdentChars => [MIN_IDENT_CHARS]);

#[derive(Clone)]
pub struct MinIdentChars {
pub allowed_idents_below_min_chars: FxHashSet<String>,
pub min_ident_chars_threshold: u64,
}

impl MinIdentChars {
#[expect(clippy::cast_possible_truncation)]
fn is_ident_too_short(&self, cx: &LateContext<'_>, str: &str, span: Span) -> bool {
!in_external_macro(cx.sess(), span)
&& str.len() <= self.min_ident_chars_threshold as usize
&& !str.starts_with('_')
&& !str.is_empty()
&& self.allowed_idents_below_min_chars.get(&str.to_owned()).is_none()
}
}

impl LateLintPass<'_> for MinIdentChars {
fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
if self.min_ident_chars_threshold == 0 {
return;
}

walk_item(&mut IdentVisitor { conf: self, cx }, item);
}

// This is necessary as `Node::Pat`s are not visited in `visit_id`. :/
fn check_pat(&mut self, cx: &LateContext<'_>, pat: &Pat<'_>) {
if let PatKind::Binding(_, _, ident, ..) = pat.kind
&& let str = ident.as_str()
&& self.is_ident_too_short(cx, str, ident.span)
{
emit_min_ident_chars(self, cx, str, ident.span);
}
}
}

struct IdentVisitor<'cx, 'tcx> {
conf: &'cx MinIdentChars,
cx: &'cx LateContext<'tcx>,
}

impl Visitor<'_> for IdentVisitor<'_, '_> {
fn visit_id(&mut self, hir_id: HirId) {
let Self { conf, cx } = *self;
// FIXME(#112534) Reimplementation of `find`, as it uses indexing, which can (and will in
// async functions, or really anything async) panic. This should probably be fixed on the
// rustc side, this is just a temporary workaround.
let node = if hir_id.local_id == ItemLocalId::from_u32(0) {
// In this case, we can just use `find`, `Owner`'s `node` field is private anyway so we can't
// reimplement it even if we wanted to
cx.tcx.hir().find(hir_id)
} else {
let Some(owner) = cx.tcx.hir_owner_nodes(hir_id.owner).as_owner() else {
return;
};
owner.nodes.get(hir_id.local_id).copied().flatten().map(|p| p.node)
};
let Some(node) = node else {
return;
};
let Some(ident) = node.ident() else {
return;
};

let str = ident.as_str();
if conf.is_ident_too_short(cx, str, ident.span) {
if let Node::Item(item) = node && let ItemKind::Use(..) = item.kind {
return;
}
// `struct Awa<T>(T)`
// ^
if let Node::PathSegment(path) = node {
if let Res::Def(def_kind, ..) = path.res && let DefKind::TyParam = def_kind {
return;
}
if matches!(path.res, Res::PrimTy(..)) || path.res.opt_def_id().is_some_and(|def_id| !def_id.is_local())
{
return;
}
}
// `struct Awa<T>(T)`
// ^
if let Node::GenericParam(generic_param) = node
&& let GenericParamKind::Type { .. } = generic_param.kind
{
return;
}

if is_from_proc_macro(cx, &ident) {
return;
}

emit_min_ident_chars(conf, cx, str, ident.span);
}
}
}

fn emit_min_ident_chars(conf: &MinIdentChars, cx: &impl LintContext, ident: &str, span: Span) {
let help = if conf.min_ident_chars_threshold == 1 {
Cow::Borrowed("this ident consists of a single char")
} else {
Cow::Owned(format!(
"this ident is too short ({} <= {})",
ident.len(),
conf.min_ident_chars_threshold,
))
};
span_lint(cx, MIN_IDENT_CHARS, span, &help);
}
18 changes: 18 additions & 0 deletions clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const DEFAULT_DOC_VALID_IDENTS: &[&str] = &[
"CamelCase",
];
const DEFAULT_DISALLOWED_NAMES: &[&str] = &["foo", "baz", "quux"];
const DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS: &[&str] = &["i", "j", "x", "y", "z", "w", "n"];
Centri3 marked this conversation as resolved.
Show resolved Hide resolved

/// Holds information used by `MISSING_ENFORCED_IMPORT_RENAMES` lint.
#[derive(Clone, Debug, Deserialize)]
Expand Down Expand Up @@ -522,6 +523,17 @@ define_Conf! {
///
/// Whether to allow module inception if it's not public.
(allow_private_module_inception: bool = false),
/// Lint: MIN_IDENT_CHARS.
///
/// Allowed names below the minimum allowed characters. 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.
(allowed_idents_below_min_chars: rustc_data_structures::fx::FxHashSet<String> =
super::DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS.iter().map(ToString::to_string).collect()),
/// Lint: MIN_IDENT_CHARS.
///
/// Minimum chars an ident can have, anything below or equal to this will be linted.
(min_ident_chars_threshold: u64 = 1),
}

/// Search for the configuration file.
Expand Down Expand Up @@ -589,6 +601,12 @@ pub fn read(sess: &Session, path: &Path) -> TryConf {
Ok(mut conf) => {
extend_vec_if_indicator_present(&mut conf.conf.doc_valid_idents, DEFAULT_DOC_VALID_IDENTS);
extend_vec_if_indicator_present(&mut conf.conf.disallowed_names, DEFAULT_DISALLOWED_NAMES);
// TODO: THIS SHOULD BE TESTED, this comment will be gone soon
if conf.conf.allowed_idents_below_min_chars.contains(&"..".to_owned()) {
conf.conf
.allowed_idents_below_min_chars
.extend(DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS.iter().map(ToString::to_string));
}

conf
},
Expand Down
29 changes: 23 additions & 6 deletions clippy_utils/src/check_proc_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use rustc_hir::{
use rustc_lint::{LateContext, LintContext};
use rustc_middle::ty::TyCtxt;
use rustc_session::Session;
use rustc_span::{Span, Symbol};
use rustc_span::{symbol::Ident, Span, Symbol};
use rustc_target::spec::abi::Abi;

/// The search pattern to look for. Used by `span_matches_pat`
Expand Down Expand Up @@ -344,14 +344,18 @@ fn ty_search_pat(ty: &Ty<'_>) -> (Pat, Pat) {
}
}

pub trait WithSearchPat {
fn ident_search_pat(ident: Ident) -> (Pat, Pat) {
(Pat::OwnedStr(ident.name.as_str().to_owned()), Pat::Str(""))
}

pub trait WithSearchPat<'cx> {
type Context: LintContext;
fn search_pat(&self, cx: &Self::Context) -> (Pat, Pat);
fn span(&self) -> Span;
}
macro_rules! impl_with_search_pat {
($cx:ident: $ty:ident with $fn:ident $(($tcx:ident))?) => {
impl<'cx> WithSearchPat for $ty<'cx> {
impl<'cx> WithSearchPat<'cx> for $ty<'cx> {
type Context = $cx<'cx>;
#[allow(unused_variables)]
fn search_pat(&self, cx: &Self::Context) -> (Pat, Pat) {
Expand All @@ -372,7 +376,7 @@ impl_with_search_pat!(LateContext: FieldDef with field_def_search_pat);
impl_with_search_pat!(LateContext: Variant with variant_search_pat);
impl_with_search_pat!(LateContext: Ty with ty_search_pat);

impl<'cx> WithSearchPat for (&FnKind<'cx>, &Body<'cx>, HirId, Span) {
impl<'cx> WithSearchPat<'cx> for (&FnKind<'cx>, &Body<'cx>, HirId, Span) {
type Context = LateContext<'cx>;

fn search_pat(&self, cx: &Self::Context) -> (Pat, Pat) {
Expand All @@ -385,7 +389,7 @@ impl<'cx> WithSearchPat for (&FnKind<'cx>, &Body<'cx>, HirId, Span) {
}

// `Attribute` does not have the `hir` associated lifetime, so we cannot use the macro
impl<'cx> WithSearchPat for &'cx Attribute {
impl<'cx> WithSearchPat<'cx> for &'cx Attribute {
type Context = LateContext<'cx>;

fn search_pat(&self, _cx: &Self::Context) -> (Pat, Pat) {
Expand All @@ -397,11 +401,24 @@ impl<'cx> WithSearchPat for &'cx Attribute {
}
}

// `Ident` does not have the `hir` associated lifetime, so we cannot use the macro
impl<'cx> WithSearchPat<'cx> for Ident {
type Context = LateContext<'cx>;

fn search_pat(&self, _cx: &Self::Context) -> (Pat, Pat) {
ident_search_pat(*self)
}

fn span(&self) -> Span {
self.span
}
}

/// Checks if the item likely came from a proc-macro.
///
/// This should be called after `in_external_macro` and the initial pattern matching of the ast as
/// it is significantly slower than both of those.
pub fn is_from_proc_macro<T: WithSearchPat>(cx: &T::Context, item: &T) -> bool {
pub fn is_from_proc_macro<'cx, T: WithSearchPat<'cx>>(cx: &T::Context, item: &T) -> bool {
let (start_pat, end_pat) = item.search_pat(cx);
!span_matches_pat(cx.sess(), item.span(), start_pat, end_pat)
}
Expand Down
3 changes: 3 additions & 0 deletions tests/ui-toml/min_ident_chars/auxiliary/extern_types.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#![allow(nonstandard_style, unused)]

pub struct Aaa;
2 changes: 2 additions & 0 deletions tests/ui-toml/min_ident_chars/clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
allowed-idents-below-min-chars = ["Owo", "Uwu", "wha", "t_e", "lse", "_do", "_i_", "put", "her", "_e"]
Centri3 marked this conversation as resolved.
Show resolved Hide resolved
min-ident-chars-threshold = 3
19 changes: 19 additions & 0 deletions tests/ui-toml/min_ident_chars/min_ident_chars.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//@aux-build:extern_types.rs
#![allow(nonstandard_style, unused)]
#![warn(clippy::min_ident_chars)]

extern crate extern_types;
use extern_types::Aaa;

struct Owo {
Uwu: u128,
aaa: Aaa,
}

fn main() {
let wha = 1;
let vvv = 1;
let uuu = 1;
let (mut a, mut b) = (1, 2);
for i in 0..1000 {}
}
Loading