[go: up one dir, main page]

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

Ideas for things to change/remove in the next major version (2.0) #114

Open
TysonAndre opened this issue Feb 2, 2019 · 15 comments
Open

Comments

@TysonAndre
Copy link
Collaborator
TysonAndre commented Feb 2, 2019
  1. Remove AST_LIST and mark it as deprecated, and remove it whenever AST version 50 is removed.

Related to #94 - It was overlooked

AST_LIST is still used in https://github.com/phan/phan/blob/1.2.2/src/Phan/Analysis/PreOrderAnalysisVisitor.php#L613 (incorrectly, will fix)

  1. Change the reflection arginfo of ast\parse_code and ast\parse_file to make the int $version mandatory

Remove legacy flags such as ast\flags\RETURNS_REF (alias of ast\flags\FUNC_RETURNS_REF mentioned in README)

@TysonAndre TysonAndre changed the title Remove AST_LIST when next major version is released (2.0) Ideas for things to change/remove in the next major version (2.0) Feb 9, 2019
@nikic
Copy link
Owner
nikic commented Feb 11, 2019

As these are functions and not methods, we can change the reflection info anytime.

@nikic
Copy link
Owner
nikic commented Feb 11, 2019

Arginfo fixed by 195036c.

@unkind
Copy link
unkind commented Jul 31, 2020

This version does not support whitespaces? it's impossible (bytecode only) or due to some other reasons?

All (?) modern code fixers work with tokenizers, but it's painful. Code fixer based on AST or higher level abstraction model looks way much better:

$this->summary('Make classes `final` by default.')
        ->apply(fn (ClassNode $c) => $c->abstract ?: $c->finalize());

PhpParser is kinda slow for these sort of tasks.

Any chance to keep whitespaces somehow?

@nikic
Copy link
Owner
nikic commented Jul 31, 2020

In a word: No.

@nikic
Copy link
Owner
nikic commented Jul 31, 2020

In a few more words: This extension only exposes what PHP provides. As PHP has no need for whitespace itself, the information is not preserved (and I expect it will never be).

For this purpose you need to use either PHP-Parser or tolerant-php-parser, both of which preserve full formatting information, though in different ways.

@unkind
Copy link
unkind commented Jul 31, 2020

Okay. I have another more or less related question for this topic. It's possible to dump in bytecode id of each closure definition? Just some bytes based on current filename + parser position, for example. And provide a relatively fast way to get it in runtime (directly from Closure object).

Closure identification needs for systems where there are a lot of closures with dynamic parameter list, and it requires to cache each signature, but there are no fast ways to identify cached closure (definition) besides deprecated ReflectionFunction::export().

Also it would solve another known issue when it's impossible to dump closure AST if there are 2 or more on the same line.

Or... just save AST of closure like it works for assert?

@TysonAndre
Copy link
Collaborator Author

Okay. I have another more or less related question for this topic. It's possible to dump in bytecode id of each closure definition? Just some bytes based on current filename + parser position, for example. And provide a relatively fast way to get it in runtime (directly from Closure object).

This only parses the php code. It doesn't evaluate it, so no closure should/would be created in the resulting ast\Node.

php-ast already provides declId if you need to uniquely identify a closure's position with a file, and it'll be the same number every time parse_code is run, but that seems unrelated to what you want.

  • That uniquely identifies two closures on the same line

@TysonAndre
Copy link
Collaborator Author

Also, the suggestions/questions here are things that can go in a minor version, so they should be different issues.

@unkind
Copy link
unkind commented Jul 31, 2020

no closure

I mean AST, text, source code. LIke it works for assert() (when it fails, it shows first argument in exception description).

That uniquely identifies two closures on the same line

It doesn't help to connect instance of Closure object with one of those nodes, no? Closure dumping is very common task.

@TysonAndre
Copy link
Collaborator Author

There's https://github.com/tpunt/php-ast-reverter , but it hasn't been updated in 3 years with new node kinds and I don't know the status of that project. You could try to create PRs to implement conversion of AST nodes to readable strings.

assert works on the raw C representation, the converted php representation returned by php would require more error handling, but looking at the C implementation

It doesn't help to connect instance of Closure object with one of those nodes, no? Closure dumping is very common task.

There's no plans to do this and it's not something I consider in scope. Additionally, files can be require()d multiple times (even after modifying those closures) and those closures would be different. E.g. these would have the same representation and same line

// line 2
$x = null; $c1 = fn() => $x; $x = 2; $c2 = fn() => $x;

@unkind
Copy link
unkind commented Jul 31, 2020

information is not preserved

@nikic btw, PHP preserves line numbers, probably it's possible to preserve offset (absolute or for this current line). In this case, it's possible to restore all whitespaces between any 2 nodes.

@TysonAndre
Copy link
Collaborator Author
TysonAndre commented Jul 31, 2020

btw, PHP preserves line numbers, probably it's possible to preserve offset (absolute or for this current line). In this case, it's possible to restore all whitespaces between any 2 nodes.

That's actually something I've wanted for a while. However, that would require changing php-src's lexer, which does not provide the column in the C ast node in older php versions (possibly the latest could be updated to add it, in 8.1). Changing the internals of php-src's lexer is something that may slightly affect performance of large php applications overall, so I was uncertain of whether other people who work on php-src would have interest in it.

  • e.g. this could be used to add getColumn() to reflection. I think

Mentioned before in #58 (comment)

@nikic - https://bugs.php.net/bug.php?id=70024 mentioned that you needed to update bison to support this - Does your merged PR php/php-src#3948 move us closer to accurate column numbers?

@unkind
Copy link
unkind commented Jul 31, 2020

I agree, such offset value is very desirable. And php-ast is really need to be built-in part of PHP 8.

@nikic
Copy link
Owner
nikic commented Jul 31, 2020

@TysonAndre We ended up reverting the location support, because it had some performance impact.

@TysonAndre
Copy link
Collaborator Author

I agree, such offset value is very desirable. And php-ast is really need to be built-in part of PHP 8.

For the reasons mentioned in #5 (comment) , it might be harder but not impossible to write tooling based on php-ast if php-ast was moved into core.

We ended up reverting the location support, because it had some performance impact.

That answers my question. What about the work being done on emitting improved syntax errors? Can that determine the column without performance impact (e.g. unexpected ')' on column X in php --syntax-check?

TysonAndre added a commit to TysonAndre/php-ast that referenced this issue Sep 13, 2020
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11).

Related to nikic#114
TysonAndre added a commit to TysonAndre/php-ast that referenced this issue Sep 13, 2020
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11).

Related to nikic#114
TysonAndre added a commit to TysonAndre/php-ast that referenced this issue Sep 13, 2020
TysonAndre added a commit to TysonAndre/php-ast that referenced this issue Oct 9, 2020
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11).

Related to nikic#114
TysonAndre added a commit to TysonAndre/php-ast that referenced this issue Nov 27, 2021
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11).

Related to nikic#114
TysonAndre added a commit to TysonAndre/php-ast that referenced this issue Nov 27, 2021
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11).

Related to nikic#114
TysonAndre added a commit to TysonAndre/php-ast that referenced this issue Aug 7, 2022
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11).

Related to nikic#114
TysonAndre added a commit that referenced this issue Aug 7, 2022
…php < 7.2 (#185)

* Deprecate AST versions 50 and 60

AST version 70 has been supported since php-ast 1.0.1 (2019-02-11).

Related to #114

* Drop support for php <= 7.1

Update documentation to mention this.

* Update test expectation for AST version 60 deprecation
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

No branches or pull requests

3 participants