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

[TypeInfo] Introduce component #52510

Merged
merged 1 commit into from
Feb 3, 2024
Merged

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Nov 9, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR TODO

Introducing the brand new TypeInfo component

This work has been done with @Korbeil

State of the art

Scope of the current Symfony\Component\PropertyInfo\Type class

Nowadays, when we need to work with types within Symfony, we have to use the Type class of the PropertyInfo component.

But what if we want to represent the type of a function's return type?

We still can use that Type class, but it won't make much sense as the Symfony\Component\PropertyInfo\Type is closely related to a property (as the namespace suggests).

Plus, when we need to extract types, we must use the PropertyTypeExtractorInterface service:

readonly class Person
{
  public function __construct(
    public string $firstName,
  ) {
  }
}

// will return an array with a single Type object representing a string
$types = $this->propertyTypeExtractor->getTypes(Person::class, 'firstName');

Therefore, type retrieval in Symfony is limited to properties only.

Symfony\Component\PropertyInfo\Type's conceptual limitations

On top of that, there is a clear limitation of the current Type class where unions, intersections or
even generics can't be properly described.

The actual workaround is that the PropertyTypeExtractorInterface is returning an array of Type, which can be interpreted as a union type.

The TypeInfo component

All these reasons bring us to create the TypeInfo component.

The aim here is to address these issues and:

  • Have a powerful Type definition that can handle union, intersections, and generics (and could be even more extended)
  • Being able to get types from anything, such as properties, method arguments, return types, and raw strings (and can also be extended).

Type classes

To ensure a powerful Type definition, we defined multiple classes:
Type classes

The base Type class is an abstract one, so you'll always need to use one of the classes that extend it.
Other types of classes are kinda self-explanatory.

Type resolving

In the TypeInfo component, we added a TypeResolverInterface, and several implementations which allow developers to get a Type from many things:

  • ReflectionParameterTypeResolver to resolve a function/method parameter type thanks to reflection
  • ReflectionPropertyTypeResolver to resolve a property type thanks to reflection
  • ReflectionReturnTypeResolver to resolve a function/method return type thanks to reflection
  • ReflectionTypeResolver to resolve a ReflectionNamedType
  • StringTypeResolver to resolve a type string representation. This can greatly work in combination with PHP documentation.

    That resolver will only be available if the phpstan/phpdoc-parser package is installed.

  • ChainTypeResolver to iterate over resolvers and to return a type as soon as a nested resolver succeeds in resolving.

Type Creation

We also improved a lot the DX for the type creation with factories:

<?php

use Symfony\Component\TypeInfo\Type;

Type::int();
Type::nullable(Type::string());
Type::generic(Type::object(Collection::class), Type::int());
Type::list(Type::bool());
Type::intersection(Type::object(\Stringable::class), Type::object(\Iterator::class));

// Many others are available and can be 
// found in Symfony\Component\TypeInfo\TypeFactoryTrait

Upgrade path

This PR only introduces the TypeInfo component, but another one (which is already ready) will deprecate the Symfony\Component\PropertyInfo\Type in favor of Symfony\Component\TypeInfo\Type.

Copy link
Contributor

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

I like this! I know that some part of the LiveComponent can be refactor to use this component. We faced some limitation with the PropertyInfo on UnionType for exemple, and it look like this new Component can solve those limitations 😁

@dunglas
Copy link
Member

dunglas commented Nov 9, 2023

As the original author of PropertyInfo, I agree that we need a new, more powerful, TypeInfo component. PHP evolved a lot on this topic since the original release of PropertyInfo, and PropertyInfo is now too limited.

One of my regrets with PropertyInfo\Type is that it didn't try to be in sync with the features introduced in PHP directly.
In my opinion, TypeInfo should be a kind of "polyfill" for new and upcoming features introduced to PHP own's ReflectionType class and should inherit from it.

When/if new features (e.g.: generics) are added to PHP directly, then TypeInfo should deprecate its own equivalent feature and use the builtin one. WDYT?

@stof
Copy link
Member

stof commented Nov 9, 2023

Extending the PHP classes would mean that we cannot really deprecate things in our API as it would be PHP itself doing changes as well. And it might also create confusion if we create our own subclasses of ReflectionType as most code expecting ReflectionType will support only the classes defined in PHP.
Thus, we cannot "polyfill" upcoming features as we have no idea how that feature will define its PHP API in the future.

@derrabus derrabus added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Nov 9, 2023
@dunglas
Copy link
Member

dunglas commented Nov 9, 2023

@stof we can implement the methods with the names we want in our subclass and deprecate them in favor of the official ones when they will be added to PHP.

@mtarld
Copy link
Contributor Author

mtarld commented Nov 10, 2023

@dunglas, if I understood well, you'd love to have a type class hierarchy like the following:

classDiagram
ReflectionNamedType <|-- Type
ReflectionUnionType <|-- UnionType
ReflectionIntersectionType <|-- IntersectionType
Type <|-- BuiltinType
Type <|-- ObjectType
Type <|-- TemplateType
Type <|-- CollectionType
Type <|-- GenericType
Type <|-- UnionType
Type <|-- IntersectionType
ObjectType <|-- EnumType
EnumType <|-- BackedEnumType
Loading

By doing that way, UnionType and IntersectionType inherit from the Type class and, as well, from the ReflectionUnionType and the ReflectionIntersectionType. And of course, it's not possible in PHP to inherit from two classes at the same type.

Plus, the UnionType and IntersectionType will be considered as a ReflectionNamedType which can be weird.

So I see two solutions here:

  1. Do not inherit from ReflectionUnionType neither ReflectionIntersectionType, and make the Type inherit from the ReflectionType:
classDiagram
ReflectionType <|-- Type
Type <|-- BuiltinType
Type <|-- ObjectType
Type <|-- TemplateType
Type <|-- CollectionType
Type <|-- GenericType
Type <|-- UnionType
Type <|-- IntersectionType
ObjectType <|-- EnumType
EnumType <|-- BackedEnumType
Loading

But we won't be that precise, and we'll miss out if something is added into ReflectionNamedType, ReflectionUnionType, or ReflectionIntersectionType.

  1. Turn the Type class into an interface and make types inherit explicitly from the proper ReflectionType (ReflectionNamedType, ReflectionUnionType, and ReflectionIntersectionType)
classDiagram
Type <|-- BuiltinType
ReflectionNamedType <|-- BuiltinType
Type <|-- ObjectType
ReflectionNamedType <|-- ObjectType
Type <|-- TemplateType
ReflectionNamedType <|-- TemplateType
Type <|-- CollectionType
ReflectionNamedType <|-- CollectionType
Type <|-- GenericType
ReflectionNamedType <|-- GenericType
Type <|-- UnionType
ReflectionUnionType <|-- UnionType
Type <|-- IntersectionType
ReflectionIntersectionType <|-- IntersectionType
ObjectType <|-- EnumType
EnumType <|-- BackedEnumType
<<interface>> Type
Loading

But we have no way to enforce the Type interface to be implemented by a ReflectionType

WDYT?

@Korbeil
Copy link
Contributor

Korbeil commented Nov 10, 2023

About using Reflection and make the types classes extend it I'm kinda against it since it will only make the component harder to maintain with no real DX improvement. All the stuff we added are there to make types easier to manage and current Reflection isn't that easy to use.

@mtarld mtarld changed the base branch from 7.0 to 7.1 November 20, 2023 07:17
@dunglas
Copy link
Member

dunglas commented Nov 20, 2023

@Korbeil Reflection is a low level API used only for advanced use cases. Type introspection as well. App developers (unlike framework and library authors) should not have to use them directly. The DX matters less than performance and maintainability for these use cases. And best perf and maintainability is always achieved by relying only on the standard library without needing extra code. Currently, the PHP stdlib doesn't expose all the features we need, but the history of PropertyInfo proved that features are being added, and it's an open goal for the PHP project to support generics and the like. In the future, this component will hopefully not be useful anymore and will be deprecated (same for PropertyInfo), which is good. IMHO TypeInfo and PropertyInfo should try to reduce the amount of work that maintainers will have to do to switch to the PHP stdlib when generics and similar features will be supported.

@Korbeil
Copy link
Contributor

Korbeil commented Nov 24, 2023

@dunglas I don't think there is much work needed out of resolvers (which was the same in PropertyInfo extractor) for the maintainers. And if PHP starts to have a better Reflection for types will be welcome but we don't have that yet. Also about performance, extending Reflection would mean having to call Reflection everytime we use a Type class, this is clearly not something that should be done, we want this component to be the most performant possible.

@mathroc
Copy link
Contributor

mathroc commented Dec 9, 2023

Hi! nice work there :)

2 questions:

  • Do you think it could be useful to use generics on the classes defined here?
  • is it scope to include even more stuff found in Psalm/PHPStan? such as int<0,max>, value-of<MyEnum>, etc. ?

@Korbeil Korbeil force-pushed the chore/type-info-component branch 2 times, most recently from 78f1788 to 94589a6 Compare December 9, 2023 15:24
Co-authored-by: Baptiste Leduc <baptiste.leduc@gmail.com>
@fabpot
Copy link
Member

fabpot commented Feb 3, 2024

Thank you @mtarld.

@fabpot fabpot merged commit e4cfb66 into symfony:7.1 Feb 3, 2024
9 checks passed
@mtarld mtarld deleted the chore/type-info-component branch February 5, 2024 13:00
fabpot added a commit that referenced this pull request Apr 5, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] Deprecate PropertyInfo Type

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        |
| License       | MIT

This PR is a follow-up of #52510.

As the TypeInfo's `Type` aims to represent types in the Symfony ecosystem, the PropertyInfo's `Type` needs to be deprecated in favor of the first one.

Commits
-------

d32e81c [PropertyInfo] Deprecate PropertyInfo Type
symfony-splitter pushed a commit to symfony/doctrine-bridge that referenced this pull request Apr 5, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] Deprecate PropertyInfo Type

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        |
| License       | MIT

This PR is a follow-up of symfony/symfony#52510.

As the TypeInfo's `Type` aims to represent types in the Symfony ecosystem, the PropertyInfo's `Type` needs to be deprecated in favor of the first one.

Commits
-------

d32e81c816 [PropertyInfo] Deprecate PropertyInfo Type
symfony-splitter pushed a commit to symfony/validator that referenced this pull request Apr 5, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] Deprecate PropertyInfo Type

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        |
| License       | MIT

This PR is a follow-up of symfony/symfony#52510.

As the TypeInfo's `Type` aims to represent types in the Symfony ecosystem, the PropertyInfo's `Type` needs to be deprecated in favor of the first one.

Commits
-------

d32e81c816 [PropertyInfo] Deprecate PropertyInfo Type
symfony-splitter pushed a commit to symfony/serializer that referenced this pull request Apr 5, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] Deprecate PropertyInfo Type

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        |
| License       | MIT

This PR is a follow-up of symfony/symfony#52510.

As the TypeInfo's `Type` aims to represent types in the Symfony ecosystem, the PropertyInfo's `Type` needs to be deprecated in favor of the first one.

Commits
-------

d32e81c816 [PropertyInfo] Deprecate PropertyInfo Type
symfony-splitter pushed a commit to symfony/property-info that referenced this pull request Apr 5, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] Deprecate PropertyInfo Type

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        |
| License       | MIT

This PR is a follow-up of symfony/symfony#52510.

As the TypeInfo's `Type` aims to represent types in the Symfony ecosystem, the PropertyInfo's `Type` needs to be deprecated in favor of the first one.

Commits
-------

d32e81c816 [PropertyInfo] Deprecate PropertyInfo Type
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Apr 5, 2024
This PR was merged into the 7.1 branch.

Discussion
----------

[PropertyInfo] Deprecate PropertyInfo Type

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        |
| License       | MIT

This PR is a follow-up of symfony/symfony#52510.

As the TypeInfo's `Type` aims to represent types in the Symfony ecosystem, the PropertyInfo's `Type` needs to be deprecated in favor of the first one.

Commits
-------

d32e81c816 [PropertyInfo] Deprecate PropertyInfo Type
@fabpot fabpot mentioned this pull request May 2, 2024
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 2, 2024
This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

[TypeInfo] Add documentation

| Q             | A
| ------------- | ---
| Doc fix? | no
| New docs? | yes (symfony/symfony#52510)
| Applies to | 7.1
| Fixed tickets | #19497

Commits
-------

b6ffad3 [TypeInfo] Add documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet