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

Allow extending the Node class #223

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

matt-allan
Copy link

@matt-allan matt-allan commented Aug 3, 2022

This PR adds the ability to extend the Node class and parse code into your own classes. The idea is to offer a similar API to the tokenizer extension:

<?php

class MyNode extends \ast\Node
{
    public bool $myFlag = false;
}

$ast = MyNode::parseCode('<?php echo "hello, world!";', 90);

$ast->myFlag = true;

var_dump($ast);
Example output
object(MyNode)#1 (5) {
  ["kind"]=>
  int(132)
  ["flags"]=>
  int(0)
  ["lineno"]=>
  int(1)
  ["children"]=>
  array(1) {
    [0]=>
    object(MyNode)#2 (5) {
      ["kind"]=>
      int(283)
      ["flags"]=>
      int(0)
      ["lineno"]=>
      int(1)
      ["children"]=>
      array(1) {
        ["expr"]=>
        string(13) "hello, world!"
      }
      ["myFlag"]=>
      bool(false)
    }
  }
  ["myFlag"]=>
  bool(true)
}

This solves a similar problem as #217 but in a different way. Compared to dynamic properties this approach lets you add methods, constants, interfaces, etc. as well and everything can be statically analyzed properly.

This is my first major contribution to a PHP extension so please let me know if anything looks off! I based the changes on the tokenizer extension's source code.

- Declare ast\Node::parseCode method
- Pass through class for subclassing
- Update stubs
return;
}

node_class = zend_get_called_scope(execute_data);
Copy link
Author

@matt-allan matt-allan Aug 3, 2022

Choose a reason for hiding this comment

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

@matt-allan matt-allan marked this pull request as draft August 4, 2022 19:52
@matt-allan
Copy link
Author

I tried using legacy arginfo to prevent the static return type from causing an error but that appears to be breaking a 'constructor throws' test. Not sure what the correct solution is here. I will try building on PHP 7.0 locally so I can see what the issue is. Seems to work fine on PHP 7.4 for me locally.

@nikic
Copy link
Owner

nikic commented Aug 6, 2022

@TysonAndre Thoughts on this functionality? The idea looks reasonable to me.

Copy link
Collaborator

@TysonAndre TysonAndre left a comment

Choose a reason for hiding this comment

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

I tried using legacy arginfo to prevent the static return type from causing an error but that appears to be breaking a 'constructor throws' test. Not sure what the correct solution is here. I will try building on PHP 7.0 locally so I can see what the issue is. Seems to work fine on PHP 7.4 for me locally.

The ast_legacy_arginfo.h is using ZEND_ARG_INFO with no types, so the result is a warning, and stops being the expected TypeError.

return 123;
PHP;

echo get_class(MyNode::parseCode($code, $version=50)), "\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. it'd be more useful to test properties are correctly set, e.g. with the node dumper
  2. could check that additional properties on the MyNode can be declared and that when read, they have the expected value. Same for properties inherited from traits (to have the unit test in case the property slot order ever changes and affects ast_node_set_prop_kind)

@@ -34,6 +34,17 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_ast_Node___construct, 0, 0, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, lineno, IS_LONG, 1, "null")
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_ast_Node_parseCode, 0, 2, IS_STATIC, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think one possible solution is to redefine IS_STATIC to ast\NODE in ast.c above the include - I forget if that would actually work

And continue using ast_arginfo.h instead of ast_legacy_arginfo.h

#ifndef IS_STATIC
// Workaround for `parse*` methods on Node with static return types in php < 8.0
#define IS_STATIC ast\\Node
#endif

@TysonAndre
Copy link
Collaborator

TysonAndre commented Aug 7, 2022

Thoughts on this functionality? The idea looks reasonable to me.

The change itself seems reasonable once tests pass as long as the impact on the overall performance of parsing is small.


Aside: also see #180 regarding build issues if 7.1 is an obstacle with this PR using the regular arg info
EDIT: If you rebase, builds will stop being tested with 7.1 and below

@matt-allan
Copy link
Author

Thanks for the detailed review! I will follow up soon, I've been a bit busy starting a new job.

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.

3 participants