scrutinizer-ci/scrutinizer

Wrong "{var} can also be of type boolean; however {FQCN} seem to accept array"

Closed this issue · 4 comments

I have the following class:

<?php

class Foo {
    public function __construct(array $var) {
        // ...
    }
}

And the following method:

<?php

/**
 * array|bool $var
 */
public function doSomething ($var) {
    if (!$var) {
        return false;
    }

   return new Foo($var);
}

I get this patch :

[Bug] It seems like $var defined by {random method calling doSomething()} on line {line} can also be of type boolean; however, Foo::__construct() does only seem to accept array, maybe add an additional type check?

In fact, Foo::__construct() cannot receive a boolean in doSomething().

The real analysis (will be quickly unavailable)

I'm not sure I understand the fix you propose?

I don't looked at the code for now so I can't give a real proposal ATM (I'll investigate as you seem to confirm the bug).

But, the problem is there is a check preventing the error that the patch is trying to prevent, and this check is not considered.
Do you see a way to look for a return (or any breaking statement) that would prevent the method to be called with an invalid argument and so make this patch not helpful?

The @param false|array has nothing to do with the real issue, I removed it from the description.
As I never worked on this code until here, it would be nice if you have a suggestion.

There is a difference between

/** @param boolean|string $x */
function($x) {
  if ( ! $x) {
     return;
  }

  // $x is still boolean|string here, since it could be true
}

and

/** @param false|string $x */
function($x) {
  if ( ! $x) {
     return;
  }

  // $x is just string here, since false is excluded by if
}

If I understand correctly, you have the first case in your code. I don't see how we can make the analysis smarter in that case though. If I'm missing something, please let me know.

Yes, you got it.

And sure, there is nothing to do. I think that I would have liked to have a patch for the inverse side, saying that if @x can be only false because it cannot be true and is not checked for true in the method, the doc should be @param false|string and not bool|string.

Overkill (or impossible), I close this.
Thank's for your time.