Phpinspectionsea: Less or no suggestion for exclamation mark instead of false comparison

[ScreamingDev]

| Subject | Details |
| :------------- | :---------------------------------------------------------------------------- |
| Issue type | False-Positive |
| Plugin | Php Inspections (EA Extended) or Php Inspections (EA Ultimate) |
| Language level | |

Current behaviour (description/screenshot:)

if ( false === $migration instanceof MigrationInterface ) {

Says

'!$migration instanceof MigrationInterface' would fit better here (reduces cognitive load). more... (Strg+F1)

Expected behaviour

No warning.

Notes

I disagree with this warning for this scenario:

if ( ! $migration instanceof MigrationInterface )

While on a bug hunt a new colleague asked me what comes first: The ! $migration or the instanceof. We changes it to false === ... so you have less cognitive load.

But even more I disagree with such hint if you show this warning for any type of value. For example:

$i = false;
$i = null;
$i = 0;
$i = 5;

if ( ! $i ) {
  echo 'douh!';
}

There is a code analysis tool for PHP that checks for such expressions where the dev was "too lazy" to have proper types. I agree with that tool. Being specific about the things that we cover/check is a good thing and leads to less side effects.

So I hope this hint about ! is simpler than false === does take care of it's context or does only appear when it is absolute sure that this does not make a strict statement more loose / inject side effects. I know I can disable that but I also not want the good PHPInspections EA to lead rookie developer into false-positives. (Just a hint, no offense)

[andreasschroth]

I disagree. For me, === and instanceof are both operators and in your above example the cognitive load is definitely higher than it would be with !, especially without parenthesis, because you have to think about in which order the comparison is executed.

if ( false === ($migration instanceof MigrationInterface) ) { would have a better readability imo.

But if ( ! $migration instanceof MigrationInterface) { being by far the easiest to read version.

Why not just turn off the inspection?

[kalessil]

The inspections is getting lots of negative feedback, so it'll be disabled by default to not distract folks during bug-hunts =)

[andreasschroth]

@kalessil Yeah sounds like a good idea to disable it by default. But personally I like it & the suggestions it provides. But yes, it's definitely a subjective thing.

[ScreamingDev]

Sorry, @kalessil - was no negative feedback. Just a hint on something we didn't expect from such nice tool: Suggesting weak assumptions over assertions. You know how it enhances our workflow and how much we appreciate it / would pay for a year long license ;D

We completely disable this in all projects, so I added our thoughts on it as a side-note (just a bit related to the specific case) because in my opinion assertions are more in favor of good software quality than assumptions.

For the specific issue / cognitive load: A developer should not start to think about the exact operator precedence (of anything / not only "instanceof") and shortcuts are just fast to read, not easy. So I opened this issue because I thought cognitive load was meant as "easy" and not "fast".

[kalessil]

@ScreamingDev: no worries, I appreciate the feedback and accepting the point (cognitive load). We probably mean different thing behind "negative feedback", in this project context it means that multiple people didn't find the inspection useful and disabling it in their projects.

Good point that the feedback brings major drawbacks (cognitive load, conflicts with other inspections, violating some specific code styles and more), so decision to disable the inspection by default was pretty easy to make =)

[kalessil]

The inspection is disabled by default.