From fa380a82a5886768d0509222dc0d49db231a3403 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 18 Jul 2022 02:58:59 +0400 Subject: [PATCH 01/19] Start uplifting `clippy::for_loops_over_fallibles` I refactored the code: - Removed handling of methods, as it felt entirely unnecessary - Removed clippy utils (obviously...) - Used some shiny compiler features (let-else is very handy for lints :eyes:) - I also renamed the lint to `for_loop_over_fallibles` (note: no `s`). I'm not sure what's the naming convention here, so maybe I'm wrong. --- .../rustc_lint/src/for_loop_over_fallibles.rs | 99 +++++++++++++++++++ compiler/rustc_lint/src/lib.rs | 3 + 2 files changed, 102 insertions(+) create mode 100644 compiler/rustc_lint/src/for_loop_over_fallibles.rs diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs new file mode 100644 index 0000000000000..c96c9efe1d8a1 --- /dev/null +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -0,0 +1,99 @@ +use crate::{LateContext, LateLintPass, LintContext}; + +use hir::{Expr, Pat}; +use rustc_hir as hir; +use rustc_middle::ty; +use rustc_span::sym; + +declare_lint! { + /// ### What it does + /// + /// Checks for `for` loops over `Option` or `Result` values. + /// + /// ### Why is this bad? + /// Readability. This is more clearly expressed as an `if + /// let`. + /// + /// ### Example + /// + /// ```rust + /// # let opt = Some(1); + /// # let res: Result = Ok(1); + /// for x in opt { + /// // .. + /// } + /// + /// for x in &res { + /// // .. + /// } + /// + /// for x in res.iter() { + /// // .. + /// } + /// ``` + /// + /// Use instead: + /// ```rust + /// # let opt = Some(1); + /// # let res: Result = Ok(1); + /// if let Some(x) = opt { + /// // .. + /// } + /// + /// if let Ok(x) = res { + /// // .. + /// } + /// ``` + pub FOR_LOOP_OVER_FALLIBLES, + Warn, + "for-looping over an `Option` or a `Result`, which is more clearly expressed as an `if let`" +} + +declare_lint_pass!(ForLoopOverFallibles => [FOR_LOOP_OVER_FALLIBLES]); + +impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + let Some((pat, arg)) = extract_for_loop(expr) else { return }; + + let ty = cx.typeck_results().expr_ty(arg); + + let ty::Adt(adt, _) = ty.kind() else { return }; + + let (article, ty, var) = match adt.did() { + did if cx.tcx.is_diagnostic_item(sym::Option, did) => ("an", "Option", "Some"), + did if cx.tcx.is_diagnostic_item(sym::Result, did) => ("a", "Result", "Ok"), + _ => return, + }; + + let Ok(pat_snip) = cx.sess().source_map().span_to_snippet(pat.span) else { return }; + let Ok(arg_snip) = cx.sess().source_map().span_to_snippet(arg.span) else { return }; + + let help_string = format!( + "consider replacing `for {pat_snip} in {arg_snip}` with `if let {var}({pat_snip}) = {arg_snip}`" + ); + let msg = format!( + "for loop over `{arg_snip}`, which is {article} `{ty}`. This is more readably written as an `if let` statement", + ); + + cx.struct_span_lint(FOR_LOOP_OVER_FALLIBLES, arg.span, |diag| { + diag.build(msg).help(help_string).emit() + }) + } +} + +fn extract_for_loop<'tcx>(expr: &Expr<'tcx>) -> Option<(&'tcx Pat<'tcx>, &'tcx Expr<'tcx>)> { + if let hir::ExprKind::DropTemps(e) = expr.kind + && let hir::ExprKind::Match(iterexpr, [arm], hir::MatchSource::ForLoopDesugar) = e.kind + && let hir::ExprKind::Call(_, [arg]) = iterexpr.kind + && let hir::ExprKind::Loop(block, ..) = arm.body.kind + && let [stmt] = block.stmts + && let hir::StmtKind::Expr(e) = stmt.kind + && let hir::ExprKind::Match(_, [_, some_arm], _) = e.kind + && let hir::PatKind::Struct(_, [field], _) = some_arm.pat.kind + { + Some((field.pat, arg)) + } else { + None + } + +} \ No newline at end of file diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 070cccd141b56..ecc64511d7f72 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -52,6 +52,7 @@ mod early; mod enum_intrinsics_non_enums; mod errors; mod expect; +mod for_loop_over_fallibles; pub mod hidden_unicode_codepoints; mod internal; mod late; @@ -86,6 +87,7 @@ use rustc_span::Span; use array_into_iter::ArrayIntoIter; use builtin::*; use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums; +use for_loop_over_fallibles::*; use hidden_unicode_codepoints::*; use internal::*; use let_underscore::*; @@ -188,6 +190,7 @@ macro_rules! late_lint_mod_passes { $macro!( $args, [ + ForLoopOverFallibles: ForLoopOverFallibles, HardwiredLints: HardwiredLints, ImproperCTypesDeclarations: ImproperCTypesDeclarations, ImproperCTypesDefinitions: ImproperCTypesDefinitions, From d030ba52e20da70ddca073bd6795d0c27aff2ddb Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 18 Jul 2022 21:08:59 +0400 Subject: [PATCH 02/19] Use structured suggestions for `for_loop_over_fallibles` lint --- .../rustc_lint/src/for_loop_over_fallibles.rs | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index c96c9efe1d8a1..d765131442f6f 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -1,6 +1,7 @@ use crate::{LateContext, LateLintPass, LintContext}; use hir::{Expr, Pat}; +use rustc_errors::Applicability; use rustc_hir as hir; use rustc_middle::ty; use rustc_span::sym; @@ -65,18 +66,24 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { _ => return, }; - let Ok(pat_snip) = cx.sess().source_map().span_to_snippet(pat.span) else { return }; let Ok(arg_snip) = cx.sess().source_map().span_to_snippet(arg.span) else { return }; - let help_string = format!( - "consider replacing `for {pat_snip} in {arg_snip}` with `if let {var}({pat_snip}) = {arg_snip}`" - ); let msg = format!( "for loop over `{arg_snip}`, which is {article} `{ty}`. This is more readably written as an `if let` statement", ); cx.struct_span_lint(FOR_LOOP_OVER_FALLIBLES, arg.span, |diag| { - diag.build(msg).help(help_string).emit() + diag.build(msg) + .multipart_suggestion_verbose( + "consider using `if let` to clear intent", + vec![ + // NB can't use `until` here because `expr.span` and `pat.span` have different syntax contexts + (expr.span.with_hi(pat.span.lo()), format!("if let {var}(")), + (pat.span.between(arg.span), format!(") = ")), + ], + Applicability::MachineApplicable, + ) + .emit() }) } } @@ -89,11 +96,10 @@ fn extract_for_loop<'tcx>(expr: &Expr<'tcx>) -> Option<(&'tcx Pat<'tcx>, &'tcx E && let [stmt] = block.stmts && let hir::StmtKind::Expr(e) = stmt.kind && let hir::ExprKind::Match(_, [_, some_arm], _) = e.kind - && let hir::PatKind::Struct(_, [field], _) = some_arm.pat.kind + && let hir::PatKind::Struct(_, [field], _) = some_arm.pat.kind { Some((field.pat, arg)) } else { None } - -} \ No newline at end of file +} From 21ec99b6fa77088b35dc747c42dfdbec3d341545 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Sun, 24 Jul 2022 21:07:23 +0400 Subject: [PATCH 03/19] `for_loop_over_fallibles`: Suggest removing `.next()` --- .../rustc_lint/src/for_loop_over_fallibles.rs | 49 ++++++++++++++----- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index d765131442f6f..78b6ca6a653c0 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -73,17 +73,30 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { ); cx.struct_span_lint(FOR_LOOP_OVER_FALLIBLES, arg.span, |diag| { - diag.build(msg) - .multipart_suggestion_verbose( - "consider using `if let` to clear intent", - vec![ - // NB can't use `until` here because `expr.span` and `pat.span` have different syntax contexts - (expr.span.with_hi(pat.span.lo()), format!("if let {var}(")), - (pat.span.between(arg.span), format!(") = ")), - ], - Applicability::MachineApplicable, - ) - .emit() + let mut warn = diag.build(msg); + + if let Some(recv) = extract_iterator_next_call(cx, arg) + && let Ok(recv_snip) = cx.sess().source_map().span_to_snippet(recv.span) + { + warn.span_suggestion( + recv.span.between(arg.span.shrink_to_hi()), + format!("to iterate over `{recv_snip}` remove the call to `next`"), + "", + Applicability::MaybeIncorrect + ); + } + + warn.multipart_suggestion_verbose( + "consider using `if let` to clear intent", + vec![ + // NB can't use `until` here because `expr.span` and `pat.span` have different syntax contexts + (expr.span.with_hi(pat.span.lo()), format!("if let {var}(")), + (pat.span.between(arg.span), format!(") = ")), + ], + Applicability::MachineApplicable, + ); + + warn.emit() }) } } @@ -103,3 +116,17 @@ fn extract_for_loop<'tcx>(expr: &Expr<'tcx>) -> Option<(&'tcx Pat<'tcx>, &'tcx E None } } + +fn extract_iterator_next_call<'tcx>( + cx: &LateContext<'_>, + expr: &Expr<'tcx>, +) -> Option<&'tcx Expr<'tcx>> { + // This won't work for `Iterator::next(iter)`, is this an issue? + if let hir::ExprKind::MethodCall(_, [recv], _) = expr.kind + && cx.typeck_results().type_dependent_def_id(expr.hir_id) == cx.tcx.lang_items().next_fn() + { + Some(recv) + } else { + return None + } +} From 5dcfdbf31e06db2f579f665195cb9471c3d4a629 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Sun, 24 Jul 2022 21:16:44 +0400 Subject: [PATCH 04/19] `for_loop_over_fallibles`: suggest `while let` loop --- compiler/rustc_lint/src/for_loop_over_fallibles.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 78b6ca6a653c0..1a6188827819c 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -84,6 +84,16 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { "", Applicability::MaybeIncorrect ); + } else { + warn.multipart_suggestion_verbose( + format!("to check pattern in a loop use `while let`"), + vec![ + // NB can't use `until` here because `expr.span` and `pat.span` have different syntax contexts + (expr.span.with_hi(pat.span.lo()), format!("while let {var}(")), + (pat.span.between(arg.span), format!(") = ")), + ], + Applicability::MaybeIncorrect + ); } warn.multipart_suggestion_verbose( From b2975ee9742d90a2a005005add4405a8f7156466 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Sun, 24 Jul 2022 23:46:04 +0400 Subject: [PATCH 05/19] `for_loop_over_fallibles`: suggest using `?` in some cases --- .../rustc_lint/src/for_loop_over_fallibles.rs | 68 ++++++++++++++++++- 1 file changed, 65 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 1a6188827819c..7807fd3a2807a 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -3,8 +3,11 @@ use crate::{LateContext, LateLintPass, LintContext}; use hir::{Expr, Pat}; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_middle::ty; -use rustc_span::sym; +use rustc_infer::traits::TraitEngine; +use rustc_infer::{infer::TyCtxtInferExt, traits::ObligationCause}; +use rustc_middle::ty::{self, List}; +use rustc_span::{sym, Span}; +use rustc_trait_selection::traits::TraitEngineExt; declare_lint! { /// ### What it does @@ -58,7 +61,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { let ty = cx.typeck_results().expr_ty(arg); - let ty::Adt(adt, _) = ty.kind() else { return }; + let &ty::Adt(adt, substs) = ty.kind() else { return }; let (article, ty, var) = match adt.did() { did if cx.tcx.is_diagnostic_item(sym::Option, did) => ("an", "Option", "Some"), @@ -96,6 +99,15 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { ); } + if suggest_question_mark(cx, adt, substs, expr.span) { + warn.span_suggestion( + arg.span.shrink_to_hi(), + "consider unwrapping the `Result` with `?` to iterate over its contents", + "?", + Applicability::MaybeIncorrect, + ); + } + warn.multipart_suggestion_verbose( "consider using `if let` to clear intent", vec![ @@ -140,3 +152,53 @@ fn extract_iterator_next_call<'tcx>( return None } } + +fn suggest_question_mark<'tcx>( + cx: &LateContext<'tcx>, + adt: ty::AdtDef<'tcx>, + substs: &List>, + span: Span, +) -> bool { + let Some(body_id) = cx.enclosing_body else { return false }; + let Some(into_iterator_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator) else { return false }; + + if !cx.tcx.is_diagnostic_item(sym::Result, adt.did()) { + return false; + } + + // Check that the function/closure/constant we are in has a `Result` type. + // Otherwise suggesting using `?` may not be a good idea. + { + let ty = cx.typeck_results().expr_ty(&cx.tcx.hir().body(body_id).value); + let ty::Adt(ret_adt, ..) = ty.kind() else { return false }; + if !cx.tcx.is_diagnostic_item(sym::Result, ret_adt.did()) { + return false; + } + } + + let ty = substs.type_at(0); + let is_iterator = cx.tcx.infer_ctxt().enter(|infcx| { + let mut fulfill_cx = >::new(infcx.tcx); + + let cause = ObligationCause::new( + span, + body_id.hir_id, + rustc_infer::traits::ObligationCauseCode::MiscObligation, + ); + fulfill_cx.register_bound( + &infcx, + ty::ParamEnv::empty(), + // Erase any region vids from the type, which may not be resolved + infcx.tcx.erase_regions(ty), + into_iterator_did, + cause, + ); + + // Select all, including ambiguous predicates + let errors = fulfill_cx.select_all_or_error(&infcx); + + errors.is_empty() + }); + + is_iterator +} From dd842ffc3df5485fe51d2ec8295e96861c524602 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 25 Jul 2022 00:17:18 +0400 Subject: [PATCH 06/19] `for_loop_over_fallibles`: remove duplication from the message --- compiler/rustc_lint/src/for_loop_over_fallibles.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 7807fd3a2807a..69d8fd84b64d5 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -69,10 +69,8 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { _ => return, }; - let Ok(arg_snip) = cx.sess().source_map().span_to_snippet(arg.span) else { return }; - let msg = format!( - "for loop over `{arg_snip}`, which is {article} `{ty}`. This is more readably written as an `if let` statement", + "for loop over {article} `{ty}`. This is more readably written as an `if let` statement", ); cx.struct_span_lint(FOR_LOOP_OVER_FALLIBLES, arg.span, |diag| { From 730856442387c9804581eae5927a5667171022ca Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 25 Jul 2022 00:37:11 +0400 Subject: [PATCH 07/19] Add a test for the `for_loop_over_fallibles` lint --- src/test/ui/lint/for_loop_over_fallibles.rs | 43 ++++++++ .../ui/lint/for_loop_over_fallibles.stderr | 102 ++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 src/test/ui/lint/for_loop_over_fallibles.rs create mode 100644 src/test/ui/lint/for_loop_over_fallibles.stderr diff --git a/src/test/ui/lint/for_loop_over_fallibles.rs b/src/test/ui/lint/for_loop_over_fallibles.rs new file mode 100644 index 0000000000000..43d71c2e808a9 --- /dev/null +++ b/src/test/ui/lint/for_loop_over_fallibles.rs @@ -0,0 +1,43 @@ +// check-pass + +fn main() { + // Common + for _ in Some(1) {} + //~^ WARN for loop over an `Option`. This is more readably written as an `if let` statement + //~| HELP to check pattern in a loop use `while let` + //~| HELP consider using `if let` to clear intent + for _ in Ok::<_, ()>(1) {} + //~^ WARN for loop over a `Result`. This is more readably written as an `if let` statement + //~| HELP to check pattern in a loop use `while let` + //~| HELP consider using `if let` to clear intent + + // `Iterator::next` specific + for _ in [0; 0].iter().next() {} + //~^ WARN for loop over an `Option`. This is more readably written as an `if let` statement + //~| HELP to iterate over `[0; 0].iter()` remove the call to `next` + //~| HELP consider using `if let` to clear intent + + // `Result`, but function doesn't return `Result` + for _ in Ok::<_, ()>([0; 0].iter()) {} + //~^ WARN for loop over a `Result`. This is more readably written as an `if let` statement + //~| HELP to check pattern in a loop use `while let` + //~| HELP consider using `if let` to clear intent +} + +fn _returns_result() -> Result<(), ()> { + // `Result` + for _ in Ok::<_, ()>([0; 0].iter()) {} + //~^ WARN for loop over a `Result`. This is more readably written as an `if let` statement + //~| HELP to check pattern in a loop use `while let` + //~| HELP consider unwrapping the `Result` with `?` to iterate over its contents + //~| HELP consider using `if let` to clear intent + + // `Result` + for _ in Ok::<_, ()>([0; 0]) {} + //~^ WARN for loop over a `Result`. This is more readably written as an `if let` statement + //~| HELP to check pattern in a loop use `while let` + //~| HELP consider unwrapping the `Result` with `?` to iterate over its contents + //~| HELP consider using `if let` to clear intent + + Ok(()) +} diff --git a/src/test/ui/lint/for_loop_over_fallibles.stderr b/src/test/ui/lint/for_loop_over_fallibles.stderr new file mode 100644 index 0000000000000..52eac945d8571 --- /dev/null +++ b/src/test/ui/lint/for_loop_over_fallibles.stderr @@ -0,0 +1,102 @@ +warning: for loop over an `Option`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:5:14 + | +LL | for _ in Some(1) {} + | ^^^^^^^ + | + = note: `#[warn(for_loop_over_fallibles)]` on by default +help: to check pattern in a loop use `while let` + | +LL | while let Some(_) = Some(1) {} + | ~~~~~~~~~~~~~~~ ~~~ +help: consider using `if let` to clear intent + | +LL | if let Some(_) = Some(1) {} + | ~~~~~~~~~~~~ ~~~ + +warning: for loop over a `Result`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:9:14 + | +LL | for _ in Ok::<_, ()>(1) {} + | ^^^^^^^^^^^^^^ + | +help: to check pattern in a loop use `while let` + | +LL | while let Ok(_) = Ok::<_, ()>(1) {} + | ~~~~~~~~~~~~~ ~~~ +help: consider using `if let` to clear intent + | +LL | if let Ok(_) = Ok::<_, ()>(1) {} + | ~~~~~~~~~~ ~~~ + +warning: for loop over an `Option`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:15:14 + | +LL | for _ in [0; 0].iter().next() {} + | ^^^^^^^^^^^^^^^^^^^^ + | +help: to iterate over `[0; 0].iter()` remove the call to `next` + | +LL - for _ in [0; 0].iter().next() {} +LL + for _ in [0; 0].iter() {} + | +help: consider using `if let` to clear intent + | +LL | if let Some(_) = [0; 0].iter().next() {} + | ~~~~~~~~~~~~ ~~~ + +warning: for loop over a `Result`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:21:14 + | +LL | for _ in Ok::<_, ()>([0; 0].iter()) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: to check pattern in a loop use `while let` + | +LL | while let Ok(_) = Ok::<_, ()>([0; 0].iter()) {} + | ~~~~~~~~~~~~~ ~~~ +help: consider using `if let` to clear intent + | +LL | if let Ok(_) = Ok::<_, ()>([0; 0].iter()) {} + | ~~~~~~~~~~ ~~~ + +warning: for loop over a `Result`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:29:14 + | +LL | for _ in Ok::<_, ()>([0; 0].iter()) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: to check pattern in a loop use `while let` + | +LL | while let Ok(_) = Ok::<_, ()>([0; 0].iter()) {} + | ~~~~~~~~~~~~~ ~~~ +help: consider unwrapping the `Result` with `?` to iterate over its contents + | +LL | for _ in Ok::<_, ()>([0; 0].iter())? {} + | + +help: consider using `if let` to clear intent + | +LL | if let Ok(_) = Ok::<_, ()>([0; 0].iter()) {} + | ~~~~~~~~~~ ~~~ + +warning: for loop over a `Result`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:36:14 + | +LL | for _ in Ok::<_, ()>([0; 0]) {} + | ^^^^^^^^^^^^^^^^^^^ + | +help: to check pattern in a loop use `while let` + | +LL | while let Ok(_) = Ok::<_, ()>([0; 0]) {} + | ~~~~~~~~~~~~~ ~~~ +help: consider unwrapping the `Result` with `?` to iterate over its contents + | +LL | for _ in Ok::<_, ()>([0; 0])? {} + | + +help: consider using `if let` to clear intent + | +LL | if let Ok(_) = Ok::<_, ()>([0; 0]) {} + | ~~~~~~~~~~ ~~~ + +warning: 6 warnings emitted + From 23a7674e3eb7e860ae59e81acc0699926dd0797c Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 25 Jul 2022 00:58:04 +0400 Subject: [PATCH 08/19] `for_loop_over_fallibles`: fix suggestion for "remove `.next()`" case if the iterator is used after the loop, we need to use `.by_ref()` --- compiler/rustc_lint/src/for_loop_over_fallibles.rs | 2 +- src/test/ui/lint/for_loop_over_fallibles.stderr | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 69d8fd84b64d5..c8d5586d39f9c 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -82,7 +82,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { warn.span_suggestion( recv.span.between(arg.span.shrink_to_hi()), format!("to iterate over `{recv_snip}` remove the call to `next`"), - "", + ".by_ref()", Applicability::MaybeIncorrect ); } else { diff --git a/src/test/ui/lint/for_loop_over_fallibles.stderr b/src/test/ui/lint/for_loop_over_fallibles.stderr index 52eac945d8571..56e3126dc09a4 100644 --- a/src/test/ui/lint/for_loop_over_fallibles.stderr +++ b/src/test/ui/lint/for_loop_over_fallibles.stderr @@ -37,9 +37,8 @@ LL | for _ in [0; 0].iter().next() {} | help: to iterate over `[0; 0].iter()` remove the call to `next` | -LL - for _ in [0; 0].iter().next() {} -LL + for _ in [0; 0].iter() {} - | +LL | for _ in [0; 0].iter().by_ref() {} + | ~~~~~~~~~ help: consider using `if let` to clear intent | LL | if let Some(_) = [0; 0].iter().next() {} From 8ca57b54c19b22b6037b44619e5773a6a378d808 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 25 Jul 2022 01:04:27 +0400 Subject: [PATCH 09/19] `for_loop_over_fallibles`: don't use `MachineApplicable` The loop could contain `break;` that won't work with an `if let` --- compiler/rustc_lint/src/for_loop_over_fallibles.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index c8d5586d39f9c..6870942af941f 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -113,7 +113,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { (expr.span.with_hi(pat.span.lo()), format!("if let {var}(")), (pat.span.between(arg.span), format!(") = ")), ], - Applicability::MachineApplicable, + Applicability::MaybeIncorrect, ); warn.emit() From 0250f0244b320b1cecabc05c108178171c733f6c Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Tue, 26 Jul 2022 14:17:15 +0400 Subject: [PATCH 10/19] allow or avoid for loops over option in compiler and tests --- compiler/rustc_ast/src/visit.rs | 14 ++++++-------- src/test/ui/drop/dropck_legal_cycles.rs | 14 +++++++------- src/test/ui/issues/issue-30371.rs | 1 + 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_ast/src/visit.rs b/compiler/rustc_ast/src/visit.rs index a71e055a4b3e5..c6c80e613e30d 100644 --- a/compiler/rustc_ast/src/visit.rs +++ b/compiler/rustc_ast/src/visit.rs @@ -244,14 +244,12 @@ pub trait Visitor<'ast>: Sized { #[macro_export] macro_rules! walk_list { - ($visitor: expr, $method: ident, $list: expr) => { - for elem in $list { - $visitor.$method(elem) - } - }; - ($visitor: expr, $method: ident, $list: expr, $($extra_args: expr),*) => { - for elem in $list { - $visitor.$method(elem, $($extra_args,)*) + ($visitor: expr, $method: ident, $list: expr $(, $($extra_args: expr),* )?) => { + { + #[cfg_attr(not(bootstrap), allow(for_loop_over_fallibles))] + for elem in $list { + $visitor.$method(elem $(, $($extra_args,)* )?) + } } } } diff --git a/src/test/ui/drop/dropck_legal_cycles.rs b/src/test/ui/drop/dropck_legal_cycles.rs index 27a599315dc1c..6a0fe7784fbcc 100644 --- a/src/test/ui/drop/dropck_legal_cycles.rs +++ b/src/test/ui/drop/dropck_legal_cycles.rs @@ -1017,7 +1017,7 @@ impl<'a> Children<'a> for HM<'a> { where C: Context + PrePost, Self: Sized { if let Some(ref hm) = self.contents.get() { - for (k, v) in hm.iter().nth(index / 2) { + if let Some((k, v)) = hm.iter().nth(index / 2) { [k, v][index % 2].descend_into_self(context); } } @@ -1032,7 +1032,7 @@ impl<'a> Children<'a> for VD<'a> { where C: Context + PrePost, Self: Sized { if let Some(ref vd) = self.contents.get() { - for r in vd.iter().nth(index) { + if let Some(r) = vd.iter().nth(index) { r.descend_into_self(context); } } @@ -1047,7 +1047,7 @@ impl<'a> Children<'a> for VM<'a> { where C: Context + PrePost> { if let Some(ref vd) = self.contents.get() { - for (_idx, r) in vd.iter().nth(index) { + if let Some((_idx, r)) = vd.iter().nth(index) { r.descend_into_self(context); } } @@ -1062,7 +1062,7 @@ impl<'a> Children<'a> for LL<'a> { where C: Context + PrePost> { if let Some(ref ll) = self.contents.get() { - for r in ll.iter().nth(index) { + if let Some(r) = ll.iter().nth(index) { r.descend_into_self(context); } } @@ -1077,7 +1077,7 @@ impl<'a> Children<'a> for BH<'a> { where C: Context + PrePost> { if let Some(ref bh) = self.contents.get() { - for r in bh.iter().nth(index) { + if let Some(r) = bh.iter().nth(index) { r.descend_into_self(context); } } @@ -1092,7 +1092,7 @@ impl<'a> Children<'a> for BTM<'a> { where C: Context + PrePost> { if let Some(ref bh) = self.contents.get() { - for (k, v) in bh.iter().nth(index / 2) { + if let Some((k, v)) = bh.iter().nth(index / 2) { [k, v][index % 2].descend_into_self(context); } } @@ -1107,7 +1107,7 @@ impl<'a> Children<'a> for BTS<'a> { where C: Context + PrePost> { if let Some(ref bh) = self.contents.get() { - for r in bh.iter().nth(index) { + if let Some(r) = bh.iter().nth(index) { r.descend_into_self(context); } } diff --git a/src/test/ui/issues/issue-30371.rs b/src/test/ui/issues/issue-30371.rs index a1ae9a36bc1d7..880558eb5b697 100644 --- a/src/test/ui/issues/issue-30371.rs +++ b/src/test/ui/issues/issue-30371.rs @@ -1,5 +1,6 @@ // run-pass #![allow(unreachable_code)] +#![allow(for_loop_over_fallibles)] #![deny(unused_variables)] fn main() { From 75ae20a42f6c429342d53b41a2592de07bee6f91 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Tue, 26 Jul 2022 16:19:58 +0400 Subject: [PATCH 11/19] allow `for_loop_over_fallibles` in a `core` test --- library/core/tests/option.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/core/tests/option.rs b/library/core/tests/option.rs index 9f5e537dcefc0..84eb4fc0aa329 100644 --- a/library/core/tests/option.rs +++ b/library/core/tests/option.rs @@ -57,6 +57,7 @@ fn test_get_resource() { } #[test] +#[cfg_attr(not(bootstrap), allow(for_loop_over_fallibles))] fn test_option_dance() { let x = Some(()); let mut y = Some(5); From b9b2059e84cb5a1d4ac35f0c79624b02a10250b9 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Sun, 14 Aug 2022 21:42:29 +0400 Subject: [PATCH 12/19] Edit documentation for `for_loop_over_fallibles` lint --- .../rustc_lint/src/for_loop_over_fallibles.rs | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 6870942af941f..48a876b157bda 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -10,43 +10,41 @@ use rustc_span::{sym, Span}; use rustc_trait_selection::traits::TraitEngineExt; declare_lint! { - /// ### What it does - /// /// Checks for `for` loops over `Option` or `Result` values. /// - /// ### Why is this bad? - /// Readability. This is more clearly expressed as an `if - /// let`. + /// ### Explanation + /// + /// Both `Option` and `Result` implement `IntoIterator` trait, which allows using them in a `for` loop. + /// `for` loop over `Option` or `Result` will iterate either 0 (if the value is `None`/`Err(_)`) + /// or 1 time (if the value is `Some(_)`/`Ok(_)`). This is not very useful and is more clearly expressed + /// via `if let`. + /// + /// `for` loop can also be accidentally written with the intention to call a function multiple times, + /// while the function returns `Some(_)`, in these cases `while let` loop should be used instead. + /// + /// The "intended" use of `IntoIterator` implementations for `Option` and `Result` is passing them to + /// generic code that expects something implementing `IntoIterator`. For example using `.chain(option)` + /// to optionally add a value to an iterator. /// /// ### Example /// /// ```rust /// # let opt = Some(1); /// # let res: Result = Ok(1); - /// for x in opt { - /// // .. - /// } - /// - /// for x in &res { - /// // .. - /// } - /// - /// for x in res.iter() { - /// // .. - /// } + /// # let recv = || Some(1); + /// for x in opt { /* ... */} + /// for x in res { /* ... */ } + /// for x in recv() { /* ... */ } /// ``` /// /// Use instead: /// ```rust /// # let opt = Some(1); /// # let res: Result = Ok(1); - /// if let Some(x) = opt { - /// // .. - /// } - /// - /// if let Ok(x) = res { - /// // .. - /// } + /// # let recv = || Some(1); + /// if let Some(x) = opt { /* ... */} + /// if let Ok(x) = res { /* ... */ } + /// while let Some(x) = recv() { /* ... */ } /// ``` pub FOR_LOOP_OVER_FALLIBLES, Warn, From 6766113c873688c64d480069a35c1c64d07c71bd Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 15 Aug 2022 05:22:00 +0400 Subject: [PATCH 13/19] remove an infinite loop --- compiler/rustc_lint/src/for_loop_over_fallibles.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 48a876b157bda..0a6b6e4163622 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -31,7 +31,7 @@ declare_lint! { /// ```rust /// # let opt = Some(1); /// # let res: Result = Ok(1); - /// # let recv = || Some(1); + /// # let recv = || None::; /// for x in opt { /* ... */} /// for x in res { /* ... */ } /// for x in recv() { /* ... */ } @@ -41,7 +41,7 @@ declare_lint! { /// ```rust /// # let opt = Some(1); /// # let res: Result = Ok(1); - /// # let recv = || Some(1); + /// # let recv = || None::; /// if let Some(x) = opt { /* ... */} /// if let Ok(x) = res { /* ... */ } /// while let Some(x) = recv() { /* ... */ } From 98e0c4df7384ca02f1d5c4dc5dc15de8b3418ce1 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Thu, 18 Aug 2022 11:43:10 +0400 Subject: [PATCH 14/19] fix `for_loop_over_fallibles` lint docs --- .../rustc_lint/src/for_loop_over_fallibles.rs | 32 ++++++------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 0a6b6e4163622..2253546b5d357 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loop_over_fallibles.rs @@ -10,7 +10,16 @@ use rustc_span::{sym, Span}; use rustc_trait_selection::traits::TraitEngineExt; declare_lint! { - /// Checks for `for` loops over `Option` or `Result` values. + /// The `for_loop_over_fallibles` lint checks for `for` loops over `Option` or `Result` values. + /// + /// ### Example + /// + /// ```rust + /// let opt = Some(1); + /// for x in opt { /* ... */} + /// ``` + /// + /// {{produces}} /// /// ### Explanation /// @@ -25,27 +34,6 @@ declare_lint! { /// The "intended" use of `IntoIterator` implementations for `Option` and `Result` is passing them to /// generic code that expects something implementing `IntoIterator`. For example using `.chain(option)` /// to optionally add a value to an iterator. - /// - /// ### Example - /// - /// ```rust - /// # let opt = Some(1); - /// # let res: Result = Ok(1); - /// # let recv = || None::; - /// for x in opt { /* ... */} - /// for x in res { /* ... */ } - /// for x in recv() { /* ... */ } - /// ``` - /// - /// Use instead: - /// ```rust - /// # let opt = Some(1); - /// # let res: Result = Ok(1); - /// # let recv = || None::; - /// if let Some(x) = opt { /* ... */} - /// if let Ok(x) = res { /* ... */ } - /// while let Some(x) = recv() { /* ... */ } - /// ``` pub FOR_LOOP_OVER_FALLIBLES, Warn, "for-looping over an `Option` or a `Result`, which is more clearly expressed as an `if let`" From 9c64bb1de14c58c76bbbcd0a5c1791e8d23b6443 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Thu, 25 Aug 2022 14:03:13 +0400 Subject: [PATCH 15/19] Fix clippy tests that trigger `for_loop_over_fallibles` lint --- src/tools/clippy/tests/ui/for_loop_unfixable.rs | 1 + src/tools/clippy/tests/ui/for_loop_unfixable.stderr | 2 +- src/tools/clippy/tests/ui/for_loops_over_fallibles.rs | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tools/clippy/tests/ui/for_loop_unfixable.rs b/src/tools/clippy/tests/ui/for_loop_unfixable.rs index efcaffce24ea4..203656fa4d6c2 100644 --- a/src/tools/clippy/tests/ui/for_loop_unfixable.rs +++ b/src/tools/clippy/tests/ui/for_loop_unfixable.rs @@ -8,6 +8,7 @@ clippy::for_kv_map )] #[allow(clippy::linkedlist, clippy::unnecessary_mut_passed, clippy::similar_names)] +#[allow(for_loop_over_fallibles)] fn main() { let vec = vec![1, 2, 3, 4]; diff --git a/src/tools/clippy/tests/ui/for_loop_unfixable.stderr b/src/tools/clippy/tests/ui/for_loop_unfixable.stderr index f769b4bdc9411..50a86eaa68f7d 100644 --- a/src/tools/clippy/tests/ui/for_loop_unfixable.stderr +++ b/src/tools/clippy/tests/ui/for_loop_unfixable.stderr @@ -1,5 +1,5 @@ error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want - --> $DIR/for_loop_unfixable.rs:14:15 + --> $DIR/for_loop_unfixable.rs:15:15 | LL | for _v in vec.iter().next() {} | ^^^^^^^^^^^^^^^^^ diff --git a/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs b/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs index 4b2a9297d084e..54661ff94f243 100644 --- a/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs +++ b/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs @@ -1,5 +1,6 @@ #![warn(clippy::for_loops_over_fallibles)] #![allow(clippy::uninlined_format_args)] +#![allow(for_loop_over_fallibles)] fn for_loops_over_fallibles() { let option = Some(1); From 7434b9f0d1e1587dc97829ffbc65a5afdf04fb7e Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Fri, 7 Oct 2022 15:59:39 +0000 Subject: [PATCH 16/19] fixup lint name --- compiler/rustc_ast/src/visit.rs | 2 +- ...p_over_fallibles.rs => for_loops_over_fallibles.rs} | 10 +++++----- compiler/rustc_lint/src/lib.rs | 6 +++--- library/core/tests/option.rs | 2 +- src/test/ui/issues/issue-30371.rs | 2 +- src/test/ui/lint/for_loop_over_fallibles.stderr | 2 +- src/tools/clippy/tests/ui/for_loop_unfixable.rs | 2 +- src/tools/clippy/tests/ui/for_loops_over_fallibles.rs | 2 +- 8 files changed, 14 insertions(+), 14 deletions(-) rename compiler/rustc_lint/src/{for_loop_over_fallibles.rs => for_loops_over_fallibles.rs} (95%) diff --git a/compiler/rustc_ast/src/visit.rs b/compiler/rustc_ast/src/visit.rs index c6c80e613e30d..145b31842ef69 100644 --- a/compiler/rustc_ast/src/visit.rs +++ b/compiler/rustc_ast/src/visit.rs @@ -246,7 +246,7 @@ pub trait Visitor<'ast>: Sized { macro_rules! walk_list { ($visitor: expr, $method: ident, $list: expr $(, $($extra_args: expr),* )?) => { { - #[cfg_attr(not(bootstrap), allow(for_loop_over_fallibles))] + #[cfg_attr(not(bootstrap), allow(for_loops_over_fallibles))] for elem in $list { $visitor.$method(elem $(, $($extra_args,)* )?) } diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loops_over_fallibles.rs similarity index 95% rename from compiler/rustc_lint/src/for_loop_over_fallibles.rs rename to compiler/rustc_lint/src/for_loops_over_fallibles.rs index 2253546b5d357..54f1f307ddd0e 100644 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loops_over_fallibles.rs @@ -10,7 +10,7 @@ use rustc_span::{sym, Span}; use rustc_trait_selection::traits::TraitEngineExt; declare_lint! { - /// The `for_loop_over_fallibles` lint checks for `for` loops over `Option` or `Result` values. + /// The `for_loops_over_fallibles` lint checks for `for` loops over `Option` or `Result` values. /// /// ### Example /// @@ -34,14 +34,14 @@ declare_lint! { /// The "intended" use of `IntoIterator` implementations for `Option` and `Result` is passing them to /// generic code that expects something implementing `IntoIterator`. For example using `.chain(option)` /// to optionally add a value to an iterator. - pub FOR_LOOP_OVER_FALLIBLES, + pub FOR_LOOPS_OVER_FALLIBLES, Warn, "for-looping over an `Option` or a `Result`, which is more clearly expressed as an `if let`" } -declare_lint_pass!(ForLoopOverFallibles => [FOR_LOOP_OVER_FALLIBLES]); +declare_lint_pass!(ForLoopsOverFallibles => [FOR_LOOPS_OVER_FALLIBLES]); -impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { +impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { let Some((pat, arg)) = extract_for_loop(expr) else { return }; @@ -59,7 +59,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopOverFallibles { "for loop over {article} `{ty}`. This is more readably written as an `if let` statement", ); - cx.struct_span_lint(FOR_LOOP_OVER_FALLIBLES, arg.span, |diag| { + cx.struct_span_lint(FOR_LOOPS_OVER_FALLIBLES, arg.span, |diag| { let mut warn = diag.build(msg); if let Some(recv) = extract_iterator_next_call(cx, arg) diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index ecc64511d7f72..fee6e080c4fc7 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -52,7 +52,7 @@ mod early; mod enum_intrinsics_non_enums; mod errors; mod expect; -mod for_loop_over_fallibles; +mod for_loops_over_fallibles; pub mod hidden_unicode_codepoints; mod internal; mod late; @@ -87,7 +87,7 @@ use rustc_span::Span; use array_into_iter::ArrayIntoIter; use builtin::*; use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums; -use for_loop_over_fallibles::*; +use for_loops_over_fallibles::*; use hidden_unicode_codepoints::*; use internal::*; use let_underscore::*; @@ -190,7 +190,7 @@ macro_rules! late_lint_mod_passes { $macro!( $args, [ - ForLoopOverFallibles: ForLoopOverFallibles, + ForLoopsOverFallibles: ForLoopsOverFallibles, HardwiredLints: HardwiredLints, ImproperCTypesDeclarations: ImproperCTypesDeclarations, ImproperCTypesDefinitions: ImproperCTypesDefinitions, diff --git a/library/core/tests/option.rs b/library/core/tests/option.rs index 84eb4fc0aa329..f36f7c268064f 100644 --- a/library/core/tests/option.rs +++ b/library/core/tests/option.rs @@ -57,7 +57,7 @@ fn test_get_resource() { } #[test] -#[cfg_attr(not(bootstrap), allow(for_loop_over_fallibles))] +#[cfg_attr(not(bootstrap), allow(for_loops_over_fallibles))] fn test_option_dance() { let x = Some(()); let mut y = Some(5); diff --git a/src/test/ui/issues/issue-30371.rs b/src/test/ui/issues/issue-30371.rs index 880558eb5b697..eea548c482ff2 100644 --- a/src/test/ui/issues/issue-30371.rs +++ b/src/test/ui/issues/issue-30371.rs @@ -1,6 +1,6 @@ // run-pass #![allow(unreachable_code)] -#![allow(for_loop_over_fallibles)] +#![allow(for_loops_over_fallibles)] #![deny(unused_variables)] fn main() { diff --git a/src/test/ui/lint/for_loop_over_fallibles.stderr b/src/test/ui/lint/for_loop_over_fallibles.stderr index 56e3126dc09a4..96efdf85c4901 100644 --- a/src/test/ui/lint/for_loop_over_fallibles.stderr +++ b/src/test/ui/lint/for_loop_over_fallibles.stderr @@ -4,7 +4,7 @@ warning: for loop over an `Option`. This is more readably written as an `if let` LL | for _ in Some(1) {} | ^^^^^^^ | - = note: `#[warn(for_loop_over_fallibles)]` on by default + = note: `#[warn(for_loops_over_fallibles)]` on by default help: to check pattern in a loop use `while let` | LL | while let Some(_) = Some(1) {} diff --git a/src/tools/clippy/tests/ui/for_loop_unfixable.rs b/src/tools/clippy/tests/ui/for_loop_unfixable.rs index 203656fa4d6c2..55fb3788a8b1a 100644 --- a/src/tools/clippy/tests/ui/for_loop_unfixable.rs +++ b/src/tools/clippy/tests/ui/for_loop_unfixable.rs @@ -8,7 +8,7 @@ clippy::for_kv_map )] #[allow(clippy::linkedlist, clippy::unnecessary_mut_passed, clippy::similar_names)] -#[allow(for_loop_over_fallibles)] +#[allow(for_loops_over_fallibles)] fn main() { let vec = vec![1, 2, 3, 4]; diff --git a/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs b/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs index 54661ff94f243..75cdcc02353f7 100644 --- a/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs +++ b/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs @@ -1,6 +1,6 @@ #![warn(clippy::for_loops_over_fallibles)] #![allow(clippy::uninlined_format_args)] -#![allow(for_loop_over_fallibles)] +#![allow(for_loops_over_fallibles)] fn for_loops_over_fallibles() { let option = Some(1); From 40f36fac49e29c9bbf5ed5e3aee03781edb045cb Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Fri, 7 Oct 2022 16:52:41 +0000 Subject: [PATCH 17/19] adopt to new rustc lint api --- .../src/for_loops_over_fallibles.rs | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_lint/src/for_loops_over_fallibles.rs b/compiler/rustc_lint/src/for_loops_over_fallibles.rs index 54f1f307ddd0e..4f0ae08116611 100644 --- a/compiler/rustc_lint/src/for_loops_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loops_over_fallibles.rs @@ -1,7 +1,7 @@ use crate::{LateContext, LateLintPass, LintContext}; use hir::{Expr, Pat}; -use rustc_errors::Applicability; +use rustc_errors::{Applicability, DelayDm}; use rustc_hir as hir; use rustc_infer::traits::TraitEngine; use rustc_infer::{infer::TyCtxtInferExt, traits::ObligationCause}; @@ -55,24 +55,24 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { _ => return, }; - let msg = format!( - "for loop over {article} `{ty}`. This is more readably written as an `if let` statement", - ); - - cx.struct_span_lint(FOR_LOOPS_OVER_FALLIBLES, arg.span, |diag| { - let mut warn = diag.build(msg); + let msg = DelayDm(|| { + format!( + "for loop over {article} `{ty}`. This is more readably written as an `if let` statement", + ) + }); + cx.struct_span_lint(FOR_LOOPS_OVER_FALLIBLES, arg.span, msg, |lint| { if let Some(recv) = extract_iterator_next_call(cx, arg) && let Ok(recv_snip) = cx.sess().source_map().span_to_snippet(recv.span) { - warn.span_suggestion( + lint.span_suggestion( recv.span.between(arg.span.shrink_to_hi()), format!("to iterate over `{recv_snip}` remove the call to `next`"), ".by_ref()", Applicability::MaybeIncorrect ); } else { - warn.multipart_suggestion_verbose( + lint.multipart_suggestion_verbose( format!("to check pattern in a loop use `while let`"), vec![ // NB can't use `until` here because `expr.span` and `pat.span` have different syntax contexts @@ -84,7 +84,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { } if suggest_question_mark(cx, adt, substs, expr.span) { - warn.span_suggestion( + lint.span_suggestion( arg.span.shrink_to_hi(), "consider unwrapping the `Result` with `?` to iterate over its contents", "?", @@ -92,7 +92,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { ); } - warn.multipart_suggestion_verbose( + lint.multipart_suggestion_verbose( "consider using `if let` to clear intent", vec![ // NB can't use `until` here because `expr.span` and `pat.span` have different syntax contexts @@ -100,9 +100,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { (pat.span.between(arg.span), format!(") = ")), ], Applicability::MaybeIncorrect, - ); - - warn.emit() + ) }) } } @@ -128,7 +126,7 @@ fn extract_iterator_next_call<'tcx>( expr: &Expr<'tcx>, ) -> Option<&'tcx Expr<'tcx>> { // This won't work for `Iterator::next(iter)`, is this an issue? - if let hir::ExprKind::MethodCall(_, [recv], _) = expr.kind + if let hir::ExprKind::MethodCall(_, recv, _, _) = expr.kind && cx.typeck_results().type_dependent_def_id(expr.hir_id) == cx.tcx.lang_items().next_fn() { Some(recv) From 5347c819241135218418ce7f9c157dbd099fd41a Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Fri, 7 Oct 2022 17:08:29 +0000 Subject: [PATCH 18/19] deprecate `clippy::for_loops_over_fallibles` --- .../clippy_lints/src/lib.register_all.rs | 1 - .../clippy_lints/src/lib.register_lints.rs | 1 - .../src/lib.register_suspicious.rs | 1 - .../src/loops/for_loops_over_fallibles.rs | 65 ------------- .../clippy_lints/src/loops/iter_next_loop.rs | 5 +- .../clippy/clippy_lints/src/loops/mod.rs | 57 +---------- .../clippy/clippy_lints/src/renamed_lints.rs | 5 +- src/tools/clippy/src/docs.rs | 1 - .../src/docs/for_loops_over_fallibles.txt | 32 ------- .../tests/ui/for_loops_over_fallibles.rs | 74 --------------- .../tests/ui/for_loops_over_fallibles.stderr | 95 ------------------- .../clippy/tests/ui/manual_map_option.fixed | 2 +- .../clippy/tests/ui/manual_map_option.rs | 2 +- src/tools/clippy/tests/ui/rename.fixed | 7 +- src/tools/clippy/tests/ui/rename.rs | 3 +- src/tools/clippy/tests/ui/rename.stderr | 34 ++++--- 16 files changed, 34 insertions(+), 351 deletions(-) delete mode 100644 src/tools/clippy/clippy_lints/src/loops/for_loops_over_fallibles.rs delete mode 100644 src/tools/clippy/src/docs/for_loops_over_fallibles.txt delete mode 100644 src/tools/clippy/tests/ui/for_loops_over_fallibles.rs delete mode 100644 src/tools/clippy/tests/ui/for_loops_over_fallibles.stderr diff --git a/src/tools/clippy/clippy_lints/src/lib.register_all.rs b/src/tools/clippy/clippy_lints/src/lib.register_all.rs index 5d26e4b336012..fe1f0b56646cd 100644 --- a/src/tools/clippy/clippy_lints/src/lib.register_all.rs +++ b/src/tools/clippy/clippy_lints/src/lib.register_all.rs @@ -109,7 +109,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(loops::EMPTY_LOOP), LintId::of(loops::EXPLICIT_COUNTER_LOOP), LintId::of(loops::FOR_KV_MAP), - LintId::of(loops::FOR_LOOPS_OVER_FALLIBLES), LintId::of(loops::ITER_NEXT_LOOP), LintId::of(loops::MANUAL_FIND), LintId::of(loops::MANUAL_FLATTEN), diff --git a/src/tools/clippy/clippy_lints/src/lib.register_lints.rs b/src/tools/clippy/clippy_lints/src/lib.register_lints.rs index 05d927dbea794..306cb6a61c943 100644 --- a/src/tools/clippy/clippy_lints/src/lib.register_lints.rs +++ b/src/tools/clippy/clippy_lints/src/lib.register_lints.rs @@ -227,7 +227,6 @@ store.register_lints(&[ loops::EXPLICIT_INTO_ITER_LOOP, loops::EXPLICIT_ITER_LOOP, loops::FOR_KV_MAP, - loops::FOR_LOOPS_OVER_FALLIBLES, loops::ITER_NEXT_LOOP, loops::MANUAL_FIND, loops::MANUAL_FLATTEN, diff --git a/src/tools/clippy/clippy_lints/src/lib.register_suspicious.rs b/src/tools/clippy/clippy_lints/src/lib.register_suspicious.rs index 6125d0f7a8628..d6d95c95c85d2 100644 --- a/src/tools/clippy/clippy_lints/src/lib.register_suspicious.rs +++ b/src/tools/clippy/clippy_lints/src/lib.register_suspicious.rs @@ -21,7 +21,6 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(formatting::SUSPICIOUS_ELSE_FORMATTING), LintId::of(formatting::SUSPICIOUS_UNARY_OP_FORMATTING), LintId::of(loops::EMPTY_LOOP), - LintId::of(loops::FOR_LOOPS_OVER_FALLIBLES), LintId::of(loops::MUT_RANGE_BOUND), LintId::of(methods::NO_EFFECT_REPLACE), LintId::of(methods::SUSPICIOUS_MAP), diff --git a/src/tools/clippy/clippy_lints/src/loops/for_loops_over_fallibles.rs b/src/tools/clippy/clippy_lints/src/loops/for_loops_over_fallibles.rs deleted file mode 100644 index 77de90fd7b94a..0000000000000 --- a/src/tools/clippy/clippy_lints/src/loops/for_loops_over_fallibles.rs +++ /dev/null @@ -1,65 +0,0 @@ -use super::FOR_LOOPS_OVER_FALLIBLES; -use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::source::snippet; -use clippy_utils::ty::is_type_diagnostic_item; -use rustc_hir::{Expr, Pat}; -use rustc_lint::LateContext; -use rustc_span::symbol::sym; - -/// Checks for `for` loops over `Option`s and `Result`s. -pub(super) fn check(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, method_name: Option<&str>) { - let ty = cx.typeck_results().expr_ty(arg); - if is_type_diagnostic_item(cx, ty, sym::Option) { - let help_string = if let Some(method_name) = method_name { - format!( - "consider replacing `for {0} in {1}.{method_name}()` with `if let Some({0}) = {1}`", - snippet(cx, pat.span, "_"), - snippet(cx, arg.span, "_") - ) - } else { - format!( - "consider replacing `for {0} in {1}` with `if let Some({0}) = {1}`", - snippet(cx, pat.span, "_"), - snippet(cx, arg.span, "_") - ) - }; - span_lint_and_help( - cx, - FOR_LOOPS_OVER_FALLIBLES, - arg.span, - &format!( - "for loop over `{0}`, which is an `Option`. This is more readably written as an \ - `if let` statement", - snippet(cx, arg.span, "_") - ), - None, - &help_string, - ); - } else if is_type_diagnostic_item(cx, ty, sym::Result) { - let help_string = if let Some(method_name) = method_name { - format!( - "consider replacing `for {0} in {1}.{method_name}()` with `if let Ok({0}) = {1}`", - snippet(cx, pat.span, "_"), - snippet(cx, arg.span, "_") - ) - } else { - format!( - "consider replacing `for {0} in {1}` with `if let Ok({0}) = {1}`", - snippet(cx, pat.span, "_"), - snippet(cx, arg.span, "_") - ) - }; - span_lint_and_help( - cx, - FOR_LOOPS_OVER_FALLIBLES, - arg.span, - &format!( - "for loop over `{0}`, which is a `Result`. This is more readably written as an \ - `if let` statement", - snippet(cx, arg.span, "_") - ), - None, - &help_string, - ); - } -} diff --git a/src/tools/clippy/clippy_lints/src/loops/iter_next_loop.rs b/src/tools/clippy/clippy_lints/src/loops/iter_next_loop.rs index e640c62ebdace..b8a263817d297 100644 --- a/src/tools/clippy/clippy_lints/src/loops/iter_next_loop.rs +++ b/src/tools/clippy/clippy_lints/src/loops/iter_next_loop.rs @@ -5,7 +5,7 @@ use rustc_hir::Expr; use rustc_lint::LateContext; use rustc_span::sym; -pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>) -> bool { +pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>) { if is_trait_method(cx, arg, sym::Iterator) { span_lint( cx, @@ -14,8 +14,5 @@ pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>) -> bool { "you are iterating over `Iterator::next()` which is an Option; this will compile but is \ probably not what you want", ); - true - } else { - false } } diff --git a/src/tools/clippy/clippy_lints/src/loops/mod.rs b/src/tools/clippy/clippy_lints/src/loops/mod.rs index c0a0444485e3b..bcf278d9c8339 100644 --- a/src/tools/clippy/clippy_lints/src/loops/mod.rs +++ b/src/tools/clippy/clippy_lints/src/loops/mod.rs @@ -3,7 +3,6 @@ mod explicit_counter_loop; mod explicit_into_iter_loop; mod explicit_iter_loop; mod for_kv_map; -mod for_loops_over_fallibles; mod iter_next_loop; mod manual_find; mod manual_flatten; @@ -173,49 +172,6 @@ declare_clippy_lint! { "for-looping over `_.next()` which is probably not intended" } -declare_clippy_lint! { - /// ### What it does - /// Checks for `for` loops over `Option` or `Result` values. - /// - /// ### Why is this bad? - /// Readability. This is more clearly expressed as an `if - /// let`. - /// - /// ### Example - /// ```rust - /// # let opt = Some(1); - /// # let res: Result = Ok(1); - /// for x in opt { - /// // .. - /// } - /// - /// for x in &res { - /// // .. - /// } - /// - /// for x in res.iter() { - /// // .. - /// } - /// ``` - /// - /// Use instead: - /// ```rust - /// # let opt = Some(1); - /// # let res: Result = Ok(1); - /// if let Some(x) = opt { - /// // .. - /// } - /// - /// if let Ok(x) = res { - /// // .. - /// } - /// ``` - #[clippy::version = "1.45.0"] - pub FOR_LOOPS_OVER_FALLIBLES, - suspicious, - "for-looping over an `Option` or a `Result`, which is more clearly expressed as an `if let`" -} - declare_clippy_lint! { /// ### What it does /// Detects `loop + match` combinations that are easier @@ -648,7 +604,6 @@ declare_lint_pass!(Loops => [ EXPLICIT_ITER_LOOP, EXPLICIT_INTO_ITER_LOOP, ITER_NEXT_LOOP, - FOR_LOOPS_OVER_FALLIBLES, WHILE_LET_LOOP, NEEDLESS_COLLECT, EXPLICIT_COUNTER_LOOP, @@ -739,30 +694,22 @@ fn check_for_loop<'tcx>( manual_find::check(cx, pat, arg, body, span, expr); } -fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) { - let mut next_loop_linted = false; // whether or not ITER_NEXT_LOOP lint was used - +fn check_for_loop_arg(cx: &LateContext<'_>, _: &Pat<'_>, arg: &Expr<'_>) { if let ExprKind::MethodCall(method, self_arg, [], _) = arg.kind { let method_name = method.ident.as_str(); // check for looping over x.iter() or x.iter_mut(), could use &x or &mut x match method_name { "iter" | "iter_mut" => { explicit_iter_loop::check(cx, self_arg, arg, method_name); - for_loops_over_fallibles::check(cx, pat, self_arg, Some(method_name)); }, "into_iter" => { explicit_iter_loop::check(cx, self_arg, arg, method_name); explicit_into_iter_loop::check(cx, self_arg, arg); - for_loops_over_fallibles::check(cx, pat, self_arg, Some(method_name)); }, "next" => { - next_loop_linted = iter_next_loop::check(cx, arg); + iter_next_loop::check(cx, arg); }, _ => {}, } } - - if !next_loop_linted { - for_loops_over_fallibles::check(cx, pat, arg, None); - } } diff --git a/src/tools/clippy/clippy_lints/src/renamed_lints.rs b/src/tools/clippy/clippy_lints/src/renamed_lints.rs index d320eea1c377d..76d6ad0b23e6a 100644 --- a/src/tools/clippy/clippy_lints/src/renamed_lints.rs +++ b/src/tools/clippy/clippy_lints/src/renamed_lints.rs @@ -11,8 +11,8 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[ ("clippy::disallowed_method", "clippy::disallowed_methods"), ("clippy::disallowed_type", "clippy::disallowed_types"), ("clippy::eval_order_dependence", "clippy::mixed_read_write_in_expression"), - ("clippy::for_loop_over_option", "clippy::for_loops_over_fallibles"), - ("clippy::for_loop_over_result", "clippy::for_loops_over_fallibles"), + ("clippy::for_loop_over_option", "for_loops_over_fallibles"), + ("clippy::for_loop_over_result", "for_loops_over_fallibles"), ("clippy::identity_conversion", "clippy::useless_conversion"), ("clippy::if_let_some_result", "clippy::match_result_ok"), ("clippy::logic_bug", "clippy::overly_complex_bool_expr"), @@ -31,6 +31,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[ ("clippy::to_string_in_display", "clippy::recursive_format_impl"), ("clippy::zero_width_space", "clippy::invisible_characters"), ("clippy::drop_bounds", "drop_bounds"), + ("clippy::for_loops_over_fallibles", "for_loops_over_fallibles"), ("clippy::into_iter_on_array", "array_into_iter"), ("clippy::invalid_atomic_ordering", "invalid_atomic_ordering"), ("clippy::invalid_ref", "invalid_value"), diff --git a/src/tools/clippy/src/docs.rs b/src/tools/clippy/src/docs.rs index 3bf488ab4779b..bd27bc7938f8e 100644 --- a/src/tools/clippy/src/docs.rs +++ b/src/tools/clippy/src/docs.rs @@ -170,7 +170,6 @@ docs! { "fn_to_numeric_cast_any", "fn_to_numeric_cast_with_truncation", "for_kv_map", - "for_loops_over_fallibles", "forget_copy", "forget_non_drop", "forget_ref", diff --git a/src/tools/clippy/src/docs/for_loops_over_fallibles.txt b/src/tools/clippy/src/docs/for_loops_over_fallibles.txt deleted file mode 100644 index c5a7508e45d40..0000000000000 --- a/src/tools/clippy/src/docs/for_loops_over_fallibles.txt +++ /dev/null @@ -1,32 +0,0 @@ -### What it does -Checks for `for` loops over `Option` or `Result` values. - -### Why is this bad? -Readability. This is more clearly expressed as an `if -let`. - -### Example -``` -for x in opt { - // .. -} - -for x in &res { - // .. -} - -for x in res.iter() { - // .. -} -``` - -Use instead: -``` -if let Some(x) = opt { - // .. -} - -if let Ok(x) = res { - // .. -} -``` \ No newline at end of file diff --git a/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs b/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs deleted file mode 100644 index 75cdcc02353f7..0000000000000 --- a/src/tools/clippy/tests/ui/for_loops_over_fallibles.rs +++ /dev/null @@ -1,74 +0,0 @@ -#![warn(clippy::for_loops_over_fallibles)] -#![allow(clippy::uninlined_format_args)] -#![allow(for_loops_over_fallibles)] - -fn for_loops_over_fallibles() { - let option = Some(1); - let mut result = option.ok_or("x not found"); - let v = vec![0, 1, 2]; - - // check over an `Option` - for x in option { - println!("{}", x); - } - - // check over an `Option` - for x in option.iter() { - println!("{}", x); - } - - // check over a `Result` - for x in result { - println!("{}", x); - } - - // check over a `Result` - for x in result.iter_mut() { - println!("{}", x); - } - - // check over a `Result` - for x in result.into_iter() { - println!("{}", x); - } - - for x in option.ok_or("x not found") { - println!("{}", x); - } - - // make sure LOOP_OVER_NEXT lint takes clippy::precedence when next() is the last call - // in the chain - for x in v.iter().next() { - println!("{}", x); - } - - // make sure we lint when next() is not the last call in the chain - for x in v.iter().next().and(Some(0)) { - println!("{}", x); - } - - for x in v.iter().next().ok_or("x not found") { - println!("{}", x); - } - - // check for false positives - - // for loop false positive - for x in v { - println!("{}", x); - } - - // while let false positive for Option - while let Some(x) = option { - println!("{}", x); - break; - } - - // while let false positive for Result - while let Ok(x) = result { - println!("{}", x); - break; - } -} - -fn main() {} diff --git a/src/tools/clippy/tests/ui/for_loops_over_fallibles.stderr b/src/tools/clippy/tests/ui/for_loops_over_fallibles.stderr deleted file mode 100644 index f09adccabd1a8..0000000000000 --- a/src/tools/clippy/tests/ui/for_loops_over_fallibles.stderr +++ /dev/null @@ -1,95 +0,0 @@ -error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:10:14 - | -LL | for x in option { - | ^^^^^^ - | - = help: consider replacing `for x in option` with `if let Some(x) = option` - = note: `-D clippy::for-loops-over-fallibles` implied by `-D warnings` - -error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:15:14 - | -LL | for x in option.iter() { - | ^^^^^^ - | - = help: consider replacing `for x in option.iter()` with `if let Some(x) = option` - -error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:20:14 - | -LL | for x in result { - | ^^^^^^ - | - = help: consider replacing `for x in result` with `if let Ok(x) = result` - -error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:25:14 - | -LL | for x in result.iter_mut() { - | ^^^^^^ - | - = help: consider replacing `for x in result.iter_mut()` with `if let Ok(x) = result` - -error: for loop over `result`, which is a `Result`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:30:14 - | -LL | for x in result.into_iter() { - | ^^^^^^ - | - = help: consider replacing `for x in result.into_iter()` with `if let Ok(x) = result` - -error: for loop over `option.ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:34:14 - | -LL | for x in option.ok_or("x not found") { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")` - -error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want - --> $DIR/for_loops_over_fallibles.rs:40:14 - | -LL | for x in v.iter().next() { - | ^^^^^^^^^^^^^^^ - | - = note: `#[deny(clippy::iter_next_loop)]` on by default - -error: for loop over `v.iter().next().and(Some(0))`, which is an `Option`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:45:14 - | -LL | for x in v.iter().next().and(Some(0)) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))` - -error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement - --> $DIR/for_loops_over_fallibles.rs:49:14 - | -LL | for x in v.iter().next().ok_or("x not found") { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")` - -error: this loop never actually loops - --> $DIR/for_loops_over_fallibles.rs:61:5 - | -LL | / while let Some(x) = option { -LL | | println!("{}", x); -LL | | break; -LL | | } - | |_____^ - | - = note: `#[deny(clippy::never_loop)]` on by default - -error: this loop never actually loops - --> $DIR/for_loops_over_fallibles.rs:67:5 - | -LL | / while let Ok(x) = result { -LL | | println!("{}", x); -LL | | break; -LL | | } - | |_____^ - -error: aborting due to 11 previous errors - diff --git a/src/tools/clippy/tests/ui/manual_map_option.fixed b/src/tools/clippy/tests/ui/manual_map_option.fixed index a59da4ae10bce..e12ea7ec14500 100644 --- a/src/tools/clippy/tests/ui/manual_map_option.fixed +++ b/src/tools/clippy/tests/ui/manual_map_option.fixed @@ -7,7 +7,7 @@ clippy::unit_arg, clippy::match_ref_pats, clippy::redundant_pattern_matching, - clippy::for_loops_over_fallibles, + for_loops_over_fallibles, dead_code )] diff --git a/src/tools/clippy/tests/ui/manual_map_option.rs b/src/tools/clippy/tests/ui/manual_map_option.rs index 0bdbefa51e8b4..325a6db06c4e5 100644 --- a/src/tools/clippy/tests/ui/manual_map_option.rs +++ b/src/tools/clippy/tests/ui/manual_map_option.rs @@ -7,7 +7,7 @@ clippy::unit_arg, clippy::match_ref_pats, clippy::redundant_pattern_matching, - clippy::for_loops_over_fallibles, + for_loops_over_fallibles, dead_code )] diff --git a/src/tools/clippy/tests/ui/rename.fixed b/src/tools/clippy/tests/ui/rename.fixed index a6e7bdba77c65..8beae8dee0854 100644 --- a/src/tools/clippy/tests/ui/rename.fixed +++ b/src/tools/clippy/tests/ui/rename.fixed @@ -12,7 +12,7 @@ #![allow(clippy::disallowed_methods)] #![allow(clippy::disallowed_types)] #![allow(clippy::mixed_read_write_in_expression)] -#![allow(clippy::for_loops_over_fallibles)] +#![allow(for_loops_over_fallibles)] #![allow(clippy::useless_conversion)] #![allow(clippy::match_result_ok)] #![allow(clippy::overly_complex_bool_expr)] @@ -45,8 +45,8 @@ #![warn(clippy::disallowed_methods)] #![warn(clippy::disallowed_types)] #![warn(clippy::mixed_read_write_in_expression)] -#![warn(clippy::for_loops_over_fallibles)] -#![warn(clippy::for_loops_over_fallibles)] +#![warn(for_loops_over_fallibles)] +#![warn(for_loops_over_fallibles)] #![warn(clippy::useless_conversion)] #![warn(clippy::match_result_ok)] #![warn(clippy::overly_complex_bool_expr)] @@ -65,6 +65,7 @@ #![warn(clippy::recursive_format_impl)] #![warn(clippy::invisible_characters)] #![warn(drop_bounds)] +#![warn(for_loops_over_fallibles)] #![warn(array_into_iter)] #![warn(invalid_atomic_ordering)] #![warn(invalid_value)] diff --git a/src/tools/clippy/tests/ui/rename.rs b/src/tools/clippy/tests/ui/rename.rs index e8f57597d02b5..9e665047baaeb 100644 --- a/src/tools/clippy/tests/ui/rename.rs +++ b/src/tools/clippy/tests/ui/rename.rs @@ -12,7 +12,7 @@ #![allow(clippy::disallowed_methods)] #![allow(clippy::disallowed_types)] #![allow(clippy::mixed_read_write_in_expression)] -#![allow(clippy::for_loops_over_fallibles)] +#![allow(for_loops_over_fallibles)] #![allow(clippy::useless_conversion)] #![allow(clippy::match_result_ok)] #![allow(clippy::overly_complex_bool_expr)] @@ -65,6 +65,7 @@ #![warn(clippy::to_string_in_display)] #![warn(clippy::zero_width_space)] #![warn(clippy::drop_bounds)] +#![warn(clippy::for_loops_over_fallibles)] #![warn(clippy::into_iter_on_array)] #![warn(clippy::invalid_atomic_ordering)] #![warn(clippy::invalid_ref)] diff --git a/src/tools/clippy/tests/ui/rename.stderr b/src/tools/clippy/tests/ui/rename.stderr index 31865a7f66d60..63eb565185f07 100644 --- a/src/tools/clippy/tests/ui/rename.stderr +++ b/src/tools/clippy/tests/ui/rename.stderr @@ -54,17 +54,17 @@ error: lint `clippy::eval_order_dependence` has been renamed to `clippy::mixed_r LL | #![warn(clippy::eval_order_dependence)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `clippy::mixed_read_write_in_expression` -error: lint `clippy::for_loop_over_option` has been renamed to `clippy::for_loops_over_fallibles` +error: lint `clippy::for_loop_over_option` has been renamed to `for_loops_over_fallibles` --> $DIR/rename.rs:48:9 | LL | #![warn(clippy::for_loop_over_option)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `clippy::for_loops_over_fallibles` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `for_loops_over_fallibles` -error: lint `clippy::for_loop_over_result` has been renamed to `clippy::for_loops_over_fallibles` +error: lint `clippy::for_loop_over_result` has been renamed to `for_loops_over_fallibles` --> $DIR/rename.rs:49:9 | LL | #![warn(clippy::for_loop_over_result)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `clippy::for_loops_over_fallibles` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `for_loops_over_fallibles` error: lint `clippy::identity_conversion` has been renamed to `clippy::useless_conversion` --> $DIR/rename.rs:50:9 @@ -174,59 +174,65 @@ error: lint `clippy::drop_bounds` has been renamed to `drop_bounds` LL | #![warn(clippy::drop_bounds)] | ^^^^^^^^^^^^^^^^^^^ help: use the new name: `drop_bounds` -error: lint `clippy::into_iter_on_array` has been renamed to `array_into_iter` +error: lint `clippy::for_loops_over_fallibles` has been renamed to `for_loops_over_fallibles` --> $DIR/rename.rs:68:9 | +LL | #![warn(clippy::for_loops_over_fallibles)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `for_loops_over_fallibles` + +error: lint `clippy::into_iter_on_array` has been renamed to `array_into_iter` + --> $DIR/rename.rs:69:9 + | LL | #![warn(clippy::into_iter_on_array)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `array_into_iter` error: lint `clippy::invalid_atomic_ordering` has been renamed to `invalid_atomic_ordering` - --> $DIR/rename.rs:69:9 + --> $DIR/rename.rs:70:9 | LL | #![warn(clippy::invalid_atomic_ordering)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `invalid_atomic_ordering` error: lint `clippy::invalid_ref` has been renamed to `invalid_value` - --> $DIR/rename.rs:70:9 + --> $DIR/rename.rs:71:9 | LL | #![warn(clippy::invalid_ref)] | ^^^^^^^^^^^^^^^^^^^ help: use the new name: `invalid_value` error: lint `clippy::mem_discriminant_non_enum` has been renamed to `enum_intrinsics_non_enums` - --> $DIR/rename.rs:71:9 + --> $DIR/rename.rs:72:9 | LL | #![warn(clippy::mem_discriminant_non_enum)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `enum_intrinsics_non_enums` error: lint `clippy::panic_params` has been renamed to `non_fmt_panics` - --> $DIR/rename.rs:72:9 + --> $DIR/rename.rs:73:9 | LL | #![warn(clippy::panic_params)] | ^^^^^^^^^^^^^^^^^^^^ help: use the new name: `non_fmt_panics` error: lint `clippy::positional_named_format_parameters` has been renamed to `named_arguments_used_positionally` - --> $DIR/rename.rs:73:9 + --> $DIR/rename.rs:74:9 | LL | #![warn(clippy::positional_named_format_parameters)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `named_arguments_used_positionally` error: lint `clippy::temporary_cstring_as_ptr` has been renamed to `temporary_cstring_as_ptr` - --> $DIR/rename.rs:74:9 + --> $DIR/rename.rs:75:9 | LL | #![warn(clippy::temporary_cstring_as_ptr)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `temporary_cstring_as_ptr` error: lint `clippy::unknown_clippy_lints` has been renamed to `unknown_lints` - --> $DIR/rename.rs:75:9 + --> $DIR/rename.rs:76:9 | LL | #![warn(clippy::unknown_clippy_lints)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `unknown_lints` error: lint `clippy::unused_label` has been renamed to `unused_labels` - --> $DIR/rename.rs:76:9 + --> $DIR/rename.rs:77:9 | LL | #![warn(clippy::unused_label)] | ^^^^^^^^^^^^^^^^^^^^ help: use the new name: `unused_labels` -error: aborting due to 38 previous errors +error: aborting due to 39 previous errors From 9d4edff1b0179e3514bcc27618edd6348903e99a Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Fri, 7 Oct 2022 18:21:32 +0000 Subject: [PATCH 19/19] adopt to building infcx --- .../src/for_loops_over_fallibles.rs | 45 +++++++++---------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/compiler/rustc_lint/src/for_loops_over_fallibles.rs b/compiler/rustc_lint/src/for_loops_over_fallibles.rs index 4f0ae08116611..ed8d424e0c62d 100644 --- a/compiler/rustc_lint/src/for_loops_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loops_over_fallibles.rs @@ -159,28 +159,25 @@ fn suggest_question_mark<'tcx>( } let ty = substs.type_at(0); - let is_iterator = cx.tcx.infer_ctxt().enter(|infcx| { - let mut fulfill_cx = >::new(infcx.tcx); - - let cause = ObligationCause::new( - span, - body_id.hir_id, - rustc_infer::traits::ObligationCauseCode::MiscObligation, - ); - fulfill_cx.register_bound( - &infcx, - ty::ParamEnv::empty(), - // Erase any region vids from the type, which may not be resolved - infcx.tcx.erase_regions(ty), - into_iterator_did, - cause, - ); - - // Select all, including ambiguous predicates - let errors = fulfill_cx.select_all_or_error(&infcx); - - errors.is_empty() - }); - - is_iterator + let infcx = cx.tcx.infer_ctxt().build(); + let mut fulfill_cx = >::new(infcx.tcx); + + let cause = ObligationCause::new( + span, + body_id.hir_id, + rustc_infer::traits::ObligationCauseCode::MiscObligation, + ); + fulfill_cx.register_bound( + &infcx, + ty::ParamEnv::empty(), + // Erase any region vids from the type, which may not be resolved + infcx.tcx.erase_regions(ty), + into_iterator_did, + cause, + ); + + // Select all, including ambiguous predicates + let errors = fulfill_cx.select_all_or_error(&infcx); + + errors.is_empty() }