Phpinspectionsea: Hint if one of the multiple types could not have a function

[panvid]

I like information, that there is a possible null pointer on calling a function on a possible null. The same I need in following case:

class Foo1{}

class Foo2
{
    public function foo(){}
}

/**
 * @return  Foo1|Foo2
 */
function bar(){}

$bar = bar();
$bar->foo();

Function bar() can return a Foo1 or Foo2. The function foo() only exists in Foo2, so the call $bar->foo() should show a hint like:

Method foo not found in every possible objects.

[rentalhost]

I think it valid. But what you are trying to do is not "impossible", but is not a good programming code. If Foo1 is not like Foo2, so you should not mix types here.

But, as I said, I think it valid once that this inspection should not validate the code quality, but if it will give an error or not, and this case is probably that we found an error on runtime.

So I think that it should be implemented, but we should think too to how avoid false-positives. And I think that it should be valid:

/** @var Foo1|Foo2 $bar */
if ($bar instanceof Foo1) {
    // Ok: as now $bar is strictly Foo1.
    $bar->foo();
}

// Warning: foo() doesn't exists on Foo2.
$bar->foo();

[kalessil]

My beloved PhpDoc limitations =)

Wouldn't it be better to check if combining classes in PhpDoc is making sense - I understand the case, but it's also absolutely expected outcome?

[panvid]

Its totally correct, that this usecase is not a good programming code :) I work with a > 3 year old code, which is started in PHP 5.x and without totally OOP, so a refactoring would be better, correct.

I also can _reproduce_ this problem with cleaner real live code:

class SomeAbstract
{
  proctected $id;

  public function getId()
  {
    return $this->id;
  }

  abstract function bar();
}

class Bar1 extends SomeAbstract
{
  protected $name;

  public function getName()
  {
    return $this->name;
  }

  function bar(){...};
}

class Bar2 extends SomeAbstract
{
  function bar(){...};
}

/**
  * @return Bar1|Bar2
  */
public function giveSomeAbstract(): SomeAbstract
{
  ...
}

$foo = giveSomeAbstract();
// works because getId() exists in abstract
$foo->getId();

// should show a hint because getName() only exist in Bar1
$foo->getName();

For me it sounds valid to use OOP like this: some function can return a instance from SomeAbstract, which can have their own functions.

[kalessil]

I'm not judging anything (IDE does xD). But speaking seriously, what will be more efficient for this specific context:

• checking that both Bar1|Bar2 has needed method
• checking PhpDoc

To clarify: I can do only one of them and want to be super-practical here =)

[panvid]

I think the first idea is the best: checking if both classes has the needed method.

So structs like

/** @var Foo1|Foo2 $bar */
if ($bar instanceof Foo1) {
    // Ok: as now $bar is strictly Foo1.
    $bar->foo();
}

help avoid this problem.

[kalessil]

@rentalhost : any other cases (except instanceof) we should be aware of?

[rentalhost]

Maybe if you override it locally?

/** @var Foo1|Foo2 $bar */
if (anyExpr()) {
    /** @var Foo1 $bar */
    $bar->foo();
}

[rentalhost]

We have a lot of possible cases like that, but I don't think feasible handle all for now. Eg. is_a(), get_class()...

[funivan]

Hello everyone.
In this use case, it will be better to create interface and change return type. Or... create own wrapper to make code more clean (php 7, return types etc)

Case is common but maybe it will be more useful to introduce "Inconsistent type declaration"
We can have multiple return types declared in following cases:

• function return type
• function argument type
• property type
• variable var doc

Inconsistent type declaration can catch this cases. The inspection will check PhpDoc and warn if we have multiple types.

class UserList {
/**
 * @var SqlDb|JsonDb  <- we should place warning here
 */
private $storage;
}

/**
 * @return  Foo1|Foo2  <-- warn here
 */
function bar(){}

We can skep types with null:

string|null    <-- ok
Foo1|null  <-- ok
Foo1|Foo2|null  <-- warning

I`m planning to implement this inspection but don't know if this will be useful for everyone. It can catch your cases and help.

@kalessil if this is interesting I can implement it in Ultimate or Community edition.

[panvid]

Its also okay for me to have the hint, that this function can return multiple (not depending) types. But mention following struct, which can work (and still work in our code):

class SomeAbstract
{
  /**
   * @return AbstractModel
   */
  public function foo()
  {
  }
}

class SomeImplementation extends SomeAbstract
{
  /**
   * @return ImplementationModel|AbstractModel
   */
  public function foo()
  {
    $model = self::foo(); // this returns an AbstractModel and do some parent stuff
    // do other, implementation, stuff
    return $model;
  }
}

In this case I need to define the _PhpDoc_ to return one of them (ImplementationModel and AbstractModel). If not I get another hint (I think).

Btw: You linked the wrong david ;)

[rentalhost]

I do think reasonable an inspection that detects multiple types on same variable. But maybe it should be discussed in another ticket, don't?

[funivan]

@dpauli why you cant specify interface here @return ImplementationModel|AbstractModel?

[panvid]

@funivan I can (and I will) but right now the code is not that good. I have a lot of refactoring tasks to do this.

[stale[bot]]

Bebo beep, the StaleBot is here. For one year nothing have happened here. It would be great if someone looked into details here within next 21 days when I'll close it.

[kalessil]

Reopening

[stale[bot]]

Bebo beep, the StaleBot is here. For one year nothing have happened here. It would be great if someone looked into details here within next 21 days when I'll close it.

[kalessil]

Commenting to keep stalebot away