forked from rust-lang/rust
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Rollup merge of rust-lang#99696 - WaffleLapkin:uplift, r=fee1-dead
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
Showing
13 changed files
with
365 additions
and
28 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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(()) | ||
| } |
Oops, something went wrong.