Skip to content

Commit a6af700

Browse files
estebankestebank
committed
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.
1 parent 8c39296 commit a6af700

28 files changed

+190
-121
lines changed

‎compiler/rustc_builtin_macros/src/assert.rs

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
mod context;
22

33
use rustc_ast::ptr::P;
4-
use rustc_ast::token::Delimiter;
4+
use rustc_ast::token::{self, Delimiter};
55
use rustc_ast::tokenstream::{DelimSpan, TokenStream};
6-
use rustc_ast::{DelimArgs, Expr, ExprKind, MacCall, Path, PathSegment, UnOp, token};
6+
use rustc_ast::{DelimArgs, Expr, ExprKind, MacCall, Path, PathSegment};
77
use rustc_ast_pretty::pprust;
88
use rustc_errors::PResult;
99
use rustc_expand::base::{DummyResult, ExpandResult, ExtCtxt, MacEager, MacroExpanderResult};
@@ -30,7 +30,7 @@ pub(crate) fn expand_assert<'cx>(
3030

3131
// `core::panic` and `std::panic` are different macros, so we use call-site
3232
// context to pick up whichever is currently in scope.
33-
let call_site_span = cx.with_call_site_ctxt(span);
33+
let call_site_span = cx.with_call_site_ctxt(cond_expr.span);
3434

3535
let panic_path = || {
3636
if use_panic_2021(span) {
@@ -64,7 +64,7 @@ pub(crate) fn expand_assert<'cx>(
6464
}),
6565
})),
6666
);
67-
expr_if_not(cx, call_site_span, cond_expr, then, None)
67+
assert_cond_check(cx, call_site_span, cond_expr, then)
6868
}
6969
// If `generic_assert` is enabled, generates rich captured outputs
7070
//
@@ -89,26 +89,30 @@ pub(crate) fn expand_assert<'cx>(
8989
)),
9090
)],
9191
);
92-
expr_if_not(cx, call_site_span, cond_expr, then, None)
92+
assert_cond_check(cx, call_site_span, cond_expr, then)
9393
};
9494

9595
ExpandResult::Ready(MacEager::expr(expr))
9696
}
9797

98+
/// `assert!($cond_expr, $custom_message)`
9899
struct Assert {
99100
cond_expr: P<Expr>,
100101
custom_message: Option<TokenStream>,
101102
}
102103

103-
// if !{ ... } { ... } else { ... }
104-
fn expr_if_not(
105-
cx: &ExtCtxt<'_>,
106-
span: Span,
107-
cond: P<Expr>,
108-
then: P<Expr>,
109-
els: Option<P<Expr>>,
110-
) -> P<Expr> {
111-
cx.expr_if(span, cx.expr(span, ExprKind::Unary(UnOp::Not, cond)), then, els)
104+
/// `match <cond> { true => {} _ => <then> }`
105+
fn assert_cond_check(cx: &ExtCtxt<'_>, span: Span, cond: P<Expr>, then: P<Expr>) -> P<Expr> {
106+
// Instead of expanding to `if !<cond> { <then> }`, we expand to
107+
// `match <cond> { true => {} _ <then> }`.
108+
// This allows us to always complain about mismatched types instead of "cannot apply unary
109+
// operator `!` to type `X`" when passing an invalid `<cond>`, while also allowing `<cond>` to
110+
// be `&true`.
111+
let els = cx.expr_block(cx.block(span, thin_vec![]));
112+
let mut arms = thin_vec![];
113+
arms.push(cx.arm(span, cx.pat_lit(span, cx.expr_bool(span, true)), els));
114+
arms.push(cx.arm(span, cx.pat_wild(span), then));
115+
cx.expr_match(span, cond, arms)
112116
}
113117

114118
fn parse_assert<'a>(cx: &ExtCtxt<'a>, sp: Span, stream: TokenStream) -> PResult<'a, Assert> {

‎compiler/rustc_hir_typeck/src/demand.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
695695
) {
696696
match (self.tcx.parent_hir_node(expr.hir_id), error) {
697697
(hir::Node::LetStmt(hir::LetStmt { ty: Some(ty), init: Some(init), .. }), _)
698-
if init.hir_id == expr.hir_id =>
698+
if init.hir_id == expr.hir_id && !ty.span.source_equal(init.span) =>
699699
{
700700
// Point at `let` assignment type.
701701
err.span_label(ty.span, "expected due to this");

‎compiler/rustc_parse/src/parser/expr.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3822,7 +3822,7 @@ impl<'a> Parser<'a> {
38223822
P(Expr { kind, span, attrs, id: DUMMY_NODE_ID, tokens: None })
38233823
}
38243824

3825-
pub(crate) fn mk_expr(&self, span: Span, kind: ExprKind) -> P<Expr> {
3825+
pub fn mk_expr(&self, span: Span, kind: ExprKind) -> P<Expr> {
38263826
self.mk_expr_with_attrs(span, kind, AttrVec::new())
38273827
}
38283828

‎compiler/rustc_parse/src/parser/stmt.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -994,12 +994,7 @@ impl<'a> Parser<'a> {
994994
Ok(Some(stmt))
995995
}
996996

997-
pub(super) fn mk_block(
998-
&self,
999-
stmts: ThinVec<Stmt>,
1000-
rules: BlockCheckMode,
1001-
span: Span,
1002-
) -> P<Block> {
997+
pub fn mk_block(&self, stmts: ThinVec<Stmt>, rules: BlockCheckMode, span: Span) -> P<Block> {
1003998
P(Block {
1004999
stmts,
10051000
id: DUMMY_NODE_ID,
@@ -1010,7 +1005,7 @@ impl<'a> Parser<'a> {
10101005
})
10111006
}
10121007

1013-
pub(super) fn mk_stmt(&self, span: Span, kind: StmtKind) -> Stmt {
1008+
pub fn mk_stmt(&self, span: Span, kind: StmtKind) -> Stmt {
10141009
Stmt { id: DUMMY_NODE_ID, kind, span }
10151010
}
10161011

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

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
456456
span,
457457
format!("this is an iterator with items of type `{}`", args.type_at(0)),
458458
);
459-
} else {
459+
} else if !span.overlaps(cause.span) {
460460
let expected_ty = self.tcx.short_string(expected_ty, err.long_ty_path());
461461
err.span_label(span, format!("this expression has type `{expected_ty}`"));
462462
}
@@ -1584,8 +1584,16 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
15841584
{
15851585
let e = self.tcx.erase_regions(e);
15861586
let f = self.tcx.erase_regions(f);
1587-
let expected = with_forced_trimmed_paths!(e.sort_string(self.tcx));
1588-
let found = with_forced_trimmed_paths!(f.sort_string(self.tcx));
1587+
let mut expected = with_forced_trimmed_paths!(e.sort_string(self.tcx));
1588+
let mut found = with_forced_trimmed_paths!(f.sort_string(self.tcx));
1589+
if let Some(def_id) = cause.span.ctxt().outer_expn_data().macro_def_id
1590+
&& self.tcx.is_diagnostic_item(sym::assert_macro, def_id)
1591+
{
1592+
// When the type error comes from `assert!()`, the cause and effect are reversed
1593+
// because that macro expands to `match val { false => {panic!()}, _ => {} }`, which
1594+
// would say something like "expected `Type`, found `bool`", confusing the user.
1595+
(found, expected) = (expected, found);
1596+
}
15891597
if expected == found {
15901598
label_or_note(span, terr.to_string(self.tcx));
15911599
} else {
@@ -2107,7 +2115,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
21072115
) -> Option<(DiagStyledString, DiagStyledString)> {
21082116
match values {
21092117
ValuePairs::Regions(exp_found) => self.expected_found_str(exp_found),
2110-
ValuePairs::Terms(exp_found) => self.expected_found_str_term(exp_found, file),
2118+
ValuePairs::Terms(exp_found) => self.expected_found_str_term(cause, exp_found, file),
21112119
ValuePairs::Aliases(exp_found) => self.expected_found_str(exp_found),
21122120
ValuePairs::ExistentialTraitRef(exp_found) => self.expected_found_str(exp_found),
21132121
ValuePairs::ExistentialProjection(exp_found) => self.expected_found_str(exp_found),
@@ -2146,15 +2154,27 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
21462154

21472155
fn expected_found_str_term(
21482156
&self,
2157+
cause: &ObligationCause<'tcx>,
21492158
exp_found: ty::error::ExpectedFound<ty::Term<'tcx>>,
21502159
path: &mut Option<PathBuf>,
21512160
) -> Option<(DiagStyledString, DiagStyledString)> {
21522161
let exp_found = self.resolve_vars_if_possible(exp_found);
21532162
if exp_found.references_error() {
21542163
return None;
21552164
}
2165+
let (mut expected, mut found) = (exp_found.expected, exp_found.found);
2166+
if let Some(def_id) = cause.span.ctxt().outer_expn_data().macro_def_id
2167+
&& self.tcx.is_diagnostic_item(sym::assert_macro, def_id)
2168+
{
2169+
// When the type error comes from `assert!()`, the cause and effect are reversed
2170+
// because that macro expands to `match val { false => {panic!()}, _ => {} }`, which
2171+
// would say something like
2172+
// = note: expected `Type`
2173+
// found `bool`"
2174+
(expected, found) = (found, expected);
2175+
}
21562176

2157-
Some(match (exp_found.expected.unpack(), exp_found.found.unpack()) {
2177+
Some(match (expected.unpack(), found.unpack()) {
21582178
(ty::TermKind::Ty(expected), ty::TermKind::Ty(found)) => {
21592179
let (mut exp, mut fnd) = self.cmp(expected, found);
21602180
// Use the terminal width as the basis to determine when to compress the printed

‎library/coretests/tests/tuple.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ fn test_partial_ord() {
3737
assert!(!((1.0f64, 2.0f64) <= (f64::NAN, 3.0)));
3838
assert!(!((1.0f64, 2.0f64) > (f64::NAN, 3.0)));
3939
assert!(!((1.0f64, 2.0f64) >= (f64::NAN, 3.0)));
40-
assert!(((1.0f64, 2.0f64) < (2.0, f64::NAN)));
40+
assert!((1.0f64, 2.0f64) < (2.0, f64::NAN));
4141
assert!(!((2.0f64, 2.0f64) < (2.0, f64::NAN)));
4242
}
4343

‎library/std/tests/env.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ fn test_self_exe_path() {
1616

1717
#[test]
1818
fn test() {
19-
assert!((!Path::new("test-path").is_absolute()));
19+
assert!(!Path::new("test-path").is_absolute());
2020

2121
#[cfg(not(target_env = "sgx"))]
2222
current_dir().unwrap();

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

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,13 +129,27 @@ impl<'tcx> LateLintPass<'tcx> for BoolAssertComparison {
129129

130130
let mut suggestions = vec![(name_span, non_eq_mac.to_string()), (lit_span, String::new())];
131131

132-
if bool_value ^ eq_macro {
133-
let Some(sugg) = Sugg::hir_opt(cx, non_lit_expr) else {
134-
return;
135-
};
136-
suggestions.push((non_lit_expr.span, (!sugg).to_string()));
132+
if let ty::Bool = non_lit_ty.kind() {
133+
if bool_value ^ eq_macro {
134+
let Some(sugg) = Sugg::hir_opt(cx, non_lit_expr) else {
135+
return;
136+
};
137+
suggestions.push((non_lit_expr.span, (!sugg).to_string()));
138+
}
139+
} else {
140+
// If we have a `value` that is *not* `bool` but that `!value` *is*, we suggest
141+
// `!!value`.
142+
suggestions.push((
143+
non_lit_expr.span.shrink_to_lo(),
144+
if bool_value ^ eq_macro {
145+
"!".to_string()
146+
} else {
147+
"!!".to_string()
148+
},
149+
));
137150
}
138151

152+
139153
diag.multipart_suggestion(
140154
format!("replace it with `{non_eq_mac}!(..)`"),
141155
suggestions,

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

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use rustc_ast::{LitKind, RangeLimits};
1010
use rustc_data_structures::packed::Pu128;
1111
use rustc_data_structures::unhash::UnindexMap;
1212
use rustc_errors::{Applicability, Diag};
13-
use rustc_hir::{BinOp, Block, Body, Expr, ExprKind, UnOp};
13+
use rustc_hir::{BinOp, Body, Expr, ExprKind};
1414
use rustc_lint::{LateContext, LateLintPass};
1515
use rustc_session::declare_lint_pass;
1616
use rustc_span::source_map::Spanned;
@@ -134,18 +134,16 @@ fn assert_len_expr<'hir>(
134134
cx: &LateContext<'_>,
135135
expr: &'hir Expr<'hir>,
136136
) -> Option<(LengthComparison, usize, &'hir Expr<'hir>)> {
137-
if let Some(higher::If { cond, then, .. }) = higher::If::hir(expr)
138-
&& let ExprKind::Unary(UnOp::Not, condition) = &cond.kind
139-
&& let ExprKind::Binary(bin_op, left, right) = &condition.kind
137+
if let higher::IfLetOrMatch::Match(cond, [_, then], _) = higher::IfLetOrMatch::parse(cx, expr)?
138+
&& let ExprKind::Binary(bin_op, left, right) = &cond.kind
140139

141140
&& let Some((cmp, asserted_len, slice_len)) = len_comparison(*bin_op, left, right)
142141
&& let ExprKind::MethodCall(method, recv, [], _) = &slice_len.kind
143142
&& cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_slice()
144143
&& method.ident.name == sym::len
145144

146145
// check if `then` block has a never type expression
147-
&& let ExprKind::Block(Block { expr: Some(then_expr), .. }, _) = then.kind
148-
&& cx.typeck_results().expr_ty(then_expr).is_never()
146+
&& cx.typeck_results().expr_ty(then.body).is_never()
149147
{
150148
Some((cmp, asserted_len, recv))
151149
} else {
@@ -337,21 +335,21 @@ fn report_indexes(cx: &LateContext<'_>, map: &UnindexMap<u64, Vec<IndexEntry<'_>
337335
// `v.len() < 5` and `v.len() <= 5` does nothing in terms of bounds checks.
338336
// The user probably meant `v.len() > 5`
339337
LengthComparison::LengthLessThanInt | LengthComparison::LengthLessThanOrEqualInt => Some(
340-
format!("assert!({}.len() > {highest_index})", snippet(cx, slice.span, "..")),
338+
format!("{}.len() > {highest_index}", snippet(cx, slice.span, "..")),
341339
),
342340
// `5 < v.len()` == `v.len() > 5`
343341
LengthComparison::IntLessThanLength if asserted_len < highest_index => Some(format!(
344-
"assert!({}.len() > {highest_index})",
342+
"{}.len() > {highest_index}",
345343
snippet(cx, slice.span, "..")
346344
)),
347345
// `5 <= v.len() == `v.len() >= 5`
348346
LengthComparison::IntLessThanOrEqualLength if asserted_len <= highest_index => Some(format!(
349-
"assert!({}.len() > {highest_index})",
347+
"{}.len() > {highest_index}",
350348
snippet(cx, slice.span, "..")
351349
)),
352350
// `highest_index` here is rather a length, so we need to add 1 to it
353351
LengthComparison::LengthEqualInt if asserted_len < highest_index + 1 => Some(format!(
354-
"assert!({}.len() == {})",
352+
"{}.len() == {}",
355353
snippet(cx, slice.span, ".."),
356354
highest_index + 1
357355
)),

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ fn main() {
9191
assert_eq!(a!(), "".is_empty());
9292
assert_eq!("".is_empty(), b!());
9393
assert_eq!(a, true);
94-
assert!(b);
94+
assert!(!!b);
9595

9696
assert_ne!("a".len(), 1);
9797
assert!("a".is_empty());
@@ -111,7 +111,7 @@ fn main() {
111111
debug_assert_eq!(a!(), "".is_empty());
112112
debug_assert_eq!("".is_empty(), b!());
113113
debug_assert_eq!(a, true);
114-
debug_assert!(b);
114+
debug_assert!(!!b);
115115

116116
debug_assert_ne!("a".len(), 1);
117117
debug_assert!("a".is_empty());
@@ -143,7 +143,7 @@ fn main() {
143143

144144
use debug_assert_eq as renamed;
145145
renamed!(a, true);
146-
debug_assert!(b);
146+
debug_assert!(!!b);
147147

148148
let non_copy = NonCopy;
149149
assert_eq!(non_copy, true);

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ LL | assert_eq!(b, true);
4545
help: replace it with `assert!(..)`
4646
|
4747
LL - assert_eq!(b, true);
48-
LL + assert!(b);
48+
LL + assert!(!!b);
4949
|
5050

5151
error: used `assert_ne!` with a literal bool
@@ -141,7 +141,7 @@ LL | debug_assert_eq!(b, true);
141141
help: replace it with `debug_assert!(..)`
142142
|
143143
LL - debug_assert_eq!(b, true);
144-
LL + debug_assert!(b);
144+
LL + debug_assert!(!!b);
145145
|
146146

147147
error: used `debug_assert_ne!` with a literal bool
@@ -297,7 +297,7 @@ LL | renamed!(b, true);
297297
help: replace it with `debug_assert!(..)`
298298
|
299299
LL - renamed!(b, true);
300-
LL + debug_assert!(b);
300+
LL + debug_assert!(!!b);
301301
|
302302

303303
error: used `assert_eq!` with a literal bool

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,11 +157,17 @@ error: this expression always evaluates to true
157157
LL | let _ = val.is_empty();
158158
| ^^^^^^^^^^^^^^
159159

160+
error: this expression always evaluates to true
161+
--> tests/ui/const_is_empty.rs:181:17
162+
|
163+
LL | assert!(EMPTY_STR.is_empty());
164+
| ^^^^^^^^^^^^^^^^^^^^
165+
160166
error: this expression always evaluates to true
161167
--> tests/ui/const_is_empty.rs:185:9
162168
|
163169
LL | EMPTY_STR.is_empty();
164170
| ^^^^^^^^^^^^^^^^^^^^
165171

166-
error: aborting due to 27 previous errors
172+
error: aborting due to 28 previous errors
167173

0 commit comments

Comments
 (0)