-
Notifications
You must be signed in to change notification settings - Fork 8k
Allow specifying keys on list() elements #1730
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
Conversation
| { $$ = zend_ast_create(ZEND_AST_ARRAY_ELEM, $3, $1); } | ||
| | expr T_DOUBLE_ARROW T_LIST '(' assignment_list ')' | ||
| { $$ = zend_ast_create(ZEND_AST_ARRAY_ELEM, $5, $1); } | ||
| ; |
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.
Combining the implementations and throwing a clear compile-time error would be preferable.
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.
That would be necessary if we ever replace list() with [], but the way I've done it thus far makes things neater.
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.
On the other hand, doing this in the compiler means the possibility of a nicer error message.
|
You might want to consider some test cases for some of the more perverse behavior you can get into with
which I fought with for like a week getting HHVM to match PHP7's semantics (and it still doesn't quite get it right in one corner case). Both PHP5 and PHP7 have differently perversely broken semantics, and it would be sad if this made things worse :) In particular, your RFC claims you can desugar |
|
Most of the edge-cases with keyed
Hmm, what's the big difference? They compile to different opcodes ( |
e2c8fe0 to
1650d74
Compare
|
Here is one difference: $a = 'hello';
$b = $a[0];
var_dump($b); // 'h'
list($b) = $a;
var_dump($b); // nullI'm not sure why that is. 😕 |
|
Oh, sure, it doesn't work for strings. That's actually specified behaviour now: https://wiki.php.net/rfc/fix_list_behavior_inconsistency |
|
Cool. From the tests @jwatzman linked to the only other different behaviour I could see is that $h = [['a'], ['b'], ['c']];
$i = 0;
list($j[$i++]) = $h[$i++];
var_dump($j);$h = [['a'], ['b'], ['c']];
$i = 0;
$j[$i++] = $h[$i++][0];
var_dump($j);While that is strange, I assume that behaviour would just be carried forward for |
|
The behaviour isn't that strange when you consider that list() makes multiple assignments. The thing it assigns from is evaluated before the things it assigns to. And yes, I think this behaviour is preserved with keys. |
|
It's still surprising that |
|
@jwatzman To make sure we're on the same page, the problematic case you have in mind (notwithstanding a string as the RHS, which is just depressing), is that $tmp = expr;
var1 = $tmp[0];
var2 = $tmp[1];but rather into $tmp = expr;
$_tmp = $tmp[0];
var1 = $_tmp;
$_tmp = $tmp[1];
var2 = $_tmp;If that's the problem you have in mind, I think we might be open to fix this. Unlike the fact that the primary expression is executed first, the fact that the individual offset accesses are executed first as well is not strictly intentional. |
|
Though honestly, I have no clue what the "correct" behavior for your complex test case is. I can see how evaluating the side effects of the LHS before the side effects of the RHS might make sense as well there. |
<?php
$c = [1, 2];
$a = $c[0];
$b = $c[1];
$c = [1, 2];
list($a, $b) = $c;☟ Am I missing something? |
|
The difference is when the target of the assignment contains a subexpression: <?php
$c = [1, 2];
$a[f] = $c[0];
$b[f] = $c[1];
$c = [1, 2];
list($a[f], $b[f]) = $c;The I suspect this would all be cleared up if plain assignment evaluated all of the RHS before any of the LHS, but the relevant question is whether or not this PR makes the situation any worse. |
That's the worst one that really makes it not work, but there are lots of other subtitles, yeah.
I don't particularly care what the "right" behavior is, just that there isn't an accidental mismatch between the existing
Haha, don't ask me, I would have sworn I lifted the test case from your UVS PR :) I guess not.
Exactly. For particular test cases, I'd probably start by taking the two HHVM tests I had above, and replacing all the occurrences of I'd also decide if You should also consider what the semantics of stuff like the below is, and write tests to make sure the behavior is consistent with the existing
|
|
@jwatzman The cases $a = $d[$b];
$c = $d[$a];
// and respectively
$_a = $a;
$a = $_a[$a];
$a = $_a[$a];The last example follows the rule that if the RHS variable is used as an LHS target we force it to be evaluated first. Another case to consider is something like Following the rule from #1730 (comment) this would be equivalent to which I guess is reasonable (apart from RHS evaluation first maybe, but that's existing behavior). |
|
I haven't written this up for the language specification yet. So I can specify the behaviour there and write tests for it. |
|
I've spent the past two weeks procrastinating about writing tests for this. I'm not sure I know where to start. |
This may be a useful test using <?php
class Foo extends ArrayObject {
function offsetGet($k) {
print "get: $k\n";
return parent::offsetGet($k);
}
function offsetSet($k, $v) {
print "set: $k => $v\n";
parent::offsetSet($k, $v);
}
function offsetExists($k) {
print "exists?: $k\n";
return parent::offsetExists($k);
}
function offsetUnset($k) {
print "unset: $k\n";
parent::offsetUnset($k);
}
}
function trace($v) {
print "val: $v\n";
return $v;
}
$b = new Foo();
list(
trace('i') => $b[trace(7)],
trace('f') => list(
trace('c') => $b[],
trace('a') => $b[trace(6)]
),
trace('g') => $b[]
) = new Foo([
'f' => new Foo([
'a' => 'b',
'c' => 'd',
]),
'g' => 'h',
'i' => 'j',
]);Even if it doesn't capture everything, it's probably a worthwhile test to pin down the result of anyway. |
|
@TazeTSchnitzel Here are some more tests based on @jwatzman's comments. <?php
// All the following should print 'a' then 'b'
list($a, $b) = ['a', 'b'];
var_dump($a);
var_dump($b);
list(0 => $a, 1 => $b) = ['a', 'b'];
var_dump($a);
var_dump($b);
list(1 => $b, 0 => $a) = ['a', 'b'];
var_dump($a);
var_dump($b);
$arr = [];
list($arr[], $arr[]) = ['a', 'b'];
var_dump($arr[0]);
var_dump($arr[1]);
$arr = [];
list(0 => $arr[], 1 => $arr[]) = ['a', 'b'];
var_dump($arr[0]);
var_dump($arr[1]);
$arr = [];
list(1 => $arr[], 0 => $arr[]) = ['b', 'a'];
var_dump($arr[0]);
var_dump($arr[1]);
$arr = [];
list(list(1 => $arr[], 0 => $arr[])) = [['b', 'a']];
var_dump($arr[0]);
var_dump($arr[1]);
$arr = [];
list('key1' => $arr[], 'key2' => $arr[]) = ['key2' => 'b', 'key1' => 'a'];
var_dump($arr[0]);
var_dump($arr[1]);
// This should print 'foo'
$a = 0;
list($a => $a) = ['foo', 'bar'];
var_dump($a);
// This should print 'bar' then 'foo'
$a = 0;
$b = 1;
list($b => $a, $a => $c) = ['foo', 'bar'];
var_dump($a);
var_dump($c); |
|
And @nikic's last comment, as a test: <?php
$i = 0;
$a = [
0 => [
'b' => 'bar',
'a' => 'foo',
],
1 => 'a',
3 => 'b',
];
list($a[$i++] => $a[$i++], $a[$i++] => $a[$i++]) = $a[$i++];
var_dump($i); // should be 5
var_dump($a[2]); // should be 'foo'
var_dump($a[4]); // should be 'bar' |
|
Okay, hikari-no-yume@ab532db adds a test which observes the evaluation order using some objects which produce log messages. It's not everything, but it's a start. |
|
And hikari-no-yume@61def0c adds a second test which looks at nesting. I like these tests because they can directly check the evaluation order. But they probably aren't enough given PHP or HHVM could do completely different things for non-ArrayAccess. |
|
I've made a language specification patch as well: php/php-langspec#152 |
Yeah, testing a mix of ways to observe assignment/evaluation order would be good for this reason. |
61def0c to
dca9d4a
Compare
|
Merged: 37c8bb5 |
Counterpart to the RFC: https://wiki.php.net/rfc/list_keys