Skip to content

Commit

Permalink
Assert that the first assert! expression is bool
Browse files Browse the repository at this point in the history
In the desugaring of `assert!`, assign the condition expression to a `bool`
biding in order to provide better type errors when passed the wrong thing.

The span will point only at the expression, and not the whole `assert!`
invokation.

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

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`
   |             expected due to this
```

Fix rust-lang#122159.
  • Loading branch information
estebank committed Mar 17, 2024
1 parent 22e241e commit 3737314
Show file tree
Hide file tree
Showing 44 changed files with 219 additions and 172 deletions.
65 changes: 59 additions & 6 deletions compiler/rustc_builtin_macros/src/assert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ mod context;
use crate::edition_panic::use_panic_2021;
use crate::errors;
use rustc_ast::ptr::P;
use rustc_ast::token;
use rustc_ast::token::Delimiter;
use rustc_ast::tokenstream::{DelimSpan, TokenStream};
use rustc_ast::{self as ast, token};
use rustc_ast::{DelimArgs, Expr, ExprKind, MacCall, Path, PathSegment, UnOp};
use rustc_ast_pretty::pprust;
use rustc_errors::PResult;
Expand All @@ -20,7 +20,7 @@ pub fn expand_assert<'cx>(
span: Span,
tts: TokenStream,
) -> MacroExpanderResult<'cx> {
let Assert { cond_expr, custom_message } = match parse_assert(cx, span, tts) {
let Assert { cond_expr, inner_cond_expr, custom_message } = match parse_assert(cx, span, tts) {
Ok(assert) => assert,
Err(err) => {
let guar = err.emit();
Expand Down Expand Up @@ -70,7 +70,9 @@ pub fn expand_assert<'cx>(
//
// FIXME(c410-f3r) See https://github.com/rust-lang/rust/issues/96949
else if cx.ecfg.features.generic_assert {
context::Context::new(cx, call_site_span).build(cond_expr, panic_path())
// 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(inner_cond_expr, panic_path())
}
// If `generic_assert` is not enabled, only outputs a literal "assertion failed: ..."
// string
Expand All @@ -85,7 +87,7 @@ pub fn expand_assert<'cx>(
DUMMY_SP,
Symbol::intern(&format!(
"assertion failed: {}",
pprust::expr_to_string(&cond_expr)
pprust::expr_to_string(&inner_cond_expr)
)),
)],
);
Expand All @@ -95,8 +97,12 @@ pub fn expand_assert<'cx>(
ExpandResult::Ready(MacEager::expr(expr))
}

// `assert!($cond_expr, $custom_message)`
struct Assert {
// `{ let assert_macro: bool = $cond_expr; assert_macro }`
cond_expr: P<Expr>,
// We keep the condition without the `bool` coercion for the panic message.
inner_cond_expr: P<Expr>,
custom_message: Option<TokenStream>,
}

Expand All @@ -118,7 +124,7 @@ fn parse_assert<'a>(cx: &mut ExtCtxt<'a>, sp: Span, stream: TokenStream) -> PRes
return Err(cx.dcx().create_err(errors::AssertRequiresBoolean { span: sp }));
}

let cond_expr = parser.parse_expr()?;
let inner_cond_expr = parser.parse_expr()?;

// Some crates use the `assert!` macro in the following form (note extra semicolon):
//
Expand Down Expand Up @@ -154,7 +160,54 @@ fn parse_assert<'a>(cx: &mut ExtCtxt<'a>, sp: Span, stream: TokenStream) -> PRes
return parser.unexpected();
}

Ok(Assert { cond_expr, custom_message })
let cond_expr = expand_cond(cx, parser, inner_cond_expr.clone());
Ok(Assert { cond_expr, inner_cond_expr, custom_message })
}

fn expand_cond(cx: &ExtCtxt<'_>, parser: Parser<'_>, cond_expr: P<Expr>) -> P<Expr> {
let span = cx.with_call_site_ctxt(cond_expr.span);
// Coerce the expression to `bool` for more accurate errors. If `assert!` is passed an
// expression that isn't `bool`, the type error will point at only the expression and not the
// entire macro call. If a non-`bool` is passed that doesn't implement `trait Not`, we won't
// talk about traits, we'll just state the appropriate type error.
// `let assert_macro: bool = $expr;`
let ident = Ident::new(sym::assert_macro, span);
let local = P(ast::Local {
ty: Some(P(ast::Ty {
kind: ast::TyKind::Path(None, ast::Path::from_ident(Ident::new(sym::bool, span))),
id: ast::DUMMY_NODE_ID,
span,
tokens: None,
})),
pat: parser.mk_pat_ident(span, ast::BindingAnnotation::NONE, ident),
kind: ast::LocalKind::Init(cond_expr),
id: ast::DUMMY_NODE_ID,
span,
colon_sp: None,
attrs: Default::default(),
tokens: None,
});
// `{ let assert_macro: bool = $expr; assert_macro }``
parser.mk_expr(
span,
ast::ExprKind::Block(
parser.mk_block(
thin_vec![
parser.mk_stmt(span, ast::StmtKind::Let(local)),
parser.mk_stmt(
span,
ast::StmtKind::Expr(parser.mk_expr(
span,
ast::ExprKind::Path(None, ast::Path::from_ident(ident))
)),
),
],
ast::BlockCheckMode::Default,
span,
),
None,
),
)
}

fn parse_custom_message(parser: &mut Parser<'_>) -> Option<TokenStream> {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_parse/src/parser/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3775,7 +3775,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())
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_parse/src/parser/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1399,7 +1399,8 @@ impl<'a> Parser<'a> {
})
}

pub(super) fn mk_pat_ident(&self, span: Span, ann: BindingAnnotation, ident: Ident) -> P<Pat> {
// `pub` because we use it from `rustc_builtin_macros`.
pub fn mk_pat_ident(&self, span: Span, ann: BindingAnnotation, ident: Ident) -> P<Pat> {
self.mk_pat(span, PatKind::Ident(ann, ident, None))
}

Expand Down
9 changes: 2 additions & 7 deletions compiler/rustc_parse/src/parser/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -833,12 +833,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,
Expand All @@ -849,7 +844,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 }
}

Expand Down
8 changes: 4 additions & 4 deletions library/core/tests/bool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
6 changes: 3 additions & 3 deletions library/core/tests/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,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);
}
}

Expand Down
2 changes: 1 addition & 1 deletion library/std/src/env/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,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();
Expand Down
26 changes: 13 additions & 13 deletions src/tools/miri/tests/pass/binops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,23 @@

fn test_nil() {
assert_eq!((), ());
assert!((!(() != ())));
assert!((!(() < ())));
assert!((() <= ()));
assert!((!(() > ())));
assert!((() >= ()));
assert!(!(() != ()));
assert!(!(() < ()));
assert!(() <= ());
assert!(!(() > ()));
assert!(() >= ());
}

fn test_bool() {
assert!((!(true < false)));
assert!((!(true <= false)));
assert!((true > false));
assert!((true >= false));
assert!(!(true < false));
assert!(!(true <= false));
assert!(true > false);
assert!(true >= false);

assert!((false < true));
assert!((false <= true));
assert!((!(false > true)));
assert!((!(false >= true)));
assert!(false < true);
assert!(false <= true);
assert!(!(false > true));
assert!(!(false >= true));

// Bools support bitwise binops
assert_eq!(false & false, false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ fn notsure() {

fn canttouchthis() -> usize {
fn p() -> bool { true }
let _a = (assert!((true)) == (assert!(p())));
let _c = (assert!((p())) == ());
let _a = (assert!(true) == (assert!(p())));
let _c = (assert!(p()) == ());
let _b: bool = (println!("{}", 0) == (return 0));
}

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/binding/expr-match-generic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ type compare<T> = extern "Rust" fn(T, T) -> bool;

fn test_generic<T:Clone>(expected: T, eq: compare<T>) {
let actual: T = match true { true => { expected.clone() }, _ => panic!("wat") };
assert!((eq(expected, actual)));
assert!(eq(expected, actual));
}

fn test_bool() {
Expand Down
10 changes: 5 additions & 5 deletions tests/ui/binding/expr-match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@

fn test_basic() {
let mut rs: bool = match true { true => { true } false => { false } };
assert!((rs));
assert!(rs);
rs = match false { true => { false } false => { true } };
assert!((rs));
assert!(rs);
}

fn test_inferrence() {
let rs = match true { true => { true } false => { false } };
assert!((rs));
assert!(rs);
}

fn test_alt_as_alt_head() {
Expand All @@ -25,7 +25,7 @@ fn test_alt_as_alt_head() {
true => { false }
false => { true }
};
assert!((rs));
assert!(rs);
}

fn test_alt_as_block_result() {
Expand All @@ -34,7 +34,7 @@ fn test_alt_as_block_result() {
true => { false }
false => { match true { true => { true } false => { false } } }
};
assert!((rs));
assert!(rs);
}

pub fn main() {
Expand Down
30 changes: 15 additions & 15 deletions tests/ui/binop/binops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,23 @@

fn test_nil() {
assert_eq!((), ());
assert!((!(() != ())));
assert!((!(() < ())));
assert!((() <= ()));
assert!((!(() > ())));
assert!((() >= ()));
assert!(!(() != ()));
assert!(!(() < ()));
assert!(() <= ());
assert!(!(() > ()));
assert!(() >= ());
}

fn test_bool() {
assert!((!(true < false)));
assert!((!(true <= false)));
assert!((true > false));
assert!((true >= false));
assert!(!(true < false));
assert!(!(true <= false));
assert!(true > false);
assert!(true >= false);

assert!((false < true));
assert!((false <= true));
assert!((!(false > true)));
assert!((!(false >= true)));
assert!(false < true);
assert!(false <= true);
assert!(!(false > true));
assert!(!(false >= true));

// Bools support bitwise binops
assert_eq!(false & false, false);
Expand Down Expand Up @@ -76,9 +76,9 @@ fn test_class() {
}
assert_eq!(q, r);
r.y = 17;
assert!((r.y != q.y));
assert!(r.y != q.y);
assert_eq!(r.y, 17);
assert!((q != r));
assert!(q != r);
}

pub fn main() {
Expand Down
14 changes: 7 additions & 7 deletions tests/ui/binop/structured-compare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ pub fn main() {
let a = (1, 2, 3);
let b = (1, 2, 3);
assert_eq!(a, b);
assert!((a != (1, 2, 4)));
assert!((a < (1, 2, 4)));
assert!((a <= (1, 2, 4)));
assert!(((1, 2, 4) > a));
assert!(((1, 2, 4) >= a));
assert!(a != (1, 2, 4));
assert!(a < (1, 2, 4));
assert!(a <= (1, 2, 4));
assert!((1, 2, 4) > a);
assert!((1, 2, 4) >= a);
let x = foo::large;
let y = foo::small;
assert!((x != y));
assert!(x != y);
assert_eq!(x, foo::large);
assert!((x != foo::small));
assert!(x != foo::small);
}
2 changes: 1 addition & 1 deletion tests/ui/cfg/conditional-compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub fn main() {
pub fn main() {
// Exercise some of the configured items in ways that wouldn't be possible
// if they had the bogus definition
assert!((b));
assert!(b);
let _x: t = true;
let _y: tg = tg::bar;

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/codemap_tests/issue-28308.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
fn main() {
assert!("foo");
//~^ ERROR cannot apply unary operator `!`
//~^ ERROR mismatched types
}
Loading

0 comments on commit 3737314

Please sign in to comment.