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

[PHP 8.0] Add attributes v2 support #661

Merged
merged 10 commits into from
Sep 13, 2020
Merged

[PHP 8.0] Add attributes v2 support #661

merged 10 commits into from
Sep 13, 2020

Conversation

TomasVotruba
Copy link
Contributor

@TomasVotruba TomasVotruba commented May 8, 2020

Closes #659


Note, this comes handy (found out 1 hour too later than needed 🤦 ):

php test/updateTests.php

@TomasVotruba TomasVotruba marked this pull request as ready for review May 8, 2020 15:52
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented May 8, 2020

@nikic It's goes pretty well so far. I just got stuck on simple test case: 8025300#diff-39ff209fb56c0dd7ab3dcde01b4c6df1

<?php

<<Deprecated>>
function a() {
}

With:

1) PhpParser\CodeParsingTest::testParse with data set "var/www/PHP-Parser/test/code/parser/stmt/function/attributes.test#0" ('Attributes (/var/www/PHP-Pars....test)', '<?php\n\n<<Deprecated>>\nfunc...) {\n}', 'array(\n    0: Stmt_Function(...  )\n)', null)
Attributes (/var/www/PHP-Parser/test/code/parser/stmt/function/attributes.test)
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'array(
-    0: Stmt_Function(
-        byRef: false
-        name: Identifier(
-            name: a
+'Syntax error, unexpected T_SL from 3:1 to 3:2
+Syntax error, unexpected T_STRING, expecting '(' from 4:10 to 4:10
+Syntax error, unexpected '{' from 4:14 to 4:14

I assume the tokens.yaml has to be updated somehow, so T_SL and T_SR are somehow part of attribute grammer, right?

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented May 8, 2020

Thanks! It helped.

  1. Now the node attributes are missing in the Attribute node. Any idea how to get them in nodeWithPhpAttributes?

  2. Where exactly in the StandardPrinter should be Attribute be printed?

  3. Feel free to give feedback on code in overall, I need some guidance.

grammar/php7.y Outdated Show resolved Hide resolved
grammar/php7.y Outdated Show resolved Hide resolved
lib/PhpParser/Node/Stmt/Attribute.php Outdated Show resolved Hide resolved
lib/PhpParser/Node/Stmt/Attribute.php Outdated Show resolved Hide resolved
lib/PhpParser/Node/Stmt/Attribute.php Outdated Show resolved Hide resolved
lib/PhpParser/NodeVisitor/NameResolver.php Outdated Show resolved Hide resolved
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented May 9, 2020

I've integreated your comments. It's ready for next review

@nikic
Copy link
Owner

nikic commented May 14, 2020

I pushed a fix for the parser conflicts. Haven't had a chance to review the rest yet.

@TomasVotruba
Copy link
Contributor Author

Thanks, that helped a lot. I'll continue

@TomasVotruba
Copy link
Contributor Author

Tests are now broken with some positions. I really don't understand why or how to fix them. Seems like attributes are not parsed anymore

@TomasVotruba TomasVotruba changed the title [PHP 8] Add attributes v2 support [PHP 8.0] Add attributes v2 support Jun 13, 2020
@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Jul 4, 2020

I tried to rebase on current master.

Could you give this priority? New merged PRs makes this hell to rebase :/

I think only details are missing, but my knowledge of php grammar errors is not that advanced, so I can fix them.

TomasVotruba and others added 4 commits September 6, 2020 17:49
We want to use Arg[] nodes, as we also need to represent attributes
with named params.
@nikic
Copy link
Owner

nikic commented Sep 6, 2020

Emulation for new T_ATTRIBUTE token added in f66a32e. I've rebased this PR and fixed the shift-reduce conflicts.

Unfortunately the #[] syntax also has an attribute grouping feature #[Attr1, Attr2], so I think it will be necessary to change the representation, i.e. introduce an additional intermediary AttributeGroup node :/

This is a bit odd, but evidently the leading empty production
is causing issues here.
@nikic nikic merged commit 4c22c62 into nikic:master Sep 13, 2020
@nikic
Copy link
Owner

nikic commented Sep 13, 2020

I've mostly finished this up and merged the initial implementation. One thing that is still missing is full FPPP support.

@ondrejmirtes
Copy link
Contributor

Awesome! 😍

But what's FPPP?

Screenshot 2020-09-13 at 21 08 24

(I'm not afraid to ask the stupid questions.)

@nikic
Copy link
Owner

nikic commented Sep 13, 2020

FPPP = Formatting-Preserving Pretty Print. It's a bit long to write out :)

@ondrejmirtes
Copy link
Contributor

Oh, of course :)

@TomasVotruba TomasVotruba deleted the php-attributes-v2 branch September 19, 2020 21:46
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.

[PHP 8] attributes 2 support
3 participants