plugin-php icon indicating copy to clipboard operation
plugin-php copied to clipboard

Migrate on own parser

Open alexander-akait opened this issue 5 years ago • 34 comments

/cc @czosel

Current parser does not improve and does not fix bugs, I propose to go to your own parser, we can use https://github.com/nikic/PHP-Parser/tree/master/grammar as example. I know it is require time, but unfortunately it seems we have no other way.

alexander-akait avatar May 15 '19 15:05 alexander-akait

Any chance we could viably compile the original PHP parser to WebAssembly?

loilo avatar May 15 '19 18:05 loilo

PHP as a whole has already been compiled to WASM, but I have no insight into that field and can't tell whether compiling parts (i.e. just the parser) would be feasible.

loilo avatar May 15 '19 21:05 loilo

@loilo i am afraid what original php parsers is not enough for prettier, a lot of information about nodes is missing because php no need this

alexander-akait avatar May 16 '19 09:05 alexander-akait

Any possibility the plugin could use the https://github.com/nikic/PHP-Parser directly, or would this have the same issue with missing nodes? Obviously that means running php processes as part of prettier PHP (no necessarily simple), but it would have the benefit that it would be easier to stay up to date with future language changes.

karptonite avatar May 16 '19 14:05 karptonite

@karptonite same problem not enough node information :disappointed: I think I can implement parser using bison/yacc (https://github.com/nikic/PHP-Parser using modified version of yacc, but it is not a problem) for two weeks, but i don't have time right now, if somebody want to help me will be great.

alexander-akait avatar May 16 '19 14:05 alexander-akait

Perhaps the PHP version is extensible enough to allow it to be extended to get the additional node information? I'm looking at this issue as an example of getting information that doesn't normally get stored by the parser: https://github.com/nikic/PHP-Parser/issues/384 It also looks like there are some lexer options that might give you more information: https://github.com/nikic/PHP-Parser/blob/1.x/doc/component/Lexer.markdown#lexer-options I don't know much about how these things work, though, so I'm sort of guessing here.

karptonite avatar May 16 '19 17:05 karptonite

To be honestly for us interesting only https://github.com/nikic/PHP-Parser/blob/master/grammar/php5.y and https://github.com/nikic/PHP-Parser/blob/master/grammar/php7.y and https://github.com/nikic/PHP-Parser/blob/master/grammar/tokens.y

What we should do:

  • Install bison/yacc
  • Create js version https://github.com/nikic/PHP-Parser/blob/master/grammar/parser.template
  • Rewrite this on js https://github.com/nikic/PHP-Parser/blob/master/grammar/rebuildParsers.php
  • Implement each Node (example https://github.com/nikic/PHP-Parser/blob/master/grammar/php7.y#L39) on js, given the names and structure of the current nodes what we use in code, most likely we will have even less of them
  • Copy https://github.com/nikic/PHP-Parser/blob/master/grammar/php5.y , https://github.com/nikic/PHP-Parser/blob/master/grammar/php7.y and https://github.com/nikic/PHP-Parser/blob/master/grammar/tokens.y

It is not hard, node(s) is very simple

alexander-akait avatar May 16 '19 17:05 alexander-akait

I can try to find time on PoC

alexander-akait avatar May 16 '19 17:05 alexander-akait

Hi @evilebottnawi, first of all sorry about the delay, I've been swamped with work lately. This sounds like a big step, and before jumping right into it, I'm wondering

  • Which are the show-stopping issues in the parser we're currently using right now?
  • Can those issues not be fixed in the current parser?
  • If we decide to implement a new parser, what would be fundamentally different in it's design that would solve the issues we're having now?

czosel avatar May 26 '19 20:05 czosel

Which are the show-stopping issues in the parser we're currently using right now?

  • maximum supporting php version is 7.2 (7.3 features doesn't support)
  • invalid ast in some cases (parens and *lookup nodes)
  • bad ast for inline nodes
  • invalid position for comments in some cases

Can those issues not be fixed in the current parser?

I think yes, but nobody do this. Also bison/yacc parser better maintenance.

If we decide to implement a new parser, what would be fundamentally different in it's design that would solve the issues we're having now?

BFN (EBFN) grammar and generate parser based on this rules. You can see https://github.com/nikic/PHP-Parser/tree/master/grammar here how it can be done.

alexander-akait avatar May 27 '19 10:05 alexander-akait

Alright, thanks for sharing the details - I'll try to take a closer look when I find the time :slightly_smiling_face:

czosel avatar May 27 '19 13:05 czosel

Hi @evilebottnawi and @czosel, interresting approach. In the past I've used https://www.npmjs.com/package/jison but at that time the LR(1) generated code was not good enough to parse some syntax collisions on PHP, and by resolving them is was too permissive and also too slow compared to now, but agree that the maintenance of the syntax may be easier on grammar files than code.

Did you succeed with a POC or prototype ?

ichiriac avatar Aug 09 '19 14:08 ichiriac

@ichiriac not, is is just idea, it it no high priority, the problem is that we get fixes/features extremely slowly and very few guys have experience with parsers, supporting parser generator based on bfn/ebfn will be easy

alexander-akait avatar Aug 09 '19 14:08 alexander-akait

@czosel @loilo i can't continue update php parser due https://github.com/glayzzle/php-parser/issues/359 and https://github.com/glayzzle/php-parser/issues/358, it is big regressions for us, i think we need fork parser and revert this and continue development

alexander-akait avatar Aug 27 '19 10:08 alexander-akait

I do not have a lot of time, but in my plan to finish the stable version this month

alexander-akait avatar Aug 27 '19 10:08 alexander-akait

@evilebottnawi I think it's fine to continue development on your fork for the moment, most likely @ichiriac will revert the change soon. Would be awesome if we can make a new release soon! :tada:

czosel avatar Aug 27 '19 16:08 czosel

@czosel i think we should:

  • full support php7.3
  • implement nodes for <?php and ?> (need discussion) for better format inline content
  • some fixes (some already done)

alexander-akait avatar Aug 27 '19 16:08 alexander-akait

Sounds good! To me, especially better inline support seems critical - unfortunately, that’s probably also the hardest part.

czosel avatar Aug 27 '19 17:08 czosel

@czosel - better inline support is related to https://github.com/glayzzle/php-parser/issues/170 @evilebottnawi - I'll reverse bad commits, and the php 7.3 https://github.com/glayzzle/php-parser/issues/345 & php 7.4 https://github.com/glayzzle/php-parser/issues/346 are planned on next release


I think the actual approach of php-parser is clearly not maintenable - I have some nights to pass on this, so I'm ok with the idea to migrate the parser on the nikic implementation, initially I was concerned by the speed, but now with hindsight I prefer maintainability - https://github.com/glayzzle/php-parser/issues/372

ichiriac avatar Aug 27 '19 21:08 ichiriac

@ichiriac maybe you can allow to me merge without your review (bug fixes), we do great work and our parser not a bad, i just need review when implement new feature

alexander-akait avatar Aug 28 '19 09:08 alexander-akait

/cc @czosel I have a lot of PRs with fixes and features https://github.com/glayzzle/php-parser/pulls, I can finish support php 7.3 and php 7.4 and inline nodes by the end of the month if my PR someone will merge (will be great if review also). But the developer is extremely inactive, and I can’t finish it in the right time because of this.

alexander-akait avatar Sep 11 '19 21:09 alexander-akait

I'd also be ok with using your fork of the parser until your PRs get merged. Unfortunately, I neither have the time nor the know-how to do reviews of your work on the parser :disappointed:

czosel avatar Sep 11 '19 21:09 czosel

@czosel hm, here there solution:

  • do fork
  • move parser to this repo (it is allow fast fixing, but increase count of PRs)
  • Alternative: create branch prettier in php-parser and allow me to merge commits in this branch and use this branch, when @ichiriac will have time he can merge commit from our branch to master

alexander-akait avatar Sep 11 '19 21:09 alexander-akait

I’d suggest either option 1 or 3. I think keeping the parser separate is important both for separation of concerns and for being able to easily merge your changes back in the upstream parser.

czosel avatar Sep 12 '19 05:09 czosel

/cc @ichiriac what about branch for prettier?

alexander-akait avatar Sep 12 '19 10:09 alexander-akait

Hi @evilebottnawi ,

You are registered as collaborator (and also @czosel) from a long time ago, and I already gave you since then the write access level on the repository.

In principle you can merge any PR by your own, even if I prefer to check them when it comes to major changes.

Lets go with the prettier branch option, here your branch : https://github.com/glayzzle/php-parser/tree/prettier (BTW you can also create as many branches you need)

  • You can make PR & merge them on it, I've started to change pending PR on it
  • You also may retrieve PR from master on it in order to keep it sync
  • Once you are ready to release, make a PR on master

ichiriac avatar Sep 17 '19 14:09 ichiriac

@ichiriac Thanks!

alexander-akait avatar Sep 23 '19 10:09 alexander-akait

This may also be of some use: https://github.com/nikic/php-ast

Same developer as PHP-Parser.

jpickwell avatar Dec 24 '19 16:12 jpickwell

Somewhat off topic but this plugin has saved me a long way keeping my code base tidy. Absolutely ♥ it!

I understand, however, that PHP plugin is a volunteer project and you can only put so much work. I'd love to help! I don't understand parsers but how about financially wise? Perhaps fellow developers like me would be interested and that would also help you.

I use it consistently with PHP 7.3 (f.ex. Laravel) projects. It works flawlessly 99% of time. However, since PHP 7.4 is standing outside and knocking the door - the tense rises. :)

flexchar avatar Dec 29 '19 19:12 flexchar

@flexchar thanks for the desire to help, but we need help in the code and not in the money, we need to implement all the constructs from php7.4 and some from php7.3, here parser https://github.com/glayzzle/php-parser/pull/407, it is not hard and not easy :smile:

alexander-akait avatar Dec 30 '19 11:12 alexander-akait