Change the desugaring of assert! for better error output

In the desugaring of `assert!`, we now expand to a `match` expression
instead of `if !cond {..}`.

The span of incorrect conditions will point only at the expression, and not
the whole `assert!` invocation.

```
error[E0308]: mismatched types
  --> $DIR/issue-14091.rs:2:13
   |
LL |     assert!(1,1);
   |             ^ expected `bool`, found integer
```

We no longer mention the expression needing to implement the `Not` trait.

```
error[E0308]: mismatched types
  --> $DIR/issue-14091-2.rs:15:13
   |
LL |     assert!(x, x);
   |             ^ expected `bool`, found `BytePos`
```

`assert!(val)` now desugars to:

```rust
match val {
    true => {},
    _ => $crate::panic::panic_2021!(),
}
```

Fix #122159.

We make some minor changes to some diagnostics to avoid span overlap on
type mismatch or inverted "expected"/"found" on type errors.

We remove some unnecessary parens from core, alloc and miri.

Author: estebank

compiler/rustc_builtin_macros/src/assert.rs

@@ -1,9 +1,9 @@
 mod context;
 
 use rustc_ast::ptr::P;
-use rustc_ast::token::Delimiter;
+use rustc_ast::token::{self, Delimiter};
 use rustc_ast::tokenstream::{DelimSpan, TokenStream};
-use rustc_ast::{DelimArgs, Expr, ExprKind, MacCall, Path, PathSegment, UnOp, token};
+use rustc_ast::{DelimArgs, Expr, ExprKind, MacCall, Path, PathSegment};
 use rustc_ast_pretty::pprust;
 use rustc_errors::PResult;
 use rustc_expand::base::{DummyResult, ExpandResult, ExtCtxt, MacEager, MacroExpanderResult};
@@ -30,7 +30,7 @@ pub(crate) fn expand_assert<'cx>(
 
     // `core::panic` and `std::panic` are different macros, so we use call-site
     // context to pick up whichever is currently in scope.
-    let call_site_span = cx.with_call_site_ctxt(span);
+    let call_site_span = cx.with_call_site_ctxt(cond_expr.span);
 
     let panic_path = || {
         if use_panic_2021(span) {
@@ -64,7 +64,7 @@ pub(crate) fn expand_assert<'cx>(
                 }),
             })),
         );
-        expr_if_not(cx, call_site_span, cond_expr, then, None)
+        assert_cond_check(cx, call_site_span, cond_expr, then)
     }
     // If `generic_assert` is enabled, generates rich captured outputs
     //
@@ -89,26 +89,30 @@ pub(crate) fn expand_assert<'cx>(
                 )),
             )],
         );
-        expr_if_not(cx, call_site_span, cond_expr, then, None)
+        assert_cond_check(cx, call_site_span, cond_expr, then)
     };
 
     ExpandResult::Ready(MacEager::expr(expr))
 }
 
+/// `assert!($cond_expr, $custom_message)`
 struct Assert {
     cond_expr: P<Expr>,
     custom_message: Option<TokenStream>,
 }
 
-// if !{ ... } { ... } else { ... }
-fn expr_if_not(
-    cx: &ExtCtxt<'_>,
-    span: Span,
-    cond: P<Expr>,
-    then: P<Expr>,
-    els: Option<P<Expr>>,
-) -> P<Expr> {
-    cx.expr_if(span, cx.expr(span, ExprKind::Unary(UnOp::Not, cond)), then, els)
+/// `match <cond> { true => {} _ => <then> }`
+fn assert_cond_check(cx: &ExtCtxt<'_>, span: Span, cond: P<Expr>, then: P<Expr>) -> P<Expr> {
+    // Instead of expanding to `if !<cond> { <then> }`, we expand to
+    // `match <cond> { true => {} _ <then> }`.
+    // This allows us to always complain about mismatched types instead of "cannot apply unary
+    // operator `!` to type `X`" when passing an invalid `<cond>`, while also allowing `<cond>` to
+    // be `&true`.
+    let els = cx.expr_block(cx.block(span, thin_vec![]));
+    let mut arms = thin_vec![];
+    arms.push(cx.arm(span, cx.pat_lit(span, cx.expr_bool(span, true)), els));
+    arms.push(cx.arm(span, cx.pat_wild(span), then));
+    cx.expr_match(span, cond, arms)
 }
 
 fn parse_assert<'a>(cx: &ExtCtxt<'a>, sp: Span, stream: TokenStream) -> PResult<'a, Assert> {

compiler/rustc_hir_typeck/src/demand.rs

@@ -695,7 +695,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ) {
         match (self.tcx.parent_hir_node(expr.hir_id), error) {
             (hir::Node::LetStmt(hir::LetStmt { ty: Some(ty), init: Some(init), .. }), _)
-                if init.hir_id == expr.hir_id =>
+                if init.hir_id == expr.hir_id && !ty.span.source_equal(init.span) =>
             {
                 // Point at `let` assignment type.
                 err.span_label(ty.span, "expected due to this");

compiler/rustc_parse/src/parser/expr.rs

@@ -3822,7 +3822,7 @@ impl<'a> Parser<'a> {
         P(Expr { kind, span, attrs, id: DUMMY_NODE_ID, tokens: None })
     }
 
-    pub(crate) fn mk_expr(&self, span: Span, kind: ExprKind) -> P<Expr> {
+    pub fn mk_expr(&self, span: Span, kind: ExprKind) -> P<Expr> {
         self.mk_expr_with_attrs(span, kind, AttrVec::new())
     }
 

compiler/rustc_parse/src/parser/stmt.rs

@@ -994,12 +994,7 @@ impl<'a> Parser<'a> {
         Ok(Some(stmt))
     }
 
-    pub(super) fn mk_block(
-        &self,
-        stmts: ThinVec<Stmt>,
-        rules: BlockCheckMode,
-        span: Span,
-    ) -> P<Block> {
+    pub fn mk_block(&self, stmts: ThinVec<Stmt>, rules: BlockCheckMode, span: Span) -> P<Block> {
         P(Block {
             stmts,
             id: DUMMY_NODE_ID,
@@ -1010,7 +1005,7 @@ impl<'a> Parser<'a> {
         })
     }
 
-    pub(super) fn mk_stmt(&self, span: Span, kind: StmtKind) -> Stmt {
+    pub fn mk_stmt(&self, span: Span, kind: StmtKind) -> Stmt {
         Stmt { id: DUMMY_NODE_ID, kind, span }
     }
 

compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs

@@ -456,7 +456,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
                             span,
                             format!("this is an iterator with items of type `{}`", args.type_at(0)),
                         );
-                    } else {
+                    } else if !span.overlaps(cause.span) {
                         let expected_ty = self.tcx.short_string(expected_ty, err.long_ty_path());
                         err.span_label(span, format!("this expression has type `{expected_ty}`"));
                     }
@@ -1584,8 +1584,16 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
         {
             let e = self.tcx.erase_regions(e);
             let f = self.tcx.erase_regions(f);
-            let expected = with_forced_trimmed_paths!(e.sort_string(self.tcx));
-            let found = with_forced_trimmed_paths!(f.sort_string(self.tcx));
+            let mut expected = with_forced_trimmed_paths!(e.sort_string(self.tcx));
+            let mut found = with_forced_trimmed_paths!(f.sort_string(self.tcx));
+            if let Some(def_id) = cause.span.ctxt().outer_expn_data().macro_def_id
+                && self.tcx.is_diagnostic_item(sym::assert_macro, def_id)
+            {
+                // When the type error comes from `assert!()`, the cause and effect are reversed
+                // because that macro expands to `match val { false => {panic!()}, _ => {} }`, which
+                // would say something like "expected `Type`, found `bool`", confusing the user.
+                (found, expected) = (expected, found);
+            }
             if expected == found {
                 label_or_note(span, terr.to_string(self.tcx));
             } else {
@@ -2107,7 +2115,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
     ) -> Option<(DiagStyledString, DiagStyledString)> {
         match values {
             ValuePairs::Regions(exp_found) => self.expected_found_str(exp_found),
-            ValuePairs::Terms(exp_found) => self.expected_found_str_term(exp_found, file),
+            ValuePairs::Terms(exp_found) => self.expected_found_str_term(cause, exp_found, file),
             ValuePairs::Aliases(exp_found) => self.expected_found_str(exp_found),
             ValuePairs::ExistentialTraitRef(exp_found) => self.expected_found_str(exp_found),
             ValuePairs::ExistentialProjection(exp_found) => self.expected_found_str(exp_found),
@@ -2146,15 +2154,27 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
 
     fn expected_found_str_term(
         &self,
+        cause: &ObligationCause<'tcx>,
         exp_found: ty::error::ExpectedFound<ty::Term<'tcx>>,
         path: &mut Option<PathBuf>,
     ) -> Option<(DiagStyledString, DiagStyledString)> {
         let exp_found = self.resolve_vars_if_possible(exp_found);
         if exp_found.references_error() {
             return None;
         }
+        let (mut expected, mut found) = (exp_found.expected, exp_found.found);
+        if let Some(def_id) = cause.span.ctxt().outer_expn_data().macro_def_id
+            && self.tcx.is_diagnostic_item(sym::assert_macro, def_id)
+        {
+            // When the type error comes from `assert!()`, the cause and effect are reversed
+            // because that macro expands to `match val { false => {panic!()}, _ => {} }`, which
+            // would say something like
+            // = note: expected `Type`
+            //            found `bool`"
+            (expected, found) = (found, expected);
+        }
 
-        Some(match (exp_found.expected.unpack(), exp_found.found.unpack()) {
+        Some(match (expected.unpack(), found.unpack()) {
             (ty::TermKind::Ty(expected), ty::TermKind::Ty(found)) => {
                 let (mut exp, mut fnd) = self.cmp(expected, found);
                 // Use the terminal width as the basis to determine when to compress the printed

library/coretests/tests/tuple.rs

@@ -37,7 +37,7 @@ fn test_partial_ord() {
     assert!(!((1.0f64, 2.0f64) <= (f64::NAN, 3.0)));
     assert!(!((1.0f64, 2.0f64) > (f64::NAN, 3.0)));
     assert!(!((1.0f64, 2.0f64) >= (f64::NAN, 3.0)));
-    assert!(((1.0f64, 2.0f64) < (2.0, f64::NAN)));
+    assert!((1.0f64, 2.0f64) < (2.0, f64::NAN));
     assert!(!((2.0f64, 2.0f64) < (2.0, f64::NAN)));
 }
 

library/std/tests/env.rs

@@ -16,7 +16,7 @@ fn test_self_exe_path() {
 
 #[test]
 fn test() {
-    assert!((!Path::new("test-path").is_absolute()));
+    assert!(!Path::new("test-path").is_absolute());
 
     #[cfg(not(target_env = "sgx"))]
     current_dir().unwrap();

src/tools/clippy/clippy_lints/src/bool_assert_comparison.rs

@@ -129,13 +129,27 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
 
                 let mut suggestions = vec![(name_span, non_eq_mac.to_string()), (lit_span, String::new())];
 
-                if bool_value ^ eq_macro {
-                    let Some(sugg) = Sugg::hir_opt(cx, non_lit_expr) else {
-                        return;
-                    };
-                    suggestions.push((non_lit_expr.span, (!sugg).to_string()));
+                if let ty::Bool = non_lit_ty.kind() {
+                    if bool_value ^ eq_macro {
+                        let Some(sugg) = Sugg::hir_opt(cx, non_lit_expr) else {
+                            return;
+                        };
+                        suggestions.push((non_lit_expr.span, (!sugg).to_string()));
+                    }
+                } else {
+                    // If we have a `value` that is *not* `bool` but that `!value` *is*, we suggest
+                    // `!!value`.
+                    suggestions.push((
+                        non_lit_expr.span.shrink_to_lo(),
+                        if bool_value ^ eq_macro {
+                            "!".to_string()
+                        } else {
+                            "!!".to_string()
+                        },
+                    ));
                 }
 
+
                 diag.multipart_suggestion(
                     format!("replace it with `{non_eq_mac}!(..)`"),
                     suggestions,

src/tools/clippy/clippy_lints/src/missing_asserts_for_indexing.rs

@@ -10,7 +10,7 @@ use rustc_ast::{LitKind, RangeLimits};
 use rustc_data_structures::packed::Pu128;
 use rustc_data_structures::unhash::UnindexMap;
 use rustc_errors::{Applicability, Diag};
-use rustc_hir::{BinOp, Block, Body, Expr, ExprKind, UnOp};
+use rustc_hir::{BinOp, Body, Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 use rustc_span::source_map::Spanned;
@@ -134,18 +134,16 @@ fn assert_len_expr<'hir>(
     cx: &LateContext<'_>,
     expr: &'hir Expr<'hir>,
 ) -> Option<(LengthComparison, usize, &'hir Expr<'hir>)> {
-    if let Some(higher::If { cond, then, .. }) = higher::If::hir(expr)
-        && let ExprKind::Unary(UnOp::Not, condition) = &cond.kind
-        && let ExprKind::Binary(bin_op, left, right) = &condition.kind
+    if let higher::IfLetOrMatch::Match(cond, [_, then], _) = higher::IfLetOrMatch::parse(cx, expr)?
+        && let ExprKind::Binary(bin_op, left, right) = &cond.kind
 
         && let Some((cmp, asserted_len, slice_len)) = len_comparison(*bin_op, left, right)
         && let ExprKind::MethodCall(method, recv, [], _) = &slice_len.kind
         && cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_slice()
         && method.ident.name == sym::len
 
         // check if `then` block has a never type expression
-        && let ExprKind::Block(Block { expr: Some(then_expr), .. }, _) = then.kind
-        && cx.typeck_results().expr_ty(then_expr).is_never()
+        && cx.typeck_results().expr_ty(then.body).is_never()
     {
         Some((cmp, asserted_len, recv))
     } else {
@@ -337,21 +335,21 @@ fn report_indexes(cx: &LateContext<'_>, map: &UnindexMap<u64, Vec<IndexEntry<'_>
                         // `v.len() < 5` and `v.len() <= 5` does nothing in terms of bounds checks.
                         // The user probably meant `v.len() > 5`
                         LengthComparison::LengthLessThanInt | LengthComparison::LengthLessThanOrEqualInt => Some(
-                            format!("assert!({}.len() > {highest_index})", snippet(cx, slice.span, "..")),
+                            format!("{}.len() > {highest_index}", snippet(cx, slice.span, "..")),
                         ),
                         // `5 < v.len()` == `v.len() > 5`
                         LengthComparison::IntLessThanLength if asserted_len < highest_index => Some(format!(
-                            "assert!({}.len() > {highest_index})",
+                            "{}.len() > {highest_index}",
                             snippet(cx, slice.span, "..")
                         )),
                         // `5 <= v.len() == `v.len() >= 5`
                         LengthComparison::IntLessThanOrEqualLength if asserted_len <= highest_index => Some(format!(
-                            "assert!({}.len() > {highest_index})",
+                            "{}.len() > {highest_index}",
                             snippet(cx, slice.span, "..")
                         )),
                         // `highest_index` here is rather a length, so we need to add 1 to it
                         LengthComparison::LengthEqualInt if asserted_len < highest_index + 1 => Some(format!(
-                            "assert!({}.len() == {})",
+                            "{}.len() == {}",
                             snippet(cx, slice.span, ".."),
                             highest_index + 1
                         )),

src/tools/clippy/tests/ui/bool_assert_comparison.fixed

@@ -91,7 +91,7 @@ fn main() {
     assert_eq!(a!(), "".is_empty());
     assert_eq!("".is_empty(), b!());
     assert_eq!(a, true);
-    assert!(b);
+    assert!(!!b);
 
     assert_ne!("a".len(), 1);
     assert!("a".is_empty());
@@ -111,7 +111,7 @@ fn main() {
     debug_assert_eq!(a!(), "".is_empty());
     debug_assert_eq!("".is_empty(), b!());
     debug_assert_eq!(a, true);
-    debug_assert!(b);
+    debug_assert!(!!b);
 
     debug_assert_ne!("a".len(), 1);
     debug_assert!("a".is_empty());
@@ -143,7 +143,7 @@ fn main() {
 
     use debug_assert_eq as renamed;
     renamed!(a, true);
-    debug_assert!(b);
+    debug_assert!(!!b);
 
     let non_copy = NonCopy;
     assert_eq!(non_copy, true);

src/tools/clippy/tests/ui/bool_assert_comparison.stderr

@@ -45,7 +45,7 @@ LL |     assert_eq!(b, true);
 help: replace it with `assert!(..)`
    |
 LL -     assert_eq!(b, true);
-LL +     assert!(b);
+LL +     assert!(!!b);
    |
 
 error: used `assert_ne!` with a literal bool
@@ -141,7 +141,7 @@ LL |     debug_assert_eq!(b, true);
 help: replace it with `debug_assert!(..)`
    |
 LL -     debug_assert_eq!(b, true);
-LL +     debug_assert!(b);
+LL +     debug_assert!(!!b);
    |
 
 error: used `debug_assert_ne!` with a literal bool
@@ -297,7 +297,7 @@ LL |     renamed!(b, true);
 help: replace it with `debug_assert!(..)`
    |
 LL -     renamed!(b, true);
-LL +     debug_assert!(b);
+LL +     debug_assert!(!!b);
    |
 
 error: used `assert_eq!` with a literal bool

src/tools/clippy/tests/ui/const_is_empty.stderr

@@ -157,11 +157,17 @@ error: this expression always evaluates to true
 LL |     let _ = val.is_empty();
    |             ^^^^^^^^^^^^^^
 
+error: this expression always evaluates to true
+  --> tests/ui/const_is_empty.rs:181:17
+   |
+LL |         assert!(EMPTY_STR.is_empty());
+   |                 ^^^^^^^^^^^^^^^^^^^^
+
 error: this expression always evaluates to true
   --> tests/ui/const_is_empty.rs:185:9
    |
 LL |         EMPTY_STR.is_empty();
    |         ^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 27 previous errors
+error: aborting due to 28 previous errors