Skip to content

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

Merged
TysonAndre merged 1 commit into
nikic:masterfrom
TysonAndre:allow-dynamic-properties
Nov 27, 2021
Merged

Allow dynamic properties (e.g. endLineno) without deprecation notice in ast\Node#217
TysonAndre merged 1 commit into
nikic:masterfrom
TysonAndre:allow-dynamic-properties

Conversation

@TysonAndre

Copy link
Copy Markdown
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;
--

Comment thread ast.c
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
Copy Markdown
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.

@TysonAndre TysonAndre Nov 27, 2021

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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.

Comment thread tests/php82_metadata.phpt Outdated
…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

@nikic nikic left a comment

Copy link
Copy Markdown
Owner

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