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

Implement Shorter Attribute Syntax #5796

Closed

Conversation

theodorejb
Copy link
Contributor

RFC: https://wiki.php.net/rfc/shorter_attribute_syntax.

Implements the @@ syntax for attributes. @kooldev did the initial implementation, and I added support for trailing commas in the attribute argument list.

// before
<<Attribute>>
class Foo {}

// after
@@Attribute
class Foo {}

Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend_language_scanner.l Outdated Show resolved Hide resolved
@carusogabriel carusogabriel added this to the PHP 8.0 milestone Jul 2, 2020
php-pulls pushed a commit that referenced this pull request Jul 2, 2020
Allow trailing comma. Syntactically allow unpacking, but forbid it
during compilation.

The trailing comma test-case is adopted from GH-5796.
@nikic
Copy link
Member

nikic commented Jul 2, 2020

I've landed the trailing comma part of this in a more general way (that will be compatible with things like named args in the future) in 6a195ca.

@theodorejb theodorejb force-pushed the shorter-attribute-syntax-rfc branch from b2c0d2f to 1b36813 Compare July 2, 2020 16:12
@SerafimArts

This comment has been minimized.

@beberlei
Copy link
Contributor

beberlei commented Jul 2, 2020

@theodorejb you can use the Github co-author feature to mention Martin in the commit message with Co-authored-by: Martin Schröder <m.schroeder2007@gmail.com>

@theodorejb theodorejb force-pushed the shorter-attribute-syntax-rfc branch from 1b36813 to d94652b Compare July 2, 2020 17:11
@beberlei
Copy link
Contributor

beberlei commented Jul 2, 2020

@SerafimArts this is not the place to discuss the selected syntax, please see the mailing list discussion on this RFC.

please also be mindful that this has been picked by democratic vote and that this vote is now completed, at some point decisions have to be made and are final, this is the PHP contribution and governance model.

@nikic
Copy link
Member

nikic commented Jul 22, 2020

I have landed #5827 now. Can you please rebase on top of it?

@theodorejb theodorejb force-pushed the shorter-attribute-syntax-rfc branch from 0b78ece to 9252f1b Compare July 22, 2020 14:55
@theodorejb
Copy link
Contributor Author

@nikic I rebased it now. For some reason test 19 "Attribute name cannot be a variable" is failing for me, though. Is there something I'm missing in the parser?

@nikic
Copy link
Member

nikic commented Jul 22, 2020

What's the new output you get there? It's normal for parse error messages to change in small ways.

Zend/zend_language_scanner.l Outdated Show resolved Hide resolved
@theodorejb theodorejb force-pushed the shorter-attribute-syntax-rfc branch from 9252f1b to 451c1c7 Compare July 22, 2020 15:08
Zend/zend_language_parser.y Outdated Show resolved Hide resolved
@theodorejb theodorejb force-pushed the shorter-attribute-syntax-rfc branch from 451c1c7 to 81f8c83 Compare July 22, 2020 15:15
@kooldev
Copy link
Contributor

kooldev commented Jul 23, 2020

I want to point out that after recent changes to the lexer in this PR it permits whitespace and comments between @@ and the attribute name. This was not allowed in my original @: / @@ implementation. For example the following code works with the current implementation:

function foo(@@A1@@ /* foo */ A1 \stdClass $p) { }

There is no need for any whitespace / non-name-char before @@. While this is not a technical problem it is pretty bad code style. All kinds of whitespace and comments are now allowed to sit between the token and the name and parse just fine. The following code can be parsed and works (as far as the parser is concerned) but is very difficult to read (and again bad code style):

@@ A1
@@

// Some comment
A1 class Test { }

The use of whitespace / comments is also possible with <<>> or #[] syntax, but having an ending delimiter improves readability in these cases. The other syntax(es) also support writing multiple attributes without any whitespace in between. Again the endling delimiter helps a little bit with readability (and grouping should have been used to avoid multiple enclosings anyways).

function foo(<<A1>><< A1>> \stdClass $p) { }
function foo(#[A1]#[ A1] \stdClass $p) { }

I am not sure if you consider this to be a problem but maybe you should adjust the lexer to enforce @@ to be immediately followed by a character that may start a name?

@nikic
Copy link
Member

nikic commented Jul 23, 2020

@kooldev Not a problem from our perspective, as we leave the enforcement of coding style to other projects. This is no different from someone deciding to write

Foo
// haha
::
// haha
Bar
// haha
;

or some equally silly, but still valid, code.

Zend/zend_compile.c Outdated Show resolved Hide resolved
@nikic
Copy link
Member

nikic commented Jul 23, 2020

The patch looks good to me, but we should probably wait for the outcome of https://externals.io/message/111101 before merging this.

@theodorejb theodorejb force-pushed the shorter-attribute-syntax-rfc branch from 81f8c83 to 551343c Compare July 23, 2020 12:05
@theodorejb
Copy link
Contributor Author

@nikic I assume this can be merged now per Sara's comment? https://externals.io/message/111101#111158

@beberlei
Copy link
Contributor

I'll merge the PR next week.

RFC: https://wiki.php.net/rfc/shorter_attribute_syntax

Co-authored-by: Martin Schröder <m.schroeder2007@gmail.com>
@theodorejb theodorejb force-pushed the shorter-attribute-syntax-rfc branch from 551343c to 8597a97 Compare July 26, 2020 23:25
@php-pulls php-pulls closed this in 470d169 Jul 28, 2020
@theodorejb theodorejb deleted the shorter-attribute-syntax-rfc branch July 28, 2020 22:17
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.

6 participants