Improve error message for assert!() macro in functions returning bool

[enthropy7]

Detect assert!() macro in error messages and provide clearer diagnostics

When assert!() is used in a function returning a value, show a message explaining that assert!() doesn't return a value, instead of the generic "if may be missing an else clause" error.

Fixes #151446

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Kivooeo]

thanks @mu001999 for this review!

but i think that this solving very different problem than original issue was about

i had a quick talk with issue author about it and we agreed that the problem lies in the error message itself

View changes since this review

[Kivooeo]

r? me

[Kivooeo]

i'm pretty sure we could simplify this by a lot like this, what do you think about this

diff --git a/compiler/rustc_hir_typeck/src/_match.rs b/compiler/rustc_hir_typeck/src/_match.rs
index 09f780c6c9e..48f36d4277b 100644
--- a/compiler/rustc_hir_typeck/src/_match.rs
+++ b/compiler/rustc_hir_typeck/src/_match.rs
@@ -291,6 +291,13 @@ pub(super) fn if_fallback_coercion(
         error
     }
 
+    fn is_from_assert_macro(&self, span: Span) -> bool {
+        span.ctxt().outer_expn_data().macro_def_id.is_some_and(|def_id| {
+            self.tcx.is_diagnostic_item(sym::assert_macro, def_id)
+                || self.tcx.is_diagnostic_item(sym::debug_assert_macro, def_id)
+        })
+    }
+
     /// Explain why `if` expressions without `else` evaluate to `()` and detect likely irrefutable
     /// `if let PAT = EXPR {}` expressions that could be turned into `let PAT = EXPR;`.
     fn explain_if_expr(
@@ -302,26 +309,6 @@ fn explain_if_expr(
         then_expr: &'tcx hir::Expr<'tcx>,
         error: &mut bool,
     ) {
-        // Check if this `if` expression comes from an `assert!()` or `debug_assert!()` macro expansion
-        // Walk the macro expansion chain to find the root assert macro, similar to how panic_call does it
-        let mut is_assert_macro = false;
-        let mut expn = if_span.ctxt().outer_expn_data();
-        loop {
-            if let Some(def_id) = expn.macro_def_id {
-                if self.tcx.is_diagnostic_item(sym::assert_macro, def_id)
-                    || self.tcx.is_diagnostic_item(sym::debug_assert_macro, def_id)
-                {
-                    is_assert_macro = true;
-                    break;
-                }
-            }
-            let parent = expn.call_site.ctxt().outer_expn_data();
-            if parent.macro_def_id.is_none() {
-                break;
-            }
-            expn = parent;
-        }
-
         if let Some((if_span, msg)) = ret_reason {
             err.span_label(if_span, msg);
         } else if let ExprKind::Block(block, _) = then_expr.kind
@@ -329,6 +316,7 @@ fn explain_if_expr(
         {
             err.span_label(expr.span, "found here");
         }
+        let is_assert_macro = self.is_from_assert_macro(if_span);`
 
         if is_assert_macro {
             err.primary_message("mismatched types");

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Kivooeo]

also please consider @mu001999 suggestion about other macros like assert_eq , loop suggestion is not relevant with this solution

[Kivooeo]

and also please add test like this to make sure it works fine with nested macros

macro_rules! g {
    () => {
        f!()
    };
}

macro_rules! f {
    () => {
        assert!(1 < 2)
    };
}

fn f() -> bool {
    g!()
}

[Kivooeo]

I appreciate you implementing the suggestions quickly, but I'd like to have more of a discussion. When I ask "what do you think about this", I'm genuinely interested in your thoughts - whether you see any issues with the approach, have alternative ideas, or questions about why we're doing it this way

Silent force-pushes make it hard to know if you understand the changes or just applying them mechanically. Could we have a bit more back-and-forth? It helps both of us and makes the review process more valuable

Code review is as much about collaboration and learning as it is about the code itself - that's what makes open source work well. I'd love to see more discussion from you in reviews in future PRs

View changes since this review

[enthropy7]

I appreciate you implementing the suggestions quickly, but I'd like to have more of a discussion. When I ask "what do you think about this", I'm genuinely interested in your thoughts - whether you see any issues with the approach, have alternative ideas, or questions about why we're doing it this way

Silent force-pushes make it hard to know if you understand the changes or just applying them mechanically. Could we have a bit more back-and-forth? It helps both of us and makes the review process more valuable

Code review is as much about collaboration and learning as it is about the code itself - that's what makes open source work well. I'd love to see more discussion from you in reviews in future PRs

Took a note. In this case - all corrections were completely right at to the place. We would solve a totally different problem without your help. Also i'm a little newbie in compile coding, so, my decisions may be not the most effective and idiomatic one, also i trust more to experienced people. That's why i'm a little shy to discuss some changes :3

[Kivooeo]

you need to run ./x test ui --bless

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Kivooeo]

r=me when ci green

@bors delegate+

[rust-bors]

✌️ @enthropy7, you can now approve this pull request!

If @Kivooeo told you to "r=me" after making some further change, then please make that change and post @bors r=Kivooeo.

[enthropy7]

@bors r+

[rust-bors]

📌 Commit 95b58ac has been approved by enthropy7

It is now in the queue for this repository.