Skip to content

fix: staticlookup inside call #225

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

Merged
merged 3 commits into from
Nov 10, 2018
Merged

fix: staticlookup inside call #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a284ac0
fix: staticlookup inside call
alexander-akait Nov 6, 2018
e3861f1
add a regression test (waiting to fail)
ichiriac Nov 10, 2018
15fbf83
Merge branch 'master' into issue-201
ichiriac Nov 10, 2018
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
4 changes: 4 additions & 0 deletions src/parser/variable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ module.exports = {
name = this.text();
this.next();
offset = offset(name);
} else if (this.token === "{") {
offset = this.next().read_expr();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks read_expr is invalid here, what better call?

Copy link
Member

Choose a reason for hiding this comment

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

it's not so clear, here an extract from the grammar :

https://github.com/php/php-src/blob/master/Zend/zend_language_parser.y#L1175

member_name:
		identifier { $$ = $1; }
	|	'{' expr '}'	{ $$ = $2; }
	|	simple_variable	{ $$ = zend_ast_create(ZEND_AST_VAR, $1); }
;

simple_variable:
		T_VARIABLE			{ $$ = $1; }
	|	'$' '{' expr '}'	{ $$ = $3; }
	|	'$' simple_variable	{ $$ = zend_ast_create(ZEND_AST_VAR, $2); }
;

function_call:
		name argument_list
			{ $$ = zend_ast_create(ZEND_AST_CALL, $1, $2); }
	|	class_name T_PAAMAYIM_NEKUDOTAYIM member_name argument_list
			{ $$ = zend_ast_create(ZEND_AST_STATIC_CALL, $1, $3, $4); }
	|	variable_class_name T_PAAMAYIM_NEKUDOTAYIM member_name argument_list
			{ $$ = zend_ast_create(ZEND_AST_STATIC_CALL, $1, $3, $4); }
	|	callable_expr argument_list
			{ $$ = zend_ast_create(ZEND_AST_CALL, $1, $2); }
;

and function_call is used by callable_variable, whic used by variable, used by expr, so that's the right place - I'll checkout and play with the code

Copy link
Member

Choose a reason for hiding this comment

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

everything is correct, but I see an extra statement. Here a sample error code :

Order::{call()};
SyntaxError: Parse Error : syntax error, unexpected ';', expecting '(' on line 1

Come from this L99 :
https://github.com/glayzzle/php-parser/pull/225/files/a284ac035ed8f7d57e97c46a94f6aa0cc7467f17#diff-bb9fff60e32f00c7312e3788510fe2acR99

As you can see from the specs, you have handled the '{' expr '}' :

member_name:
		identifier { $$ = $1; }
	|	'{' expr '}'	{ $$ = $2; }
	|	simple_variable	{ $$ = zend_ast_create(ZEND_AST_VAR, $1); }
;

Copy link
Member

Choose a reason for hiding this comment

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

Damn, after checking with PHP 7.2, here the result :

ioan@devbox:~/Documents/glayzzle/php-parser$ php -r "Order::{call()};"
PHP Parse error:  syntax error, unexpected ';', expecting '(' in Command line code on line 1

Need to digg more into this 👎

Copy link
Member

Choose a reason for hiding this comment

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

Ok, just for information, in order to check what is wrong, I make a search on the zend parser language on the token that make static lookups T_PAAMAYIM_NEKUDOTAYIM and try to understand what can come next - Im' doing this to understand allowed syntax.

Here the results :

class_name T_PAAMAYIM_NEKUDOTAYIM simple_variable
variable_class_name T_PAAMAYIM_NEKUDOTAYIM simple_variable
new_variable T_PAAMAYIM_NEKUDOTAYIM simple_variable
name T_PAAMAYIM_NEKUDOTAYIM identifier
class_name T_PAAMAYIM_NEKUDOTAYIM member_name argument_list
variable_class_name T_PAAMAYIM_NEKUDOTAYIM member_name argument_list
class_name T_PAAMAYIM_NEKUDOTAYIM identifier
variable_class_name T_PAAMAYIM_NEKUDOTAYIM identifier

We are in a member_name related expression, before your change member_name was either an identifier or a simple_variable, so no mandatory argument_list.

Now with the expr introduction, member_name has an differentiation, identier or simple_variable (previous code) can run just as before without a constraint on calling expression, but now, you need a call statement just after the expr, because of member_name argument_list.

So your expect ( statement was a good way to force the parser to validate the syntax and continue the AST with a call node.

Now in order to be sure if you don't introduce a miss-behavior you need to check the read_static_getter helper usage into the parser and also to check the member_name from the php source.

In php-parser, it's used by read_class_name_reference, read_scalar, read_variable

In php yaccs file : function_call, which is itself called from callable_variable, which in turn is called from callable_expr, variable (equivalent of our read_variable)

So I conclude that read_class_name_reference + read_scalar may introduce problems, need to digg more.

Checking the read_scallar function I found a comment about https://github.com/php/php-src/blob/master/Zend/zend_language_parser.y#L1151 pointing to nothing as I did not include the commit-id on links 👎 - so I need to digg with the opening token T_CONSTANT_ENCAPSED_STRING, used by dereferencable_scalar -> scallar, dereferencable, callable_expr. As callable_expr is used to trigger a function_call we are good with this one.

The same for the other, the read_new_expr is the only reference to this one, so I go to the yaccs definition, and find that corresponds to the new_expr definition defined as :

new_expr:
		T_NEW class_name_reference ctor_arguments
			{ $$ = zend_ast_create(ZEND_AST_NEW, $2, $3); }
	|	T_NEW anonymous_class
			{ $$ = $2; }
;

Our block of code concerns onlye the class_name_reference ctor_arguments part, and here the definition of the class_name_reference

class_name_reference:
		class_name		{ $$ = $1; }
	|	new_variable	{ $$ = $1; }
;
new_variable:
		simple_variable
			{ $$ = zend_ast_create(ZEND_AST_VAR, $1); }
	|	new_variable '[' optional_expr ']'
			{ $$ = zend_ast_create(ZEND_AST_DIM, $1, $3); }
	|	new_variable '{' expr '}'
			{ $$ = zend_ast_create(ZEND_AST_DIM, $1, $3); }
	|	new_variable T_OBJECT_OPERATOR property_name
			{ $$ = zend_ast_create(ZEND_AST_PROP, $1, $3); }
	|	class_name T_PAAMAYIM_NEKUDOTAYIM simple_variable
			{ $$ = zend_ast_create(ZEND_AST_STATIC_PROP, $1, $3); }
	|	new_variable T_PAAMAYIM_NEKUDOTAYIM simple_variable
			{ $$ = zend_ast_create(ZEND_AST_STATIC_PROP, $1, $3); }
;
simple_variable:
		T_VARIABLE			{ $$ = $1; }
	|	'$' '{' expr '}'	{ $$ = $3; }
	|	'$' simple_variable	{ $$ = zend_ast_create(ZEND_AST_VAR, $2); }
;

As you can see here, T_PAAMAYIM_NEKUDOTAYIM must be handled in our case, but just followed with simple_variable which excludes an identifier or an '{' expr '}'

Copy link
Member

Choose a reason for hiding this comment

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

Here our parser is just not enough strict and will parse these extra syntax, and PHP not :

ioan@devbox:~/Documents/glayzzle/php-parser$ php -r "new Foo::{call()}();"
PHP Parse error:  syntax error, unexpected '{', expecting variable (T_VARIABLE) or '$' in Command line code on line 1
ioan@devbox:~/Documents/glayzzle/php-parser$ php -r "new Foo::call();"
PHP Parse error:  syntax error, unexpected 'call' (T_STRING), expecting variable (T_VARIABLE) or '$' in Command line code on line 1
ioan@devbox:~/Documents/glayzzle/php-parser$ php -r "new Foo::call;"
PHP Parse error:  syntax error, unexpected 'call' (T_STRING), expecting variable (T_VARIABLE) or '$' in Command line code on line 1
ioan@devbox:~/Documents/glayzzle/php-parser$ php -r "new Foo::$bar;"
PHP Parse error:  syntax error, unexpected ';', expecting variable (T_VARIABLE) or '$' in Command line code on line 1

this.expect("}") && this.next();
this.expect("(");
} else {
this.error([this.tok.T_VARIABLE, this.tok.T_STRING]);
// graceful mode : set getter as error node and continue
Expand Down
59 changes: 59 additions & 0 deletions test/snapshot/__snapshots__/call.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,65 @@ Program {
}
`;

exports[`Test call inside staticlookup (10) 1`] = `
Program {
"children": Array [
Call {
"arguments": Array [],
"kind": "call",
"what": Call {
"arguments": Array [],
"kind": "call",
"what": StaticLookup {
"kind": "staticlookup",
"offset": Identifier {
"kind": "identifier",
"name": "call",
},
"what": ClassReference {
"kind": "classreference",
"name": "Order",
"resolution": "uqn",
},
},
},
},
],
"errors": Array [],
"kind": "program",
}
`;

exports[`Test call inside staticlookup (11) 1`] = `
Program {
"children": Array [
Call {
"arguments": Array [],
"kind": "call",
"what": StaticLookup {
"kind": "staticlookup",
"offset": Call {
"arguments": Array [],
"kind": "call",
"what": ClassReference {
"kind": "classreference",
"name": "call",
"resolution": "uqn",
},
},
"what": ClassReference {
"kind": "classreference",
"name": "Order",
"resolution": "uqn",
},
},
},
],
"errors": Array [],
"kind": "program",
}
`;

exports[`Test call inside staticlookup 1`] = `
Program {
"children": Array [
Expand Down
67 changes: 67 additions & 0 deletions test/snapshot/__snapshots__/graceful.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,73 @@ Program {
}
`;

exports[`Test graceful mode to suppress errors should fail ! 1`] = `
Program {
"children": Array [
ExpressionStatement {
"expression": New {
"arguments": Array [],
"kind": "new",
"what": StaticLookup {
"kind": "staticlookup",
"offset": Call {
"arguments": Array [],
"kind": "call",
"what": ClassReference {
"kind": "classreference",
"name": "call",
"resolution": "uqn",
},
},
"what": ClassReference {
"kind": "classreference",
"name": "Foo",
"resolution": "uqn",
},
},
},
"kind": "expressionstatement",
},
],
"errors": Array [],
"kind": "program",
}
`;

exports[`Test graceful mode to suppress errors staticlookup 1`] = `
Program {
"children": Array [
StaticLookup {
"kind": "staticlookup",
"offset": Call {
"arguments": Array [],
"kind": "call",
"what": ClassReference {
"kind": "classreference",
"name": "call",
"resolution": "uqn",
},
},
"what": ClassReference {
"kind": "classreference",
"name": "Order",
"resolution": "uqn",
},
},
],
"errors": Array [
Error {
"expected": "(",
"kind": "error",
"line": 1,
"message": "Parse Error : syntax error, unexpected ';', expecting '(' on line 1",
"token": "';'",
},
],
"kind": "program",
}
`;

exports[`Test graceful mode to suppress errors test class 1`] = `
Program {
"children": Array [
Expand Down
18 changes: 18 additions & 0 deletions test/snapshot/call.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,4 +280,22 @@ describe("Test call", function() {
);
expect(ast).toMatchSnapshot();
});
it("inside staticlookup (10)", function() {
const ast = parser.parseEval(
'Order::call()();',
{
parser: { debug: false }
}
);
expect(ast).toMatchSnapshot();
});
it("inside staticlookup (11)", function() {
const ast = parser.parseEval(
'Order::{call()}();',
{
parser: { debug: false }
}
);
expect(ast).toMatchSnapshot();
});
});
9 changes: 9 additions & 0 deletions test/snapshot/graceful.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,14 @@ describe("Test graceful mode", function() {
->
`)).toMatchSnapshot();
});

it("staticlookup", function() {
expect(test.parseEval('Order::{call()};')).toMatchSnapshot();
});

it("should fail !", function() {
expect(test.parseEval('new Foo::{call()}();')).toMatchSnapshot();
});

});
});