Skip to content

Check if if conditions always evaluate to true in never_loop #11005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 51 additions & 31 deletions clippy_lints/src/loops/never_loop.rs
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
use super::utils::make_iterator_snippet;
use super::NEVER_LOOP;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::consts::constant;
use clippy_utils::higher::ForLoop;
use clippy_utils::source::snippet;
use clippy_utils::{consts::Constant, diagnostics::span_lint_and_then};
use rustc_errors::Applicability;
use rustc_hir::{Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind};
use rustc_lint::LateContext;
use rustc_span::Span;
use std::iter::{once, Iterator};

pub(super) fn check(
cx: &LateContext<'_>,
block: &Block<'_>,
pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
block: &Block<'tcx>,
loop_id: HirId,
span: Span,
for_loop: Option<&ForLoop<'_>>,
) {
match never_loop_block(block, &mut Vec::new(), loop_id) {
match never_loop_block(cx, block, &mut Vec::new(), loop_id) {
NeverLoopResult::AlwaysBreak => {
span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| {
if let Some(ForLoop {
Expand Down Expand Up @@ -95,18 +96,23 @@ fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult, ignore_ids: &[HirI
}
}

fn never_loop_block(block: &Block<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: HirId) -> NeverLoopResult {
fn never_loop_block<'tcx>(
cx: &LateContext<'tcx>,
block: &Block<'tcx>,
ignore_ids: &mut Vec<HirId>,
main_loop_id: HirId,
) -> NeverLoopResult {
let iter = block
.stmts
.iter()
.filter_map(stmt_to_expr)
.chain(block.expr.map(|expr| (expr, None)));

iter.map(|(e, els)| {
let e = never_loop_expr(e, ignore_ids, main_loop_id);
let e = never_loop_expr(cx, e, ignore_ids, main_loop_id);
// els is an else block in a let...else binding
els.map_or(e, |els| {
combine_branches(e, never_loop_block(els, ignore_ids, main_loop_id), ignore_ids)
combine_branches(e, never_loop_block(cx, els, ignore_ids, main_loop_id), ignore_ids)
})
})
.fold(NeverLoopResult::Otherwise, combine_seq)
Expand All @@ -122,61 +128,72 @@ fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'t
}

#[allow(clippy::too_many_lines)]
fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: HirId) -> NeverLoopResult {
fn never_loop_expr<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'tcx>,
ignore_ids: &mut Vec<HirId>,
main_loop_id: HirId,
) -> NeverLoopResult {
match expr.kind {
ExprKind::Unary(_, e)
| ExprKind::Cast(e, _)
| ExprKind::Type(e, _)
| ExprKind::Field(e, _)
| ExprKind::AddrOf(_, _, e)
| ExprKind::Repeat(e, _)
| ExprKind::DropTemps(e) => never_loop_expr(e, ignore_ids, main_loop_id),
ExprKind::Let(let_expr) => never_loop_expr(let_expr.init, ignore_ids, main_loop_id),
ExprKind::Array(es) | ExprKind::Tup(es) => never_loop_expr_all(&mut es.iter(), ignore_ids, main_loop_id),
| ExprKind::DropTemps(e) => never_loop_expr(cx, e, ignore_ids, main_loop_id),
ExprKind::Let(let_expr) => never_loop_expr(cx, let_expr.init, ignore_ids, main_loop_id),
ExprKind::Array(es) | ExprKind::Tup(es) => never_loop_expr_all(cx, &mut es.iter(), ignore_ids, main_loop_id),
ExprKind::MethodCall(_, receiver, es, _) => never_loop_expr_all(
cx,
&mut std::iter::once(receiver).chain(es.iter()),
ignore_ids,
main_loop_id,
),
ExprKind::Struct(_, fields, base) => {
let fields = never_loop_expr_all(&mut fields.iter().map(|f| f.expr), ignore_ids, main_loop_id);
let fields = never_loop_expr_all(cx, &mut fields.iter().map(|f| f.expr), ignore_ids, main_loop_id);
if let Some(base) = base {
combine_seq(fields, never_loop_expr(base, ignore_ids, main_loop_id))
combine_seq(fields, never_loop_expr(cx, base, ignore_ids, main_loop_id))
} else {
fields
}
},
ExprKind::Call(e, es) => never_loop_expr_all(&mut once(e).chain(es.iter()), ignore_ids, main_loop_id),
ExprKind::Call(e, es) => never_loop_expr_all(cx, &mut once(e).chain(es.iter()), ignore_ids, main_loop_id),
ExprKind::Binary(_, e1, e2)
| ExprKind::Assign(e1, e2, _)
| ExprKind::AssignOp(_, e1, e2)
| ExprKind::Index(e1, e2) => never_loop_expr_all(&mut [e1, e2].iter().copied(), ignore_ids, main_loop_id),
| ExprKind::Index(e1, e2) => never_loop_expr_all(cx, &mut [e1, e2].iter().copied(), ignore_ids, main_loop_id),
ExprKind::Loop(b, _, _, _) => {
// Break can come from the inner loop so remove them.
absorb_break(never_loop_block(b, ignore_ids, main_loop_id))
absorb_break(never_loop_block(cx, b, ignore_ids, main_loop_id))
},
ExprKind::If(e, e2, e3) => {
let e1 = never_loop_expr(e, ignore_ids, main_loop_id);
let e2 = never_loop_expr(e2, ignore_ids, main_loop_id);
let e1 = never_loop_expr(cx, e, ignore_ids, main_loop_id);
let e2 = never_loop_expr(cx, e2, ignore_ids, main_loop_id);
// If we know the `if` condition evaluates to `true`, don't check everything past it; it
// should just return whatever's evaluated for `e1` and `e2` since `e3` is unreachable
if let Some(Constant::Bool(true)) = constant(cx, cx.typeck_results(), e) {
return combine_seq(e1, e2);
}
let e3 = e3.as_ref().map_or(NeverLoopResult::Otherwise, |e| {
never_loop_expr(e, ignore_ids, main_loop_id)
never_loop_expr(cx, e, ignore_ids, main_loop_id)
});
combine_seq(e1, combine_branches(e2, e3, ignore_ids))
},
ExprKind::Match(e, arms, _) => {
let e = never_loop_expr(e, ignore_ids, main_loop_id);
let e = never_loop_expr(cx, e, ignore_ids, main_loop_id);
if arms.is_empty() {
e
} else {
let arms = never_loop_expr_branch(&mut arms.iter().map(|a| a.body), ignore_ids, main_loop_id);
let arms = never_loop_expr_branch(cx, &mut arms.iter().map(|a| a.body), ignore_ids, main_loop_id);
combine_seq(e, arms)
}
},
ExprKind::Block(b, l) => {
if l.is_some() {
ignore_ids.push(b.hir_id);
}
let ret = never_loop_block(b, ignore_ids, main_loop_id);
let ret = never_loop_block(cx, b, ignore_ids, main_loop_id);
if l.is_some() {
ignore_ids.pop();
}
Expand All @@ -198,11 +215,11 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
// checks if break targets a block instead of a loop
ExprKind::Break(Destination { target_id: Ok(t), .. }, e) if ignore_ids.contains(&t) => e
.map_or(NeverLoopResult::IgnoreUntilEnd(t), |e| {
never_loop_expr(e, ignore_ids, main_loop_id)
never_loop_expr(cx, e, ignore_ids, main_loop_id)
}),
ExprKind::Break(_, e) | ExprKind::Ret(e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| {
combine_seq(
never_loop_expr(e, ignore_ids, main_loop_id),
never_loop_expr(cx, e, ignore_ids, main_loop_id),
NeverLoopResult::AlwaysBreak,
)
}),
Expand All @@ -211,12 +228,13 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
.iter()
.map(|(o, _)| match o {
InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => {
never_loop_expr(expr, ignore_ids, main_loop_id)
never_loop_expr(cx, expr, ignore_ids, main_loop_id)
},
InlineAsmOperand::Out { expr, .. } => {
never_loop_expr_all(&mut expr.iter().copied(), ignore_ids, main_loop_id)
never_loop_expr_all(cx, &mut expr.iter().copied(), ignore_ids, main_loop_id)
},
InlineAsmOperand::SplitInOut { in_expr, out_expr, .. } => never_loop_expr_all(
cx,
&mut once(*in_expr).chain(out_expr.iter().copied()),
ignore_ids,
main_loop_id,
Expand All @@ -236,22 +254,24 @@ fn never_loop_expr(expr: &Expr<'_>, ignore_ids: &mut Vec<HirId>, main_loop_id: H
}
}

fn never_loop_expr_all<'a, T: Iterator<Item = &'a Expr<'a>>>(
fn never_loop_expr_all<'tcx, T: Iterator<Item = &'tcx Expr<'tcx>>>(
cx: &LateContext<'tcx>,
es: &mut T,
ignore_ids: &mut Vec<HirId>,
main_loop_id: HirId,
) -> NeverLoopResult {
es.map(|e| never_loop_expr(e, ignore_ids, main_loop_id))
es.map(|e| never_loop_expr(cx, e, ignore_ids, main_loop_id))
.fold(NeverLoopResult::Otherwise, combine_seq)
}

fn never_loop_expr_branch<'a, T: Iterator<Item = &'a Expr<'a>>>(
fn never_loop_expr_branch<'tcx, T: Iterator<Item = &'tcx Expr<'tcx>>>(
cx: &LateContext<'tcx>,
e: &mut T,
ignore_ids: &mut Vec<HirId>,
main_loop_id: HirId,
) -> NeverLoopResult {
e.fold(NeverLoopResult::AlwaysBreak, |a, b| {
combine_branches(a, never_loop_expr(b, ignore_ids, main_loop_id), ignore_ids)
combine_branches(a, never_loop_expr(cx, b, ignore_ids, main_loop_id), ignore_ids)
})
}

Expand Down
4 changes: 3 additions & 1 deletion clippy_utils/src/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use if_chain::if_chain;
use rustc_ast::ast::{self, LitFloatType, LitKind};
use rustc_data_structures::sync::Lrc;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{BinOp, BinOpKind, Block, Expr, ExprKind, HirId, Item, ItemKind, Node, QPath, UnOp};
use rustc_hir::{AnonConst, BinOp, BinOpKind, Block, Expr, ExprKind, HirId, Item, ItemKind, Node, QPath, UnOp};
use rustc_lexer::tokenize;
use rustc_lint::LateContext;
use rustc_middle::mir;
Expand Down Expand Up @@ -344,6 +344,8 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
/// Simple constant folding: Insert an expression, get a constant or none.
pub fn expr(&mut self, e: &Expr<'_>) -> Option<Constant<'tcx>> {
match e.kind {
ExprKind::ConstBlock(AnonConst { body, .. }) => self.expr(self.lcx.tcx.hir().body(body).value),
ExprKind::DropTemps(e) => self.expr(e),
ExprKind::Path(ref qpath) => self.fetch_path(qpath, e.hir_id, self.typeck_results.expr_ty(e)),
ExprKind::Block(block, _) => self.block(block),
ExprKind::Lit(lit) => {
Expand Down
1 change: 1 addition & 0 deletions tests/ui/large_futures.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#![feature(generators)]
#![warn(clippy::large_futures)]
#![allow(clippy::never_loop)]
#![allow(clippy::future_not_send)]
#![allow(clippy::manual_async_fn)]

Expand Down
16 changes: 8 additions & 8 deletions tests/ui/large_futures.stderr
Original file line number Diff line number Diff line change
@@ -1,43 +1,43 @@
error: large future with a size of 16385 bytes
--> $DIR/large_futures.rs:10:9
--> $DIR/large_futures.rs:11:9
|
LL | big_fut([0u8; 1024 * 16]).await;
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Box::pin` on it: `Box::pin(big_fut([0u8; 1024 * 16]))`
|
= note: `-D clippy::large-futures` implied by `-D warnings`

error: large future with a size of 16386 bytes
--> $DIR/large_futures.rs:12:5
--> $DIR/large_futures.rs:13:5
|
LL | f.await
| ^ help: consider `Box::pin` on it: `Box::pin(f)`

error: large future with a size of 16387 bytes
--> $DIR/large_futures.rs:16:9
--> $DIR/large_futures.rs:17:9
|
LL | wait().await;
| ^^^^^^ help: consider `Box::pin` on it: `Box::pin(wait())`

error: large future with a size of 16387 bytes
--> $DIR/large_futures.rs:20:13
--> $DIR/large_futures.rs:21:13
|
LL | wait().await;
| ^^^^^^ help: consider `Box::pin` on it: `Box::pin(wait())`

error: large future with a size of 65540 bytes
--> $DIR/large_futures.rs:27:5
--> $DIR/large_futures.rs:28:5
|
LL | foo().await;
| ^^^^^ help: consider `Box::pin` on it: `Box::pin(foo())`

error: large future with a size of 49159 bytes
--> $DIR/large_futures.rs:28:5
--> $DIR/large_futures.rs:29:5
|
LL | calls_fut(fut).await;
| ^^^^^^^^^^^^^^ help: consider `Box::pin` on it: `Box::pin(calls_fut(fut))`

error: large future with a size of 65540 bytes
--> $DIR/large_futures.rs:40:5
--> $DIR/large_futures.rs:41:5
|
LL | / async {
LL | | let x = [0i32; 1024 * 16];
Expand All @@ -56,7 +56,7 @@ LL + })
|

error: large future with a size of 65540 bytes
--> $DIR/large_futures.rs:51:13
--> $DIR/large_futures.rs:52:13
|
LL | / async {
LL | | let x = [0i32; 1024 * 16];
Expand Down
38 changes: 38 additions & 0 deletions tests/ui/never_loop.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#![feature(inline_const)]
#![allow(
clippy::eq_op,
clippy::single_match,
unused_assignments,
unused_variables,
Expand Down Expand Up @@ -295,6 +297,42 @@ pub fn test24() {
}
}

// Do not lint, we can evaluate `true` to always succeed thus can short-circuit before the `return`
pub fn test25() {
loop {
'label: {
if const { true } {
break 'label;
}
return;
}
}
}

pub fn test26() {
loop {
'label: {
if 1 == 1 {
break 'label;
}
return;
}
}
}

pub fn test27() {
loop {
'label: {
let x = true;
// Lints because we cannot prove it's always `true`
if x {
break 'label;
}
return;
}
}
}

fn main() {
test1();
test2();
Expand Down
Loading