From f4b02aa3741299b8226f50ba63ec1f4e27a64d14 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Mon, 15 May 2023 11:20:04 -0500 Subject: [PATCH 1/5] fix #10776 --- .../src/mixed_read_write_in_expression.rs | 6 +-- tests/ui/diverging_sub_expression.rs | 4 ++ tests/ui/diverging_sub_expression.stderr | 40 ------------------- 3 files changed, 6 insertions(+), 44 deletions(-) diff --git a/clippy_lints/src/mixed_read_write_in_expression.rs b/clippy_lints/src/mixed_read_write_in_expression.rs index f0be7771bb1..2dfab259915 100644 --- a/clippy_lints/src/mixed_read_write_in_expression.rs +++ b/clippy_lints/src/mixed_read_write_in_expression.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_note}; use clippy_utils::{get_parent_expr, path_to_local, path_to_local_id}; use if_chain::if_chain; -use rustc_hir::intravisit::{walk_expr, Visitor}; +use rustc_hir::intravisit::{walk_block, walk_expr, Visitor}; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, Guard, HirId, Local, Node, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; @@ -155,6 +155,7 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { self.report_diverging_sub_expr(e); } }, + ExprKind::Block(block, ..) => walk_block(self, block), _ => { // do not lint expressions referencing objects of type `!`, as that required a // diverging expression @@ -163,9 +164,6 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { } self.maybe_walk_expr(e); } - fn visit_block(&mut self, _: &'tcx Block<'_>) { - // don't continue over blocks, LateLintPass already does that - } } /// Walks up the AST from the given write expression (`vis.write_expr`) looking diff --git a/tests/ui/diverging_sub_expression.rs b/tests/ui/diverging_sub_expression.rs index e8f992e6dde..1dd3f3b8bc4 100644 --- a/tests/ui/diverging_sub_expression.rs +++ b/tests/ui/diverging_sub_expression.rs @@ -1,5 +1,6 @@ #![warn(clippy::diverging_sub_expression)] #![allow(clippy::match_same_arms, clippy::overly_complex_bool_expr)] +#![allow(clippy::nonminimal_bool)] #[allow(clippy::empty_loop)] fn diverge() -> ! { loop {} @@ -35,6 +36,9 @@ fn foobar() { 99 => return, _ => true || panic!("boo"), }, + // lint blocks as well + 15 => true || { return; }, + 16 => false || { return; }, _ => true || break, }; } diff --git a/tests/ui/diverging_sub_expression.stderr b/tests/ui/diverging_sub_expression.stderr index 51a3b0d972e..e69de29bb2d 100644 --- a/tests/ui/diverging_sub_expression.stderr +++ b/tests/ui/diverging_sub_expression.stderr @@ -1,40 +0,0 @@ -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:19:10 - | -LL | b || diverge(); - | ^^^^^^^^^ - | - = note: `-D clippy::diverging-sub-expression` implied by `-D warnings` - -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:20:10 - | -LL | b || A.foo(); - | ^^^^^^^ - -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:29:26 - | -LL | 6 => true || return, - | ^^^^^^ - -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:30:26 - | -LL | 7 => true || continue, - | ^^^^^^^^ - -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:33:26 - | -LL | 3 => true || diverge(), - | ^^^^^^^^^ - -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:38:26 - | -LL | _ => true || break, - | ^^^^^ - -error: aborting due to 6 previous errors - From 5825b9e3e29c98ca0b8ae74cc5ece8a05747a416 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Mon, 15 May 2023 14:46:24 -0500 Subject: [PATCH 2/5] actually fix it --- .../src/mixed_read_write_in_expression.rs | 18 +++++++++++++++--- tests/ui/diverging_sub_expression.rs | 1 + tests/ui/never_loop.stderr | 10 +++++++++- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/mixed_read_write_in_expression.rs b/clippy_lints/src/mixed_read_write_in_expression.rs index 2dfab259915..942453498cd 100644 --- a/clippy_lints/src/mixed_read_write_in_expression.rs +++ b/clippy_lints/src/mixed_read_write_in_expression.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_note}; use clippy_utils::{get_parent_expr, path_to_local, path_to_local_id}; use if_chain::if_chain; -use rustc_hir::intravisit::{walk_block, walk_expr, Visitor}; +use rustc_hir::intravisit::{walk_expr, Visitor}; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, Guard, HirId, Local, Node, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; @@ -114,7 +114,7 @@ struct DivergenceVisitor<'a, 'tcx> { impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { fn maybe_walk_expr(&mut self, e: &'tcx Expr<'_>) { match e.kind { - ExprKind::Closure { .. } => {}, + ExprKind::Closure(..) | ExprKind::If(..) | ExprKind::Loop(..) => {}, ExprKind::Match(e, arms, _) => { self.visit_expr(e); for arm in arms { @@ -128,6 +128,7 @@ impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { _ => walk_expr(self, e), } } + fn report_diverging_sub_expr(&mut self, e: &Expr<'_>) { span_lint(self.cx, DIVERGING_SUB_EXPRESSION, e.span, "sub-expression diverges"); } @@ -136,6 +137,15 @@ impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { fn visit_expr(&mut self, e: &'tcx Expr<'_>) { match e.kind { + // fix #10776 + ExprKind::Block(block, ..) => { + if let Some(stmt) = block.stmts.first() && block.stmts.len() == 1 { + match stmt.kind { + StmtKind::Expr(e) | StmtKind::Semi(e) => self.visit_expr(e), + _ => {}, + } + } + }, ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => self.report_diverging_sub_expr(e), ExprKind::Call(func, _) => { let typ = self.cx.typeck_results().expr_ty(func); @@ -155,7 +165,6 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { self.report_diverging_sub_expr(e); } }, - ExprKind::Block(block, ..) => walk_block(self, block), _ => { // do not lint expressions referencing objects of type `!`, as that required a // diverging expression @@ -164,6 +173,9 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { } self.maybe_walk_expr(e); } + fn visit_block(&mut self, _: &'tcx Block<'_>) { + // don't continue over blocks, LateLintPass already does that + } } /// Walks up the AST from the given write expression (`vis.write_expr`) looking diff --git a/tests/ui/diverging_sub_expression.rs b/tests/ui/diverging_sub_expression.rs index 1dd3f3b8bc4..510eca21f71 100644 --- a/tests/ui/diverging_sub_expression.rs +++ b/tests/ui/diverging_sub_expression.rs @@ -22,6 +22,7 @@ fn main() { } #[allow(dead_code, unused_variables)] +#[rustfmt::skip] fn foobar() { loop { let x = match 5 { diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr index 704d448644e..b2eafb345e3 100644 --- a/tests/ui/never_loop.stderr +++ b/tests/ui/never_loop.stderr @@ -126,6 +126,14 @@ LL | | } LL | | } | |_____^ +error: sub-expression diverges + --> $DIR/never_loop.rs:247:17 + | +LL | break 'a; + | ^^^^^^^^ + | + = note: `-D clippy::diverging-sub-expression` implied by `-D warnings` + error: this loop never actually loops --> $DIR/never_loop.rs:278:13 | @@ -139,5 +147,5 @@ help: if you need the first element of the iterator, try writing LL | if let Some(_) = (0..20).next() { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -error: aborting due to 12 previous errors +error: aborting due to 13 previous errors From 0c545c7bccdb45055f8874fa2700afb1ecb69f1d Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Thu, 18 May 2023 22:13:44 -0500 Subject: [PATCH 3/5] also lint single expression blocks Update mixed_read_write_in_expression.rs Update diverging_sub_expression.stderr --- .../src/mixed_read_write_in_expression.rs | 8 +- tests/ui/diverging_sub_expression.rs | 9 ++ tests/ui/diverging_sub_expression.stderr | 84 +++++++++++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/mixed_read_write_in_expression.rs b/clippy_lints/src/mixed_read_write_in_expression.rs index 942453498cd..caa3463b864 100644 --- a/clippy_lints/src/mixed_read_write_in_expression.rs +++ b/clippy_lints/src/mixed_read_write_in_expression.rs @@ -139,7 +139,13 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { match e.kind { // fix #10776 ExprKind::Block(block, ..) => { - if let Some(stmt) = block.stmts.first() && block.stmts.len() == 1 { + if let Some(e) = block.expr { + self.visit_expr(e); + + return; + } + + if let [stmt] = block.stmts && block.stmts.len() == 1 { match stmt.kind { StmtKind::Expr(e) | StmtKind::Semi(e) => self.visit_expr(e), _ => {}, diff --git a/tests/ui/diverging_sub_expression.rs b/tests/ui/diverging_sub_expression.rs index 510eca21f71..3a4d66c5ba5 100644 --- a/tests/ui/diverging_sub_expression.rs +++ b/tests/ui/diverging_sub_expression.rs @@ -40,6 +40,15 @@ fn foobar() { // lint blocks as well 15 => true || { return; }, 16 => false || { return; }, + // ... and when it's a single expression + 17 => true || { return }, + 18 => false || { return }, + // ... but not when there's both an expression and a statement + 19 => true || { _ = 1; return }, + 20 => false || { _ = 1; return }, + // ... or multiple statements + 21 => true || { _ = 1; return; }, + 22 => false || { _ = 1; return; }, _ => true || break, }; } diff --git a/tests/ui/diverging_sub_expression.stderr b/tests/ui/diverging_sub_expression.stderr index e69de29bb2d..e6396de2bb7 100644 --- a/tests/ui/diverging_sub_expression.stderr +++ b/tests/ui/diverging_sub_expression.stderr @@ -0,0 +1,84 @@ +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:20:10 + | +LL | b || diverge(); + | ^^^^^^^^^ + | + = note: `-D clippy::diverging-sub-expression` implied by `-D warnings` + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:21:10 + | +LL | b || A.foo(); + | ^^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:31:26 + | +LL | 6 => true || return, + | ^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:32:26 + | +LL | 7 => true || continue, + | ^^^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:35:26 + | +LL | 3 => true || diverge(), + | ^^^^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:38:30 + | +LL | _ => true || panic!("boo"), + | ^^^^^^^^^^^^^ + | + = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:41:29 + | +LL | 15 => true || { return; }, + | ^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:42:30 + | +LL | 16 => false || { return; }, + | ^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:44:29 + | +LL | 17 => true || { return }, + | ^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:45:30 + | +LL | 18 => false || { return }, + | ^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:47:36 + | +LL | 19 => true || { _ = 1; return }, + | ^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:48:37 + | +LL | 20 => false || { _ = 1; return }, + | ^^^^^^ + +error: sub-expression diverges + --> $DIR/diverging_sub_expression.rs:52:26 + | +LL | _ => true || break, + | ^^^^^ + +error: aborting due to 13 previous errors + From c1c134a28828c21230bb54ca4c70d027725aa175 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Fri, 9 Jun 2023 11:43:58 -0500 Subject: [PATCH 4/5] ensure there are no stmts for expr check --- clippy_lints/src/mixed_read_write_in_expression.rs | 4 ++-- tests/ui/diverging_sub_expression.stderr | 14 +------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/mixed_read_write_in_expression.rs b/clippy_lints/src/mixed_read_write_in_expression.rs index caa3463b864..ac94820b40b 100644 --- a/clippy_lints/src/mixed_read_write_in_expression.rs +++ b/clippy_lints/src/mixed_read_write_in_expression.rs @@ -139,13 +139,13 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { match e.kind { // fix #10776 ExprKind::Block(block, ..) => { - if let Some(e) = block.expr { + if let Some(e) = block.expr && block.stmts.is_empty() { self.visit_expr(e); return; } - if let [stmt] = block.stmts && block.stmts.len() == 1 { + if let [stmt, rest @ ..] = block.stmts && rest.is_empty() { match stmt.kind { StmtKind::Expr(e) | StmtKind::Semi(e) => self.visit_expr(e), _ => {}, diff --git a/tests/ui/diverging_sub_expression.stderr b/tests/ui/diverging_sub_expression.stderr index e6396de2bb7..f121159ca3e 100644 --- a/tests/ui/diverging_sub_expression.stderr +++ b/tests/ui/diverging_sub_expression.stderr @@ -62,23 +62,11 @@ error: sub-expression diverges LL | 18 => false || { return }, | ^^^^^^ -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:47:36 - | -LL | 19 => true || { _ = 1; return }, - | ^^^^^^ - -error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:48:37 - | -LL | 20 => false || { _ = 1; return }, - | ^^^^^^ - error: sub-expression diverges --> $DIR/diverging_sub_expression.rs:52:26 | LL | _ => true || break, | ^^^^^ -error: aborting due to 13 previous errors +error: aborting due to 11 previous errors From 35aff1ae8608beb9637eed518bc4f2f57186df42 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Fri, 9 Jun 2023 15:32:42 -0500 Subject: [PATCH 5/5] refactor --- .../src/mixed_read_write_in_expression.rs | 20 +++++++------------ tests/ui/diverging_sub_expression.rs | 2 ++ tests/ui/diverging_sub_expression.stderr | 2 +- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/mixed_read_write_in_expression.rs b/clippy_lints/src/mixed_read_write_in_expression.rs index ac94820b40b..57ec3a1f1e6 100644 --- a/clippy_lints/src/mixed_read_write_in_expression.rs +++ b/clippy_lints/src/mixed_read_write_in_expression.rs @@ -138,19 +138,13 @@ impl<'a, 'tcx> Visitor<'tcx> for DivergenceVisitor<'a, 'tcx> { fn visit_expr(&mut self, e: &'tcx Expr<'_>) { match e.kind { // fix #10776 - ExprKind::Block(block, ..) => { - if let Some(e) = block.expr && block.stmts.is_empty() { - self.visit_expr(e); - - return; - } - - if let [stmt, rest @ ..] = block.stmts && rest.is_empty() { - match stmt.kind { - StmtKind::Expr(e) | StmtKind::Semi(e) => self.visit_expr(e), - _ => {}, - } - } + ExprKind::Block(block, ..) => match (block.stmts, block.expr) { + ([], Some(e)) => self.visit_expr(e), + ([stmt], None) => match stmt.kind { + StmtKind::Expr(e) | StmtKind::Semi(e) => self.visit_expr(e), + _ => {}, + }, + _ => {}, }, ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => self.report_diverging_sub_expr(e), ExprKind::Call(func, _) => { diff --git a/tests/ui/diverging_sub_expression.rs b/tests/ui/diverging_sub_expression.rs index 3a4d66c5ba5..9b1619baf0e 100644 --- a/tests/ui/diverging_sub_expression.rs +++ b/tests/ui/diverging_sub_expression.rs @@ -49,6 +49,8 @@ fn foobar() { // ... or multiple statements 21 => true || { _ = 1; return; }, 22 => false || { _ = 1; return; }, + 23 => true || { return; true }, + 24 => true || { return; true }, _ => true || break, }; } diff --git a/tests/ui/diverging_sub_expression.stderr b/tests/ui/diverging_sub_expression.stderr index f121159ca3e..243a5cf5369 100644 --- a/tests/ui/diverging_sub_expression.stderr +++ b/tests/ui/diverging_sub_expression.stderr @@ -63,7 +63,7 @@ LL | 18 => false || { return }, | ^^^^^^ error: sub-expression diverges - --> $DIR/diverging_sub_expression.rs:52:26 + --> $DIR/diverging_sub_expression.rs:54:26 | LL | _ => true || break, | ^^^^^