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.

Sign up for GitHub

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

Jump to bottom

Added integer range phpdoc support #596

Merged
merged 1 commit into from Jul 30, 2021
Merged

Added integer range phpdoc support #596

merged 1 commit into from Jul 30, 2021

Conversation

clxmstaab
Copy link
Contributor

@clxmstaab clxmstaab commented Jul 28, 2021 •
edited

as suggested in phpstan/phpstan#3383 (comment) I added a failling unit test and started to look arround in TypeNodeResolver.

@clxmstaab
Copy link
Contributor Author

clxmstaab commented Jul 28, 2021 •
edited

I have not really a idea where to start more in detail investigation of TypeNodeResolver... could someone give me a hint?

the test is currently failling with

There was 1 failure:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "C:\dvl\GitHub\phpstan-src\tests\PHPStan\Analyser/data/integer-range-types.php:167" ('type', 'C:\dvl\GitHub\phpstan-src\tes...es.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\ErrorType Object (...), 167)
Expected type int<0, 100>, got type *ERROR* in C:\dvl\GitHub\phpstan-src\tests\PHPStan\Analyser/data/integer-range-types.php on line 167.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'int<0, 100>'
+'*ERROR*'

C:\dvl\GitHub\phpstan-src\src\Testing\TypeInferenceTestCase.php:124
C:\dvl\GitHub\phpstan-src\tests\PHPStan\Analyser\NodeScopeResolverTest.php:466

@ondrejmirtes
Copy link
Member

As I said it needs to be implemented in TypeNodeResolver, specifically in resolveGenericTypeNode. It needs to resolve to IntegerRangeType.

@clxmstaab
Copy link
Contributor Author

As I said it needs to be implemented in TypeNodeResolver

sorry, I mixed up TypeNodeResolver and NodeScopeResolver :-/

specifically in resolveGenericTypeNode. It needs to resolve to IntegerRangeType.

thx for the hint, this was really usefull

I was able to implement the minmal test-cases which came to my mind. I am pretty sure you will not like the if-cases arround the GenericClassStringType class-string comparison, but maybe you can give me another hint, how this could be improved

ondrejmirtes
ondrejmirtes requested changes Jul 28, 2021
View reviewed changes
src/PhpDoc/TypeNodeResolver.php Outdated
if ($minType instanceof ConstantIntegerType) {
$min = $minType->getValue();
} elseif ($minType instanceof ObjectType) {
if ($minType->getClassName() === "PHPStan\Generics\GenericClassStringType\min") {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense, it's not going to work in a different namespace. Try looking for IdentifierTypeNode in $typeNode->genericTypes instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, after reading your comment, I realized why the class-name contained this PHPStan prefixed string, which I initially thought of is a magic-value ;-)

changed the impl to use IdentifierTypeNode and added a few more testcases.

thanks again for your input

@clxmstaab clxmstaab changed the title Added failling testcase for integer range phpdoc Added integer range phpdoc support Jul 28, 2021
@clxmstaab clxmstaab marked this pull request as ready for review July 28, 2021 16:13
staabm
staabm reviewed Jul 30, 2021
View reviewed changes
src/Dependency/ExportedNode/ExportedClassConstantNode.php Outdated
@@ -83,7 +83,7 @@ public static function decode(array $data): ExportedNode
$data['public'],
$data['private'],
$data['final'],
$data['phpDoc'],
$data['phpDoc'] !== null ? ExportedPhpDocNode::decode($data['phpDoc']['data']) : null
Copy link
Contributor

Choose a reason for hiding this comment

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

Fyi, This change was not added by me

@ondrejmirtes
Copy link
Member

Just rebased and force-pushed this. Thank you!

@ondrejmirtes ondrejmirtes merged commit d218134 into phpstan:master Jul 30, 2021
@clxmstaab clxmstaab deleted the integer-range branch July 30, 2021 07:57
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
3 participants
@clxmstaab @ondrejmirtes @staabm