Auto merge of #122661 - estebank:assert-macro-span, r=<try>

Change the desugaring of `assert!` for better error output

Author: rust-bors[bot]

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,33 @@ 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));
+
+    // We wrap the `match` in a statement to limit the length of any borrows introduced in the
+    // condition.
+    cx.expr_block(cx.block(span, [cx.stmt_expr(cx.expr_match(span, cond, arms))].into()))
 }
 
 fn parse_assert<'a>(cx: &ExtCtxt<'a>, sp: Span, stream: TokenStream) -> PResult<'a, Assert> {

compiler/rustc_hir_typeck/src/demand.rs

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

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

@@ -1045,16 +1045,11 @@ 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, rules, span, tokens: None })
     }
 
-    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

@@ -457,7 +457,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}`"));
                     }
@@ -1618,8 +1618,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 {
@@ -2141,7 +2149,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),
@@ -2180,15 +2188,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.kind(), exp_found.found.kind()) {
+        Some(match (expected.kind(), found.kind()) {
             (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

@@ -130,13 +130,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

@@ -11,7 +11,7 @@ use rustc_ast::{BinOpKind, LitKind, RangeLimits};
 use rustc_data_structures::packed::Pu128;
 use rustc_data_structures::unhash::UnindexMap;
 use rustc_errors::{Applicability, Diag};
-use rustc_hir::{Block, Body, Expr, ExprKind, UnOp};
+use rustc_hir::{Body, Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 use rustc_span::source_map::Spanned;
@@ -135,12 +135,12 @@ fn assert_len_expr<'hir>(
     cx: &LateContext<'_>,
     expr: &'hir Expr<'hir>,
 ) -> Option<(LengthComparison, usize, &'hir Expr<'hir>)> {
-    let (cmp, asserted_len, slice_len) = 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
+    let (cmp, asserted_len, slice_len) = if let Some(
+        higher::IfLetOrMatch::Match(cond, [_, then], _)
+    ) = higher::IfLetOrMatch::parse(cx, expr)
+        && let ExprKind::Binary(bin_op, left, right) = &cond.kind
         // 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()
     {
         len_comparison(bin_op.node, left, right)?
     } else if let Some((macro_call, bin_op)) = first_node_macro_backtrace(cx, expr).find_map(|macro_call| {

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

@@ -94,7 +94,7 @@ fn main() {
     assert_eq!(a!(), "".is_empty());
     assert_eq!("".is_empty(), b!());
     assert_eq!(a, true);
-    assert!(b);
+    assert!(!!b);
     //~^ bool_assert_comparison
 
     assert_ne!("a".len(), 1);
@@ -122,7 +122,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);
     //~^ bool_assert_comparison
 
     debug_assert_ne!("a".len(), 1);
@@ -167,7 +167,7 @@ fn main() {
 
     use debug_assert_eq as renamed;
     renamed!(a, true);
-    debug_assert!(b);
+    debug_assert!(!!b);
     //~^ bool_assert_comparison
 
     let non_copy = NonCopy;