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

Implement api for conditional type #1120

Closed
wants to merge 1 commit into from

Conversation

rvanvelzen
Copy link
Contributor

Part of implementation for phpstan/phpstan#3853, together with phpstan/phpdoc-parser#106.

This is a prototype implementation. Comments are welcome, and if I'm too ambitious consider just closing this :)

@rvanvelzen rvanvelzen force-pushed the conditional-type branch 3 times, most recently from 5eea9c0 to a5dcf8e Compare March 25, 2022 11:36
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Mar 28, 2022

I've been thinking about this for the past few days and I think that representing the "conditional return type" using PHPStan\Type\Type implementation and nothing else isn't the way to go.

I see this feature to be similar to generics, and for generics we had to add TemplateTypeMap getters to ParametersAcceptor, so that GenericParametersAcceptorResolver could read it and decide to act upon it.

With conditional types I want to be able to add them without any BC breaks. See the following path forward:

  • Easy: TypeNodeResolver: resolve the new AST nodes simply as unions of if/else branches. The "conditional type" information is lost on purpose.
  • Easy: PhpDocNodeResolver: add new method resolveConditionalReturnTag(): ConditionalReturnTypeTag|ConditionalReturnTypeForParameterTag|null
  • Easy: ResolvedPhpDocBlock: add new method getConditionalReturnTag(): ConditionalReturnTypeTag|ConditionalReturnTypeForParameterTag|null
  • Hard: Add new interface ParametersAcceptorWithConditionalReturnTag. Go through the implementations of ParametersAcceptor and decide where it should be implemented. I suspect that FunctionVariant and FunctionVariantWithPhpDocs will gain a new optional constructor parameter with the conditional return type tag.
  • Hard: Go through the places where FunctionVariant and FunctionVariantWithPhpDocs are instantiated and try to pass the conditional return type tag there if it makes sense.
  • Medium: GenericParametersAcceptorResolver - if the $parametersAcceptor is ParametersAcceptorWithConditionalReturnTag, read the tag and pass the information into ResolvedFunctionVariant. This is also an opportunity to add support for named arguments here (which would make them work for generics too). ResolvedFunctionVariant::getReturnType() will now return the correct type resolved based on the condition!
  • Easy: Deprecate ParametersAcceptorSelector::selectSingle(), user selectFromArgs everywhere.
  • Easy: sanity rule - is the type in ConditionalTypeNode part of the function's @template type map?
  • Easy: sanity rule - does the parameter in ConditionalTypeForParameterNode exist?
  • Hard and questionable - verify the function body does what the conditional return type says
  • Easy: Identify which dynamic return type extensions in PHPStan could be replaced with a stub that has conditional return type

Each of these items can roughly be a separate PR. In the end we should have something working :) I'd welcome if you could do the Easy items one after another. When it's time for the Hard ones, I'm gonna take over the work.

We can then discuss who should do the last "Medium" one.

Thanks :)

@ondrejmirtes
Copy link
Member

I've added a few more items :)

Part of implementation for phpstan/phpstan#3853, together with phpstan/phpdoc-parser#106.

This is a prototype implementation. Comments are welcome, and if I'm too ambitious consider just closing this :)
@ondrejmirtes
Copy link
Member

Linking phpstan/phpstan#3853

@rvanvelzen
Copy link
Contributor Author

  • Easy: PhpDocNodeResolver: add new method resolveConditionalReturnTag(): ConditionalReturnTypeTag|ConditionalReturnTypeForParameterTag|null
  • Easy: ResolvedPhpDocBlock: add new method getConditionalReturnTag(): ConditionalReturnTypeTag|ConditionalReturnTypeForParameterTag|null

I'm a bit stuck here: given that conditional types can be anywhere in the return type node (e.g. array<T is int ? int : float>) how should this be reflected in a ConditionalReturnTypeTag?

It's easy enough if the top type node is a conditional, but that won't cover all cases.

@ondrejmirtes
Copy link
Member

It's easy enough if the top type node is a conditional, but that won't cover all cases.

Are you sure that conditional type inside a normal type is supported by Psalm? I've never seen an example like that.

@rvanvelzen
Copy link
Contributor Author

Sure: https://psalm.dev/r/e07dd9374d. Psalm's stubs also contain nested conditionals

@ondrejmirtes
Copy link
Member

It's possible to solve that - TypeTraverser::map would take care of that - see how it's used around the codebase.

But we'd have to revert #1133 and rethink what to do about these types in places that don't care about them.

@rvanvelzen
Copy link
Contributor Author

How about:

  • Represent conditionals as union types (e.g. ConditionalType extends UnionType and ConditionalTypeForParameter extends UnionType)
  • Ensure that TypeCombinator doesn't collapse them into their branch types
  • When traversing the type and the result is know return the appropriate branch

This will at least allow inspecting the type completely. But I'm not entirely convinced that subclassing UnionType is a good way forward.

Looking forward to hearing your thoughts. :)

rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 29, 2022
Still a proof of concept, but possibly slightly more useful than phpstan#1120
@rvanvelzen
Copy link
Contributor Author

Looking forward to hearing your thoughts. :)

I've implemented this in #1140

@rvanvelzen rvanvelzen closed this Mar 30, 2022
rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 31, 2022
Still a proof of concept, but possibly slightly more useful than phpstan#1120
ondrejmirtes pushed a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 31, 2022
Still a proof of concept, but possibly slightly more useful than phpstan#1120
rvanvelzen added a commit to rvanvelzen/phpstan-src that referenced this pull request Mar 31, 2022
Still a proof of concept, but possibly slightly more useful than phpstan#1120
@rvanvelzen rvanvelzen deleted the conditional-type branch April 25, 2022 11:49
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