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 support for the key-of<...> type #800

Merged
merged 3 commits into from
Nov 26, 2021
Merged

Add support for the key-of<...> type #800

merged 3 commits into from
Nov 26, 2021

Conversation

ste93cry
Copy link
Contributor

@ste93cry ste93cry commented Nov 24, 2021

Fixes phpstan/phpstan#6084. This is my attempt at implementing the key-of<...> type borrowed from Psalm. However, I'm not sure where I should place the tests as I see a lot of files, but none that looks the right one. Also, Psalm tells the user precise errors when an invalid value is used as generic argument of the key-of type, however the resolveGenericTypeNode() method cannot return a RuleError and so I wonder how I could return a custom error message for certain cases. Any advice on these two qustions would be highly appreciated

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.

When TypeNodeResolver returns ErrorType, it's picked up by existing rules, for example: https://phpstan.org/r/2ae98fc7-9829-42bc-8a25-51e4ef022dad ("contains unresolvable type")

The tests should be done in NodeScopeResolverTest.

Also please add support for value-of.

src/PhpDoc/TypeNodeResolver.php Outdated Show resolved Hide resolved
@ste93cry
Copy link
Contributor Author

Also please add support for value-of.

Since it's a separated feature (although it works basically the same), wouldn't be better to implement key-of first and then contribute value-of in a separate PR?

When TypeNodeResolver returns ErrorType, it's picked up by existing rules

I took as reference the class-string type and I could not find a rule that prints an error if the type could not be resolved, and in fact PHPStan just tells me that it is unresolvable. I would like to go even further and personalize the error message for certain cases. Making these checks inside the rule means that I should account for all possible places where the type could be used (param, variable, etc), which doesn't look reasonable and maintenable...

@ondrejmirtes
Copy link
Member

wouldn't be better to implement key-of first and then contribute value-of in a separate PR?

Since they're closely connected and the implementation is just three lines of code, I want them together.

would like to go even further and personalize the error message for certain cases

I don't see this being possible without major refactoring, it certainly shouldn't be a concern for this PR

@ste93cry
Copy link
Contributor Author

ste93cry commented Nov 26, 2021

Since they're closely connected and the implementation is just three lines of code, I want them together.

Ok, I've added the missing feature

I don't see this being possible without major refactoring, it certainly shouldn't be a concern for this PR

Absolute agree, if that's the case 😃

The tests should be done in NodeScopeResolverTest.

I've added them, are they enough? I would have expected also some tests to assert that when a wrong value is given an error is thrown, but in this file it looks like all tests just assert the type is the expected one 🤔

@ondrejmirtes
Copy link
Member

I've added them, are they enough?

You can add tests for the rule that you expect to report an error.

@ste93cry
Copy link
Contributor Author

Done. PR is ready for review, let me know if there is something that I still have to do or feel free to make the changes yourself if you want

@ste93cry ste93cry marked this pull request as ready for review November 26, 2021 21:50
@ondrejmirtes ondrejmirtes merged commit 1056f69 into phpstan:master Nov 26, 2021
@ondrejmirtes
Copy link
Member

Thank you! Feel free to update the docs at https://github.com/phpstan/phpstan/edit/master/website/src/writing-php-code/phpdoc-types.md (https://phpstan.org/writing-php-code/phpdoc-types)

@ste93cry ste93cry deleted the add-key-of-type-support branch November 26, 2021 21:55
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