Skip to content

Commit

Permalink
Auto merge of rust-lang#125156 - zachs18:for_loops_over_fallibles_beh…
Browse files Browse the repository at this point in the history
…ind_refs, r=Nilstrieb

Expand `for_loops_over_fallibles` lint to lint on fallibles behind references.

Extends the scope of the (warn-by-default) lint `for_loops_over_fallibles` from just `for _ in x` where `x: Option<_>/Result<_, _>` to also cover `x: &(mut) Option<_>/Result<_>`

```rs
fn main() {
    // Current lints
    for _ in Some(42) {}
    for _ in Ok::<_, i32>(42) {}

    // New lints
    for _ in &Some(42) {}
    for _ in &mut Some(42) {}
    for _ in &Ok::<_, i32>(42) {}
    for _ in &mut Ok::<_, i32>(42) {}

    // Should not lint
    for _ in Some(42).into_iter() {}
    for _ in Some(42).iter() {}
    for _ in Some(42).iter_mut() {}
    for _ in Ok::<_, i32>(42).into_iter() {}
    for _ in Ok::<_, i32>(42).iter() {}
    for _ in Ok::<_, i32>(42).iter_mut() {}
}
```

<details><summary><code>cargo build</code> diff</summary>

```diff
diff --git a/old.out b/new.out
index 84215aa..ca195a7 100644
--- a/old.out
+++ b/new.out
`@@` -1,33 +1,93 `@@`
 warning: for loop over an `Option`. This is more readably written as an `if let` statement
  --> src/main.rs:3:14
   |
 3 |     for _ in Some(42) {}
   |              ^^^^^^^^
   |
   = note: `#[warn(for_loops_over_fallibles)]` on by default
 help: to check pattern in a loop use `while let`
   |
 3 |     while let Some(_) = Some(42) {}
   |     ~~~~~~~~~~~~~~~ ~~~
 help: consider using `if let` to clear intent
   |
 3 |     if let Some(_) = Some(42) {}
   |     ~~~~~~~~~~~~ ~~~

 warning: for loop over a `Result`. This is more readably written as an `if let` statement
  --> src/main.rs:4:14
   |
 4 |     for _ in Ok::<_, i32>(42) {}
   |              ^^^^^^^^^^^^^^^^
   |
 help: to check pattern in a loop use `while let`
   |
 4 |     while let Ok(_) = Ok::<_, i32>(42) {}
   |     ~~~~~~~~~~~~~ ~~~
 help: consider using `if let` to clear intent
   |
 4 |     if let Ok(_) = Ok::<_, i32>(42) {}
   |     ~~~~~~~~~~ ~~~

-warning: `for-loops-over-fallibles` (bin "for-loops-over-fallibles") generated 2 warnings
-    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
+warning: for loop over a `&Option`. This is more readably written as an `if let` statement
+ --> src/main.rs:7:14
+  |
+7 |     for _ in &Some(42) {}
+  |              ^^^^^^^^^
+  |
+help: to check pattern in a loop use `while let`
+  |
+7 |     while let Some(_) = &Some(42) {}
+  |     ~~~~~~~~~~~~~~~ ~~~
+help: consider using `if let` to clear intent
+  |
+7 |     if let Some(_) = &Some(42) {}
+  |     ~~~~~~~~~~~~ ~~~
+
+warning: for loop over a `&mut Option`. This is more readably written as an `if let` statement
+ --> src/main.rs:8:14
+  |
+8 |     for _ in &mut Some(42) {}
+  |              ^^^^^^^^^^^^^
+  |
+help: to check pattern in a loop use `while let`
+  |
+8 |     while let Some(_) = &mut Some(42) {}
+  |     ~~~~~~~~~~~~~~~ ~~~
+help: consider using `if let` to clear intent
+  |
+8 |     if let Some(_) = &mut Some(42) {}
+  |     ~~~~~~~~~~~~ ~~~
+
+warning: for loop over a `&Result`. This is more readably written as an `if let` statement
+ --> src/main.rs:9:14
+  |
+9 |     for _ in &Ok::<_, i32>(42) {}
+  |              ^^^^^^^^^^^^^^^^^
+  |
+help: to check pattern in a loop use `while let`
+  |
+9 |     while let Ok(_) = &Ok::<_, i32>(42) {}
+  |     ~~~~~~~~~~~~~ ~~~
+help: consider using `if let` to clear intent
+  |
+9 |     if let Ok(_) = &Ok::<_, i32>(42) {}
+  |     ~~~~~~~~~~ ~~~
+
+warning: for loop over a `&mut Result`. This is more readably written as an `if let` statement
+  --> src/main.rs:10:14
+   |
+10 |     for _ in &mut Ok::<_, i32>(42) {}
+   |              ^^^^^^^^^^^^^^^^^^^^^
+   |
+help: to check pattern in a loop use `while let`
+   |
+10 |     while let Ok(_) = &mut Ok::<_, i32>(42) {}
+   |     ~~~~~~~~~~~~~ ~~~
+help: consider using `if let` to clear intent
+   |
+10 |     if let Ok(_) = &mut Ok::<_, i32>(42) {}
+   |     ~~~~~~~~~~ ~~~
+
+warning: `for-loops-over-fallibles` (bin "for-loops-over-fallibles") generated 6 warnings
+    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s

```

</details>

-----

Question:

* ~~Currently, the article `an` is used for `&Option`, and `&mut Option` in the lint diagnostic, since that's what `Option` uses. Is this okay or should it be changed? (likewise, `a` is used for `&Result` and `&mut Result`)~~ The article `a` is used for `&Option`, `&mut Option`, `&Result`, `&mut Result` and (as before) `Result`. Only `Option` uses `an` (as before).

`@rustbot` label +A-lint
  • Loading branch information
bors committed May 16, 2024
2 parents b71e8cb + 376a8c0 commit 4027f3d
Show file tree
Hide file tree
Showing 10 changed files with 110 additions and 13 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_builtin_macros/src/deriving/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_analysis/src/check/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
17 changes: 15 additions & 2 deletions compiler/rustc_lint/src/for_loops_over_fallibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,27 @@ 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, 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)),
_ => return,
},
_ => return,
};

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,
};

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)
{
Expand All @@ -85,7 +98,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 },
);
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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);
});
}

Expand Down
1 change: 1 addition & 0 deletions library/core/tests/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ pub fn test_iter() {
}

#[test]
#[allow(for_loops_over_fallibles)]
pub fn test_iter_mut() {
let mut ok: Result<isize, &'static str> = Ok(100);
for loc in ok.iter_mut() {
Expand Down
22 changes: 22 additions & 0 deletions tests/ui/lint/for_loop_over_fallibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,25 @@ fn _returns_result() -> Result<(), ()> {

Ok(())
}

fn _by_ref() {
// Shared refs
for _ in &Some(1) {}
//~^ 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) {}
//~^ 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 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) {}
//~^ 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
}
62 changes: 61 additions & 1 deletion tests/ui/lint/for_loop_over_fallibles.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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 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) {}
| ^^^^^^^^
|
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 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) {}
| ^^^^^^^^^^^^
|
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

0 comments on commit 4027f3d

Please sign in to comment.