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

{"WIP"=>"Add to support List shapes"} #2202

Merged
merged 2 commits into from
Feb 10, 2023
Merged

{"WIP"=>"Add to support List shapes"} #2202

merged 2 commits into from
Feb 10, 2023

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Jan 29, 2023

@zonuexe zonuexe changed the base branch from 1.10.x to 1.9.x January 29, 2023 15:08
@zonuexe
Copy link
Contributor Author

zonuexe commented Jan 29, 2023

continue work in a few days

@zonuexe zonuexe changed the base branch from 1.9.x to 1.10.x February 5, 2023 18:38
@zonuexe zonuexe changed the base branch from 1.10.x to 1.9.x February 7, 2023 17:43
@zonuexe zonuexe marked this pull request as ready for review February 7, 2023 17:43
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@zonuexe zonuexe changed the title WIP: Add to support List shapes Add to support List shapes Feb 7, 2023
@phpstan-bot phpstan-bot changed the title Add to support List shapes {"WIP"=>"Add to support List shapes"} Feb 7, 2023
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'd choose a different implementation path.

  1. Reset everything in src/, leave just the tests.
  2. In TypeNodeResolver, intersect $builder->getArray() with AccessoryArrayListType if kind is list.
  3. In IntersectionType::describeItself there's a section that makes sure that array<...>&list is described as list<...>. Please write similar logic there so that array{...}&list is described as list{...}.

Thank you.

$arrayType = $builder->getArray();
if ($typeNode->kind === ArrayShapeNode::KIND_LIST) {
$arrayType = AccessoryArrayListType::intersectWith($arrayType);
var_dump($arrayType);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

array{0: 'a', 1: 'b'}&list is normalized to array{0: 'a', 1: 'b'}.

AccessoryArrayListType::intersectWith($arrayType) always returns the operand $arrayType: ConstantArrayType instead of IntersectionType because the following equality always holds.

(new AccessoryArrayListType)->isSuperTypeOf($arrayType)->yes() === true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ondrejmirtes AccessoryArrayListType::intersectWith($arrayType) never returns IntersectionType so changing IntersectionType::describeItself() doesn't seem to accomplish what you want, any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

array{0: 'a', 1: 'b'} is always a list so that's why the &list part is omitted and that's why it's not an intersection type. So we probably don't need to worry about this at all. The point of list{...} should be to reject types like list{foo: 1}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok.

}
$describedTypes[$i] = 'list{' . substr($typeDescription, strlen('array{'));
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. In IntersectionType::describeItself there's a section that makes sure that array<...>&list is described as list<...>. Please write similar logic there so that array{...}&list is described as list{...}.

array{0: 'a', 1: 'b'} is always a list so that's why the &list part is omitted and that's why it's not an intersection type. So we probably don't need to worry about this at all. The point of list{...} should be to reject types like list{foo: 1}.

As you say array{0: 'a', 1: 'b'}&list is always ConstantArrayType not IntersectionType. Changing IntersectionType::describeItself() for print list{} seems to have no effect.

Reset everything in src/, leave just the tests.

Maybe I need to reapply the reverted fa7bcee and change ConstantArrayType.

Copy link
Member

Choose a reason for hiding this comment

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

Do not overthink this :) It's fine to describe list{1} in PHPDoc as array{1} with dumpType.

Copy link
Member

Choose a reason for hiding this comment

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

I think you can delete this change without anything failing.

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.

Please add a test so that we can see that list{foo: 1} reports:

PHPDoc tag @param for parameter $a contains unresolvable type.

in IncompatiblePhpDocTypeRuleTest

}
$describedTypes[$i] = 'list{' . substr($typeDescription, strlen('array{'));
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you can delete this change without anything failing.

@ondrejmirtes
Copy link
Member

Rebasing to see if the Rector failures are fixed.

@ondrejmirtes ondrejmirtes merged commit 0e2e1b8 into phpstan:1.9.x Feb 10, 2023
@ondrejmirtes
Copy link
Member

Thank you.

@zonuexe zonuexe deleted the feature/list-shapes branch February 10, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants