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
Binary operations on non-empty-string
and string
should error
#1010
Conversation
The right solution here is to introduce |
hmm do you mean adding this method on the |
It's not a BC break, implementing custom Type isn't covered by BC promise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise 👍
src/Analyser/MutatingScope.php
Outdated
@@ -1419,6 +1424,12 @@ private function resolveType(Expr $node): Type | |||
|
|||
$resultType = TypeCombinator::union($leftNumberType, $rightNumberType); | |||
if ($node instanceof Expr\AssignOp\Div || $node instanceof Expr\BinaryOp\Div) { | |||
// division on strings is not allowed. its allowed on numeric strings though. | |||
if ($leftType->isString()->yes() && !$leftType->isNumericString()->yes() && !$leftType->isLiteralString()->yes() | |||
|| $rightType->isString()->yes() && !$rightType->isNumericString()->yes() && !$rightType->isLiteralString()->yes()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
||
vs.&&
operator priority needs to be disambiguated with parentheses- I don't know why literal-string is mentioned in this condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- fixed
- I mixed up my mental model in "123" would be a literal-string, but it is also a numeric-string, so the additional check is not necessary. fixed a bug so we don't accept
literal-string
for binary operations
src/Analyser/MutatingScope.php
Outdated
@@ -1400,6 +1400,11 @@ private function resolveType(Expr $node): Type | |||
return new ErrorType(); | |||
} | |||
|
|||
if ($leftType->isNonEmptyString()->yes() && !$leftType->isNumericString()->yes() || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
||
vs.&&
operator priority needs to be disambiguated with parentheses- Why
isNonEmptyString
and notisString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- fixed
- I used
non-empty-string
because the initial bug report was about non-emptry-string problems, while it already worked for regularstring
.
I am now usingisString
instead ofisNonEmptyString
in the first block, which renders this second block of changes unnecessary. I removed it.
initially I had a different assumption what this method would provide
Thank you! |
closes phpstan/phpstan#6624