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 rust-lang#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,12 +64,14 @@ pub(crate) fn expand_assert<'cx>(
                 }),
             })),
         );
-        expr_if_not(cx, call_site_span, cond_expr, then, None)
+        expr_if_not(cx, call_site_span, cond_expr, then)
     }
     // If `generic_assert` is enabled, generates rich captured outputs
     //
     // FIXME(c410-f3r) See https://github.com/rust-lang/rust/issues/96949
     else if cx.ecfg.features.generic_assert() {
+        // FIXME(estebank): we use the condition the user passed without coercing to `bool` when
+        // `generic_assert` is enabled, but we could use `cond_expr` instead.
         context::Context::new(cx, call_site_span).build(cond_expr, panic_path())
     }
     // If `generic_assert` is not enabled, only outputs a literal "assertion failed: ..."
@@ -89,26 +91,30 @@ pub(crate) fn expand_assert<'cx>(
                 )),
             )],
         );
-        expr_if_not(cx, call_site_span, cond_expr, then, None)
+        expr_if_not(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 expr_if_not(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

@@ -697,7 +697,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

@@ -3859,7 +3859,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/pat.rs

@@ -1732,6 +1732,7 @@ impl<'a> Parser<'a> {
         })
     }
 
+    // `pub` because we use it from `rustc_builtin_macros`.
     pub(super) fn mk_pat_ident(&self, span: Span, ann: BindingMode, ident: Ident) -> P<Pat> {
         self.mk_pat(span, PatKind::Ident(ann, ident, None))
     }

compiler/rustc_parse/src/parser/stmt.rs

@@ -988,12 +988,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,
@@ -1004,7 +999,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 {
@@ -2108,7 +2116,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),
@@ -2147,15 +2155,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/alloc/tests/str.rs

@@ -2297,21 +2297,21 @@ fn utf8_chars() {
     assert_eq!(schs.len(), 4);
     assert_eq!(schs.iter().cloned().collect::<String>(), s);
 
-    assert!((from_utf8(s.as_bytes()).is_ok()));
+    assert!(from_utf8(s.as_bytes()).is_ok());
     // invalid prefix
-    assert!((!from_utf8(&[0x80]).is_ok()));
+    assert!(!from_utf8(&[0x80]).is_ok());
     // invalid 2 byte prefix
-    assert!((!from_utf8(&[0xc0]).is_ok()));
-    assert!((!from_utf8(&[0xc0, 0x10]).is_ok()));
+    assert!(!from_utf8(&[0xc0]).is_ok());
+    assert!(!from_utf8(&[0xc0, 0x10]).is_ok());
     // invalid 3 byte prefix
-    assert!((!from_utf8(&[0xe0]).is_ok()));
-    assert!((!from_utf8(&[0xe0, 0x10]).is_ok()));
-    assert!((!from_utf8(&[0xe0, 0xff, 0x10]).is_ok()));
+    assert!(!from_utf8(&[0xe0]).is_ok());
+    assert!(!from_utf8(&[0xe0, 0x10]).is_ok());
+    assert!(!from_utf8(&[0xe0, 0xff, 0x10]).is_ok());
     // invalid 4 byte prefix
-    assert!((!from_utf8(&[0xf0]).is_ok()));
-    assert!((!from_utf8(&[0xf0, 0x10]).is_ok()));
-    assert!((!from_utf8(&[0xf0, 0xff, 0x10]).is_ok()));
-    assert!((!from_utf8(&[0xf0, 0xff, 0xff, 0x10]).is_ok()));
+    assert!(!from_utf8(&[0xf0]).is_ok());
+    assert!(!from_utf8(&[0xf0, 0x10]).is_ok());
+    assert!(!from_utf8(&[0xf0, 0xff, 0x10]).is_ok());
+    assert!(!from_utf8(&[0xf0, 0xff, 0xff, 0x10]).is_ok());
 }
 
 #[test]

library/coretests/tests/bool.rs

@@ -71,14 +71,14 @@ fn test_bool() {
 #[test]
 pub fn test_bool_not() {
     if !false {
-        assert!((true));
+        assert!(true);
     } else {
-        assert!((false));
+        assert!(false);
     }
     if !true {
-        assert!((false));
+        assert!(false);
     } else {
-        assert!((true));
+        assert!(true);
     }
 }
 

library/coretests/tests/ptr.rs

@@ -42,11 +42,11 @@ fn test() {
         let mut v1 = vec![0u16, 0u16, 0u16];
 
         copy(v0.as_ptr().offset(1), v1.as_mut_ptr().offset(1), 1);
-        assert!((v1[0] == 0u16 && v1[1] == 32001u16 && v1[2] == 0u16));
+        assert!(v1[0] == 0u16 && v1[1] == 32001u16 && v1[2] == 0u16);
         copy(v0.as_ptr().offset(2), v1.as_mut_ptr(), 1);
-        assert!((v1[0] == 32002u16 && v1[1] == 32001u16 && v1[2] == 0u16));
+        assert!(v1[0] == 32002u16 && v1[1] == 32001u16 && v1[2] == 0u16);
         copy(v0.as_ptr(), v1.as_mut_ptr().offset(2), 1);
-        assert!((v1[0] == 32002u16 && v1[1] == 32001u16 && v1[2] == 32000u16));
+        assert!(v1[0] == 32002u16 && v1[1] == 32001u16 && v1[2] == 32000u16);
     }
 }
 

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/istr.rs

@@ -5,7 +5,7 @@ fn test_stack_assign() {
     let t: String = "a".to_string();
     assert_eq!(s, t);
     let u: String = "b".to_string();
-    assert!((s != u));
+    assert!(s != u);
 }
 
 #[test]
@@ -19,7 +19,7 @@ fn test_heap_assign() {
     let t: String = "a big ol' string".to_string();
     assert_eq!(s, t);
     let u: String = "a bad ol' string".to_string();
-    assert!((s != u));
+    assert!(s != u);
 }
 
 #[test]

library/std/tests/seq-compare.rs

@@ -1,15 +1,15 @@
 #[test]
 fn seq_compare() {
-    assert!(("hello".to_string() < "hellr".to_string()));
-    assert!(("hello ".to_string() > "hello".to_string()));
-    assert!(("hello".to_string() != "there".to_string()));
-    assert!((vec![1, 2, 3, 4] > vec![1, 2, 3]));
-    assert!((vec![1, 2, 3] < vec![1, 2, 3, 4]));
-    assert!((vec![1, 2, 4, 4] > vec![1, 2, 3, 4]));
-    assert!((vec![1, 2, 3, 4] < vec![1, 2, 4, 4]));
-    assert!((vec![1, 2, 3] <= vec![1, 2, 3]));
-    assert!((vec![1, 2, 3] <= vec![1, 2, 3, 3]));
-    assert!((vec![1, 2, 3, 4] > vec![1, 2, 3]));
+    assert!("hello".to_string() < "hellr".to_string());
+    assert!("hello ".to_string() > "hello".to_string());
+    assert!("hello".to_string() != "there".to_string());
+    assert!(vec![1, 2, 3, 4] > vec![1, 2, 3]);
+    assert!(vec![1, 2, 3] < vec![1, 2, 3, 4]);
+    assert!(vec![1, 2, 4, 4] > vec![1, 2, 3, 4]);
+    assert!(vec![1, 2, 3, 4] < vec![1, 2, 4, 4]);
+    assert!(vec![1, 2, 3] <= vec![1, 2, 3]);
+    assert!(vec![1, 2, 3] <= vec![1, 2, 3, 3]);
+    assert!(vec![1, 2, 3, 4] > vec![1, 2, 3]);
     assert_eq!(vec![1, 2, 3], vec![1, 2, 3]);
-    assert!((vec![1, 2, 3] != vec![1, 1, 3]));
+    assert!(vec![1, 2, 3] != vec![1, 1, 3]);
 }

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,