Skip to content

Commit

Permalink
Rollup merge of rust-lang#99696 - WaffleLapkin:uplift, r=fee1-dead
Browse files Browse the repository at this point in the history
Uplift `clippy::for_loops_over_fallibles` lint into rustc

This PR, as the title suggests, uplifts [`clippy::for_loops_over_fallibles`] lint into rustc. This lint warns for code like this:
```rust
for _ in Some(1) {}
for _ in Ok::<_, ()>(1) {}
```
i.e. directly iterating over `Option` and `Result` using `for` loop.

There are a number of suggestions that this PR adds (on top of what clippy suggested):
1. If the argument (? is there a better name for that expression) of a `for` loop is a `.next()` call, then we can suggest removing it (or rather replacing with `.by_ref()` to allow iterator being used later)
   ```rust
    for _ in iter.next() {}
    // turns into
    for _ in iter.by_ref() {}
    ```
2. (otherwise) We can suggest using `while let`, this is useful for non-iterator, iterator-like things like [async] channels
   ```rust
   for _ in rx.recv() {}
   // turns into
   while let Some(_) = rx.recv() {}
   ```
3. If the argument type is `Result<impl IntoIterator, _>` and the body has a `Result<_, _>` type, we can suggest using `?`
   ```rust
   for _ in f() {}
   // turns into
   for _ in f()? {}
   ```
4. To preserve the original behavior and clear intent, we can suggest using `if let`
   ```rust
   for _ in f() {}
   // turns into
   if let Some(_) = f() {}
   ```
(P.S. `Some` and `Ok` are interchangeable depending on the type)

I still feel that the lint wording/look is somewhat off, so I'll be happy to hear suggestions (on how to improve suggestions :D)!

Resolves rust-lang#99272

[`clippy::for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles
  • Loading branch information
JohnTitor authored Aug 24, 2022
2 parents bf3fc63 + 71b8c89 commit ec74ca0
Show file tree
Hide file tree
Showing 9 changed files with 351 additions and 16 deletions.
14 changes: 6 additions & 8 deletions compiler/rustc_ast/src/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,)* )?)
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/type_check/input_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {

debug!("{:?} normalized to {:?}", t, norm_ty);

for data in constraints {
if let Some(data) = constraints {
ConstraintConversion::new(
self.infcx,
&self.borrowck_context.universal_regions,
Expand Down
188 changes: 188 additions & 0 deletions compiler/rustc_lint/src/for_loop_over_fallibles.rs
Original file line number Diff line number Diff line change
@@ -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_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<ty::GenericArg<'tcx>>,
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 = <dyn TraitEngine<'_>>::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
}
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mod context;
mod early;
mod enum_intrinsics_non_enums;
mod expect;
mod for_loop_over_fallibles;
pub mod hidden_unicode_codepoints;
mod internal;
mod late;
Expand Down Expand Up @@ -80,6 +81,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 methods::*;
Expand Down Expand Up @@ -178,6 +180,7 @@ macro_rules! late_lint_mod_passes {
$macro!(
$args,
[
ForLoopOverFallibles: ForLoopOverFallibles,
HardwiredLints: HardwiredLints,
ImproperCTypesDeclarations: ImproperCTypesDeclarations,
ImproperCTypesDefinitions: ImproperCTypesDefinitions,
Expand Down
1 change: 1 addition & 0 deletions library/core/tests/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
14 changes: 7 additions & 7 deletions src/test/ui/drop/dropck_legal_cycles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,7 @@ impl<'a> Children<'a> for HM<'a> {
where C: Context + PrePost<Self>, 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);
}
}
Expand All @@ -1032,7 +1032,7 @@ impl<'a> Children<'a> for VD<'a> {
where C: Context + PrePost<Self>, 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);
}
}
Expand All @@ -1047,7 +1047,7 @@ impl<'a> Children<'a> for VM<'a> {
where C: Context + PrePost<VM<'a>>
{
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);
}
}
Expand All @@ -1062,7 +1062,7 @@ impl<'a> Children<'a> for LL<'a> {
where C: Context + PrePost<LL<'a>>
{
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);
}
}
Expand All @@ -1077,7 +1077,7 @@ impl<'a> Children<'a> for BH<'a> {
where C: Context + PrePost<BH<'a>>
{
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);
}
}
Expand All @@ -1092,7 +1092,7 @@ impl<'a> Children<'a> for BTM<'a> {
where C: Context + PrePost<BTM<'a>>
{
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);
}
}
Expand All @@ -1107,7 +1107,7 @@ impl<'a> Children<'a> for BTS<'a> {
where C: Context + PrePost<BTS<'a>>
{
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);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/test/ui/issues/issue-30371.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// run-pass
#![allow(unreachable_code)]
#![allow(for_loop_over_fallibles)]
#![deny(unused_variables)]

fn main() {
Expand Down
43 changes: 43 additions & 0 deletions src/test/ui/lint/for_loop_over_fallibles.rs
Original file line number Diff line number Diff line change
@@ -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<impl Iterator, _>`, 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<impl Iterator, _>`
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<impl IntoIterator>`
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(())
}
Loading

0 comments on commit ec74ca0

Please sign in to comment.