Skip to content

Allow process multiple UnreachableStatementNode on NodeScopeResolver::processNodes() #3745

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

samsonasik
Copy link
Contributor

/cc @TomasVotruba if this is accepted, this should hopefully will allow us to remove UnreachableStatementNodeVisitor on rector side.

https://github.com/rectorphp/rector-src/blob/e5d221e40d2c5acc1a92b1ded501321ee690be06/src/PHPStan/NodeVisitor/UnreachableStatementNodeVisitor.php#L17

which used if found a first UnreachableStatementNode, as phpstan only process first UnreachableStatementNode

@ondrejmirtes
Copy link
Member

I don't want to spam the user with more of these messages, one of them is enough. But I'd accept a PR that adds this information to UnreachableStatementNode as a new getter, something like getNextStatements(): array<Stmt>.

@samsonasik
Copy link
Contributor Author

I see, I will try

@samsonasik
Copy link
Contributor Author

@ondrejmirtes implemented 👍 , I added addNextStatement() and getNextStatements() methods

@samsonasik
Copy link
Contributor Author

samsonasik commented Dec 21, 2024

@ondrejmirtes I am not sure how unit test error is related:

There was 1 failure:

1) PHPStan\Analyser\NodeScopeResolverTest::testFile with data set "D:\a\phpstan-src\phpstan-src\tests\PHPStan\Analyser\nsrt\generic-unions.php" ('D:\a\phpstan-src\phpstan-src\...ns.php')
Failed assertions in D:\a\phpstan-src\phpstan-src\tests\PHPStan\Analyser\nsrt\generic-unions.php:

Line 120:
Expected: 3|'foo'
Actual:   mixed

could you help verify? thank you.

@samsonasik
Copy link
Contributor Author

@ondrejmirtes it seems working ok now, Ready to merge 👍

@ondrejmirtes ondrejmirtes changed the base branch from 2.0.x to 2.1.x December 21, 2024 15:05
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.

You're not actually testing getNextStatements anywhere.

Please create a new RuleTestCase inside tests/ and write an anonymous class in getRule() that would verify this information, for example by putting the next statements into reported errors.

@samsonasik
Copy link
Contributor Author

I will try

@samsonasik samsonasik force-pushed the allow-process-multiple-unreachable branch from 4ee2951 to ebc0855 Compare December 21, 2024 23:32
@samsonasik
Copy link
Contributor Author

I've added test for getNextStatements() 1083c0b

@samsonasik
Copy link
Contributor Author

@ondrejmirtes test added, Ready to merge 👍

@samsonasik
Copy link
Contributor Author

@ondrejmirtes mergeable?

@samsonasik samsonasik force-pushed the allow-process-multiple-unreachable branch from 747ca1c to 2d7efbc Compare January 1, 2025 07:25
@samsonasik
Copy link
Contributor Author

@ondrejmirtes Rebased 👍

@ondrejmirtes
Copy link
Member

I just pushed some test adjustments and a failing test for you.

The output for testRuleTopLevel is currently wrong. See: https://phpstan.org/r/7a2aafcd-10b3-405d-9402-90037f780059 - the top level function is not considered unreachable currently by PHPStan. But it's included in your "next statements" which is wrong.

@samsonasik
Copy link
Contributor Author

@ondrejmirtes I've fixed handling top level function 2427d86 👍

@ondrejmirtes ondrejmirtes merged commit e36bb83 into phpstan:2.1.x Jan 2, 2025
424 of 426 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@samsonasik samsonasik deleted the allow-process-multiple-unreachable branch January 2, 2025 11:24
@samsonasik
Copy link
Contributor Author

Thank you @ondrejmirtes I will test on rector-src 👍

@samsonasik
Copy link
Contributor Author

linked PR on rector-src for future reference, tested the UnreachableStatementNode::getNextStatements() and it seems working ok 🎉, see

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.

None yet

2 participants