PHPStan loses type certainty outside of assertion blocks

[gdsmith]

Bug report

On the last line, PHPStan is no longer able to understand that the queue is of the type returned by the context.

Code snippet that reproduces the problem

        if ($context instanceof SnsQsContext) {
            $this->queue = $context->createQueue('test');
        } elseif ($context instanceof SqsContext) {
            $this->queue = $context->createQueue('test');
        } else {
            throw new RuntimeException('nope');
        }
        $context->declareQueue($this->queue);

Complete example: https://phpstan.org/r/f0317936-b04f-4c77-ab21-5f4997ba4479

Not sure if it's possible to fix this kind of edge case, the offending classes are in an external library so don't have much control over the Context/Queue just how we are implementing it. I can live with the Success variant, I just prefer the Fail variant.

Expected output

Fail class should pass but fails with:

Parameter #1 $name of method SnsQsContext::declareQueue() expects SnsQsQueue, SnsQsQueue|SqsQueue given.

Did PHPStan help you today? Did it make you happy in any way?

I love how much stuff PHPStan does pick up for me, and it's been a great help to ensuring our code quality and consistency remains high.

[ondrejmirtes]

Hi, thank you for your kind words! :)

To be honest, I have a hard time following the code amd your expectations. You can see the resulting types below the conditions: https://phpstan.org/r/0eed894a-1eab-4a16-9d91-d9a019bf49a3

So $context can be SnsQsContext|SqsContext and $this–>queue can be SnsQsQueue|SqsQueue. It's apparent that putting SnsQsQueue into SqsContext isn't possible, as putting SqsQueue into SnsQsContext isn't either. That's why the error is reported.

Do you expect some tanglement of the property and the variable so that PHPStan understands "if $context is SqsContext then $this->queue is SqsQueue"? Although there's some logic in PHPStan that does this in some cases, it's pretty limited and usually something that you can't expect from a static analyser. Try to write the same code in TypeScript, I bet it wouldn't understand it either.

[gdsmith]

Sorry, I tried to simplify the code, but obviously didn't go far enough.

Do you expect some tanglement of the property and the variable so that PHPStan understands "if $context is SqsContext then $this->queue is SqsQueue"

Yes, this is the expectation, It's possible to determine from the code that that is what is happening.

For what it's worth, Typescript does seems happy with it

interface Queue {}

class SnsQsQueue implements Queue {}

class SqsQueue implements Queue {}

interface Context {
  createQueue(name: string): Queue
}

class SnsQsContext implements Context {
  createQueue(name: string): SnsQsQueue {
    return new SnsQsQueue;
  }
  declareQueue(queue: SnsQsQueue): void {}
}

class SqsContext implements Context {
  createQueue(name: string): SqsQueue {
    return new SqsQueue;
  }
  declareQueue(queue: SqsQueue): void {}
}


class Success {
  private queue: SnsQsQueue|SqsQueue

  constructor(context: Context) {
    if (context instanceof SnsQsContext) {
      this.queue = context.createQueue('test');
      context.declareQueue(this.queue);
    } else if (context instanceof SqsContext) {
      this.queue = context.createQueue('test');
      context.declareQueue(this.queue);
    } else {
      throw new Error('nope');
    }
  }
}

class Fail {
  private queue: SnsQsQueue|SqsQueue

  constructor(context: Context) {
    if (context instanceof SnsQsContext) {
      this.queue = context.createQueue('test');
    } else if (context instanceof SqsContext) {
      this.queue = context.createQueue('test');
    } else {
      throw new Error('nope');
    }
    context.declareQueue(this.queue);
  }
}

[ondrejmirtes]

Currently it should be possible to make it work at least with variables: https://phpstan.org/r/481e0499-a664-42d2-851b-bd5df9c7c55a if we write the code that checks each inner type of the union type separately

To also make it work with properties means that "dependent variables" feature should be generalized to "dependent expressions" feature.

[phpstan-bot]

@gdsmith After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 7.4 – 8.0 (1 error)
+PHP 7.4 – 8.0
 ==========
 
-64: Parameter #1 $name of method SnsQsContext::declareQueue() expects SnsQsQueue, SnsQsQueue|SqsQueue given.
+No errors
 
-PHP 7.1 – 7.3 (3 errors)
+PHP 7.1 – 7.3 (2 errors)
 ==========
 
 12: Return type SnsQsQueue of method SnsQsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
-19: Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
-64: Parameter #1 $name of method SnsQsContext::declareQueue() expects SnsQsQueue, SnsQsQueue|SqsQueue given.
+19: Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().

Full report

PHP 7.4 – 8.0

No errors

PHP 7.1 – 7.3 (2 errors)

Line	Error
12	Return type SnsQsQueue of method SnsQsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
19	Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().

[phpstan-bot]

@ondrejmirtes After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 7.4 – 8.0 (3 errors)
+PHP 7.4 – 8.0 (2 errors)
 ==========
 
 64: Dumped type: SnsQsContext|SqsContext
 65: Dumped type: SnsQsQueue|SqsQueue
-66: Parameter #1 $name of method SnsQsContext::declareQueue() expects SnsQsQueue, SnsQsQueue|SqsQueue given.
 
-PHP 7.1 – 7.3 (5 errors)
+PHP 7.1 – 7.3 (4 errors)
 ==========
 
 12: Return type SnsQsQueue of method SnsQsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
 19: Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
 64: Dumped type: SnsQsContext|SqsContext
-65: Dumped type: SnsQsQueue|SqsQueue
-66: Parameter #1 $name of method SnsQsContext::declareQueue() expects SnsQsQueue, SnsQsQueue|SqsQueue given.
+65: Dumped type: SnsQsQueue|SqsQueue

Full report

PHP 7.4 – 8.0 (2 errors)

Line	Error
64	`Dumped type: SnsQsContext
65	`Dumped type: SnsQsQueue

PHP 7.1 – 7.3 (4 errors)

Line	Error
12	Return type SnsQsQueue of method SnsQsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
19	Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
64	`Dumped type: SnsQsContext
65	`Dumped type: SnsQsQueue

[phpstan-bot]

@ondrejmirtes After the latest push in 1.8.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 7.4 – 8.0 (1 error)
+PHP 7.4 – 8.0
 ==========
 
-60: Parameter #1 $name of method SnsQsContext::declareQueue() expects SnsQsQueue, SnsQsQueue|SqsQueue given.
+No errors
 
-PHP 7.1 – 7.3 (3 errors)
+PHP 7.1 – 7.3 (2 errors)
 ==========
 
 12: Return type SnsQsQueue of method SnsQsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
-19: Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
-60: Parameter #1 $name of method SnsQsContext::declareQueue() expects SnsQsQueue, SnsQsQueue|SqsQueue given.
+19: Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().

Full report

PHP 7.4 – 8.0

No errors

PHP 7.1 – 7.3 (2 errors)

Line	Error
12	Return type SnsQsQueue of method SnsQsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().
19	Return type SqsQueue of method SqsContext::createQueue() is not compatible with return type Queue of method Context::createQueue().

[ondrejmirtes]

/cc @rvanvelzen Regression test please 😊

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.