Skip to content

take subtracted type into account in MixedType->isArray()#1656

Merged
ondrejmirtes merged 7 commits into
phpstan:1.8.xfrom
staabm:subtract
Aug 29, 2022
Merged

take subtracted type into account in MixedType->isArray()#1656
ondrejmirtes merged 7 commits into
phpstan:1.8.xfrom
staabm:subtract

Conversation

@staabm

@staabm staabm commented Aug 29, 2022

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/Type/MixedType.php Outdated
Comment thread tests/PHPStan/Type/MixedTypeTest.php Outdated
Comment thread tests/PHPStan/Type/MixedTypeTest.php Outdated
Comment on lines 173 to 175

@staabm staabm Aug 29, 2022

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.

subtracting only array<int, string> could still leave e.g. a array<string, mixed> within the mixed.
therefore I am wondering, whether the only case where $mixed->isArray() can say NO is, when a general array is substracted?

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.

Exactly. Switching the callee and the argument works even when more than just an array are subtracted, like array|null for example.

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.

I just see #1656 (comment), so I think we are on the same page.. will adjust the PR.

@ondrejmirtes ondrejmirtes left a comment

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.

While you're at it, please do all the other methods as well.

@staabm

staabm commented Aug 29, 2022

Copy link
Copy Markdown
Contributor Author

implemented in all methods I found useful.

here are a few notes:

  • should isIterableAtLeastOnce return NO when isIterable returns NO?
  • was not sure what todo with isOffsetAccessible and friends ..?
  • was wondering whether isCallable should also use a $this->subtractedType->isSuperTypeOf(new CallableType(...)) construction instead?
    see
    public function isCallable(): TrinaryLogic
    {
    if (
    $this->subtractedType !== null
    && $this->subtractedType->isCallable()->yes()
    ) {
    return TrinaryLogic::createNo();
    }
    return TrinaryLogic::createMaybe();
    }

@staabm
staabm marked this pull request as ready for review August 29, 2022 09:27
@ondrejmirtes

Copy link
Copy Markdown
Member

should isIterableAtLeastOnce return NO when isIterable returns NO?

Exactly, it should just call return $this->isIterable(). I don't think we can get more precise than that.

  • was not sure what todo with isOffsetAccessible and friends ..?

Can we get a union of all offset-accessible types? Is it string|array|ArrayAccess? Yeah, probably.

  • was wondering whether isCallable should also use a $this->subtractedType->isSuperTypeOf(new CallableType(...)) construction instead?

Yes, definitely, the current code is wrong.

@staabm
staabm force-pushed the subtract branch 2 times, most recently from 00b7b03 to e131ca2 Compare August 29, 2022 11:54
@staabm

staabm commented Aug 29, 2022

Copy link
Copy Markdown
Contributor Author

implemented and rebased

@ondrejmirtes
ondrejmirtes merged commit a80a5fc into phpstan:1.8.x Aug 29, 2022
@ondrejmirtes

Copy link
Copy Markdown
Member

Thank you.

@staabm
staabm deleted the subtract branch August 29, 2022 12:04
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