-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Make numeric operations on resources, arrays and objects type errors #5331
Conversation
f0065ba
to
48de974
Compare
👍 for this, the silent coercion behavior in php 7 is unexpected and can hide bugs such as the example in the RFC ( As a followup if the RFC gets approved, it may be useful to infer in opcache that the operands (excluding arrays for
|
case ZEND_BW_NOT: | ||
return (t1 & (MAY_BE_NULL|MAY_BE_FALSE|MAY_BE_TRUE|MAY_BE_ARRAY|MAY_BE_OBJECT|MAY_BE_RESOURCE)); | ||
case ZEND_BOOL_NOT: |
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.
What about ZEND_ASSIGN_OP below on line 4422 (e.g. $x = []; $x -= 2;
)?
Should that be updated to include MAY_BE_ARRAY, MAY_BE_RESOURCE and MAY_BE_OBJECT in more cases?
case ZEND_ASSIGN_OP:
if (opline->extended_value == ZEND_ADD) {
if ((t1 & MAY_BE_ANY) == MAY_BE_ARRAY
&& (t2 & MAY_BE_ANY) == MAY_BE_ARRAY) {
return 0;
}
return (t1 & (MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT)) ||
(t2 & (MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT));
} else if (opline->extended_value == ZEND_DIV ||
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.
Still waiting for a response, but this is more of an opcache issue that can easily be fixed, not something to change in the RFC itself
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.
Thanks, this should be fixed now.
<?php | ||
|
||
$binops = [ | ||
'+', |
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.
Would it be useful to have a test like this for AST_ASSIGN_OP (+=
, -=
, etc) to brute force all operands for that as well?
Also, I'm not familiar with the internals of eval()
enough to know this - Does the code fragment get passed to opcache if opcache is enabled? If not, it might be useful to file_put_contents() a script with various functions, to see if the zend_may_throw() value was correct, especially with opcache's JIT.
(But that may be overkill)
function test1() {
$x = STDERR;
$x /= 2;
echo "Unreachable resource /= int\n"; // test fails if this is output.
}
// ... thousands of automatically generated tests
try {
test1();
} catch(Error $e) {
echo $e->getMessage(), "\n";
}
There's actually quite a few cases where we could do this (the main thing I had in mind are function args with type hints). However, this would greatly increase the amount of SSA variables we need, I'm not sure if it's a good idea. |
Good point, I hadn't thought that this would need new SSA variables, but I guess that's reasonable - I assume the brand new SSA variables are needed for opcache to infer the new types of variables after operations, so there's no easy way to avoid creating them |
PCRE gives up on regular expressions of this size.
@nikic I'm sorry to rise the old thread but could you please say whether these cases are solved too: $a = [];
$a[] .= 'why';
$a[] += 1;
$a[] -= 2;
$a[] /= 3;
$a[] *= 4;
$a[] %= 5;
$a[] &= 6;
$a[] |= 7;
$a[] ^= 8;
$a[] <<= 9;
$a[] >>= 0;
var_dump($a); |
RFC: https://wiki.php.net/rfc/arithmetic_operator_type_checks
Using
array
,resource
orobject
as an operand of+
,-
,*
,/
,%
,**
,<<
,>>
,&
,|
,^
,~
,++
,--
now results in aTypeError
. As an exception,array + array
remains supported. Of course, objects that define numeric coercions or overloaded operators also remain supported.The behavior with scalar values of type null, bool, int, float and string is not affected.