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.

Sign up for GitHub

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

Reject multi hooked property #1052

Closed
wants to merge 2 commits into from
Closed

Reject multi hooked property #1052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df875a0
Rename ParserAbstract method checkPropertyHookList to checkEmptyPrope…
ondrejmirtes Dec 11, 2024
99972e9
Emit error - Multiple properties cannot share the same hooks
ondrejmirtes Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions grammar/php.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,8 @@ class_statement:
#if PHP8
| optional_attributes variable_modifiers optional_type_without_static property_declaration_list '{' property_hook_list '}'
{ $$ = new Stmt\Property($2, $4, attributes(), $3, $1, $6);
$this->checkPropertyHookList($6, #5); }
$this->checkPropertyHooksForMultiProperty($$);
$this->checkEmptyPropertyHookList($6, #5); }
#endif
| optional_attributes method_modifiers T_CONST class_const_list semi
{ $$ = new Stmt\ClassConst($4, $2, attributes(), $1);
Expand Down Expand Up @@ -949,7 +950,7 @@ property_hook_list:
optional_property_hook_list:
/* empty */ { $$ = []; }
#if PHP8
| '{' property_hook_list '}' { $$ = $2; $this->checkPropertyHookList($2, #1); }
| '{' property_hook_list '}' { $$ = $2; $this->checkEmptyPropertyHookList($2, #1); }
#endif
;

Expand Down
5 changes: 3 additions & 2 deletions lib/PhpParser/Parser/Php8.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1993,7 +1993,8 @@ protected function initReduceCallbacks(): void {
},
348 => static function ($self, $stackPos) {
$self->semValue = new Stmt\Property($self->semStack[$stackPos-(7-2)], $self->semStack[$stackPos-(7-4)], $self->getAttributes($self->tokenStartStack[$stackPos-(7-1)], $self->tokenEndStack[$stackPos]), $self->semStack[$stackPos-(7-3)], $self->semStack[$stackPos-(7-1)], $self->semStack[$stackPos-(7-6)]);
$self->checkPropertyHookList($self->semStack[$stackPos-(7-6)], $stackPos-(7-5));
$self->checkPropertyHooksForMultiProperty($self->semValue);
$self->checkEmptyPropertyHookList($self->semStack[$stackPos-(7-6)], $stackPos-(7-5));
},
349 => static function ($self, $stackPos) {
$self->semValue = new Stmt\ClassConst($self->semStack[$stackPos-(5-4)], $self->semStack[$stackPos-(5-2)], $self->getAttributes($self->tokenStartStack[$stackPos-(5-1)], $self->tokenEndStack[$stackPos]), $self->semStack[$stackPos-(5-1)]);
Expand Down Expand Up @@ -2122,7 +2123,7 @@ protected function initReduceCallbacks(): void {
$self->semValue = [];
},
394 => static function ($self, $stackPos) {
$self->semValue = $self->semStack[$stackPos-(3-2)]; $self->checkPropertyHookList($self->semStack[$stackPos-(3-2)], $stackPos-(3-1));
$self->semValue = $self->semStack[$stackPos-(3-2)]; $self->checkEmptyPropertyHookList($self->semStack[$stackPos-(3-2)], $stackPos-(3-1));
},
395 => static function ($self, $stackPos) {
$self->semValue = new Node\PropertyHook($self->semStack[$stackPos-(5-4)], $self->semStack[$stackPos-(5-5)], ['flags' => $self->semStack[$stackPos-(5-2)], 'byRef' => $self->semStack[$stackPos-(5-3)], 'params' => [], 'attrGroups' => $self->semStack[$stackPos-(5-1)]], $self->getAttributes($self->tokenStartStack[$stackPos-(5-1)], $self->tokenEndStack[$stackPos]));
Expand Down
9 changes: 8 additions & 1 deletion lib/PhpParser/ParserAbstract.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,8 +1158,15 @@ protected function checkUseUse(UseItem $node, int $namePos): void {
}
}

protected function checkPropertyHooksForMultiProperty(Property $property): void {
if (count($property->props) > 1) {
$this->emitError(new Error(
'Multiple properties cannot share the same hooks', $property->getAttributes()));
Copy link
Owner

Choose a reason for hiding this comment

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

This is marking the entire property as the error location. This should probably use the opening brace of the hooks instead, like the one below. (Or alternatively the name of the second property, not sure which is better.)

}
}

/** @param PropertyHook[] $hooks */
protected function checkPropertyHookList(array $hooks, int $hookPos): void {
protected function checkEmptyPropertyHookList(array $hooks, int $hookPos): void {
if (empty($hooks)) {
$this->emitError(new Error(
'Property hook list cannot be empty', $this->getAttributesAt($hookPos)));
Expand Down
113 changes: 113 additions & 0 deletions test/code/parser/stmt/class/property_hooks.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,3 +523,116 @@ array(
)
)
)
-----
<?php
class Test
{

public $foo, $bar { get { return 42; } }

}
-----
Multiple properties cannot share the same hooks from 5:5 to 5:44
array(
0: Stmt_Class(
attrGroups: array(
)
flags: 0
name: Identifier(
name: Test
)
extends: null
implements: array(
)
stmts: array(
0: Stmt_Property(
attrGroups: array(
)
flags: PUBLIC (1)
type: null
props: array(
0: PropertyItem(
name: VarLikeIdentifier(
name: foo
)
default: null
)
1: PropertyItem(
name: VarLikeIdentifier(
name: bar
)
default: null
)
)
hooks: array(
0: PropertyHook(
attrGroups: array(
)
flags: 0
byRef: false
name: Identifier(
name: get
)
params: array(
)
body: array(
0: Stmt_Return(
expr: Scalar_Int(
value: 42
)
)
)
)
)
)
)
)
)
-----
<?php
class Test
{

public $foo, $bar { }

}
-----
Multiple properties cannot share the same hooks from 5:5 to 5:25
Property hook list cannot be empty from 5:23 to 5:23
array(
0: Stmt_Class(
attrGroups: array(
)
flags: 0
name: Identifier(
name: Test
)
extends: null
implements: array(
)
stmts: array(
0: Stmt_Property(
attrGroups: array(
)
flags: PUBLIC (1)
type: null
props: array(
0: PropertyItem(
name: VarLikeIdentifier(
name: foo
)
default: null
)
1: PropertyItem(
name: VarLikeIdentifier(
name: bar
)
default: null
)
)
hooks: array(
)
)
)
)
)
Loading