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

Fix trait detection recursion for anonymous classes #946

Merged
merged 6 commits into from
Jan 27, 2022
Merged

Fix trait detection recursion for anonymous classes #946

merged 6 commits into from
Jan 27, 2022

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Jan 26, 2022

Closes phpstan/phpstan#6442, aftermath of 11176c7.

This was a missing edge case that lead to endless recursion in case the trait looking for and anonymous class using it were in the same file. By not skipping the node, it would start looking at the anonymous class, find the trait usage, start looking for the trait, find it, come to the anonymous class, find the trait usage, start looking for the trait, ..

@ondrejmirtes
Copy link
Member

Hi, I really need the test to be able to merge this. Can you push what you've tried to reproduce it?

@herndlm
Copy link
Contributor Author

herndlm commented Jan 27, 2022

Oh I have a different behaviour now on my other machine. Sure, I'll check that out and push that then. Might take a bit.
UPDATE: there seems to be a second recursion problem. Thinking about splitting the PRs :/

@ondrejmirtes
Copy link
Member

Also - please use the original source code (it has more classes):

trait T
{
    public function x(): void
	{
		\PHPStan\dumpType(parent::class);
	}
}

class A {}

class B extends A
{
	use T;
}

new class() extends B
{
	use T;
};

@herndlm
Copy link
Contributor Author

herndlm commented Jan 27, 2022

Very odd, it started behaving the same strange way now too. Initially it was working (and showing another recursion problem), then I added and removed classes from the test file and now I always get reflection errors. It feels like some caching or so could be the culprit, but even after deleting all caches I can think of I have the same errors :/
Let's see what CI tells..
I get e.g.

Reflection error: Bug6442\T not found

when running via php vendor/bin/phpunit tests/PHPStan/Analyser/AnalyserIntegrationTest.php. Am I short-circuiting something there?

@herndlm
Copy link
Contributor Author

herndlm commented Jan 27, 2022

Also - please use the original source code (it has more classes):

this was on purpose for now, as the PR here does not yet fix another recursion problem with parent:: in the trait :/

@ondrejmirtes
Copy link
Member

So please fix all of the issues related to that bug report (otherwise it's not really a regression test for that bugreport and cannot be merged/closed). Thanks :)

@ondrejmirtes
Copy link
Member

Your reflection error might be fixable by running "composer dump" whenever you move/add classes.

@herndlm
Copy link
Contributor Author

herndlm commented Jan 27, 2022

Your reflection error might be fixable by running "composer dump" whenever you move/add classes.

that was really it! thx! I was not thinking that composer might be doing anything with those test files.
ok, I can now work on the full/clean fix :)

@herndlm herndlm marked this pull request as draft January 27, 2022 09:53
@herndlm
Copy link
Contributor Author

herndlm commented Jan 27, 2022

well, looks like something is still/now broken on Windows

@ondrejmirtes
Copy link
Member

$fileName === $originalClassFileName will be false probably because of different directory separators

Use FileHelper:normalizePath.

@herndlm
Copy link
Contributor Author

herndlm commented Jan 27, 2022

maybe I should try to normalize it earlier / only once, give me a minute

@herndlm herndlm marked this pull request as ready for review January 27, 2022 11:36
@ondrejmirtes ondrejmirtes merged commit d33a581 into phpstan:master Jan 27, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@herndlm herndlm deleted the fix-trait-detection-for-anonymous-classes branch January 27, 2022 12:02
@herndlm
Copy link
Contributor Author

herndlm commented Jan 27, 2022

Thank you too for the pointers 🎉

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