Skip to content

Support every argument syntax for clone() #18938

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

Merged
merged 4 commits into from
Jun 30, 2025

Conversation

TimWolla
Copy link
Member

Follow-up for #18919 and extracted out of #18747.

@nielsdos
Copy link
Member

Right... shift-reduce conflict on argument vs expression...
I'll have a better look tomorrow or so, but on first sight this seems fine. I wonder if it's possible to make it even simpler, but this is already quite simple.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

It seems right to me. Please add a comment to the parser to explain that the reason you do this is to resolve a shift-reduce conflict.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks correct. As mentioned, this could also have been solved with lexer lookahead, which would have been simpler. But I understand why people don't like these solutions as much. 😋

@TimWolla TimWolla merged commit c9249e2 into php:master Jun 30, 2025
9 checks passed
@TimWolla TimWolla deleted the clone-all-syntax branch June 30, 2025 10:31
@iluuu1994
Copy link
Member

A few minutes too late, I had the idea that we could just treat clone as an identifier rather than a keyword when it's followed by (. This way, clone ($foo) is just treated as a function call, which solves the problem automatically, while also not requiring any new special tokens. Something like:

diff --git a/Zend/zend_language_parser.y b/Zend/zend_language_parser.y
index 805f378cb98..c92e830b8f1 100644
--- a/Zend/zend_language_parser.y
+++ b/Zend/zend_language_parser.y
@@ -287,7 +287,7 @@ static YYSIZE_T zend_yytnamerr(char*, const char*);
 %type <ast> enum_declaration_statement enum_backing_type enum_case enum_case_expr
 %type <ast> function_name non_empty_member_modifiers
 %type <ast> property_hook property_hook_list optional_property_hook_list hooked_property property_hook_body
-%type <ast> optional_parameter_list clone_argument_list non_empty_clone_argument_list
+%type <ast> optional_parameter_list
 
 %type <num> returns_ref function fn is_reference is_variadic property_modifiers property_hook_modifiers
 %type <num> method_modifiers class_const_modifiers member_modifier optional_cpp_modifiers
@@ -914,31 +914,6 @@ non_empty_argument_list:
 			{ $$ = zend_ast_list_add($1, $3); }
 ;
 
-/* `clone_argument_list` is necessary to resolve a parser ambiguity (shift-reduce conflict)
- * of `clone($expr)`, which could either be parsed as a function call with `$expr` as the first
- * argument or as a use of the `clone` language construct with an expression with useless
- * parenthesis. Both would be valid and result in the same AST / the same semantics.
- * `clone_argument_list` is defined in a way that an `expr` in the first position needs to
- * be followed by a `,` which is not valid syntax for a parenthesized `expr`, ensuring
- * that calling `clone()` with a single unnamed parameter is handled by the language construct
- * syntax.
- */
-clone_argument_list:
-		'(' ')'	{ $$ = zend_ast_create_list(0, ZEND_AST_ARG_LIST); }
-	|	'(' non_empty_clone_argument_list possible_comma ')' { $$ = $2; }
-	|	'(' expr ',' ')' { $$ = zend_ast_create_list(1, ZEND_AST_ARG_LIST, $2); }
-	|	'(' T_ELLIPSIS ')' { $$ = zend_ast_create_fcc(); }
-;
-
-non_empty_clone_argument_list:
-		expr ',' argument
-			{ $$ = zend_ast_create_list(2, ZEND_AST_ARG_LIST, $1, $3); }
-	|	argument_no_expr
-			{ $$ = zend_ast_create_list(1, ZEND_AST_ARG_LIST, $1); }
-	|	non_empty_clone_argument_list ',' argument
-			{ $$ = zend_ast_list_add($1, $3); }
-;
-
 argument_no_expr:
 		identifier ':' expr
 			{ $$ = zend_ast_create(ZEND_AST_NAMED_ARG, $1, $3); }
@@ -1257,11 +1232,6 @@ expr:
 			{ $$ = zend_ast_create(ZEND_AST_ASSIGN, $1, $3); }
 	|	variable '=' ampersand variable
 			{ $$ = zend_ast_create(ZEND_AST_ASSIGN_REF, $1, $4); }
-	|	T_CLONE clone_argument_list {
-			zend_ast *name = zend_ast_create_zval_from_str(ZSTR_KNOWN(ZEND_STR_CLONE));
-			name->attr = ZEND_NAME_FQ;
-			$$ = zend_ast_create(ZEND_AST_CALL, name, $2);
-		}
 	|	T_CLONE expr {
 			zend_ast *name = zend_ast_create_zval_from_str(ZSTR_KNOWN(ZEND_STR_CLONE));
 			name->attr = ZEND_NAME_FQ;
diff --git a/Zend/zend_language_scanner.l b/Zend/zend_language_scanner.l
index 5e377249422..d3917c4dc89 100644
--- a/Zend/zend_language_scanner.l
+++ b/Zend/zend_language_scanner.l
@@ -1613,6 +1613,11 @@ OPTIONAL_WHITESPACE_OR_COMMENTS ({WHITESPACE}|{MULTI_LINE_COMMENT}|{SINGLE_LINE_
 	RETURN_TOKEN_WITH_IDENT(T_NEW);
 }
 
+<ST_IN_SCRIPTING>"clone"{OPTIONAL_WHITESPACE_OR_COMMENTS}"(" {
+	yyless(5);
+	RETURN_TOKEN_WITH_STR(T_NAME_FULLY_QUALIFIED, 0);
+}
+
 <ST_IN_SCRIPTING>"clone" {
 	RETURN_TOKEN_WITH_IDENT(T_CLONE);
 }

WDYT?

TimWolla added a commit to TimWolla/php-src that referenced this pull request Jun 30, 2025
This greatly simplifies the parser to support `clone()` as a function and
effectively reverts php#18938.

Co-authored-by: Ilija Tovilo <[email protected]>
TimWolla added a commit to TimWolla/PHP-Parser that referenced this pull request Jul 2, 2025
TimWolla added a commit to TimWolla/PHP-Parser that referenced this pull request Jul 2, 2025
nikic pushed a commit to nikic/PHP-Parser that referenced this pull request Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants