Skip to content

Fix array_sum return type#494

Merged
ondrejmirtes merged 1 commit intophpstan:masterfrom
VincentLanglet:fixArraySum
Apr 15, 2021
Merged

Fix array_sum return type#494
ondrejmirtes merged 1 commit intophpstan:masterfrom
VincentLanglet:fixArraySum

Conversation

@VincentLanglet
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Better comparison would be:

(new UnionType([new IntegerType(), new FloatType()]))->isSuperTypeOf($itemType)->yes()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dtto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it makes sense to modify this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For an array of string, $itemType is a stringType.
So TypeCombinator::intersect($itemType, new UnionType([new IntegerType(), new FloatType()]) is nothing.
That's why the dynamic return type return new ConstantIntegerType(0).

The intersect seems to be the reason of the bug.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But on the other hand, this intersect allows us to specify that when we only have array<int>, the result is going to be int.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Isn't this handle by the previous check

if ($intUnionFloat->isSuperTypeOf($itemType)->yes()) {
    return TypeCombinator::union(new ConstantIntegerType(0), $itemType);
}

?

@VincentLanglet VincentLanglet marked this pull request as ready for review April 15, 2021 13:31
@ondrejmirtes
Copy link
Copy Markdown
Member

Awesome! 👍

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.

2 participants