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

[WIP][RFC]Spread Operator in Array #3640

Closed
wants to merge 12 commits into from
Closed

Conversation

jhdxr
Copy link
Member

@jhdxr jhdxr commented Oct 28, 2018

this is the implement for RFC Spread Operator in Array Expression

both the RFC and this PR are still WIP, but since most of the implement has been done, I'd like to create PR first. it's no harm to give people more time to review this, isn't it?

todo list:

  • finish RFC
  • test cases
  • opcache
  • deal with list(), since it also use array_pair

@jhdxr jhdxr changed the title Feature/array unpack Spread Operator in Array Oct 28, 2018
@jhdxr jhdxr changed the title Spread Operator in Array [WIP][RFC]Spread Operator in Array Oct 28, 2018
@petk petk added the RFC label Oct 28, 2018
@nikic
Copy link
Member

nikic commented Nov 7, 2018

Thanks for working on this! From a quick look the patch looks reasonable.

One somewhat contentious issue that came up the last time we discussed this in room 11 is the behavior of string keys. This PR implements the same semantics as array_merge. An alternative would be to always discard keys and append elements. Another would be to error on string keys (that's what argument unpacking does). I think it would be good to discuss this question in the RFC and provide a rationale why the behavior was chosen.

@azjezz
Copy link

azjezz commented Dec 10, 2018

great work, i was going to propose something similar to this before but never get to it.
you may take a look at this gist : https://gist.github.com/azjezz/e174bb34a23d298fea4b9b25ff0ca8ca

@Llbe
Copy link

Llbe commented Apr 4, 2019

A question: will this be supported?

class C
  {
    public const FOO = [1, 2, 3];

    public const BAR = [... self::FOO, 4, 5];
  }

@jhdxr
Copy link
Member Author

jhdxr commented Apr 5, 2019

hi @Llbe , this is supposed to be supported since it's a constant expression. Unfortunately I got a core dump when I try to run your example. I'll look into it later. thanks.


update: I cannot reproduce the crash on my windows. I'll finish updating the implement first then go back to the core dump.

@jhdxr jhdxr changed the base branch from master to PHP-7.4 April 5, 2019 12:04
@Llbe
Copy link

Llbe commented Apr 5, 2019

@jhdxr: another question (sorry I'm not on internals yet), the RFC doesn't mention how empty arrays are handled.

I assume that $foo = []; $bar = [... $foo]; will lead to $bar === []. Like array_push() since 7.3.

@jhdxr
Copy link
Member Author

jhdxr commented Apr 7, 2019

I assume that $foo = []; $bar = [... $foo]; will lead to $bar === [].

Yes, you are right.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Some changes in opcache are likely going to be needed. A good starting point is to look for ADD_ARRAY_ELEMENT uses in ext/opcache and see if the new UNPACK opcode needs similar treatment.

Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/tests/array_unpack/classes.phpt Show resolved Hide resolved
@jhdxr jhdxr force-pushed the feature/array_unpack branch 2 times, most recently from 5507d8b to 9d91a88 Compare April 9, 2019 10:50
Zend/zend_ast.c Outdated Show resolved Hide resolved
Zend/zend_ast.c Outdated Show resolved Hide resolved
Zend/zend_compile.c Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Show resolved Hide resolved
Zend/zend_vm_def.h Show resolved Hide resolved
@datashaman
Copy link

datashaman commented Apr 24, 2019

Question: will this support destructuring assignment as well?

$arr = ['a', 'b', 'c', 'd'];
[$head, ...$tail] = $arr;
// $head == 'a';
// $tail == ['b', 'c', 'd'];

That's pretty neat for Haskell-style list splitting like x:xs.

@jhdxr
Copy link
Member Author

jhdxr commented Apr 29, 2019

@datashaman no, spread operator with list is not included in the RFC. I do have a plan for it but there is some questions I have to clear before proposing another RFC.

jhdxr and others added 3 commits April 29, 2019 15:18
Squashed commit:

[6996c36] wip

[2900439] revert unrelated changes

[b514401] stop unpacking after insertion fails

[03c36b8] clean up (+1 squashed commits)

Squashed commits:

[d52c6c7bf7] spread operator for array
@nikic
Copy link
Member

nikic commented May 9, 2019

Do you need help with opcache support?

@@ -7234,6 +7346,7 @@ ZEND_VM_HANDLER(149, ZEND_HANDLE_EXCEPTION, ANY, ANY)
if (throw_op->result_type & (IS_VAR | IS_TMP_VAR)) {
switch (throw_op->opcode) {
case ZEND_ADD_ARRAY_ELEMENT:
case ZEND_ADD_ARRAY_UNPACK:
Copy link
Member

Choose a reason for hiding this comment

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

zend_opcode.c also needs updates (add ADD_ARRAY_UNPACK next to ADD_ARRAY_ELEMENT).

@jhdxr
Copy link
Member Author

jhdxr commented May 10, 2019

@nikic Yes, it will be very helpful. thank you.

I have another branch used for developing and testing with opcache, but the progress is very slow. maybe you can just ignore this one (:з」∠)

jhdxr and others added 3 commits May 10, 2019 17:03
Result will be freed by exception handling, don't destroy it
ourselves.
nikic added 4 commits May 13, 2019 12:10
And also fix one discrepancy in how many warnings you get when the
unpack is AST evaluated.
And tweak error message for grammar.
@nikic
Copy link
Member

nikic commented May 13, 2019

Opcache support is done, and I've also added a few more bits of test coverage.

@php-pulls php-pulls closed this in e829d08 May 13, 2019
@jhdxr
Copy link
Member Author

jhdxr commented May 29, 2019

thank you @nikic

@isaiddestroy
Copy link

Maybe it makes sense to use a different syntax instead of ..., but add support of arrays with string keys? It will be very helpful in many cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants