scrutinizer-ci / scrutinizer

Legacy repository - archives past feature requests/bug reports
https://scrutinizer-ci.com/docs
140 stars 36 forks source link

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

Closed chalasr closed 8 years ago

chalasr commented 8 years ago

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)

schmittjoh commented 8 years ago

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

chalasr commented 8 years ago

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.

schmittjoh commented 8 years ago

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.

chalasr commented 8 years ago

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.