Sitelet https://github.com/phpstan/phpstan-src/pull/2241
Skip to content

Give class template context to conditional checker - #2241

Merged
ondrejmirtes merged 7 commits into
phpstan:1.10.xfrom
KmeCnin:bug8609
Mar 9, 2023
Merged

Give class template context to conditional checker#2241
ondrejmirtes merged 7 commits into
phpstan:1.10.xfrom
KmeCnin:bug8609

Conversation

@KmeCnin

@KmeCnin KmeCnin commented Feb 17, 2023

Copy link
Copy Markdown
Contributor

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

phpstan-bot commented Feb 20, 2023

Copy link
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

@KmeCnin
KmeCnin force-pushed the bug8609 branch 2 times, most recently from f7f7f40 to 7c3e695 Compare February 24, 2023 15:09
@KmeCnin KmeCnin mentioned this pull request Mar 3, 2023
Closed
staabm
staabm reviewed Mar 3, 2023
View reviewed changes
Comment thread tests/PHPStan/Generics/data/bug8609.php Outdated

@staabm staabm Mar 3, 2023

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

@ondrejmirtes ondrejmirtes Mar 8, 2023

Copy link
Copy Markdown
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
ondrejmirtes requested changes Mar 8, 2023
View reviewed changes

@ondrejmirtes ondrejmirtes left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/Rules/PhpDoc/MethodConditionalReturnTypeRule.php Outdated

@ondrejmirtes ondrejmirtes Mar 8, 2023

Copy link
Copy Markdown
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:

phpstan-src/src/Analyser/NameScope.php

Lines 164 to 167 in b250a8b

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

Comment thread tests/PHPStan/Generics/data/bug8609.php Outdated

@ondrejmirtes ondrejmirtes Mar 8, 2023

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

ondrejmirtes commented Mar 9, 2023

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

Reviewers

@staabm staabm staabm left review comments
@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KmeCnin @phpstan-bot @ondrejmirtes @staabm