mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix Clippy with changed for loop desugar

Browse Source
This commit is contained in:
Cameron Steffen 2021-10-27 09:48:06 -05:00
parent 9c83f8c4d1
commit 9d5ea8f709
10 changed files with 90 additions and 140 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/tools/clippy/clippy_lints/src/loops/explicit_counter_loop.rs
View File

@ -1,6 +1,4 @@
use super::{
get_span_of_entire_for_loop, make_iterator_snippet, IncrementVisitor, InitializeVisitor, EXPLICIT_COUNTER_LOOP,
};
use super::{make_iterator_snippet, IncrementVisitor, InitializeVisitor, EXPLICIT_COUNTER_LOOP};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::{get_enclosing_block, is_integer_const};
@ -37,12 +35,10 @@ pub(super) fn check<'tcx>(
then {
let mut applicability = Applicability::MachineApplicable;
let for_span = get_span_of_entire_for_loop(expr);
span_lint_and_sugg(
cx,
EXPLICIT_COUNTER_LOOP,
for_span.with_hi(arg.span.hi()),
expr.span.with_hi(arg.span.hi()),
&format!("the variable `{}` is used as a loop counter", name),
"consider using",
format!(

4
src/tools/clippy/clippy_lints/src/loops/manual_memcpy.rs
View File

@ -1,4 +1,4 @@
use super::{get_span_of_entire_for_loop, IncrementVisitor, InitializeVisitor, MANUAL_MEMCPY};
use super::{IncrementVisitor, InitializeVisitor, MANUAL_MEMCPY};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet;
use clippy_utils::sugg::Sugg;
@ -86,7 +86,7 @@ pub(super) fn check<'tcx>(
span_lint_and_sugg(
cx,
MANUAL_MEMCPY,
get_span_of_entire_for_loop(expr),
expr.span,
"it looks like you're manually copying between slices",
"try replacing the loop by",
big_sugg,

19
src/tools/clippy/clippy_lints/src/loops/mod.rs
View File

@ -23,7 +23,7 @@
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Span;
use utils::{get_span_of_entire_for_loop, make_iterator_snippet, IncrementVisitor, InitializeVisitor};
use utils::{make_iterator_snippet, IncrementVisitor, InitializeVisitor};
declare_clippy_lint! {
/// ### What it does
@ -566,7 +566,15 @@
impl<'tcx> LateLintPass<'tcx> for Loops {
#[allow(clippy::too_many_lines)]
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if let Some(higher::ForLoop { pat, arg, body, span }) = higher::ForLoop::hir(expr) {
let for_loop = higher::ForLoop::hir(expr);
if let Some(higher::ForLoop {
pat,
arg,
body,
loop_id,
span,
}) = for_loop
{
// we don't want to check expanded macros
// this check is not at the top of the function
// since higher::for_loop expressions are marked as expansions
@ -574,6 +582,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
return;
}
check_for_loop(cx, pat, arg, body, expr, span);
if let ExprKind::Block(block, _) = body.kind {
never_loop::check(cx, block, loop_id, span, for_loop.as_ref());
}
}
// we don't want to check expanded macros
@ -582,7 +593,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
}
// check for never_loop
never_loop::check(cx, expr);
if let ExprKind::Loop(block, ..) = expr.kind {
never_loop::check(cx, block, expr.hir_id, expr.span, None);
}
// check for `loop { if let {} else break }` that could be `while let`
// (also matches an explicit "match" instead of "if let")

58
src/tools/clippy/clippy_lints/src/loops/never_loop.rs
View File

@ -4,35 +4,41 @@
use clippy_utils::higher::ForLoop;
use clippy_utils::source::snippet;
use rustc_errors::Applicability;
use rustc_hir::{Block, Expr, ExprKind, HirId, InlineAsmOperand, LoopSource, Node, Pat, Stmt, StmtKind};
use rustc_hir::{Block, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind};
use rustc_lint::LateContext;
use rustc_span::Span;
use std::iter::{once, Iterator};
pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if let ExprKind::Loop(block, _, source, _) = expr.kind {
match never_loop_block(block, expr.hir_id) {
NeverLoopResult::AlwaysBreak => {
span_lint_and_then(cx, NEVER_LOOP, expr.span, "this loop never actually loops", |diag| {
if_chain! {
if source == LoopSource::ForLoop;
if let Some((_, Node::Expr(parent_match))) = cx.tcx.hir().parent_iter(expr.hir_id).nth(1);
if let Some(ForLoop { arg: iterator, pat, span: for_span, .. }) = ForLoop::hir(parent_match);
then {
// Suggests using an `if let` instead. This is `Unspecified` because the
// loop may (probably) contain `break` statements which would be invalid
// in an `if let`.
diag.span_suggestion_verbose(
for_span.with_hi(iterator.span.hi()),
"if you need the first element of the iterator, try writing",
for_to_if_let_sugg(cx, iterator, pat),
Applicability::Unspecified,
);
}
};
});
},
NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (),
}
pub(super) fn check(
cx: &LateContext<'tcx>,
block: &'tcx Block<'_>,
loop_id: HirId,
span: Span,
for_loop: Option<&ForLoop<'_>>,
) {
match never_loop_block(block, loop_id) {
NeverLoopResult::AlwaysBreak => {
span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| {
if let Some(ForLoop {
arg: iterator,
pat,
span: for_span,
..
}) = for_loop
{
// Suggests using an `if let` instead. This is `Unspecified` because the
// loop may (probably) contain `break` statements which would be invalid
// in an `if let`.
diag.span_suggestion_verbose(
for_span.with_hi(iterator.span.hi()),
"if you need the first element of the iterator, try writing",
for_to_if_let_sugg(cx, iterator, pat),
Applicability::Unspecified,
);
}
});
},
NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (),
}
}

5
src/tools/clippy/clippy_lints/src/loops/single_element_loop.rs
View File

@ -1,4 +1,4 @@
use super::{get_span_of_entire_for_loop, SINGLE_ELEMENT_LOOP};
use super::SINGLE_ELEMENT_LOOP;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::single_segment_path;
use clippy_utils::source::{indent_of, snippet};
@ -30,7 +30,6 @@ pub(super) fn check<'tcx>(
if !block.stmts.is_empty();
then {
let for_span = get_span_of_entire_for_loop(expr);
let mut block_str = snippet(cx, block.span, "..").into_owned();
block_str.remove(0);
block_str.pop();
@ -39,7 +38,7 @@ pub(super) fn check<'tcx>(
span_lint_and_sugg(
cx,
SINGLE_ELEMENT_LOOP,
for_span,
expr.span,
"for loop over a single element",
"try",
format!("{{\n{}let {} = &{};{}}}", " ".repeat(indent_of(cx, block.stmts[0].span).unwrap_or(0)), target.name, list_item_name, block_str),

12
src/tools/clippy/clippy_lints/src/loops/utils.rs
View File

@ -7,7 +7,6 @@
use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Stmt, StmtKind};
use rustc_lint::LateContext;
use rustc_middle::hir::map::Map;
use rustc_span::source_map::Span;
use rustc_span::symbol::{sym, Symbol};
use std::iter::Iterator;
@ -300,17 +299,6 @@ fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
}
}
// this function assumes the given expression is a `for` loop.
pub(super) fn get_span_of_entire_for_loop(expr: &Expr<'_>) -> Span {
// for some reason this is the only way to get the `Span`
// of the entire `for` loop
if let ExprKind::Match(_, arms, _) = &expr.kind {
arms[0].body.span
} else {
unreachable!()
}
}
/// If `arg` was the argument to a `for` loop, return the "cleanest" way of writing the
/// actual `Iterator` that the loop uses.
pub(super) fn make_iterator_snippet(cx: &LateContext<'_>, arg: &Expr<'_>, applic_ref: &mut Applicability) -> String {

5
src/tools/clippy/clippy_lints/src/misc.rs
View File

@ -20,8 +20,8 @@
use clippy_utils::consts::{constant, Constant};
use clippy_utils::sugg::Sugg;
use clippy_utils::{
expr_path_res, get_item_name, get_parent_expr, higher, in_constant, is_diag_trait_item, is_integer_const,
iter_input_pats, last_path_segment, match_any_def_paths, paths, unsext, SpanlessEq,
expr_path_res, get_item_name, get_parent_expr, in_constant, is_diag_trait_item, is_integer_const, iter_input_pats,
last_path_segment, match_any_def_paths, paths, unsext, SpanlessEq,
};
declare_clippy_lint! {
@ -312,7 +312,6 @@ fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
if let StmtKind::Local(local) = stmt.kind;
if let PatKind::Binding(an, .., name, None) = local.pat.kind;
if let Some(init) = local.init;
if !higher::is_from_for_desugar(local);
if an == BindingAnnotation::Ref || an == BindingAnnotation::RefMut;
then {
// use the macro callsite when the init span (but not the whole local span)

4
src/tools/clippy/clippy_lints/src/unit_types/let_unit_value.rs
View File

@ -1,5 +1,4 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::higher;
use clippy_utils::source::snippet_with_macro_callsite;
use rustc_errors::Applicability;
use rustc_hir::{Stmt, StmtKind};
@ -14,9 +13,6 @@ pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() {
return;
}
if higher::is_from_for_desugar(local) {
return;
}
span_lint_and_then(
cx,
LET_UNIT_VALUE,

62
src/tools/clippy/clippy_utils/src/higher.rs
View File

@ -22,31 +22,31 @@ pub struct ForLoop<'tcx> {
pub arg: &'tcx hir::Expr<'tcx>,
/// `for` loop body
pub body: &'tcx hir::Expr<'tcx>,
/// Compare this against `hir::Destination.target`
pub loop_id: HirId,
/// entire `for` loop span
pub span: Span,
}
impl<'tcx> ForLoop<'tcx> {
#[inline]
/// Parses a desugared `for` loop
pub fn hir(expr: &Expr<'tcx>) -> Option<Self> {
if_chain! {
if let hir::ExprKind::Match(iterexpr, arms, hir::MatchSource::ForLoopDesugar) = expr.kind;
if let Some(first_arm) = arms.get(0);
if let hir::ExprKind::Call(_, iterargs) = iterexpr.kind;
if let Some(first_arg) = iterargs.get(0);
if iterargs.len() == 1 && arms.len() == 1 && first_arm.guard.is_none();
if let hir::ExprKind::Loop(block, ..) = first_arm.body.kind;
if block.expr.is_none();
if let [ _, _, ref let_stmt, ref body ] = *block.stmts;
if let hir::StmtKind::Local(local) = let_stmt.kind;
if let hir::StmtKind::Expr(body_expr) = body.kind;
if let hir::ExprKind::DropTemps(e) = expr.kind;
if let hir::ExprKind::Match(iterexpr, [arm], hir::MatchSource::ForLoopDesugar) = e.kind;
if let hir::ExprKind::Call(_, [arg]) = iterexpr.kind;
if let hir::ExprKind::Loop(block, ..) = arm.body.kind;
if let [stmt] = &*block.stmts;
if let hir::StmtKind::Expr(e) = stmt.kind;
if let hir::ExprKind::Match(_, [_, some_arm], _) = e.kind;
if let hir::PatKind::Struct(_, [field], _) = some_arm.pat.kind;
then {
return Some(Self {
pat: &*local.pat,
arg: first_arg,
body: body_expr,
span: first_arm.span
pat: field.pat,
arg,
body: some_arm.body,
loop_id: arm.body.hir_id,
span: expr.span.ctxt().outer_expn_data().call_site,
});
}
}
@ -678,38 +678,6 @@ pub fn is_display(&self) -> bool {
}
}
/// Checks if a `let` statement is from a `for` loop desugaring.
pub fn is_from_for_desugar(local: &hir::Local<'_>) -> bool {
// This will detect plain for-loops without an actual variable binding:
//
// ```
// for x in some_vec {
// // do stuff
// }
// ```
if_chain! {
if let Some(expr) = local.init;
if let hir::ExprKind::Match(_, _, hir::MatchSource::ForLoopDesugar) = expr.kind;
then {
return true;
}
}
// This detects a variable binding in for loop to avoid `let_unit_value`
// lint (see issue #1964).
//
// ```
// for _ in vec![()] {
// // anything
// }
// ```
if let hir::LocalSource::ForLoopDesugar = local.source {
return true;
}
false
}
/// A parsed `panic!` expansion
pub struct PanicExpn<'tcx> {
/// Span of `panic!(..)`

53
src/tools/clippy/tests/ui/author/for_loop.stdout
View File

@ -11,11 +11,8 @@ if_chain! {
// unimplemented: field checks
if arms.len() == 1;
if let ExprKind::Loop(ref body, ref label, LoopSource::ForLoop) = arms[0].body.kind;
if body.stmts.len() == 4;
if let StmtKind::Local(ref local) = body.stmts[0].kind;
if let PatKind::Binding(BindingAnnotation::Mutable, _, name, None) = local.pat.kind;
if name.as_str() == "__next";
if let StmtKind::Expr(ref e, _) = body.stmts[1].kind
if body.stmts.len() == 1;
if let StmtKind::Expr(ref e, _) = body.stmts[0].kind
if let ExprKind::Match(ref expr2, ref arms1, MatchSource::ForLoopDesugar) = e.kind;
if let ExprKind::Call(ref func1, ref args1) = expr2.kind;
if let ExprKind::Path(ref path2) = func1.kind;
@ -25,39 +22,27 @@ if_chain! {
if let ExprKind::Path(ref path3) = inner.kind;
if match_qpath(path3, &["iter"]);
if arms1.len() == 2;
if let ExprKind::Assign(ref target, ref value, ref _span) = arms1[0].body.kind;
if let ExprKind::Path(ref path4) = target.kind;
if match_qpath(path4, &["__next"]);
if let ExprKind::Path(ref path5) = value.kind;
if match_qpath(path5, &["val"]);
if let PatKind::Struct(ref path6, ref fields1, false) = arms1[0].pat.kind;
if matches!(path6, QPath::LangItem(LangItem::OptionSome, _));
if fields1.len() == 1;
if let ExprKind::Break(ref destination, None) = arms1[0].body.kind;
if let PatKind::Struct(ref path4, ref fields1, false) = arms1[0].pat.kind;
if matches!(path4, QPath::LangItem(LangItem::OptionNone, _));
if fields1.len() == 0;
// unimplemented: field checks
if let ExprKind::Break(ref destination, None) = arms1[1].body.kind;
if let PatKind::Struct(ref path7, ref fields2, false) = arms1[1].pat.kind;
if matches!(path7, QPath::LangItem(LangItem::OptionNone, _));
if fields2.len() == 0;
// unimplemented: field checks
if let StmtKind::Local(ref local1) = body.stmts[2].kind;
if let Some(ref init) = local1.init;
if let ExprKind::Path(ref path8) = init.kind;
if match_qpath(path8, &["__next"]);
if let PatKind::Binding(BindingAnnotation::Unannotated, _, name1, None) = local1.pat.kind;
if name1.as_str() == "y";
if let StmtKind::Expr(ref e1, _) = body.stmts[3].kind
if let ExprKind::Block(ref block) = e1.kind;
if let ExprKind::Block(ref block) = arms1[1].body.kind;
if block.stmts.len() == 1;
if let StmtKind::Local(ref local2) = block.stmts[0].kind;
if let Some(ref init1) = local2.init;
if let ExprKind::Path(ref path9) = init1.kind;
if match_qpath(path9, &["y"]);
if let PatKind::Binding(BindingAnnotation::Unannotated, _, name2, None) = local2.pat.kind;
if name2.as_str() == "z";
if let StmtKind::Local(ref local) = block.stmts[0].kind;
if let Some(ref init) = local.init;
if let ExprKind::Path(ref path5) = init.kind;
if match_qpath(path5, &["y"]);
if let PatKind::Binding(BindingAnnotation::Unannotated, _, name, None) = local.pat.kind;
if name.as_str() == "z";
if block.expr.is_none();
if let PatKind::Struct(ref path6, ref fields2, false) = arms1[1].pat.kind;
if matches!(path6, QPath::LangItem(LangItem::OptionSome, _));
if fields2.len() == 1;
// unimplemented: field checks
if body.expr.is_none();
if let PatKind::Binding(BindingAnnotation::Mutable, _, name3, None) = arms[0].pat.kind;
if name3.as_str() == "iter";
if let PatKind::Binding(BindingAnnotation::Mutable, _, name1, None) = arms[0].pat.kind;
if name1.as_str() == "iter";
then {
// report your lint here
}