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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

ondrejmirtes merged 7 commits into phpstan:master from alexeyinkin:bug2914_always-inheritdoc_rebase

May 8, 2020

Merged

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

ondrejmirtes merged 7 commits into phpstan:master from alexeyinkin:bug2914_always-inheritdoc_rebase

May 8, 2020

Conversation

@alexeyinkin

Copy link

@alexeyinkin alexeyinkin commented May 6, 2020

A rebased copy of #193

@alexeyinkin alexeyinkin force-pushed the bug2914_always-inheritdoc_rebase branch from 46eb7de to 1b6acac Compare

May 6, 2020 10:39

staabm

staabm reviewed

May 6, 2020

View reviewed changes

README.md Outdated

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Author

@alexeyinkin alexeyinkin May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link

Member

@ondrejmirtes ondrejmirtes May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Author

@alexeyinkin alexeyinkin May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

adaamz reviewed

May 7, 2020

View reviewed changes

README.md Outdated

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Author

@alexeyinkin alexeyinkin May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Member

@ondrejmirtes ondrejmirtes May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Author

@alexeyinkin alexeyinkin May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

--xdebug option is not passed to phpstan by the test.
CommandHelper checks !$allowXdebug and calls XdebugHandler::check().
XdebugHandler checks getenv($this->envAllowXdebug) and finds that it is not set.
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'

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.

Copy link

Member

@ondrejmirtes ondrejmirtes May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Author

@alexeyinkin alexeyinkin May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Alexey Inkin added 2 commits

May 8, 2020 10:43

@ondrejmirtes


 PHPCS - ignore tests cache

0beb9dd

@ondrejmirtes


 Merge inherited PHPDocs

127bb52

@ondrejmirtes ondrejmirtes force-pushed the bug2914_always-inheritdoc_rebase branch from 8bcb460 to 127bb52 Compare

May 8, 2020 08:43

@ondrejmirtes

Copy link

Member

ondrejmirtes commented May 8, 2020

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 added 4 commits

May 8, 2020 10:57

@ondrejmirtes


 Get rid of cloning

a3b712b

@ondrejmirtes


 Get rid of ClassReflection::requireFileName()

ca7ac7d

@ondrejmirtes


 Reworked InvalidThrowsPhpDocValueRule without NodeScopeResolver

f073e73

@ondrejmirtes


 Not sure about this

ea57a4f

@ondrejmirtes ondrejmirtes force-pushed the bug2914_always-inheritdoc_rebase branch from 048ccf9 to ea57a4f Compare

May 8, 2020 09:28

@ondrejmirtes


 Reworked tests

cad7f20

ondrejmirtes

ondrejmirtes requested changes

May 8, 2020

View reviewed changes

Copy link

Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tests/PHPStan/Analyser/data/inherit-phpdoc-merging-var.php

public function method(): void

{

// assertType('InheritDocMergingVar\B', $this->property);

Copy link

Member

@ondrejmirtes ondrejmirtes May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ondrejmirtes

Copy link

Member

ondrejmirtes commented May 8, 2020

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).

@ondrejmirtes ondrejmirtes merged commit b86f3a4 into phpstan:master

May 8, 2020

This was referenced May 8, 2020

Always @inheritdoc phpstan/phpstan#2914

Closed

Parent PHPDoc type not used when child has its own PHPDoc phpstan/phpstan#2898

Closed

{@inheritdoc} annotation overrides any annotation of the extended method phpstan/phpstan#3230

Closed

PhpDoc overrides are ignored phpstan/phpstan#2728

Closed

False negative when using inheritDoc and overwriting return type phpstan/phpstan#2561

Closed

Unexpected phpdoc @return behaviour with class implementing interface. phpstan/phpstan#2552

Closed

Replacing parameter type in @inerhitdoc with @param is no longer working phpstan/phpstan#2281

Closed

Incorrect {@inheritdoc} precedence phpstan/phpstan#2206

Closed

Improve PHPDoc inheritance phpstan/phpstan#671

Closed

Incorrect {@inheritdoc} precedence phpstan/phpstan#306

Closed

False positive error in return type with inheritdoc and comment in interface phpstan/phpstan#3219

Closed

@phil-davis

Copy link

Contributor

phil-davis commented May 11, 2020

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

Copy link

Member

ondrejmirtes commented May 11, 2020

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.

@deleugpn deleugpn mentioned this pull request

May 11, 2020

/** @phpstan ignore */ stopped working phpstan/phpstan#3279

Closed

@VincentLanglet

Copy link

Contributor

VincentLanglet commented May 27, 2020

@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

Copy link

Member

ondrejmirtes commented May 28, 2020

This is expected. See phpstan/phpstan#3350

@Majkl578 Majkl578 mentioned this pull request

May 29, 2020

Exceptions declared on interfaces unused in implementations reported as unused pepakriz/phpstan-exception-rules#124

Closed

@pepakriz pepakriz mentioned this pull request

Jul 29, 2020

Issue after phpstan upgrade (unused & missing at the same time) pepakriz/phpstan-exception-rules#134

Closed

Labels

None yet

6 participants

@alexeyinkin @ondrejmirtes @phil-davis @VincentLanglet @staabm @adaamz

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

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

Uh oh!

Conversation

@alexeyinkin alexeyinkin commented May 6, 2020

Uh oh!

@staabm staabm May 6, 2020

Choose a reason for hiding this comment

Uh oh!

@alexeyinkin alexeyinkin May 7, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes May 7, 2020

Choose a reason for hiding this comment

Uh oh!

@alexeyinkin alexeyinkin May 8, 2020

Choose a reason for hiding this comment

Uh oh!

@adaamz adaamz May 7, 2020

Choose a reason for hiding this comment

Uh oh!

@alexeyinkin alexeyinkin May 7, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes May 7, 2020

Choose a reason for hiding this comment

Uh oh!

@adaamz adaamz May 7, 2020

Choose a reason for hiding this comment

Uh oh!

@alexeyinkin alexeyinkin May 8, 2020

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes May 8, 2020

Choose a reason for hiding this comment

Uh oh!

@alexeyinkin alexeyinkin May 8, 2020

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented May 8, 2020

Uh oh!

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes May 8, 2020

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented May 8, 2020

Uh oh!

phil-davis commented May 11, 2020

Uh oh!

ondrejmirtes commented May 11, 2020

Uh oh!

VincentLanglet commented May 27, 2020

Uh oh!

ondrejmirtes commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants