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

invalid ast for self and parent #235

Closed
alexander-akait opened this issue Nov 12, 2018 · 4 comments
Closed

invalid ast for self and parent #235

alexander-akait opened this issue Nov 12, 2018 · 4 comments
Labels
bug high-pri High Priority bug RELEASE-READY Waiting to release

Comments

@alexander-akait
Copy link
Collaborator

Input:

self();
sElF();
parent();
pArEnT();

Resolves as SelfReference and ParentReference, but it is invalid

@alexander-akait alexander-akait added bug high-pri High Priority bug labels Nov 12, 2018
@alexander-akait
Copy link
Collaborator Author

Any ideas how we can fix? I try but looks it is not easy.

Oh, we can't update parser around 10-20 commits due only regressions 😞

@ichiriac
Copy link
Member

Hi, this behavior was introduced here :

if (!relative && names.length === 1) {

The reason is that the read_variable is used in many parsing contexts, and we introduced this change only for handling the instanceof and new keywords.

Take a look at the tests/debug.js, it's an helper in order to run step by step the parser and create breakpoints with vscode, it will help you in the future to debug more easily.

Here the full story from the https://github.com/php/php-src/blob/master/Zend/zend_language_parser.y :

new_expr:
		T_NEW class_name_reference ctor_arguments
			{ $$ = zend_ast_create(ZEND_AST_NEW, $2, $3); }
	|	T_NEW anonymous_class
			{ $$ = $2; }
;
expr: .... 
   expr T_INSTANCEOF 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); }
;

And actually the code says :

  /**
   * Reads a class name
   * ```ebnf
   * class_name_reference ::= namespace_name | variable
   * ```
   */
  read_class_name_reference: function() {
    if (
      this.token === this.tok.T_NS_SEPARATOR ||
      this.token === this.tok.T_STRING ||
      this.token === this.tok.T_NAMESPACE
    ) {
      let result = this.read_namespace_name();
      if (this.token === this.tok.T_DOUBLE_COLON) {
        result = this.read_static_getter(result);
      }
      return result;
    } else if (this.is("VARIABLE")) {
      return this.read_variable(true, false, false);
    } else {
      this.expect([this.tok.T_STRING, "VARIABLE"]);
    }

We have here multiple problems, first the read_variable is not the good version following the spec, should introduce a new function read_new_variable.

Also the read_namespace_name is used in both contexts (from a read_class_name_reference and also read_variable) and so the namespace can be both a reference to self keyword, but also a function name...

The problem here it's the look ahead, you can't know how to resolve the type without knowing the next token. The solution here is to check it before resolving the type of node.

ichiriac added a commit that referenced this issue Nov 18, 2018
@ichiriac
Copy link
Member

it's fixed, you can test and if it's OK for you we can close this issue

@ichiriac ichiriac added the RELEASE-READY Waiting to release label Nov 18, 2018
@ichiriac
Copy link
Member

ichiriac commented Jan 7, 2019

released in 3.0.0-prerelease.8

@ichiriac ichiriac closed this as completed Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug high-pri High Priority bug RELEASE-READY Waiting to release
Projects
None yet
Development

No branches or pull requests

2 participants