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

Fix const bool subtraction from mixed #3421

Open
wants to merge 1 commit into
base: 1.12.x
Choose a base branch
from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 8, 2024

No description provided.

@staabm staabm changed the base branch from 2.0.x to 1.12.x September 8, 2024 13:30
@phpstan-bot
Copy link
Collaborator

You've opened the pull request against the latest branch 2.0.x. PHPStan 2.0 is not going to be released for months. If your code is relevant on 1.12.x and you want it to be released sooner, please rebase your pull request and change its target to 1.12.x.

src/Type/MixedType.php Outdated Show resolved Hide resolved
@staabm staabm marked this pull request as ready for review September 8, 2024 13:51
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

if ($this->subtractedType !== null && StaticTypeFactory::falsey()->equals($this->subtractedType)) {
return new ConstantBooleanType(true);
if ($this->subtractedType !== null) {
if ($this->subtractedType->isSuperTypeOf(new ConstantBooleanType(false))->yes()) {
Copy link
Member

Choose a reason for hiding this comment

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

This logic is wrong. For example if subtracted type is bool, this condition will pass and toBoolean will return true.

Also many things cast to true or false, making this also wrong.

Here you need something more like if ($this->subtractedType->toBoolean()->isFalse()->yes()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch!

@staabm
Copy link
Contributor Author

staabm commented Sep 9, 2024

suprisingly its more complicated then initially expected. I think its in better shape now.

return new ConstantBooleanType(true);
}
if ((new ConstantBooleanType(true))->isSuperTypeOf($this->subtractedType)->yes()) {
return new ConstantBooleanType(false);
Copy link
Member

Choose a reason for hiding this comment

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

I still feel this logic is wrong.

  1. Let's say we subtract more than what's just falsy. Falsy is 0|0.0|''|'0'|array{}|false|null. But if we subtract more than that, like int|0.0|''|'0'|array{}|false|null or 0|0.0|''|'0'|array{}|bool|null, the result should still be the same.
  2. (new ConstantBooleanType(false))->isSuperTypeOf($this->subtractedType)->yes() is also wrong. This represents mixed~false. But something from mixed could still be falsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after more iterations, I think it turns out I am chasing ghosts and there is no bug.

the tests I came up with now don't fail any longer before this PR.
you may decide whether the tests are worth merging or we just close here, I guess?

}

if ($m !== $moreThenFalsy) {
assertType('mixed', $m);
Copy link
Contributor Author

@staabm staabm Sep 9, 2024

Choose a reason for hiding this comment

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

seems the substraction logic atm cannot deal with "subtract more then falsy", as the type stays mixed.
(I think its a different unrelated bug)

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