Lint: comparing two expressions of unit type

[timotree3]

I recently wrote a test like this

assert_eq!(&my_complicated_function().sort(), &[4, 3, 7, 1].sort()) // works!

but much to my surprise, the test kept passing even if I changed the literal

assert_eq!(&my_complicated_function().sort(), &[4, 3].sort()) // works!

at this point I realized that what I wrote was as good as this useless comparison

assert_eq!((), ())

In order to catch people who try to do things like this, I suggest that we lint comparing two expressions of unit type, since the result will always be true and it would be more straightforward just to write two lines for each expression e.g.

my_complicated_function();
[4, 3, 7, 1];

In addition to the canonical Unit Type (), I suggest that we lint for expressions of any user-created unit type such as:

struct UnitStruct;
enum UnitEnum {
    OnlyVariant,
}
// and maybe
enum TechnichallyUnitStruct {
    unit_field: (),
}
enum TechnichallyUnitEnum {
    UnitVariant(UnitStruct, ()),
    NeverVariant(!),
}
// etc...

Thoughts?

[ghost]

unit_cmp already checks for comparisons. It can be extended to work with assert_eq.

I wonder if we could lint any expression that contains a call to function that returns (). The return value is meaningless so the code either contains a bug or is misleading.

[flip1995]

We explicitly exclude expanded code from that lint:

rust-clippy/clippy_lints/src/types.rs

Lines 529 to 531 in a3fcaee

	if expr.span.from_expansion() {
	return;
	}

I'm not sure if that is necessary, since even when () is compared in an external macro, it most likely isn't something you wanted to do. I think removing this check would solve this issue without causing false positives because of macros. The macro check was added because of this FP: #307. So a test for this has to be added, whether this still happens today (without the macro check).

[timotree3]

I could imagine a more complicated macro that is general to a lot of types and does a comparison somewhere without being incorrect. Is there any precedent for doing some sort of heuristic as to how complex the macro is and therefore how likely it is that its sole purpose is the comparison?

[flip1995]

We can't look into the macro, we only can check the expanded code, so there is no way to know the complexity of a macro. I would change this lint, to not trigger on external_macros, but exclude assert_eq! from this.

[jakubadamw]

@flip1995, and perhaps the whole family: (debug_)?assert(_eq|_ne)?!?

[Lythenas]

@flip1995 Commenting out

rust-clippy/clippy_lints/src/types.rs

Lines 529 to 531 in a3fcaee

	if expr.span.from_expansion() {
	return;
	}

will still generate warnings when deriving e.g. PartialEq. And according to the comment in the unit_cmp test it should not generate warnings for this:

rust-clippy/tests/ui/unit_cmp.rs

Lines 4 to 5 in 737f0a6

	#[derive(PartialEq)]
	pub struct ContainsUnit(()); // should be fine

So I think that check is still needed.

But I was digging around and I think I found a way to implement special exceptions for asserts using expr.span.source_callee() and checking the kind and symbol of the macro expansion.

I would like to work on this issue if there isn't already someone working on it.