feat: remove SlevomatCodingStandard.Operators.DisallowEqualOperators

[simPod]

We can use phpstan for that which handles it better

https://phpstan.org/r/977a0017-a744-4a9c-8bc8-6e580ff5ec1b (strict+bleeding edge checked)

[greg0ire]

Where is the breaking change?

[simPod]

I guess this was not tested.

And I consider breaking that CS will not check it any further.

[morozov]

What is the exact configuration under which PHPStan flags this? When I uncheck any of the "Strict rules" and
"Bleeding edge" boxes in the playground, this error disappears.

We have one such error deliberately suppressed in the PHP_CodeSniffer configuration for the DBAL but I do not believe this error is reported by PHPStan in the current configuration.

It would be nice to document the upgrade path to keep these errors reported if the coding standard will no longer do that.

[Ocramius]

IMO still to be preserved: I don't use phpstan, and psalm is perfectly fine with non-strict comparisons.

[simPod]

When I uncheck any of the "Strict rules" and
"Bleeding edge" boxes in the playground, this error disappears.

this but checked

@morozov Where can I document this eventually?

---

@Ocramius reasonable as well. I don't use psalm anymore as there's no support for intersections etc. But sure, not everyone uses phpstan. Dunno how Doctrine want to approach this but CS is kind of stupid in terms of checking things that SA should check instead so I'd rather remove those rules that fall into that category.

[morozov]

Where can I document this eventually?

I'd start here.

[morozov]

CS is kind of stupid in terms of checking things that SA should check instead so I'd rather remove those rules that fall into that category.

+1 that the coding standard should focus primarily on code formatting. Basically, enforcing a certain way of doing things that can be done in multiple ways (e.g. tabs vs. spaces, order of import statements, the position of curly braces, etc). Enforcing strict comparison over loose is not enforcing one of the many equal options.

[Ocramius]

CS is kind of stupid in terms of checking things that SA should check instead so I'd rather remove those rules that fall into that category.

Static analysis verifies correctness: == is perfectly valid in many contexts, and preventing its usage is a stylistic choice.

[morozov]

preventing its usage is a stylistic choice.

Totally disagree.

[Ocramius]

As I said, from a SA PoV, == is valid.

[morozov]

It is fine. But as I said, I believe a coding standard should focus on code formatting, not on semantics. As long as == and === have different semantics, it shouldn't be the business of a coding standard to enforce either of the options.

[Ocramius]

Since it seems like there isn't clarity, let me ask this the opposite way: why would psalm deny == then?

Preventing usage of == upfront is an arbitrary rule, rather than based on context.

What an SA tool does is evaluating the context of each expression, to determine validity . Seems like a scope mismatch to me.

[morozov]

why would psalm deny == then?

No idea. I'd guess that if the operands are not strictly typed, it is possible that the code may not work as expected in certain cases.

If we put the upgrade path aside, the "why psalm" question is out of the scope of this discussion. I'm trying to make a point that such a rule doesn't belong to a coding standard which this project is about.

[simPod]

Yup, it is the same as #258 which is stale.

Let's not check SA issues using CS. Use CS only for stylistics.

Let SA issues be handled individually. There are people who use either phpstan/psalm or both with custom flavours.

[Ocramius]

My point is that it is an upfront stylistic choice (much like early returns, preventing nested ternaries, or usage of empty()) hence fitting a CS ruleset.

I didn't read logical arguments against this reasoning here, so far.

I will gladly change my mind after that happens though 👍

[Ocramius]

@simPod #258 is the same: you can rely on type inference and skip declaring types, or declare the types and make sure they align with inference.

The SA engine can usually work with it anyway.

As an extreme example, in Haskell, you can decide to declare function signatures and implementation, or just the implementation.

The decision whether the signature should be declared or not falls on the developer: the type analysis doesn't care. Enforcing the declaration of type signatures is a stylistic choice.

[simPod]

It seems we all have different baseline what stylistic issue is. From my POV, usage of empty() is not stylistic issue. Usage of nested ternary is as it's about formatting the code and not the content of code.

Same is with == and ===. From my POV that's the content of code. I'd expect CS to handle e.g. number of spaces around x == y and x === y but not tell me which operator I should use.

[Ocramius]

I disallow ++, and only allow += 1.

Pretty much style decision. Style is not just "alignment".

[simPod]

I disallow ++, and only allow += 1.

This, I agree with. The ~same thing, written differently. == === are not the same thing.

[Ocramius]

++ and += 1 are, interesting, exactly in the same scope of this patch.

• ++ can be both prefix and/or suffix
• ++ can legally operate on string|numeric (you can increment Latin alphabet letter sequences, for extra pain)
• += 1 can legally operate on numeric-string|numeric (stricter operator)

My stylistic decision is to avoid ++, removing the need to reason about prefix/suffic and string semantics upfront even if the code may be perfectly legal.

It is, ironically, pretty much the same scenario of == vs === 😛

[Ocramius]

Also, I know it is a frustratingly long discussion (sorry for that), but this is probably also the best place to hold this sort of discussion.

[simPod]

+= 1 can legally operate on numeric-string|numeric (stricter operator)

I did not realise that to such detail. I'll prefer += as well since now but also would prefer SA to notify me about that, not CS. It's the same scenario 😄 🤔.
I agree we both said all we could so I'll restrain myself for now.

[derrabus]

It seems we all have different baseline what stylistic issue is. From my POV, usage of empty() is not stylistic issue.

Code style is not purely about code formatting and PHPCS is not just a glorified code-formatter. If we disallow empty() or ==, that means that we as the Doctrine project agreed not to write code that uses those operations. And yes, that is a stilistic choice.

My understanding is that you still don't want to allow == in codebases you maintain and neither is this rule broken or abandoned in any way. Your motivation is (correct me if I'm wrong) to remove a redundancy since two tools you are using combined with the settings you are using would report the same error. I don't think that this redundancy alone is problematic enough to remove the rule.

Furthermore, codebases that don't use PHPStan or configure PHPStand to run without the strict ruleset would still benefit from this rule if we keep it.

tl;dr: I'd like to keep the rule.

[simPod]

Aight, thx for input everyone!