Skip to content

Commit

Permalink
Extend iter_on to catch more cases
Browse files Browse the repository at this point in the history
  • Loading branch information
Centri3 committed Jun 27, 2023
1 parent ecdea8c commit 5385139
Show file tree
Hide file tree
Showing 50 changed files with 785 additions and 342 deletions.
3 changes: 2 additions & 1 deletion clippy_lints/src/dereference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use rustc_span::{symbol::sym, Span, Symbol};
use rustc_trait_selection::infer::InferCtxtExt as _;
use rustc_trait_selection::traits::{query::evaluate_obligation::InferCtxtExt as _, Obligation, ObligationCause};
use std::collections::VecDeque;
use std::iter::once;

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -893,7 +894,7 @@ fn walk_parents<'tcx>(
&& infcx
.type_implements_trait(
trait_id,
[impl_ty.into()].into_iter().chain(subs.iter().copied()),
once(impl_ty.into()).chain(subs.iter().copied()),
cx.param_env,
)
.must_apply_modulo_regions()
Expand Down
8 changes: 5 additions & 3 deletions clippy_lints/src/manual_strip.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::iter::once;

use clippy_utils::consts::{constant, Constant};
use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then};
use clippy_utils::msrvs::{self, Msrv};
Expand Down Expand Up @@ -113,11 +115,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualStrip {
multispan_sugg(
diag,
&format!("try using the `strip_{kind_word}` method"),
vec![(test_span,
once((test_span,
format!("if let Some(<stripped>) = {}.strip_{kind_word}({}) ",
snippet(cx, target_arg.span, ".."),
snippet(cx, pattern.span, "..")))]
.into_iter().chain(strippings.into_iter().map(|span| (span, "<stripped>".into()))),
snippet(cx, pattern.span, ".."))))
.chain(strippings.into_iter().map(|span| (span, "<stripped>".into()))),
);
});
}
Expand Down
92 changes: 0 additions & 92 deletions clippy_lints/src/methods/iter_on_single_or_empty_collections.rs

This file was deleted.

11 changes: 5 additions & 6 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ mod iter_kv_map;
mod iter_next_slice;
mod iter_nth;
mod iter_nth_zero;
mod iter_on_single_or_empty_collections;
mod iter_overeager_cloned;
mod iter_skip_next;
mod iter_with_drain;
Expand Down Expand Up @@ -79,6 +78,7 @@ mod single_char_add_str;
mod single_char_insert_string;
mod single_char_pattern;
mod single_char_push_string;
mod single_or_empty_collections_iter;
mod skip_while_next;
mod stable_sort_primitive;
mod str_splitn;
Expand Down Expand Up @@ -2437,7 +2437,7 @@ declare_clippy_lint! {
/// The type of the resulting iterator might become incompatible with its usage
#[clippy::version = "1.65.0"]
pub ITER_ON_SINGLE_ITEMS,
nursery,
pedantic,
"Iterator for array of length 1"
}

Expand Down Expand Up @@ -2469,7 +2469,7 @@ declare_clippy_lint! {
/// The type of the resulting iterator might become incompatible with its usage
#[clippy::version = "1.65.0"]
pub ITER_ON_EMPTY_COLLECTIONS,
nursery,
pedantic,
"Iterator for empty array"
}

Expand Down Expand Up @@ -3432,6 +3432,8 @@ fn method_call<'tcx>(

impl<'tcx> LateLintPass<'tcx> for Methods {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
single_or_empty_collections_iter::check(cx, expr, &self.msrv);

if expr.span.from_expansion() {
return;
}
Expand Down Expand Up @@ -3725,9 +3727,6 @@ impl Methods {
("is_digit", [radix]) => is_digit_ascii_radix::check(cx, expr, recv, radix, &self.msrv),
("is_none", []) => check_is_some_is_none(cx, expr, recv, false),
("is_some", []) => check_is_some_is_none(cx, expr, recv, true),
("iter" | "iter_mut" | "into_iter", []) => {
iter_on_single_or_empty_collections::check(cx, expr, name, recv);
},
("join", [join_arg]) => {
if let Some(("collect", _, _, span, _)) = method_call(recv) {
unnecessary_join::check(cx, expr, recv, join_arg, span);
Expand Down
209 changes: 209 additions & 0 deletions clippy_lints/src/methods/single_or_empty_collections_iter.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
use clippy_utils::{
diagnostics::span_lint_and_then,
get_parent_expr,
higher::VecArgs,
is_diagnostic_item_or_ctor, is_lang_item_or_ctor, last_path_segment,
macros::root_macro_call_first_node,
msrvs::{Msrv, ITER_ONCE_AND_EMPTY},
path_res,
source::snippet_opt,
std_or_core,
};
use rustc_errors::{Applicability, Diagnostic};
use rustc_hir::{Expr, ExprKind, GenericArg, LangItem};
use rustc_lint::{LateContext, Lint, LintContext};
use rustc_middle::{
lint::in_external_macro,
ty::{Clause, PredicateKind},
};
use rustc_span::{sym, Span};

use super::{ITER_ON_EMPTY_COLLECTIONS, ITER_ON_SINGLE_ITEMS};

#[derive(Clone, Copy, Debug)]
enum Variant<'tcx> {
SomeToOnce,
NoneToEmpty,
OneLenToOnce(&'tcx Expr<'tcx>),
ZeroLenToEmpty,
}

impl<'tcx> Variant<'tcx> {
fn as_lint(self) -> &'static Lint {
match self {
Self::SomeToOnce | Self::OneLenToOnce(_) => ITER_ON_SINGLE_ITEMS,
Self::NoneToEmpty | Self::ZeroLenToEmpty => ITER_ON_EMPTY_COLLECTIONS,
}
}

fn as_str(self) -> &'static str {
match self {
Self::SomeToOnce => "Some",
Self::NoneToEmpty => "None",
Self::OneLenToOnce(_) => "[T; 1]",
Self::ZeroLenToEmpty => "[T; 0]",
}
}

fn desc(self) -> &'static str {
match self {
Self::SomeToOnce | Self::OneLenToOnce(_) => "iterator with only one element",
Self::NoneToEmpty | Self::ZeroLenToEmpty => "empty iterator",
}
}

fn sugg_fn(self, cx: &LateContext<'_>) -> Option<String> {
Some(format!(
"{}::iter::{}",
std_or_core(cx)?,
match self {
Self::SomeToOnce | Self::OneLenToOnce(_) => "once",
Self::NoneToEmpty | Self::ZeroLenToEmpty => "empty",
},
))
}

fn turbofish_or_args_snippet(
self,
cx: &LateContext<'_>,
expr: &Expr<'_>,
method_name: Option<&str>,
) -> Option<String> {
let ref_prefix = ref_prefix(method_name.unwrap_or(""));

match self {
Self::SomeToOnce if let ExprKind::Call(_, [arg]) = expr.kind => {
snippet_opt(cx, arg.span).map(|s| format!("({ref_prefix}{s})"))
},
Self::NoneToEmpty if let ExprKind::Path(qpath) = expr.kind
&& let Some(args) = last_path_segment(&qpath).args
&& let [GenericArg::Type(ty)] = args.args =>
{
snippet_opt(cx, ty.span).map(|s| format!("::<{s}>()"))
},
Self::OneLenToOnce(one) => {
snippet_opt(cx, one.span).map(|s| format!("({ref_prefix}{s})"))
}
Self::NoneToEmpty | Self::ZeroLenToEmpty => Some("()".to_owned()),
Self::SomeToOnce => None,
}
}
}

pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, msrv: &Msrv) {
if !msrv.meets(ITER_ONCE_AND_EMPTY) {
return;
}

let (variant, is_vec, sugg_span) = match expr.kind {
// `[T; 1]`
ExprKind::Array([one]) => (Variant::OneLenToOnce(one), false, expr.span),
// `[T; 0]`
ExprKind::Array([]) => (Variant::ZeroLenToEmpty, false, expr.span),
// `Some`
ExprKind::Call(path, _) if let Some(def_id) = path_res(cx, path).opt_def_id()
&& is_lang_item_or_ctor(cx, def_id, LangItem::OptionSome) =>
{
(Variant::SomeToOnce, false, expr.span)
},
// `None`
ExprKind::Path(qpath) if let Some(def_id) = cx.qpath_res(&qpath, expr.hir_id).opt_def_id()
&& is_lang_item_or_ctor(cx, def_id, LangItem::OptionNone) =>
{
(Variant::NoneToEmpty, false, expr.span)
}
// `vec![]`
_ if let Some(mac_call) = root_macro_call_first_node(cx, expr)
&& let Some(VecArgs::Vec(elems)) = VecArgs::hir(cx, expr) =>
{
if let [one] = elems {
(Variant::OneLenToOnce(one), true, mac_call.span.source_callsite())
} else if elems.is_empty() {
(Variant::ZeroLenToEmpty, true, mac_call.span.source_callsite())
} else {
return;
}
},
_ => return,
};

// `vec![]` must be external
if !is_vec && in_external_macro(cx.sess(), expr.span) {
return;
}

// FIXME: `get_expr_use_or_unification_node` doesn't work here. Even with the exact same check
// that the old code used. Please help

if let Some(parent) = get_parent_expr(cx, expr)
&& let ExprKind::MethodCall(path, recv, args, _) = parent
.peel_blocks()
.peel_borrows()
.kind
{
if recv.hir_id == expr.hir_id
&& let method_name = path.ident.as_str()
&& matches!(method_name, "iter" | "iter_mut" | "into_iter")
{
emit_lint(cx, expr, Some(path.ident.as_str()), parent.span, variant, |_| {});
} else if args.iter().any(|arg| arg.hir_id == expr.hir_id)
&& let Some(def_id) = cx.typeck_results().type_dependent_def_id(parent.hir_id)
{
for predicate in cx.tcx.param_env(def_id).caller_bounds() {
match predicate.kind().skip_binder() {
// FIXME: This doesn't take into account whether this `Clause` is related
// to `arg`, afaik
PredicateKind::Clause(Clause::Trait(trait_predicate))
if is_diagnostic_item_or_ctor(cx, trait_predicate.def_id(), sym::Iterator) =>
{
emit_lint(cx, expr, None, sugg_span, variant, |diag| {
diag.note("this method is generic over `Iterator`");
});
},
_ => {},
}
}
}
}
{}
}

fn emit_lint<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'tcx>,
method_name: Option<&str>,
sugg_span: Span,
variant: Variant<'_>,
f: impl FnOnce(&mut Diagnostic),
) {
let Some(sugg_fn) = variant.sugg_fn(cx) else {
return;
};
let Some(turbofish_or_args) = variant.turbofish_or_args_snippet(cx, expr, method_name) else {
return;
};

span_lint_and_then(
cx,
variant.as_lint(),
expr.span.source_callsite(),
&format!("usage of `{}` to create an {}", variant.as_str(), variant.desc()),
|diag| {
diag.span_suggestion(
sugg_span,
format!("use `{sugg_fn}` instead"),
format!("{sugg_fn}{turbofish_or_args}"),
Applicability::MachineApplicable,
);
f(diag);
},
);
}

fn ref_prefix(method_name: &str) -> &str {
match method_name {
"iter" => "&",
"iter_mut" => "&mut ",
_ => "",
}
}
Loading

0 comments on commit 5385139

Please sign in to comment.