Skip to content

Conversation

@KmeCnin
Copy link
Contributor

@KmeCnin KmeCnin commented Feb 17, 2023

This PR fixes bug reported here: phpstan/phpstan#8609

@KmeCnin KmeCnin force-pushed the bug8609 branch 4 times, most recently from a68edf9 to f7f7f40 Compare February 20, 2023 09:14
@KmeCnin KmeCnin marked this pull request as ready for review February 20, 2023 10:12
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems you are adding a new test-case here, but you did not analyze this file from any of the *Test classes.
you need to make sure that this file is analyzed from the correct *Test class - the one which is testing the rule which emits the error you are trying to fix

Copy link
Member

Choose a reason for hiding this comment

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

  1. File name needs to be in bug-8609.php format.
  2. File needs to be analysed by tests/PHPStan/Rules/PhpDoc/MethodConditionalReturnTypeRuleTest.php.

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.

Additionally, we need to test that this actually works. A new file tested in NodeScopeResolverTest would be nice.

Copy link
Member

Choose a reason for hiding this comment

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

union isn't the best operation for this. I suggest array_merge instead, like here:

new TemplateTypeMap(array_merge(
$this->templateTypeMap->getTypes(),
$map->getTypes(),
)),

Copy link
Member

Choose a reason for hiding this comment

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

  1. File name needs to be in bug-8609.php format.
  2. File needs to be analysed by tests/PHPStan/Rules/PhpDoc/MethodConditionalReturnTypeRuleTest.php.

@ondrejmirtes
Copy link
Member

Finished it myself :) Thank you!

@ondrejmirtes ondrejmirtes merged commit 1292afe into phpstan:1.10.x Mar 9, 2023
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.

4 participants