Fix 2914 (rebased): Merge PHPDoc block tags when inheriting

[alexeyinkin]

A rebased copy of #193

[staabm]

Should use code fences (like other similar commands in this file)

[alexeyinkin]

Done.

[ondrejmirtes]

The development version should be run as vendor/bin/phing phpstan instead.

[alexeyinkin]

vendor/bin/phing phpstan is to run phpstan on itself. I mean the command to analyse custom code using dev phpstan as one modifies phpstan.

[adaamz]

does PHPSTAN_ALLOW_XDEBUG=1 vendor/bin/phpunit --bootstrap=tests/bootstrap.php --stop-on-failure tests work, or it needs to be exported?

[alexeyinkin]

This should work as well, but I find export easier to read.

[ondrejmirtes]

Not sure what you're referring to, this variable isn't anywhere in the codebase: https://github.com/phpstan/phpstan-src/search?q=PHPSTAN_ALLOW_XDEBUG&unscoped_q=PHPSTAN_ALLOW_XDEBUG

[adaamz]

but export sets variable for whole terminal session, if i prepend it to command, it should be set only for that process

[alexeyinkin]

Run this with xdebug enabled:

vendor/bin/phpunit --bootstrap=tests/bootstrap.php --stop-on-failure --filter=testConfigurationAutoDiscovery tests/PHPStan/Command/AnalyseCommandTest.php

The following will happen:

1. --xdebug option is not passed to phpstan by the test.
2. CommandHelper checks !$allowXdebug and calls XdebugHandler::check().
3. XdebugHandler checks getenv($this->envAllowXdebug) and finds that it is not set.
4. XdebugHandler then restarts the command using the only command it can find:

'/usr/bin/php7.3' '-n' '-c' '/tmp/rIqFYd' '/home/alexey/work/phpstan-src/vendor/phpunit/phpunit/phpunit' '--bootstrap=tests/bootstrap.php' '--stop-on-failure' '--filter=testConfigurationAutoDiscovery' 'tests/PHPStan/Command/AnalyseCommandTest.php' '--ansi'

5. PHPUnit dies as it cannot recognize the --ansi option.

The stack of this restart is

/home/alexey/work/phpstan-src/vendor/composer/xdebug-handler/src/XdebugHandler.php:136
/home/alexey/work/phpstan-src/src/Command/CommandHelper.php:51
/home/alexey/work/phpstan-src/src/Command/AnalyseCommand.php:126
/home/alexey/work/phpstan-src/vendor/symfony/console/Command/Command.php:255
/home/alexey/work/phpstan-src/vendor/symfony/console/Tester/CommandTester.php:76
/home/alexey/work/phpstan-src/tests/PHPStan/Command/AnalyseCommandTest.php:84
/home/alexey/work/phpstan-src/tests/PHPStan/Command/AnalyseCommandTest.php:25
/home/alexey/work/phpstan-src/vendor/phpunit/phpunit/src/Framework/TestCase.php:1154

The way to prevent this is to set the environment variable with a name constructed in XdebugHandler::__construct as

$this->envAllowXdebug = self::$name.self::SUFFIX_ALLOW;

which results in the name of PHPSTAN_ALLOW_XDEBUG.

[ondrejmirtes]

Can you test this has the same effect? Thanks! 6fa7b67

[alexeyinkin]

Yes, it worked. You may want to add a comment there since as you said PHPSTAN_ALLOW_XDEBUG is not in the codebase.

[ondrejmirtes]

Added the README update to master separately: e96473d and forcepushed this branch. I'm gonna start review, it's possible I'll divide it more again :)

[ondrejmirtes]

I like this a lot! BTW have you considered what should be done about @template T and for example @param T tags? They can't be simply merged, the scope of the types (TemplateTypeScope) needs to change to reflect the child class.

I'm gonna merge this, please send a new PR with the fixed test. Thanks <3

[ondrejmirtes]

This should pass but fails, that's why it's commented for now.

[ondrejmirtes]

Also feel free to check out the commits I added. The main reason why I changed the tests is that checking out types inside the method (through PhpFunctionFromParserNodeReflection) and checking the types outside the method (getting the return type from Reflection) are two different code paths - see that NodeScopeResolver::getPhpDocs is called in itself (for PhpFunctionFromParserNodeReflection), and in PhpClassReflectionExtension (for outsiders).

[phil-davis]

This is "a good thing" (TM) - thanks.
Now we are getting error reports where the parent interface declares some @return, but the class that implements the interface does not actually return anything. In particular it found places where the interface had:

	 * @return null

and the implementation has no return statement at all.

Probably in that case the interface should just have:

	 * @return void

And that keeps phpstan happy that the implementation has no return statement at all.

Otherwise at the end of the method implementation I have to add:

return null;

which seems a crappy thing to do.

[ondrejmirtes]

This is expected - void and null and interchangeable as values in PHP (void method returns null) but not types, so PHPStan does the right thing here - implementation should follow interface.

[VincentLanglet]

@alexeyinkin @ondrejmirtes I think there is something wrong with throws tag.

When writing

class A {
  /** @throws \LogicException */
  public method foo()
  {
    throw \LogicException();
  }
}

class B extends A {
  public method foo()
  {
    return;
  }
}

Since B::foo has no @throws tag, it uses the @throw tag of A::foo.
Phpstan then believe it's

class B extends A {
  /** @throws \LogicException */
  public method foo()
  {
    return;
  }
}

This is a big issue for https://packagist.org/packages/pepakriz/phpstan-exception-rules users.
Because phpstan will report an unused exception for B::foo or will ask for using try/catch when using B::foo.

[ondrejmirtes]

This is expected. See phpstan/phpstan#3350