From 4be041a2cd9f134ee42579beb14840b9abebcb75 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 10:00:42 -0500 Subject: [PATCH 1/8] Also apply `warn(for_loops_over_fallibles)` to &T and &mut T, not just T = Result/Option. --- compiler/rustc_lint/src/for_loops_over_fallibles.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/for_loops_over_fallibles.rs b/compiler/rustc_lint/src/for_loops_over_fallibles.rs index a6876d8aae793..d766393db29a5 100644 --- a/compiler/rustc_lint/src/for_loops_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loops_over_fallibles.rs @@ -52,7 +52,15 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { let ty = cx.typeck_results().expr_ty(arg); - let &ty::Adt(adt, args) = ty.kind() else { return }; + // let &ty::Adt(adt, args) = ty.kind() else { return }; + let (adt, args, _) = match ty.kind() { + &ty::Adt(adt, args) => (adt, args, None), + &ty::Ref(_, ty, mutability) => match ty.kind() { + &ty::Adt(adt, args) => (adt, args, Some(mutability)), + _ => return, + }, + _ => return, + }; let (article, ty, var) = match adt.did() { did if cx.tcx.is_diagnostic_item(sym::Option, did) => ("an", "Option", "Some"), From 7d7eb973d0118a52209e5ee7adafc29b341957f0 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 10:01:06 -0500 Subject: [PATCH 2/8] Fix new for_loops_over_fallibles hits in compiler. --- compiler/rustc_builtin_macros/src/deriving/default.rs | 4 ++-- compiler/rustc_resolve/src/late.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_builtin_macros/src/deriving/default.rs b/compiler/rustc_builtin_macros/src/deriving/default.rs index bf92ddb33701c..577523a1d5a32 100644 --- a/compiler/rustc_builtin_macros/src/deriving/default.rs +++ b/compiler/rustc_builtin_macros/src/deriving/default.rs @@ -3,7 +3,7 @@ use crate::deriving::generic::*; use crate::errors; use core::ops::ControlFlow; use rustc_ast as ast; -use rustc_ast::visit::walk_list; +use rustc_ast::visit::visit_opt; use rustc_ast::{attr, EnumDef, VariantData}; use rustc_expand::base::{Annotatable, DummyResult, ExtCtxt}; use rustc_span::symbol::Ident; @@ -224,7 +224,7 @@ impl<'a, 'b> rustc_ast::visit::Visitor<'a> for DetectNonVariantDefaultAttr<'a, ' self.visit_ident(v.ident); self.visit_vis(&v.vis); self.visit_variant_data(&v.data); - walk_list!(self, visit_anon_const, &v.disr_expr); + visit_opt!(self, visit_anon_const, &v.disr_expr); for attr in &v.attrs { rustc_ast::visit::walk_attribute(self, attr); } diff --git a/compiler/rustc_resolve/src/late.rs b/compiler/rustc_resolve/src/late.rs index 322f2922f9253..796e3e38aebfb 100644 --- a/compiler/rustc_resolve/src/late.rs +++ b/compiler/rustc_resolve/src/late.rs @@ -12,7 +12,7 @@ use crate::{Module, ModuleOrUniformRoot, NameBinding, ParentScope, PathResult}; use crate::{ResolutionError, Resolver, Segment, UseError}; use rustc_ast::ptr::P; -use rustc_ast::visit::{walk_list, AssocCtxt, BoundKind, FnCtxt, FnKind, Visitor}; +use rustc_ast::visit::{visit_opt, walk_list, AssocCtxt, BoundKind, FnCtxt, FnKind, Visitor}; use rustc_ast::*; use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap}; use rustc_errors::{codes::*, Applicability, DiagArgValue, IntoDiagArg, StashKey}; @@ -3286,7 +3286,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> { fn resolve_local(&mut self, local: &'ast Local) { debug!("resolving local ({:?})", local); // Resolve the type. - walk_list!(self, visit_ty, &local.ty); + visit_opt!(self, visit_ty, &local.ty); // Resolve the initializer. if let Some((init, els)) = local.kind.init_else_opt() { @@ -3485,8 +3485,8 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> { fn resolve_arm(&mut self, arm: &'ast Arm) { self.with_rib(ValueNS, RibKind::Normal, |this| { this.resolve_pattern_top(&arm.pat, PatternSource::Match); - walk_list!(this, visit_expr, &arm.guard); - walk_list!(this, visit_expr, &arm.body); + visit_opt!(this, visit_expr, &arm.guard); + visit_opt!(this, visit_expr, &arm.body); }); } From 77f288c18d638f3cfda91810e0c4fb6a6c7e6963 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 10:59:13 -0500 Subject: [PATCH 3/8] Include reference in lint diagnostic --- compiler/rustc_lint/messages.ftl | 2 +- compiler/rustc_lint/src/for_loops_over_fallibles.rs | 10 +++++++--- compiler/rustc_lint/src/lints.rs | 1 + 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index 5180fce2eb378..8c9ad5b6c6b7a 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -215,7 +215,7 @@ lint_expectation = this lint expectation is unfulfilled .rationale = {$rationale} lint_for_loops_over_fallibles = - for loop over {$article} `{$ty}`. This is more readably written as an `if let` statement + for loop over {$article} `{$ref_prefix}{$ty}`. This is more readably written as an `if let` statement .suggestion = consider using `if let` to clear intent .remove_next = to iterate over `{$recv_snip}` remove the call to `next` .use_while_let = to check pattern in a loop use `while let` diff --git a/compiler/rustc_lint/src/for_loops_over_fallibles.rs b/compiler/rustc_lint/src/for_loops_over_fallibles.rs index d766393db29a5..12a790bb0faf5 100644 --- a/compiler/rustc_lint/src/for_loops_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loops_over_fallibles.rs @@ -52,8 +52,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { let ty = cx.typeck_results().expr_ty(arg); - // let &ty::Adt(adt, args) = ty.kind() else { return }; - let (adt, args, _) = match ty.kind() { + let (adt, args, ref_mutability) = match ty.kind() { &ty::Adt(adt, args) => (adt, args, None), &ty::Ref(_, ty, mutability) => match ty.kind() { &ty::Adt(adt, args) => (adt, args, Some(mutability)), @@ -68,6 +67,11 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { _ => return, }; + let ref_prefix = match ref_mutability { + None => "", + Some(ref_mutability) => ref_mutability.ref_prefix_str(), + }; + let sub = if let Some(recv) = extract_iterator_next_call(cx, arg) && let Ok(recv_snip) = cx.sess().source_map().span_to_snippet(recv.span) { @@ -93,7 +97,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { cx.emit_span_lint( FOR_LOOPS_OVER_FALLIBLES, arg.span, - ForLoopsOverFalliblesDiag { article, ty, sub, question_mark, suggestion }, + ForLoopsOverFalliblesDiag { article, ref_prefix, ty, sub, question_mark, suggestion }, ); } } diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs index 7efa5245baa20..382532b17880a 100644 --- a/compiler/rustc_lint/src/lints.rs +++ b/compiler/rustc_lint/src/lints.rs @@ -613,6 +613,7 @@ pub enum PtrNullChecksDiag<'a> { #[diag(lint_for_loops_over_fallibles)] pub struct ForLoopsOverFalliblesDiag<'a> { pub article: &'static str, + pub ref_prefix: &'static str, pub ty: &'static str, #[subdiagnostic] pub sub: ForLoopsOverFalliblesLoopSub<'a>, From ea549fd1761c8f1fbc99df2ad7d3c1ac7d8f7824 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 11:53:40 -0500 Subject: [PATCH 4/8] Add tests for 'Also apply `warn(for_loops_over_fallibles)` to &T and &mut T, not just T = Result/Option.' --- tests/ui/lint/for_loop_over_fallibles.rs | 22 +++++++ tests/ui/lint/for_loop_over_fallibles.stderr | 62 +++++++++++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/tests/ui/lint/for_loop_over_fallibles.rs b/tests/ui/lint/for_loop_over_fallibles.rs index 52c3b8f2aae5f..76049453126b2 100644 --- a/tests/ui/lint/for_loop_over_fallibles.rs +++ b/tests/ui/lint/for_loop_over_fallibles.rs @@ -41,3 +41,25 @@ fn _returns_result() -> Result<(), ()> { Ok(()) } + +fn _by_ref() { + // Shared refs + 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 + + // Mutable refs + for _ in &mut Some(1) {} + //~^ WARN for loop over an `&mut 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 &mut Ok::<_, ()>(1) {} + //~^ WARN for loop over a `&mut 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 +} diff --git a/tests/ui/lint/for_loop_over_fallibles.stderr b/tests/ui/lint/for_loop_over_fallibles.stderr index 96efdf85c4901..00134e468ba65 100644 --- a/tests/ui/lint/for_loop_over_fallibles.stderr +++ b/tests/ui/lint/for_loop_over_fallibles.stderr @@ -97,5 +97,65 @@ help: consider using `if let` to clear intent LL | if let Ok(_) = Ok::<_, ()>([0; 0]) {} | ~~~~~~~~~~ ~~~ -warning: 6 warnings emitted +warning: for loop over an `&Option`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:47:14 + | +LL | for _ in &Some(1) {} + | ^^^^^^^^ + | +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:51: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 `&mut Option`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:57:14 + | +LL | for _ in &mut Some(1) {} + | ^^^^^^^^^^^^ + | +help: to check pattern in a loop use `while let` + | +LL | while let Some(_) = &mut Some(1) {} + | ~~~~~~~~~~~~~~~ ~~~ +help: consider using `if let` to clear intent + | +LL | if let Some(_) = &mut Some(1) {} + | ~~~~~~~~~~~~ ~~~ + +warning: for loop over a `&mut Result`. This is more readably written as an `if let` statement + --> $DIR/for_loop_over_fallibles.rs:61:14 + | +LL | for _ in &mut Ok::<_, ()>(1) {} + | ^^^^^^^^^^^^^^^^^^^ + | +help: to check pattern in a loop use `while let` + | +LL | while let Ok(_) = &mut Ok::<_, ()>(1) {} + | ~~~~~~~~~~~~~ ~~~ +help: consider using `if let` to clear intent + | +LL | if let Ok(_) = &mut Ok::<_, ()>(1) {} + | ~~~~~~~~~~ ~~~ + +warning: 10 warnings emitted From 0e467596ffa85c68504b94b951a0c32fefb3c889 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 12:17:25 -0500 Subject: [PATCH 5/8] Fix more new for_loops_over_fallibles hits in compiler. --- compiler/rustc_hir_analysis/src/check/region.rs | 4 ++-- compiler/rustc_mir_build/src/build/matches/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index 4540310937d02..e51f95eed021b 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -6,7 +6,7 @@ //! //! [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/borrow_check.html -use rustc_ast::visit::walk_list; +use rustc_ast::visit::visit_opt; use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_hir::def_id::DefId; @@ -168,7 +168,7 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h hir::StmtKind::Expr(..) | hir::StmtKind::Semi(..) => visitor.visit_stmt(statement), } } - walk_list!(visitor, visit_expr, &blk.expr); + visit_opt!(visitor, visit_expr, &blk.expr); } visitor.cx = prev_cx; diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 3fc719394bf9e..d948354bc4959 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -925,7 +925,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { for subpattern in prefix.iter() { self.visit_primary_bindings(subpattern, pattern_user_ty.clone().index(), f); } - for subpattern in slice { + if let Some(subpattern) = slice { self.visit_primary_bindings( subpattern, pattern_user_ty.clone().subslice(from, to), From 66573b70a9b1807337f1980538d524e1dfd2aba1 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 12:45:52 -0500 Subject: [PATCH 6/8] Use 'a' article for &Option. --- compiler/rustc_lint/src/for_loops_over_fallibles.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/rustc_lint/src/for_loops_over_fallibles.rs b/compiler/rustc_lint/src/for_loops_over_fallibles.rs index 12a790bb0faf5..b05f5e7638b4e 100644 --- a/compiler/rustc_lint/src/for_loops_over_fallibles.rs +++ b/compiler/rustc_lint/src/for_loops_over_fallibles.rs @@ -62,6 +62,7 @@ impl<'tcx> LateLintPass<'tcx> for ForLoopsOverFallibles { }; let (article, ty, var) = match adt.did() { + did if cx.tcx.is_diagnostic_item(sym::Option, did) && ref_mutability.is_some() => ("a", "Option", "Some"), 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, From 6b818ddac61e9771e3b97f17d8ab21cf9f7bc2de Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 13:15:59 -0500 Subject: [PATCH 7/8] Fix article in test --- tests/ui/lint/for_loop_over_fallibles.rs | 4 ++-- tests/ui/lint/for_loop_over_fallibles.stderr | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ui/lint/for_loop_over_fallibles.rs b/tests/ui/lint/for_loop_over_fallibles.rs index 76049453126b2..89ac20c352178 100644 --- a/tests/ui/lint/for_loop_over_fallibles.rs +++ b/tests/ui/lint/for_loop_over_fallibles.rs @@ -45,7 +45,7 @@ fn _returns_result() -> Result<(), ()> { fn _by_ref() { // Shared refs for _ in &Some(1) {} - //~^ WARN for loop over an `&Option`. This is more readably written as an `if let` statement + //~^ WARN for loop over a `&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) {} @@ -55,7 +55,7 @@ fn _by_ref() { // Mutable refs for _ in &mut Some(1) {} - //~^ WARN for loop over an `&mut Option`. This is more readably written as an `if let` statement + //~^ WARN for loop over a `&mut 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 &mut Ok::<_, ()>(1) {} diff --git a/tests/ui/lint/for_loop_over_fallibles.stderr b/tests/ui/lint/for_loop_over_fallibles.stderr index 00134e468ba65..f695de082572a 100644 --- a/tests/ui/lint/for_loop_over_fallibles.stderr +++ b/tests/ui/lint/for_loop_over_fallibles.stderr @@ -97,7 +97,7 @@ help: consider using `if let` to clear intent LL | if let Ok(_) = Ok::<_, ()>([0; 0]) {} | ~~~~~~~~~~ ~~~ -warning: for loop over an `&Option`. This is more readably written as an `if let` statement +warning: for loop over a `&Option`. This is more readably written as an `if let` statement --> $DIR/for_loop_over_fallibles.rs:47:14 | LL | for _ in &Some(1) {} @@ -127,7 +127,7 @@ help: consider using `if let` to clear intent LL | if let Ok(_) = &Ok::<_, ()>(1) {} | ~~~~~~~~~~ ~~~ -warning: for loop over an `&mut Option`. This is more readably written as an `if let` statement +warning: for loop over a `&mut Option`. This is more readably written as an `if let` statement --> $DIR/for_loop_over_fallibles.rs:57:14 | LL | for _ in &mut Some(1) {} From 376a8c0ae5ad6cb8d0c5d6631874bdd3c950436f Mon Sep 17 00:00:00 2001 From: Zachary S Date: Wed, 15 May 2024 13:51:16 -0500 Subject: [PATCH 8/8] Allow for_loops_over_fallibles in test that tests &mut Result as IntoIterator. --- library/core/tests/result.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/core/tests/result.rs b/library/core/tests/result.rs index a47ef7aa1ebc6..00a6fd75b4f46 100644 --- a/library/core/tests/result.rs +++ b/library/core/tests/result.rs @@ -170,6 +170,7 @@ pub fn test_iter() { } #[test] +#[allow(for_loops_over_fallibles)] pub fn test_iter_mut() { let mut ok: Result = Ok(100); for loc in ok.iter_mut() {