Add AST_CLASS_NAME in AST version 70 #111
Conversation
0407165 to
efc4315
Compare
|
The merge conflicts were fixed, tests in php 7.4-dev are still passing locally EDIT: Added missing |
7d0dcd1 to
c117795
Compare
ast.c
Outdated
| // Convert to an AST_CLASS_NAME instead. The opposite of the work done in the ZEND_AST_CLASS_NAME case. | ||
| zend_ast *const_name_ast = ast->child[1]; | ||
| if (const_name_ast->kind != ZEND_AST_ZVAL) { | ||
| break; // Currently impossible |
There was a problem hiding this comment.
If it's impossible, we should drop the check. zend_ast_get_zval contains an assertion that checks this. Even better would be to use zend_ast_get_str() to directly get at the zend_string.
There was a problem hiding this comment.
php > var_export([]::class);
Fatal error: Cannot use ::class with dynamic class name in php shell code on line 1
Okay, it's possible (I just didn't test every possible edge case) - that's a runtime error, not a syntax error
There was a problem hiding this comment.
Okay, I'm going to have to report a bug with php 7.4-dev debug ZTS on bugs.php.net:
php > var_export((2)::class);
php: /path/to/php-src/Zend/zend_ast.h:298: zend_ast_get_str: Assertion `zval_get_type(&(*(zv))) == 6' failed.
[1] 4949 abort php -aThere was a problem hiding this comment.
There was a problem hiding this comment.
It turns out that I was mistaken and it indeed something I should check for (I tried 2::class the first time but not (2)::class)
276ca09 to
9f92d29
Compare
Fixes nikic#109 Tests pass with a local PHP 7.4 installation
e1babc2 to
7258537
Compare
|
All review comments have been addressed, this is ready for another look. The new tests continue to pass in PHP 7.4. Note that the flags on the AST_ARRAY of |
ast.c
Outdated
| if (state->version >= 70) { | ||
| // Convert to an AST_CLASS_NAME instead. This is the opposite of the work done in the ZEND_AST_CLASS_NAME case. | ||
| zend_ast *const_name_ast = ast->child[1]; | ||
| if (const_name_ast->kind != ZEND_AST_ZVAL) { |
There was a problem hiding this comment.
While the class can be an expression, the constant name can't be (we don't have Foo::{$bar} in PHP), so these checks at least can be replaced with zend_ast_get_str.
| } else { | ||
| // e.g. []::class (not a parse error, but a runtime error) | ||
| ast_to_zval(&class_name_zval, class_name_ast, state); | ||
| } |
There was a problem hiding this comment.
Wouldn't ast_to_zval here work for both branches?
There was a problem hiding this comment.
It didn't create the ast\Node when I tried that, probably because the parent kind or position was wrong.
There was a problem hiding this comment.
Ah yeah, I guess it will just convert it to a simple value. In that case it's fine.
|
Thanks! |
Fixes #109
This also makes PHP 7.4 emulate the old behavior (AST_CLASS_CONST) for versions 50 and 60.
Tests pass with a local PHP 7.4 installation