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.