Remove the assert_eq! macro in favor of special-casing a check for equality in the assert! macro

[bstrie]

We currently have four macros for runtime assertions: assert!, assert_eq!, debug_assert!, and debug_assert_eq!. See https://github.com/rust-lang/rust/blob/master/src/libcore/macros.rs#L50 for their definitions, which are all quite simple. The reason for having the _eq form for both assert! and debug_assert! is because equality checks are the most common kinds of assertions (assert_eq! is found 7100 times in the Rust repo, vs 4600 for assert!) and that by encoding the intent of the assertion we can produce a better error message, like so:

assert!(1 == 2);  // task '<main>' failed at 'assertion failed: 1 == 2'
assert_eq!(1, 2);  // task '<main>' failed at 'assertion failed: `(left == right) && (right == left)` (left: `1`, right: `2`)', assert.rs:2

However, I don't see a reason for the existence of this extra macro. I propose that we leverage our existing macro_rules! facilities to special-case the form assert!(a == b) such that it implements the current logic of assert_eq!. In non-tested copy-pasted code:

macro_rules! assert(
    ($cond1:expr == $cond2:expr) => ({  // literally just the inner bit of the assert_eq! definition
        let c1 = $cond1;
        let c2 = $cond2;
        if c1 != c2 || c2 != c1 {
            fail!("expressions not equal, left: {}, right: {}", c1, c2);
        }
    })
    ($cond:expr) => (  // the current assert! definition starts here
        if !$cond {
            fail!(concat!("assertion failed: ", stringify!($cond)))
        }
    );
    ($cond:expr, $($arg:tt)*) => (
        if !$cond {
            fail!($($arg)*)
        }
    );
)

Does anyone see any problems with this?

[sinistersnare]

Hey, random question, but it seems debug_assert is created twice? any reason for this?

rust/src/libcore/macros.rs

Lines 65 to 97 in 43f040d

	macro_rules! debug_assert(
	($(a:tt)*) => ({
	if cfg!(not(ndebug)) {
	assert!($($a)*);
	}
	})
	)
	/// Runtime assertion for equality, for details see std::macros
	#[macro_export]
	macro_rules! assert_eq(
	($cond1:expr, $cond2:expr) => ({
	let c1 = $cond1;
	let c2 = $cond2;
	if c1 != c2 || c2 != c1 {
	fail!("expressions not equal, left: {}, right: {}", c1, c2);
	}
	})
	)
	/// Runtime assertion for equality, only without `--cfg ndebug`
	#[macro_export]
	macro_rules! debug_assert_eq(
	($($a:tt)*) => ({
	if cfg!(not(ndebug)) {
	assert_eq!($($a)*);
	}
	})
	)
	/// Runtime assertion, disableable at compile time
	#[macro_export]
	macro_rules! debug_assert(

[bstrie]

Github accidentally submitted this issue (twice...), hold on commenting until I finish it! :P

[bstrie]

@sinistersnare, that's a good question. At a quick glance it looks totally redundant.

[bstrie]

cc @gankro

[bstrie]

Prior IRC discussion: https://botbot.me/mozilla/rust-internals/2014-08-20/?msg=20089459&page=3

[bstrie]

An open question is whether to preserve the current (and rather surprising behavior) of assert_eq!(a, b), which is that it checks both a == b and b == a. It would be easy to shove this logic into the special form of the assert! macro (as shown in the code up in the original ticket). However, it would be a bit surprising that assert!(a == b) would provide a stronger equality check than just the typical a == b.

[bstrie]

Upon further consideration I think that the current two-way equality check in assert_eq! is both surprising and unnecessary, and I'd vote to get rid of it if this bug were implemented. Does anyone remember why it's there in the first place?

[pczarn]

This must be implemented as a syntax extension, because macros defined with macro_rules parse nonterminals eagerly.

[Gankra]

I'd be interested in having the double-equality-check be relegated to a different special form like a === b or something, since as far as I can tell, the scenarios where this is desired is exceptionally rare, except for maybe when you're actually testing eq itself, in which case you're probably writing both checks anyway.

[bstrie]

@pczarn, I'm fine with this becoming a syntax extension. Heading off a combinatorial explosion of macros is a win in my book.

[bstrie]

@gankro, I'd be tempted to just tell those people to do assert!(a == b && b == a) rather than introducing a whole new === operator (let's not go down that road!). It wouldn't trigger the special form mentioned here, but it seems like quite a rare thing to desire afaik.

Also, wasn't there some upcoming changes to traits that could end up making == commutative?

[Gankra]

@bstrie that's kind of annoying when a and b aren't variables, but compound expressions. e.g.
assert_eq(stack.pop(), Some(5)). You would have to clutter up your tests with tons of temp variables if you were interested in that compound expression.

[Gankra]

Also of course, I think it would be great to be able to do this for basically all of the comparison operators.

In fact, if it could be written in a general enough way to basically dump the values of all the variables in the expression, regardless of the expression, that would be amazing.

If it could be done in a way that it outputs intermediate results of compound expressions, I would basically have the author's child.

[bstrie]

@gankro, I agree that it's definitely annoying if you happen to need it. I still question whether anyone happens to need it, and intuitively I'm guessing that most people who use assert_eq! have no idea that it performs this extra check at all.

[Gankra]

@bstrie Fair enough.

[thehydroimpulse]

I second the use of combing them into one macro, instead of two. I was also not aware of the extra equality check.

[cgaebel]

Can all other comparison operators be special-cased too? It's kind of annoying when I want a check like: assert!(fd > 0) and it doesn't tell me what the fd was in the error message.

[bstrie]

@cgaebel @gankro, if we can do it for ==, I see no reason why we couldn't do it for the other comparison operators as well.

[bluss]

See issue #16625 why assert! must be made simpler. In effect two separate assert! are needed; and efficient version for implementing library APIs and a convenient and verbose version for writing tests.

[alexcrichton]

In light of rust-lang/rfcs#550 (implemented in #20563), this is unfortunately no longer possible.

The == token is not in the follow set of the expr matcher for macros, and it's likely we won't make it so (due to ambiguities), so I'm going to close this issue as "not possible today".

[Gankra]

Does "not possible today" suggest "maybe possible eventually", or does this change preclude such a design from ever working?

[alexcrichton]

To allow this design I believe we would have to add == to the follow set of expressions in macros, but then this would be ambiguous:

assert!(foo == bar == baz)

For that macro invocation, what's $lhs and what's $rhs? As a result, I don't think that we can ever add == to the follow set of expressions, so I don't think that we'll ever be able to implement this.

[pczarn]

It can be implemented as a syntax extension. However, the change to assert! might be breaking, because it doesn't currently check for right == left.

If it could be done in a way that it outputs intermediate results of compound expressions, I would basically have the author's child.

How could you detect whether a compound expression's type implements Show? It's going to be possible with negative bounds.

There's an issue with intermediate results of compound expressions like these:

assert!(consume(foo) == bar)

In this example, consume moves the value out of foo which can't be printed after the check.