Skip to content

Conversation

TomasVotruba
Copy link
Contributor

@TomasVotruba TomasVotruba commented Apr 21, 2019

This PR based on PR to php-src: php/php-src#3941

Current RFC voting is 33:5, so if nothing complicated happens in implementation, this feature will get accepted to PHP 7.4.

I need feedback on this. I'm not sure the ArrowFunction should inherit FunctionLike - because ArrowFunction has exactly 1 statement, not more.

  1. Should new interface be created?
  2. Should FunctionLike be parrent to the new interface?
  3. Should it extend Closure?

I tried to mimic php-src grammar Zend/zend_language_parser.y

Copy link
Owner

@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.

I need feedback on this. I'm not sure the ArrowFunction should inherit FunctionLike - because ArrowFunction has exactly 1 statement, not more.

I think it should. We can construct an equivalent statement array for the expression, and code should be able to treat arrow functions just like other functions then.

@TomasVotruba
Copy link
Contributor Author

Thank you! Ready for 2nd feedback

@TomasVotruba
Copy link
Contributor Author

On running parser test:

<?php
fn($a) => $a;

with:

vendor/bin/phpunit test/PhpParser/CodeParsingTest.php

I get error:

Syntax error, unexpected T_DOUBLE_ARROW from 2:8 to 2:9

The grammar file is somehow wrong, not sure how.

@TomasVotruba
Copy link
Contributor Author

I resolved all the comments, ready to review

@TomasVotruba
Copy link
Contributor Author

What needs to be done here?

I need this feature so I can add and test migration path in Rector.

@nikic
Copy link
Owner

nikic commented May 7, 2019

7.4 and nightly builds are green now. We should update the used php-src archive to make the 7.2 build also pass.

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented May 7, 2019

We should update the used php-src archive to make the 7.2 build also pass

Is that part of test_old? That part of tests is not very clear to me. How should I do that?

@nikic
Copy link
Owner

nikic commented May 7, 2019

@TomasVotruba Yes. https://github.com/nikic/PHP-Parser/blob/master/test_old/run-php-src.sh needs to be changed to fetch a 7.4 archive instead.

@TomasVotruba
Copy link
Contributor Author

I see, just a sec 👍

Recently I've also noticed "The return keyword can be omitted" in https://stitcher.io/blog/short-closures-in-php

Is that true?

$ids = array_map(fn(Post $post): int => $post->id, $posts);
$ids = array_map(fn(Post $post): return int => $post->id, $posts);

If so, I shall add test case for that

@nikic
Copy link
Owner

nikic commented May 7, 2019

Is that true?

No, adding a return is not allowed there.

Copy link
Owner

@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.

Looks good, some final comments...

@TomasVotruba
Copy link
Contributor Author

I was trying to rebase pull-request to merge the fixup! commits, but I keep failing on so many conflicts.

Could you merge it locally or squash to one commit on merge?

@nikic nikic merged commit f3b19c1 into nikic:master May 9, 2019
@nikic
Copy link
Owner

nikic commented May 9, 2019

Merged as squash. Thanks for your work on this!

Next up: https://wiki.php.net/rfc/spread_operator_for_array has been accepted :)

@TomasVotruba TomasVotruba deleted the arrow-fun branch May 9, 2019 12:20
@TomasVotruba
Copy link
Contributor Author

Merged as squash. Thanks for your work on this!

Awesome 👍

Next up: wiki.php.net/rfc/spread_operator_for_array has been accepted :)

Gimme a sec 😆

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.

2 participants