Do not remove semicolon if it changes the block type (#14103)

Removing the semicolon of the last statement of an expressionless block
may change the block type even if the statement's type is `()`. If the
block type is `!` because of a systematic early return, typing it as
`()` may make it incompatible with the expected type for the block (to
which `!` is cast).

Fix #14100

changelog: [`unnecessary_semicolon`]: do not remove semicolon if it
could change the block type from `!` to `()`

Author: dswij

clippy_lints/src/unnecessary_semicolon.rs

@@ -37,35 +37,33 @@ declare_clippy_lint! {
 
 #[derive(Default)]
 pub struct UnnecessarySemicolon {
-    last_statements: Vec<HirId>,
+    last_statements: Vec<(HirId, bool)>,
 }
 
 impl_lint_pass!(UnnecessarySemicolon => [UNNECESSARY_SEMICOLON]);
 
 impl UnnecessarySemicolon {
     /// Enter or leave a block, remembering the last statement of the block.
     fn handle_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>, enter: bool) {
-        // Up to edition 2021, removing the semicolon of the last statement of a block
-        // may result in the scrutinee temporary values to live longer than the block
-        // variables. To avoid this problem, we do not lint the last statement of an
-        // expressionless block.
-        if cx.tcx.sess.edition() <= Edition2021
-            && block.expr.is_none()
+        // The last statement of an expressionless block deserves a special treatment.
+        if block.expr.is_none()
             && let Some(last_stmt) = block.stmts.last()
         {
             if enter {
-                self.last_statements.push(last_stmt.hir_id);
+                let block_ty = cx.typeck_results().node_type(block.hir_id);
+                self.last_statements.push((last_stmt.hir_id, block_ty.is_unit()));
             } else {
                 self.last_statements.pop();
             }
         }
     }
 
-    /// Checks if `stmt` is the last statement in an expressionless block for edition ≤ 2021.
-    fn is_last_in_block(&self, stmt: &Stmt<'_>) -> bool {
+    /// Checks if `stmt` is the last statement in an expressionless block. In this case,
+    /// return `Some` with a boolean which is `true` if the block type is `()`.
+    fn is_last_in_block(&self, stmt: &Stmt<'_>) -> Option<bool> {
         self.last_statements
             .last()
-            .is_some_and(|last_stmt_id| last_stmt_id == &stmt.hir_id)
+            .and_then(|&(stmt_id, is_unit)| (stmt_id == stmt.hir_id).then_some(is_unit))
     }
 }
 
@@ -90,8 +88,22 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessarySemicolon {
             )
             && cx.typeck_results().expr_ty(expr) == cx.tcx.types.unit
         {
-            if self.is_last_in_block(stmt) && leaks_droppable_temporary_with_limited_lifetime(cx, expr) {
-                return;
+            if let Some(block_is_unit) = self.is_last_in_block(stmt) {
+                if cx.tcx.sess.edition() <= Edition2021 && leaks_droppable_temporary_with_limited_lifetime(cx, expr) {
+                    // The expression contains temporaries with limited lifetimes in edition lower than 2024. Those may
+                    // survive until after the end of the current scope instead of until the end of the statement, so do
+                    // not lint this situation.
+                    return;
+                }
+
+                if !block_is_unit {
+                    // Although the expression returns `()`, the block doesn't. This may happen if the expression
+                    // returns early in all code paths, such as a `return value` in the condition of an `if` statement,
+                    // in which case the block type would be `!`. Do not lint in this case, as the statement would
+                    // become the block expression; the block type would become `()` and this may not type correctly
+                    // if the expected type for the block is not `()`.
+                    return;
+                }
             }
 
             let semi_span = expr.span.shrink_to_hi().to(stmt.span.shrink_to_hi());

tests/ui/unnecessary_semicolon.edition2021.fixed

@@ -55,3 +55,9 @@ fn no_borrow_issue(a: u32, b: u32) {
         None => {},
     }
 }
+
+fn issue14100() -> bool {
+    // Removing the `;` would make the block type be `()` instead of `!`, and this could no longer be
+    // cast into the `bool` function return type.
+    if return true {};
+}

tests/ui/unnecessary_semicolon.edition2024.fixed

@@ -55,3 +55,9 @@ fn no_borrow_issue(a: u32, b: u32) {
         None => {},
     }
 }
+
+fn issue14100() -> bool {
+    // Removing the `;` would make the block type be `()` instead of `!`, and this could no longer be
+    // cast into the `bool` function return type.
+    if return true {};
+}

tests/ui/unnecessary_semicolon.rs

@@ -55,3 +55,9 @@ fn no_borrow_issue(a: u32, b: u32) {
         None => {},
     };
 }
+
+fn issue14100() -> bool {
+    // Removing the `;` would make the block type be `()` instead of `!`, and this could no longer be
+    // cast into the `bool` function return type.
+    if return true {};
+}