From 730856442387c9804581eae5927a5667171022ca Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 25 Jul 2022 00:37:11 +0400 Subject: Add a test for the `for_loop_over_fallibles` lint --- src/test/ui/lint/for_loop_over_fallibles.rs | 43 ++++++++++ src/test/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 (limited to 'src/test') 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 00000000000..43d71c2e808 --- /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 00000000000..52eac945d85 --- /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 + -- cgit 1.4.1-3-g733a5 From 23a7674e3eb7e860ae59e81acc0699926dd0797c Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Mon, 25 Jul 2022 00:58:04 +0400 Subject: `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(-) (limited to 'src/test') diff --git a/compiler/rustc_lint/src/for_loop_over_fallibles.rs b/compiler/rustc_lint/src/for_loop_over_fallibles.rs index 69d8fd84b64..c8d5586d39f 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 52eac945d85..56e3126dc09 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() {} -- cgit 1.4.1-3-g733a5 From 0250f0244b320b1cecabc05c108178171c733f6c Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Tue, 26 Jul 2022 14:17:15 +0400 Subject: 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(-) (limited to 'src/test') diff --git a/compiler/rustc_ast/src/visit.rs b/compiler/rustc_ast/src/visit.rs index a71e055a4b3..c6c80e613e3 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 27a599315dc..6a0fe7784fb 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 a1ae9a36bc1..880558eb5b6 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() { -- cgit 1.4.1-3-g733a5 From 7434b9f0d1e1587dc97829ffbc65a5afdf04fb7e Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Fri, 7 Oct 2022 15:59:39 +0000 Subject: fixup lint name --- compiler/rustc_ast/src/visit.rs | 2 +- compiler/rustc_lint/src/for_loop_over_fallibles.rs | 188 --------------------- .../rustc_lint/src/for_loops_over_fallibles.rs | 188 +++++++++++++++++++++ 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 +- .../clippy/tests/ui/for_loops_over_fallibles.rs | 2 +- 9 files changed, 197 insertions(+), 197 deletions(-) delete mode 100644 compiler/rustc_lint/src/for_loop_over_fallibles.rs create mode 100644 compiler/rustc_lint/src/for_loops_over_fallibles.rs (limited to 'src/test') diff --git a/compiler/rustc_ast/src/visit.rs b/compiler/rustc_ast/src/visit.rs index c6c80e613e3..145b31842ef 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_loop_over_fallibles.rs deleted file mode 100644 index 2253546b5d3..00000000000 --- a/compiler/rustc_lint/src/for_loop_over_fallibles.rs +++ /dev/null @@ -1,188 +0,0 @@ -use crate::{LateContext, LateLintPass, LintContext}; - -use hir::{Expr, Pat}; -use rustc_errors::Applicability; -use rustc_hir as hir; -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! { - /// 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 - /// - /// 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. - 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, 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"), - did if cx.tcx.is_diagnostic_item(sym::Result, did) => ("a", "Result", "Ok"), - _ => return, - }; - - let msg = format!( - "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| { - 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`"), - ".by_ref()", - 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 - ); - } - - 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![ - // 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::MaybeIncorrect, - ); - - warn.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 - } -} - -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 - } -} - -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 -} diff --git a/compiler/rustc_lint/src/for_loops_over_fallibles.rs b/compiler/rustc_lint/src/for_loops_over_fallibles.rs new file mode 100644 index 00000000000..54f1f307ddd --- /dev/null +++ b/compiler/rustc_lint/src/for_loops_over_fallibles.rs @@ -0,0 +1,188 @@ +use crate::{LateContext, LateLintPass, LintContext}; + +use hir::{Expr, Pat}; +use rustc_errors::Applicability; +use rustc_hir as hir; +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! { + /// The `for_loops_over_fallibles` lint checks for `for` loops over `Option` or `Result` values. + /// + /// ### Example + /// + /// ```rust + /// let opt = Some(1); + /// for x in opt { /* ... */} + /// ``` + /// + /// {{produces}} + /// + /// ### 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. + 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!(ForLoopsOverFallibles => [FOR_LOOPS_OVER_FALLIBLES]); + +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 }; + + let ty = cx.typeck_results().expr_ty(arg); + + 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"), + did if cx.tcx.is_diagnostic_item(sym::Result, did) => ("a", "Result", "Ok"), + _ => 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); + + 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`"), + ".by_ref()", + 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 + ); + } + + 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![ + // 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::MaybeIncorrect, + ); + + warn.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 + } +} + +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 + } +} + +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 +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index ecc64511d7f..fee6e080c4f 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 84eb4fc0aa3..f36f7c26806 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 880558eb5b6..eea548c482f 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 56e3126dc09..96efdf85c49 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 203656fa4d6..55fb3788a8b 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 54661ff94f2..75cdcc02353 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); -- cgit 1.4.1-3-g733a5