Skip to content

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

Merged

Conversation

alexeyinkin
Copy link

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
README.md Outdated

The executable is bin/phpstan
Rut it as
bin/phpstan analyze -c your-config.neon -l 8 path-to-analyze
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

Choose a reason for hiding this comment

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

Done.

Copy link
Member

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

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.

README.md Outdated
Run:
```bash
export PHPSTAN_ALLOW_XDEBUG=1
vendor/bin/phpunit --bootstrap=tests/bootstrap.php --stop-on-failure tests
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

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

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

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:

  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'
  1. 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

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

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.

@ondrejmirtes ondrejmirtes force-pushed the bug2914_always-inheritdoc_rebase branch from 8bcb460 to 127bb52 Compare May 8, 2020 08:43
@ondrejmirtes
Copy link
Member

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 ondrejmirtes force-pushed the bug2914_always-inheritdoc_rebase branch from 048ccf9 to ea57a4f Compare May 8, 2020 09:28
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


public function method(): void
{
// assertType('InheritDocMergingVar\B', $this->property);
Copy link
Member

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

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
Copy link
Contributor

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

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
Copy link
Contributor

@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

This is expected. See phpstan/phpstan#3350

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants