-
Notifications
You must be signed in to change notification settings - Fork 533
Local type aliases #460
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
Local type aliases #460
Conversation
Thank you, this looks very promising. I'm also doing everything with "80/20" in mind 😊
c4f3c13 to
383b9f7
Compare
Hi, I'm gonna rebase this myself as I just implemented nested generic bounds on master (e671cc0) and I caused quite a few conflicts for you. Stay tuned :)
c823aa2 to
c8de393
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased this and added one commit that fixes prefix priority. I really like this! :)
Another test idea: Please try to use an alias right after defining it/importing in class docblock, for example in a @method tag.
Oh, wrong rebase, will fix the problem :)
And please, go through the review one-by-one and add each fix as a new commit :)
57e1645 to
8d275bd
Compare
Alright, giving the branch back to you :) I don't know about the memory limited exceeded in the few failing jobs - it's probably a coincidence that it happened in this branch.
Maybe the memory is leaking somewhere after all...
643ffd8 to
eff0cad
Compare
So, I've fixed some of the issues from your review. I'd say the most notable change is 210cb62 in which I've wrapped every type alias declaration in a TypeAlias class along with its name scope, allowing it to be resolved lazily later. This works around a couple of issues with nested and imported aliases.
I'll try and take a look at the memory issues too, they don't seem to be just a coincidence 🤔
Looking forward to this being finished, it looks awesome already :)
This adds support for nested type aliases and helps preserve scope when importing aliases. Aliases are resolved lazily because eager resolution would create a chicken-and-egg problem where aliases need to be resolved while they are already being resolved, thus not being resolved at all as a result of recursion protection.
...no longer stored in cache
eff0cad to
98a8e8b
Compare
...failing the resolution
98a8e8b to
3519a52
Compare
...lving the doc block
While thinking about ObjectType and IdentifierTypeNode, I realized unwanted behaviour:
<?php /** @phpstan-type int \stdClass */ class Fooooo { /** @param int $a */ public function doFoo($a) { \PHPStan\dumpType($a); // should be int, but current is stdClass } }
I'm gonna solve it by deleting TypeAliasesTypeNodeResolverExtension and doing the logic in the right place in TypeNodeResolver::resolveIdentifierTypeNode() (after things that are never supposed to be aliased are handled). It should have performance benefits as well.
src/Type/TypeAliasResolver.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fine? @jiripudil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks fine.
Alright, I think we're green :)
I have two more things to ask from you:
- Please add another condition checked in the rule - that
@phpstan-type int ShouldNotHappenisn't allowed. I think you could pass theintportion to theTypeNodeResolver::resolveand it should result inObjectType. - Please send a PR that edits https://github.com/phpstan/phpstan/edit/master/website/src/writing-php-code/phpdoc-types.md and adds documentation about local type aliases :)
Thank you.
Glad to hear that :) I'll add the check and the docs.
Thank you for your help!
I wonder why nobody noticed these issues in phpstan/phpstan-src#460... And lack of circular reference means we can't really typecheck `yield $promise;` properly.
Resolves phpstan/phpstan#3001. Depends on phpstan/phpdoc-parser#62.
Since Psalm only seems to support class-scoped type aliases, I did the same here. As I've said before, "it seems to be the easiest direction to take and is imo a good first iteration" that neatly follows the 80/20 rule.
I think the implementation still has some rough edges, but it appears to work surprisingly well 😅 looking forward to your thoughts and remarks!