Implement AssertEqualsIsDiscouragedRule

[staabm]

Simplified version which is concentrated on the main point: point people to assertSame when possible.
I intentionally stripped of the "assertEquals is fine when a comment is given" case, as I think its not important.

closes #6

refs #7

[ondrejmirtes]

I don't think we should do this here. It'd only make sense when combined with strict-rules.

[staabm]

Do you mean this rule should move into the strict rules repo?

Or do you mean I should somehow detect whether strict rules are loaded in tandem and skip the rule if not?

[ondrejmirtes]

We could do it similarly to deprecationRulesInstalled parameter which is used here I think.

[staabm]

I think it should be good to go now

[ondrejmirtes]

I'd rather use conditionalTags instead of passing the parameter in the rule.

Also: we can do it only on bleeding edge. Conditional tags can do and condition based on multiple parameters since phpstan/phpstan-src#1697 (see the tests).

[staabm]

TIL, thanks

[ondrejmirtes]

I feel like they have a more specific class parent than just NodeAbstract. Feel free to check out and optimize other similar rules as well.

[staabm]

fixed here and will do for more rules in a separate PR with a 1.4.x target in #217

[ondrejmirtes]

Thank you.

[VincentLanglet]

Hi @staabm, just saw this new rules.

I feel like it should be stricter.
Reporting AssertEquals($int, $int) is kinda useless since there is no risk of error here.
What could be useful would be to report thing like AssertEquals($string, $int) or AssertEquals($string, null) when both values are scalar, even if it's not the same type.

It's like if we were reporting $string == $string but not null == $string, which is currently the case with strict-rules
https://phpstan.org/r/d4a18742-8e2c-40fa-97a9-7e0e8440106a

Enforcing a stricter comparison, like '' === $string... is better. Here would be the same, we're enforcing AssertSame($string,'') rather than AssertEquals($string, null). WDYT ?

[staabm]

the goal of this PRs rule is about reporting assertions which can use strict-comparison without risk (unnecessary loose comparision).

I feel like it should be stricter.
Reporting AssertEquals($int, $int) is kinda useless since there is no risk of error here.
What could be useful would be to report thing like AssertEquals($string, $int) or AssertEquals($string, null) when both values are scalar, even if it's not the same type.

I think what you are suggesting is about "always true/false" errors (so verification of arguments given to AssertEquals) which sounds like a separate rule (maybe its even already implemented.. did not check)

[VincentLanglet]

the goal of this PRs rule is about reporting assertions which can use strict-comparison without risk (unnecessary loose comparision).

I understood that, but then it doesn't require to be behind the strictRulesInstalled flag IMHO.

There is already rules asking to change things like

• AssertSame(true, $foo) to AssertTrue($foo)
• AssertSame(42, count($foo)) to AssertCount(42, $foo)
• ...

All these changes are safe. And not behind the strictRulesInstalled flag.

I would think that

• Changing AssertEqual($scalarType, $sameScalarType) to AssertSame($scalarType, $sameScalarType) wouldn't require strictRulesInstalled
• Changing AssertEqual($scalarType, $differentScalarType) to AssertSame($scalarType, $differentScalarType) would require strictRulesInstalled since it's not safe

The same way $foo == $bar is reported as error by strictRule, even if it's not safe to change it to ===.

[staabm]

I understood that, but then it doesn't require to be behind the strictRulesInstalled flag IMHO.

AssertEqualsIsDiscouragedRule is about loose comparison, which is a strict-rules thing.

• AssertSame(true, $foo) to AssertTrue($foo)

this turns a strict comparison into a different syntax but still strict comparison

• Changing AssertEqual($scalarType, $sameScalarType) to AssertSame($scalarType, $sameScalarType) wouldn't require strictRulesInstalled

• Changing AssertEqual($scalarType, $differentScalarType) to AssertSame($scalarType, $differentScalarType) would require strictRulesInstalled since it's not safe

if this is how regular strict-rules comparison work, then it makes sense to me (did not check).