Skip to content
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

Function nesting causes hangs and fatal error #254

Open
lookyman opened this issue May 5, 2017 · 11 comments

Comments

Projects
None yet
4 participants
@lookyman
Copy link
Member

commented May 5, 2017

I have discovered something interesting while testing a certain piece of code, and a followup investigation lead to this minimal example:

class Foo
{
	public function bar(): self
	{
		return $this;
	}

	public function baz()
	{
		$this
			->bar()
			->bar()
			->bar();
	}
}

Now, this code if perfectly ok, no problems here. But now, if you keep adding more ->bar() calls into the baz method, things start to happen. The analysis takes longer and longer, and at around 60 ->bar()s, it starts to get reeeally slow, and at 79 ->bar()s, you get a fatal error Maximum function nesting level of '256' reached. PHP itself can run the code with no problems.

This is a pretty extreme example, but with more complex classes, it is possile that this starts happening much earlier, and it could affect certain design patterns (builders, configurators, basicly everything with fluent interfaces) pretty quickly. I am not yet familiar enough with the PHPStan's core to even suggest if anything could be done with this.

Also, this could possibly be related to #191.

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented May 5, 2017

Yep, looks like the same issue, I will probably need to cache something along the way, because I suspect that it's getting the type repeatedly for each node the method is called on.

@lookyman

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

Quick fix for the user codebase would be to split these long chains into multiple, shorter ones.

$foo = $this->bar()->bar()->bar()->bar()->bar();
$foo = $foo->bar()->bar()->bar()->bar()->bar();
$foo = $foo->bar()->bar()->bar()->bar()->bar();
@lookyman

This comment has been minimized.

Copy link
Member Author

commented May 6, 2017

The long type resolution issue was fixed by #256, but the fatal error remains.

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented May 6, 2017

I know, I just reproduced it with b1524d3 - if you comment out the markTestSkipped line, you will get the same crash.

For now, I recommend you to run PHPStan with xdebug disabled, it will run much faster and you will not encounter this error. I don't have an easy fix for it now, it's caused by two things:

  • prettyPrintExpr in PHP-Parser is implemented recursively and it will crash on it. This was true even before you implemented the caching because it's already used for some other stuff.
  • getType and resolveType are also implemented recursively and the nested method calls represent a long chain of nested nodes.

So, at least we know exactly what it's caused by and we can fix it in the future.

@ondrejmirtes ondrejmirtes added the bug label Sep 1, 2017

@AJenbo

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

Both composer and phan (another static analyzer) solves this by restarting the process without xdebug.

https://github.com/composer/composer/blob/26a50b37624b20d73c861b7714c3d34cbba8e4e1/src/Composer/XdebugHandler.php

This is currently preventing me from running phpstan in a CI so if you like the idea I can try and port it to phpstan and make a pull request.

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Nov 15, 2017

@AJenbo You can definitely try that, thanks!

BTW Alternate solution is to have Xdebug disabled by default, and enable it only for PHPUnit (or for whatever tool you have it enabled in the first place).

@AJenbo

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

Unfortunately disabeling Xdebug is not an option in my case. I'll see if I can't squeeze out a patch by the end of the week :)

@JanTvrdik

This comment has been minimized.

Copy link
Member

commented Nov 15, 2017

Restarting the process without xdebug is just an ugly hack. It's better to teach people how to setup their dev environment properly, i.e. not loading xdebug extension in CLI by default and using some xdebug wrapper to debug CLI scripts, for example

#!/usr/bin/env bash

XDEBUG_PATH='path/to/xdebug/php_xdebug.dll'
XDEBUG_IDEKEY=PHPSTORM

export XDEBUG_CONFIG="idekey=$XDEBUG_IDEKEY"
php -dzend_extension="$XDEBUG_PATH$XDEBUG_FILE" "$@"
@AJenbo

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

@JanTvrdik I would tend to agree with you, but after failing to educate the world I'm now going to go for option two :)

@AJenbo

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2018

As per this commit 44af5fc this should no longer result in a crash.

@ondrejmirtes

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

@ondrejmirtes ondrejmirtes added performance and removed bug labels Jan 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.