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

Add resolution of generic @method tags #2931

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

mad-briller
Copy link
Contributor

@mad-briller mad-briller commented Feb 22, 2024

Attempt to close: phpstan/phpstan#6371 (comment)

This is as far as i've managed to get today on this issue; thanks to your help i've managed to forward the TemplateTags through and correctly build the template type map, but now i can't get it to resolve the return types correctly 😅

The output i'm getting from the test is:

There were 2 failures:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/home/brad.miller/code/phpstan-src/tests/PHPStan/Analyser/data/generic-method-tags.php:21" ('type', '/home/brad.miller/code/phpsta...gs.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\ObjectType Object (...), 21)
Expected type int, got type GenericMethodTags\TVal in /home/brad.miller/code/phpstan-src/tests/PHPStan/Analyser/data/generic-method-tags.php on line 21.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'int'
+'GenericMethodTags\TVal'

/home/brad.miller/code/phpstan-src/src/Testing/TypeInferenceTestCase.php:102
/home/brad.miller/code/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:1450

2) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/home/brad.miller/code/phpstan-src/tests/PHPStan/Analyser/data/generic-method-tags.php:22" ('type', '/home/brad.miller/code/phpsta...gs.php', PHPStan\Type\Constant\ConstantStringType Object (...), PHPStan\Type\ObjectType Object (...), 22)
Expected type string, got type GenericMethodTags\TVal in /home/brad.miller/code/phpstan-src/tests/PHPStan/Analyser/data/generic-method-tags.php on line 22.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'string'
+'GenericMethodTags\TVal'

i suspect it's because the $returnType passed to AnnotationMethodReflection is resolved with only the class-level templates and not both class and method level templates in TemplateTypeHelper::resolveTemplateTypes. resolveTemplateTypes() is returning an ObjectType() which seems incorrect, but i'm not sure what it's supposed to be to cause it to be templated properly.

To resolve this i've tried:

  1. union the $classReflection->getActiveTemplateTypeMap() and my generated $templateTypeMap before passing it in to resolveTemplateTypes()
  2. removed the call to TemplateTypeHelper::resolveTemplateTypes, as i suspected it may be resolving too soon?

Sorry to keep asking, but is there any further pointers you have for me? Thanks.

@ondrejmirtes
Copy link
Member

Thi has something to do with NameScope (which contains information about what T refers to in PHPDoc) and might not be actually trivial to fix, I'll think about it 😊

@mad-briller mad-briller force-pushed the generic-method-tags branch 2 times, most recently from fab3208 to f94b1df Compare February 22, 2024 16:09
@mad-briller mad-briller marked this pull request as draft February 22, 2024 17:28
src/PhpDoc/PhpDocNodeResolver.php Outdated Show resolved Hide resolved
@ondrejmirtes ondrejmirtes marked this pull request as ready for review February 23, 2024 10:27
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes
Copy link
Member

I think I did it right.

@ondrejmirtes ondrejmirtes changed the title Wip: added resolution of generic @method tags. Add resolution of generic @method tags Feb 23, 2024
@mad-briller
Copy link
Contributor Author

ah nice! beat me to it, was just finishing breakfast and then going to take a look at this

@ondrejmirtes
Copy link
Member

Anyway, there's a very similar feature request - to do the same thing for callables and Closures. So first we need to finish phpstan/phpdoc-parser#199 and then to a very similar PR in phpstan-src to this one.

@ondrejmirtes ondrejmirtes merged commit e9b40b7 into phpstan:1.10.x Feb 23, 2024
367 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@staabm
Copy link
Contributor

staabm commented Feb 23, 2024

Nice!

@ondrejmirtes
Copy link
Member

I just realized we're missing some rules about this. Basically the equivalents of these: https://phpstan.org/r/ca12c484-b4f1-4e5b-a418-3069ec569411

The best way to implement them is to create a new rule similar to https://github.com/phpstan/phpstan-src/blob/1.11.x/src/Rules/Generics/MethodTemplateTypeRule.php#L78 that also calls TemplateTypeCheck.

Do you have time to implement this? Thanks .)

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