Skip to content

Commit

Permalink
Auto merge of #11818 - y21:more_redundant_guards, r=llogiq
Browse files Browse the repository at this point in the history 
[`redundant_guards`]: catch `is_empty`, `starts_with` and `ends_with` on slices and `str`s

Fixes #11807

Few things worth mentioning:
- Taking `snippet`s is now done at callsite, instead of passing a span and doing it in `emit_redundant_guards`. This is because we now need custom suggestion strings in certain places, like `""` for `str::is_empty`.
- This now uses `snippet` instead of `snippet_with_applicability`. I don't think this really makes any difference for `MaybeIncorrect`, though?
- This could also lint byte strings, as they're of type `&[u8; N]`, but that can be ugly so I decided to leave it out for now

changelog: [`redundant_guards`]: catch `str::is_empty`, `slice::is_empty`, `slice::starts_with` and `slice::ends_with`
  • Loading branch information
@bors
bors committed Nov 29, 2023
2 parents 57397a5 + 8f9c738 commit 8b0bf64
Show file tree
Hide file tree
Showing 5 changed files with 293 additions and 24 deletions.
15 changes: 6 additions & 9 deletions clippy_lints/src/manual_async_fn.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,11 @@ fn desugared_async_block<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>)
}

fn suggested_ret(cx: &LateContext<'_>, output: &Ty<'_>) -> Option<(&'static str, String)> {
match output.kind {
TyKind::Tup(tys) if tys.is_empty() => {
let sugg = "remove the return type";
Some((sugg, String::new()))
},
_ => {
let sugg = "return the output of the future directly";
snippet_opt(cx, output.span).map(|snip| (sugg, format!(" -> {snip}")))
},
if let TyKind::Tup([]) = output.kind {
let sugg = "remove the return type";
Some((sugg, String::new()))
} else {
let sugg = "return the output of the future directly";
snippet_opt(cx, output.span).map(|snip| (sugg, format!(" -> {snip}")))
}
}
102 changes: 88 additions & 14 deletions clippy_lints/src/matches/redundant_guards.rs
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::path_to_local;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::source::snippet;
use clippy_utils::visitors::{for_each_expr, is_local_used};
use rustc_ast::{BorrowKind, LitKind};
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, Guard, MatchSource, Node, Pat, PatKind};
use rustc_lint::LateContext;
use rustc_span::symbol::Ident;
use rustc_span::Span;
use rustc_span::{Span, Symbol};
use std::borrow::Cow;
use std::ops::ControlFlow;

use super::REDUNDANT_GUARDS;
Expand Down Expand Up @@ -41,7 +42,14 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
(PatKind::Ref(..), None) | (_, Some(_)) => continue,
_ => arm.pat.span,
};
emit_redundant_guards(cx, outer_arm, if_expr.span, pat_span, &binding, arm.guard);
emit_redundant_guards(
cx,
outer_arm,
if_expr.span,
snippet(cx, pat_span, "<binding>"),
&binding,
arm.guard,
);
}
// `Some(x) if let Some(2) = x`
else if let Guard::IfLet(let_expr) = guard
Expand All @@ -52,7 +60,14 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
(PatKind::Ref(..), None) | (_, Some(_)) => continue,
_ => let_expr.pat.span,
};
emit_redundant_guards(cx, outer_arm, let_expr.span, pat_span, &binding, None);
emit_redundant_guards(
cx,
outer_arm,
let_expr.span,
snippet(cx, pat_span, "<binding>"),
&binding,
None,
);
}
// `Some(x) if x == Some(2)`
// `Some(x) if Some(2) == x`
Expand All @@ -78,11 +93,76 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
(ExprKind::AddrOf(..), None) | (_, Some(_)) => continue,
_ => pat.span,
};
emit_redundant_guards(cx, outer_arm, if_expr.span, pat_span, &binding, None);
emit_redundant_guards(
cx,
outer_arm,
if_expr.span,
snippet(cx, pat_span, "<binding>"),
&binding,
None,
);
} else if let Guard::If(if_expr) = guard
&& let ExprKind::MethodCall(path, recv, args, ..) = if_expr.kind
&& let Some(binding) = get_pat_binding(cx, recv, outer_arm)
{
check_method_calls(cx, outer_arm, path.ident.name, recv, args, if_expr, &binding);
}
}
}

fn check_method_calls<'tcx>(
cx: &LateContext<'tcx>,
arm: &Arm<'tcx>,
method: Symbol,
recv: &Expr<'_>,
args: &[Expr<'_>],
if_expr: &Expr<'_>,
binding: &PatBindingInfo,
) {
let ty = cx.typeck_results().expr_ty(recv).peel_refs();
let slice_like = ty.is_slice() || ty.is_array();

let sugg = if method == sym!(is_empty) {
// `s if s.is_empty()` becomes ""
// `arr if arr.is_empty()` becomes []

if ty.is_str() {
r#""""#.into()
} else if slice_like {
"[]".into()
} else {
return;
}
} else if slice_like
&& let Some(needle) = args.first()
&& let ExprKind::AddrOf(.., needle) = needle.kind
&& let ExprKind::Array(needles) = needle.kind
&& needles.iter().all(|needle| expr_can_be_pat(cx, needle))
{
// `arr if arr.starts_with(&[123])` becomes [123, ..]
// `arr if arr.ends_with(&[123])` becomes [.., 123]
// `arr if arr.starts_with(&[])` becomes [..] (why would anyone write this?)

let mut sugg = snippet(cx, needle.span, "<needle>").into_owned();

if needles.is_empty() {
sugg.insert_str(1, "..");
} else if method == sym!(starts_with) {
sugg.insert_str(sugg.len() - 1, ", ..");
} else if method == sym!(ends_with) {
sugg.insert_str(1, ".., ");
} else {
return;
}

sugg.into()
} else {
return;
};

emit_redundant_guards(cx, arm, if_expr.span, sugg, binding, None);
}

struct PatBindingInfo {
span: Span,
byref_ident: Option<Ident>,
Expand Down Expand Up @@ -134,19 +214,16 @@ fn emit_redundant_guards<'tcx>(
cx: &LateContext<'tcx>,
outer_arm: &Arm<'tcx>,
guard_span: Span,
pat_span: Span,
binding_replacement: Cow<'static, str>,
pat_binding: &PatBindingInfo,
inner_guard: Option<Guard<'_>>,
) {
let mut app = Applicability::MaybeIncorrect;

span_lint_and_then(
cx,
REDUNDANT_GUARDS,
guard_span.source_callsite(),
"redundant guard",
|diag| {
let binding_replacement = snippet_with_applicability(cx, pat_span, "<binding_repl>", &mut app);
let suggestion_span = match *pat_binding {
PatBindingInfo {
span,
Expand All @@ -170,14 +247,11 @@ fn emit_redundant_guards<'tcx>(
Guard::IfLet(l) => ("if let", l.span),
};

format!(
" {prefix} {}",
snippet_with_applicability(cx, span, "<guard>", &mut app),
)
format!(" {prefix} {}", snippet(cx, span, "<guard>"))
}),
),
],
app,
Applicability::MaybeIncorrect,
);
},
);
Expand Down
57 changes: 57 additions & 0 deletions tests/ui/redundant_guards.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,3 +193,60 @@ mod issue11465 {
}
}
}

fn issue11807() {
#![allow(clippy::single_match)]

match Some(Some("")) {
Some(Some("")) => {},
_ => {},
}

match Some(Some(String::new())) {
// Do not lint: String deref-coerces to &str
Some(Some(x)) if x.is_empty() => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some([])) => {},
_ => {},
}

match Some(Some([] as [i32; 0])) {
Some(Some([])) => {},
_ => {},
}

match Some(Some(Vec::<()>::new())) {
// Do not lint: Vec deref-coerces to &[T]
Some(Some(x)) if x.is_empty() => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some([..])) => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some([1, ..])) => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some([1, 2, ..])) => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some([.., 1, 2])) => {},
_ => {},
}

match Some(Some(Vec::<i32>::new())) {
// Do not lint: deref coercion
Some(Some(x)) if x.starts_with(&[1, 2]) => {},
_ => {},
}
}
57 changes: 57 additions & 0 deletions tests/ui/redundant_guards.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,3 +193,60 @@ mod issue11465 {
}
}
}

fn issue11807() {
#![allow(clippy::single_match)]

match Some(Some("")) {
Some(Some(x)) if x.is_empty() => {},
_ => {},
}

match Some(Some(String::new())) {
// Do not lint: String deref-coerces to &str
Some(Some(x)) if x.is_empty() => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some(x)) if x.is_empty() => {},
_ => {},
}

match Some(Some([] as [i32; 0])) {
Some(Some(x)) if x.is_empty() => {},
_ => {},
}

match Some(Some(Vec::<()>::new())) {
// Do not lint: Vec deref-coerces to &[T]
Some(Some(x)) if x.is_empty() => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some(x)) if x.starts_with(&[]) => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some(x)) if x.starts_with(&[1]) => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some(x)) if x.starts_with(&[1, 2]) => {},
_ => {},
}

match Some(Some(&[] as &[i32])) {
Some(Some(x)) if x.ends_with(&[1, 2]) => {},
_ => {},
}

match Some(Some(Vec::<i32>::new())) {
// Do not lint: deref coercion
Some(Some(x)) if x.starts_with(&[1, 2]) => {},
_ => {},
}
}
86 changes: 85 additions & 1 deletion tests/ui/redundant_guards.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,5 +203,89 @@ LL - B { ref c, .. } if matches!(c, &1) => {},
LL + B { c: 1, .. } => {},
|

error: aborting due to 17 previous errors
error: redundant guard
--> $DIR/redundant_guards.rs:201:26
|
LL | Some(Some(x)) if x.is_empty() => {},
| ^^^^^^^^^^^^
|
help: try
|
LL - Some(Some(x)) if x.is_empty() => {},
LL + Some(Some("")) => {},
|

error: redundant guard
--> $DIR/redundant_guards.rs:212:26
|
LL | Some(Some(x)) if x.is_empty() => {},
| ^^^^^^^^^^^^
|
help: try
|
LL - Some(Some(x)) if x.is_empty() => {},
LL + Some(Some([])) => {},
|

error: redundant guard
--> $DIR/redundant_guards.rs:217:26
|
LL | Some(Some(x)) if x.is_empty() => {},
| ^^^^^^^^^^^^
|
help: try
|
LL - Some(Some(x)) if x.is_empty() => {},
LL + Some(Some([])) => {},
|

error: redundant guard
--> $DIR/redundant_guards.rs:228:26
|
LL | Some(Some(x)) if x.starts_with(&[]) => {},
| ^^^^^^^^^^^^^^^^^^
|
help: try
|
LL - Some(Some(x)) if x.starts_with(&[]) => {},
LL + Some(Some([..])) => {},
|

error: redundant guard
--> $DIR/redundant_guards.rs:233:26
|
LL | Some(Some(x)) if x.starts_with(&[1]) => {},
| ^^^^^^^^^^^^^^^^^^^
|
help: try
|
LL - Some(Some(x)) if x.starts_with(&[1]) => {},
LL + Some(Some([1, ..])) => {},
|

error: redundant guard
--> $DIR/redundant_guards.rs:238:26
|
LL | Some(Some(x)) if x.starts_with(&[1, 2]) => {},
| ^^^^^^^^^^^^^^^^^^^^^^
|
help: try
|
LL - Some(Some(x)) if x.starts_with(&[1, 2]) => {},
LL + Some(Some([1, 2, ..])) => {},
|

error: redundant guard
--> $DIR/redundant_guards.rs:243:26
|
LL | Some(Some(x)) if x.ends_with(&[1, 2]) => {},
| ^^^^^^^^^^^^^^^^^^^^
|
help: try
|
LL - Some(Some(x)) if x.ends_with(&[1, 2]) => {},
LL + Some(Some([.., 1, 2])) => {},
|

error: aborting due to 24 previous errors

0 comments on commit 8b0bf64

Please sign in to comment.