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

Local type aliases #460

Merged
merged 25 commits into from
Apr 18, 2021
Merged

Conversation

jiripudil
Copy link
Contributor

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!

@ondrejmirtes
Copy link
Member

Thank you, this looks very promising. I'm also doing everything with "80/20" in mind 😊

@jiripudil jiripudil force-pushed the local-type-aliases branch 2 times, most recently from c4f3c13 to 383b9f7 Compare February 27, 2021 20:51
@ondrejmirtes
Copy link
Member

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 :)

@ondrejmirtes ondrejmirtes force-pushed the local-type-aliases branch 2 times, most recently from c823aa2 to c8de393 Compare February 28, 2021 14:39
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.

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.

src/PhpDoc/Tag/TypeAliasTag.php Outdated Show resolved Hide resolved
src/PhpDoc/Tag/TypeAliasTag.php Outdated Show resolved Hide resolved
src/Rules/Generics/TemplateTypeCheck.php Show resolved Hide resolved
src/Type/TypeAliasResolver.php Outdated Show resolved Hide resolved
src/PhpDoc/Tag/TypeAliasImportTag.php Outdated Show resolved Hide resolved
src/Reflection/ClassReflection.php Outdated Show resolved Hide resolved
@ondrejmirtes
Copy link
Member

Oh, wrong rebase, will fix the problem :)

And please, go through the review one-by-one and add each fix as a new commit :)

@ondrejmirtes
Copy link
Member

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.

@ondrejmirtes
Copy link
Member

Maybe the memory is leaking somewhere after all...

@jiripudil jiripudil force-pushed the local-type-aliases branch 2 times, most recently from 643ffd8 to eff0cad Compare March 27, 2021 17:23
@jiripudil jiripudil marked this pull request as ready for review March 27, 2021 17:26
@jiripudil
Copy link
Contributor Author

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 🤔

@ondrejmirtes
Copy link
Member

Looking forward to this being finished, it looks awesome already :)

@ondrejmirtes
Copy link
Member

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.

if (array_key_exists($aliasNameInClassScope, $this->resolvedLocalTypeAliases)) {
return $this->resolvedLocalTypeAliases[$aliasNameInClassScope];
}

Copy link
Member

Choose a reason for hiding this comment

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

Is this fine? @jiripudil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, looks fine.

@ondrejmirtes
Copy link
Member

Alright, I think we're green :)

I have two more things to ask from you:

  1. Please add another condition checked in the rule - that @phpstan-type int ShouldNotHappen isn't allowed. I think you could pass the int portion to the TypeNodeResolver::resolve and it should result in ObjectType.
  2. 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.

@ondrejmirtes ondrejmirtes merged commit b9aeed4 into phpstan:master Apr 18, 2021
@jiripudil
Copy link
Contributor Author

Glad to hear that :) I'll add the check and the docs.

Thank you for your help!

@jiripudil jiripudil deleted the local-type-aliases branch April 18, 2021 18:42
SOF3 added a commit to SOF3/await-generator that referenced this pull request Apr 21, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants