prettier / plugin-php

Prettier PHP Plugin
https://loilo.github.io/prettier-php-playground/
MIT License
1.74k stars 128 forks source link

Migrate on own parser #1053

Open alexander-akait opened 5 years ago

alexander-akait commented 5 years ago

/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.

loilo commented 5 years ago

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

loilo commented 5 years ago

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.

alexander-akait commented 5 years ago

@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

karptonite commented 5 years ago

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.

alexander-akait commented 5 years ago

@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.

karptonite commented 5 years ago

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.

alexander-akait commented 5 years ago

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:

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

alexander-akait commented 5 years ago

I can try to find time on PoC

czosel commented 5 years ago

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

alexander-akait commented 5 years ago

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?

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.

czosel commented 5 years ago

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

ichiriac commented 5 years ago

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 ?

alexander-akait commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

czosel commented 5 years ago

@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:

alexander-akait commented 5 years ago

@czosel i think we should:

czosel commented 5 years ago

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

ichiriac commented 5 years ago

@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

alexander-akait commented 5 years ago

@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 commented 5 years ago

/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.

czosel commented 5 years ago

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:

alexander-akait commented 5 years ago

@czosel hm, here there solution:

czosel commented 5 years ago

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.

alexander-akait commented 5 years ago

/cc @ichiriac what about branch for prettier?

ichiriac commented 5 years ago

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)

alexander-akait commented 5 years ago

@ichiriac Thanks!

jpickwell commented 4 years ago

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

Same developer as PHP-Parser.

flexchar commented 4 years ago

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. :)

alexander-akait commented 4 years ago

@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:

czosel commented 4 years ago

Thanks for the kind words @flexchar, I’m happy to hear the plugin is working well for you! I totally agree with @evilebottnawi though, what the project currently needs most is active contributors. My knowledge on the parsing side is also limited, but I’m happy to help with onboarding in the printing side.

joemaller commented 4 years ago

@czosel & @evilebottnawi I also want to thank you for your work on this, the project is a huge help. I would really like to try and contribute, but having just spent a few hours attempting to get things working, I ended up stuck on setup, tooling and workflow.

I know this is a huge ask, but any additional documentation towards getting a basic development environment working would be really welcome. I've read over the Contributing doc (and Prettier's) but some things don't make sense or seem to point to missing files (the docs REPL?).

A few places I got stuck

czosel commented 4 years ago

Hi @joemaller

I'll answer the questions right here - afterwards we can think about what can be added to the documentation.

Update: See #1267

czosel commented 4 years ago

@joemaller The docs should be a little better now - let me know if you see further improvements!