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

Idea: Use tolerant-php-parser for error tolerance (Time consuming, requires php7) #520

Closed
TysonAndre opened this issue Feb 17, 2018 · 2 comments

Comments

@TysonAndre
Copy link
Contributor

TysonAndre commented Feb 17, 2018

I'm mentioning this because it wasn't mentioned in other issues.
This is probably a long term project, and probably couldn't be started until php 5.6 support is dropped. (ext-suggest might work in composer.json)

Benefits of tolerant-php-parser:

  • Better recovery from parse errors. If nikic/php-parser encounters an error (E.g. { without a match in a method), it gives up on parsing nodes in the rest of the file.

    Error tolerance is useful if there were plans to run psalm as a background process that could respond to analysis requests (e.g. Language Server Protocol)

  • Reduced CPU usage from parsing and error diagnostics. (I assume that serialize()/igbinary_serialize() would be smaller on disk and faster to unserialize as well, but haven't benchmarked that)

    See Adopt Microsoft/tolerant-php-parser felixfbecker/php-language-server#357

    Traversing the tree would hopefully also be faster, but I haven't benchmarked that.

Drawbacks of tolerant-php-parser

  • A few known bugs for uncommon syntax: https:/Microsoft/tolerant-php-parser/issues?q=is%3Aissue+is%3Aopen+label%3Abug

    Might fail to catch invalid syntax or incorrectly warn about rare valid syntax

  • Would break any existing plugins that used \PHPParser\Node

  • Requires php 7 to run, and I think token_get_all() may differ between php 5 and php 7 (may be revisited)

  • Missing some APIs (E.g. to convert from a complex string literal to the raw string. Probably feasible to implement)

Other notes:

A hybrid approach of converting tolerant-php-parser to php-parser (Only in cases where php-parser had at least one error) could work in theory. (https:/TysonAndre/tolerant-php-parser-to-php-ast/ is an example of converting one AST format to another)

  • Writing a project to convert from one ast to another is time consuming to implement, debug, and test
  • The only benefit is better recovery from parse errors on syntactically invalid files, which isn't important right now
@muglug muglug added this to the Server mode milestone Feb 17, 2018
@muglug
Copy link
Collaborator

muglug commented Feb 17, 2018

The only benefit is better recovery from parse errors on syntactically invalid files, which isn't important right now

Yeah, that's the key thing for me. And at the moment the parsing performance difference is negligible given how expensive analysis is.

But obviously a language server would need this, along with a couple of other Psalm features. I've created the server mode milestone to track that support.

Other thoughts

I'm currently ok with Psalm acting as a gatekeeper rather than as an assistant, and I slightly worry that LSP support might sidetrack the project. It would need to be usable at Vimeo's scale, where analysis on a single (much-too-large) file can take over a second (with all dependencies already loaded into memory).

But it would also be very cool.

@muglug muglug removed this from the Server mode milestone Feb 20, 2018
@muglug
Copy link
Collaborator

muglug commented Oct 17, 2018

I went with PHP Parser 4 instead, it improved its error tolerance substantially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants