Skip to content

Allow dynamic properties (e.g. endLineno) without deprecation notice in ast\Node #217

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 1 commit into from
Nov 27, 2021

Conversation

TysonAndre
Copy link
Collaborator

(But use the default deprecation behavior for ast\Metadata)

  1. php-ast itself sets the dynamic property on endLineno right now,
    causing deprecation notices in PHP 8.2.
    Because all declaration types are associative arrays(not lists),
    a future AST version number may add this as a property of
    $node->children instead to avoid this notice.

  2. WeakMap is only available in PHP 8.0+ (and WeakReference 7.4),
    but https://github.com/phan/phan supports PHP 7.2+.

    Having a Phan version (or external plugins) that can work (and be fast) with a wider range of php versions is useful for organizations trying to detect uses of undeclared properties that need to migrate.

  3. Because some nodes make $node->children a list,
    applications such as Phan using php-ast can't associate string keys
    with that.
    (e.g. to mark nodes that were already processed in a certain step)

    https://github.com/nikic/php-parser solves that by adding
    attributes, setAttribute, getAttribute, etc. separately from
    children, but php-ast currently doesn't currently have an attributes node.
    That may be useful to add (and to remove the AllowsDynamicProperty annotation in an incompatible php-ast major release)

    That may be worth it in a future release.

  4. When processing a large number of ast nodes in a large number of
    files, the deprecation notices have a noticeable performance impact,
    even when suppressed.

Related to #214

Phan has a lot of uses of dynamic properties - e.g.

  • to associate a hash with a node to check if two nodes are equivalent AST trees (probabalistically)
  • to check (when using the polyfill fallback for php-ast) if syntax that's deprecated (unparenthesized ambiguous ?: syntax, trailing commas).
  • to check if an AST node is already normalized to an easier to analyze form
  • to check for infinite recursion
  • to track information about a function's AST_STMT_LIST such as the set of used goto labels (can probably be migrated?)
  • various others
src/Phan/AST/ASTHasher.php
66:        // @phan-suppress-next-line PhanUndeclaredProperty
67-        return $node->hash ?? ($node->hash = self::computeHash($node));

src/Phan/AST/TolerantASTConverter/TolerantASTConverter.php
1046:                    // @phan-suppress-next-line PhanUndeclaredProperty
1047-                    $result->is_not_parenthesized = true;
--
2096:            // @phan-suppress-next-line PhanUndeclaredProperty
2097-            $result->polyfill_has_trailing_comma = true;
--

@@ -1518,6 +1522,10 @@ PHP_MINIT_FUNCTION(ast) {
ast_declare_property(ast_node_ce, AST_STR(str_flags), &zv_null);
ast_declare_property(ast_node_ce, AST_STR(str_lineno), &zv_null);
ast_declare_property(ast_node_ce, AST_STR(str_children), &zv_null);
#ifdef ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES
zend_add_class_attribute(ast_node_ce, zend_ce_allow_dynamic_properties->name, 0);
ast_node_ce->ce_flags |= ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES;
Copy link
Owner

Choose a reason for hiding this comment

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

Setting the flag explicitly should not be necessary.

Copy link
Collaborator Author

@TysonAndre TysonAndre Nov 27, 2021

Choose a reason for hiding this comment

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

I tried removing the line. Deprecated: Creation of dynamic property ast\Node::$undeclaredDynamic is deprecated in /home/tyson/programming/php-ast/tests/php82_metadata.php on line 18 is the result.

This is the same as what https://github.com/php/php-src/blob/master/Zend/zend_builtin_functions.c does
at the time of writing:

https://github.com/php/php-src/blob/902d64390e49f8bf970588cf53cd8e00630c68bb/Zend/zend_builtin_functions.c#L39-L41

Copy link
Owner

Choose a reason for hiding this comment

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

Ah yes, looks like the callback doesn't get called on interned classes for some reason.

…in ast\Node

(But use the default deprecation behavior for ast\Metadata)

1. php-ast itself sets the dynamic property on endLineno right now,
   causing deprecation notices in PHP 8.2.
   Because all declaration types are associative arrays(not lists),
   a future AST version number may add this as a property of
   $node->children instead to avoid this notice.
2. WeakMap is only available in PHP 8.0+ (and WeakReference 7.4),
   but https://github.com/phan/phan targets PHP 7.2+.
3. Because some nodes make `$node->children` a list,
   applications such as Phan using php-ast can't associate string keys
   with that.
   (e.g. to mark nodes that were already processed in a certain step)

   https://github.com/nikic/php-parser solves that by adding
   `attributes`, `setAttribute`, `getAttribute`, etc. separately from
   children, but php-ast currently doesn't have an attributes node.

   That may be worth it in a future release.
4. When processing a large number of ast nodes in a large number of
   files, the deprecation notices have a noticeable performance impact,
   even when suppressed.

Related to nikic#214
@TysonAndre TysonAndre force-pushed the allow-dynamic-properties branch from 7001c93 to d246ddd Compare November 27, 2021 15:16
Copy link
Owner

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Fine by me, allowing custom node extensions seems like a pragmatic choice.

@TysonAndre TysonAndre merged commit e36cc7d into nikic:master Nov 27, 2021
@TysonAndre TysonAndre deleted the allow-dynamic-properties branch November 27, 2021 15:26
TysonAndre added a commit to TysonAndre/php-ast that referenced this pull request Nov 27, 2021
Make it possible to test with php 8.2 without deprecation notes for
endLineno, etc. See nikic#217

Also, document how to properly update cache slot numbers if new
properties are added to ast\Node in the future.
TysonAndre added a commit that referenced this pull request Nov 27, 2021
* Update php versions tested in CI

PHP '8.1' isn't published yet on docker hub.

Start testing with php 8.1 in appveyor

* Prepare for a 1.0.15 release

Make it possible to test with php 8.2 without deprecation notes for
endLineno, etc. See #217

Also, document how to properly update cache slot numbers if new
properties are added to ast\Node in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants