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

Avoid duplicate in Union type description #2973

Merged
merged 4 commits into from Mar 18, 2024

Conversation

VincentLanglet
Copy link
Contributor

This is related to phpstan/phpstan-doctrine#552 (comment)

When a custom Type has

  • templates (like index and values)
  • other internal metadata (like DQL here)

The union Type of this custom Type encounter a weird situation

  • Type CustomType('A') and CustomType('B') cannot be merged or the metadata will be lost
  • description of the union will be CustomType|CustomType

A simple solution is to call array_unique on the described types.

@ondrejmirtes
Copy link
Member

The root issue is in type normalization, not describing UnionType.

If two types with the same description come up in a UnionType, it's because they haven't been properly normalized (due to isSuperTypeOf being implemented in a wrong way). See https://phpstan.org/developing-extensions/type-system#type-normalization

@VincentLanglet
Copy link
Contributor Author

The root issue is in type normalization, not describing UnionType.

If two types with the same description come up in a UnionType, it's because they haven't been properly normalized (due to isSuperTypeOf being implemented in a wrong way). See phpstan.org/developing-extensions/type-system#type-normalization

But in case of custom Type with metadata like DQL here, we have the situation

$queryTypeA = new QueryType($dqlA);
$queryTypeB = new QueryType($dqlB);

$queryTypeA->isSuperType($queryTypeB); // FALSE
$queryTypeB->isSuperType($queryTypeA); // FALSE

To me it's because the QueryType is templated as QueryType<index, value> when it should/could be QueryType<index, value, DQL>.
The union type would now be

'Doctrine\ORM\Query<mixed, mixed, 'foo'>'|'Doctrine\ORM\Query<mixed, mixed, 'bar'>'

instead of

'Doctrine\ORM\Query<mixed, mixed>'|'Doctrine\ORM\Query<mixed, mixed>'

But I assume this template was not added because it's too complicated for phpdoc, people would end with

'Doctrine\ORM\Query<mixed, mixed, 'SELECT * from ...'>'

@ondrejmirtes ondrejmirtes reopened this Mar 18, 2024
@ondrejmirtes
Copy link
Member

Alright, I understand it now. But I want a slightly different behaviour - for the most-detailed VerbosityLevel (precise I think) I don't want to unique the types, I want to keep them separate and mark them with a number, so the result is going to be:

int#1|int#2

Thanks

@VincentLanglet
Copy link
Contributor Author

Alright, I understand it now. But I want a slightly different behaviour - for the most-detailed VerbosityLevel (precise I think) I don't want to unique the types, I want to keep them separate and mark them with a number, so the result is going to be:

int#1|int#2

Thanks

Done in 851f7c3

@staabm
Copy link
Contributor

staabm commented Mar 18, 2024

(lint job errors)

@VincentLanglet
Copy link
Contributor Author

(lint job errors)

Should be ok now

'(TFoo of TBar)|null',
'(TFoo of TBar)|null',
],
[
new UnionType([new QueryType('foo'), new QueryType('bar')]),
'Doctrine\ORM\Query<mixed, mixed>#1|Doctrine\ORM\Query<mixed, mixed>#2',
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to add QueryType class here, you can just add the same type twice into UnionType constructor.

@ondrejmirtes ondrejmirtes merged commit 8c64537 into phpstan:1.10.x Mar 18, 2024
439 of 440 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants