-
Notifications
You must be signed in to change notification settings - Fork 78
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
Comments
As these are functions and not methods, we can change the reflection info anytime. |
Arginfo fixed by 195036c. |
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());
Any chance to keep whitespaces somehow? |
In a word: No. |
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. |
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 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 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 |
This only parses the php code. It doesn't evaluate it, so no closure should/would be created in the resulting 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.
|
Also, the suggestions/questions here are things that can go in a minor version, so they should be different issues. |
I mean AST, text, source code. LIke it works for
It doesn't help to connect instance of Closure object with one of those nodes, no? Closure dumping is very common task. |
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.
There's no plans to do this and it's not something I consider in scope. Additionally, files can be // line 2
$x = null; $c1 = fn() => $x; $x = 2; $c2 = fn() => $x; |
@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. |
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.
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? |
I agree, such offset value is very desirable. And |
@TysonAndre We ended up reverting the location support, because it had some performance impact. |
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.
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. |
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11). Related to nikic#114
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11). Related to nikic#114
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11). Related to nikic#114
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11). Related to nikic#114
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11). Related to nikic#114
AST version 70 has been supported since php-ast 1.0.1 (2019-02-11). Related to nikic#114
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)
Remove legacy flags such as
ast\flags\RETURNS_REF
(alias ofast\flags\FUNC_RETURNS_REF
mentioned in README)The text was updated successfully, but these errors were encountered: