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

feat: update parser #1270

Merged
merged 2 commits into from
Jan 3, 2020
Merged

feat: update parser #1270

merged 2 commits into from
Jan 3, 2020

Conversation

czosel
Copy link
Collaborator

@czosel czosel commented Jan 2, 2020

This adds support for the spread operator for arrays (PHP 7.4) and fixes
an issue with silent / retif precedence.

Fixes #1009

This adds support for the spread operator for arrays (PHP 7.4) and fixes
an issue with silent / retif precedence.

Fixes #1009
@czosel
Copy link
Collaborator Author

czosel commented Jan 2, 2020

@loilo ready for review. Not sure why appveyor is still running...

Copy link
Collaborator

@loilo loilo left a comment

Choose a reason for hiding this comment

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

We may want to check the GitHub Actions CI pipeline.

It doesn't catch an error I encounter locally (in yarn test tests/parens): A cast node doesn't seem to have an expr property, the error looks genuine:

 FAIL   test-node  tests/parens/jsfmt.spec.js
  ● Test suite failed to run

    TypeError: Cannot read property 'comments' of undefined

      2555 |         node.type,
      2556 |         ") ",
    > 2557 |         node.expr.comments
           |                   ^
      2558 |           ? indent(path.call(print, "expr"))
      2559 |           : path.call(print, "expr")
      2560 |       ]);

      at comments (src/printer.js:2557:19)

@czosel
Copy link
Collaborator Author

czosel commented Jan 3, 2020

You’re probably still using an old (cached) version of the parser. Try running
yarn cache clean
rm -rf node_modules
yarn

In the latest version of the parser “what” has been renamed to “expr” on cast nodes.

@loilo
Copy link
Collaborator

loilo commented Jan 3, 2020

Right, that fixed it. And I thought that's one of the things a lockfile should prevent. 🙄

@czosel
Copy link
Collaborator Author

czosel commented Jan 3, 2020

Seems like that's an open issue in yarn: yarnpkg/yarn#4722

Good to go? 😉

@loilo
Copy link
Collaborator

loilo commented Jan 3, 2020

Gimme a second, just doing the actual review now. Was too tired yesterday. 😁

Everything looks about right to me, but I have some questions to better understand the changes, gonna put them in review comments.

case "entry": {
const ref = node.byRef ? "&" : "";
const unpack = node.unpack ? "..." : "";
return node.key
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do I understand this correctly that this differentiation is needed because of PHP 7.4 array unpacking and no key is available when unpacking something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the new parser version, entry objects are created for every array entry, not just for associative arrays anymore (see old behavior here). Also, unpack and byRef were added to the entry node for a generic way to describe the context of a specific entry.

So with the new version, we have to account for entrys that don't have a key (non-associative arrays).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, this explains my follow-up question as well (why we didn't previously differentiate beetween kinds of array entries). 😁

What I discovered (but I need to create an issue over at the parser for that): It's possible to do [...&$var] which should cause a parser error instead.

case "entry": {
const ref = node.byRef ? "&" : "";
const unpack = node.unpack ? "..." : "";
return node.key
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, this explains my follow-up question as well (why we didn't previously differentiate beetween kinds of array entries). 😁

What I discovered (but I need to create an issue over at the parser for that): It's possible to do [...&$var] which should cause a parser error instead.

@czosel czosel merged commit 8192b82 into master Jan 3, 2020
@czosel czosel deleted the update-parser-2 branch January 3, 2020 11:38
@loilo
Copy link
Collaborator

loilo commented Jan 3, 2020

See also: glayzzle/php-parser#461

@alexander-akait
Copy link
Member

Good job!

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.

Stripping of brackets leads to corrupt code
3 participants