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

Feature check mixed in binary operator #3231

Merged

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented Jul 12, 2024

The goal of this PR is to report mixed in binary operators. There's still a few other places where mixed is not properly reported (https://phpstan.org/r/c46a6ace-0135-46c0-a860-dd9d9b77b4ff), but those can be dealt with later.

Fixes: phpstan/phpstan#7538
Fixes: phpstan/phpstan#10440

@@ -856,6 +856,10 @@ public function getModType(Expr $left, Expr $right, callable $getTypeCallback):
return $this->getNeverType($leftType, $rightType);
}

if ($leftType->toNumber() instanceof ErrorType || $rightType->toNumber() instanceof ErrorType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether this should only be enabled in bleeding edge. On the one hand, I don't see any other case in this class where return new ErrorType would be conditioned by bleeding edge. On the other hand, this can definitely result in new errors being reported without bleeding edge. E.g. 5 % [] now reports Binary operation "%" between 5 and array{} results in an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The $int % $string use-case is even more compelling example of why it should apply only to bleeding edge.

* @template T
* @param T $a
*/
function genericMixed(mixed $a): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some operators are missing from this test:

  • instanceof is not a binary operator in php-parser.
  • &&, ||, and, or and xor - everything can be converted to boolean. And they're already covered by strict-rules.
  • ?? is OK to use with mixed.
  • Comparison operators are handled by InvalidComparisonOperationRule.

src/Rules/LazyRegistry.php Outdated Show resolved Hide resolved
@schlndh schlndh marked this pull request as draft July 13, 2024 06:41
Comment on lines +76 to +77
} elseif ($node instanceof Node\Expr\AssignOp\Plus || $node instanceof Node\Expr\BinaryOp\Plus) {
$callback = static fn (Type $type): bool => !$type->toNumber() instanceof ErrorType || $type->isArray()->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.

This is new, otherwise the code in this file should be the same with the exception of the early exit and not requiring $scope->getType($node) instanceof ErrorType anymore in bleeding edge.

@schlndh schlndh marked this pull request as ready for review July 15, 2024 18:08
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@staabm
Copy link
Contributor

staabm commented Jul 15, 2024

the error in shopsys/shopsys on line

https://github.com/shopsys/shopsys/blob/4a0cce895273dba7021736d0b3984b454f77e70e/packages/framework/src/Component/Cron/CronTimeResolver.php#L40-L41

looks like it could be fixed with a improved preg_match inference. I will have a look into simple numeric-string cases

@ondrejmirtes ondrejmirtes merged commit ef2a246 into phpstan:1.11.x Jul 15, 2024
450 of 455 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
Development

Successfully merging this pull request may close these issues.

4 participants