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

Error recovery for missing return type with colon #544

Closed
muglug opened this issue Oct 25, 2018 · 8 comments
Closed

Error recovery for missing return type with colon #544

muglug opened this issue Oct 25, 2018 · 8 comments

Comments

@muglug
Copy link
Contributor

muglug commented Oct 25, 2018

Continuing adventures in just-in-time parsing - this fails to parse:

<?php
class X {
    public function foo() : 
    {
        return $a;
    }
}

I can probably work around this with a hacky regex for temporary updates FWIW

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 25, 2018

Architecture fo this parser works with only with valid code.

If you need to work with incomplete code, use https://packagist.org/packages/microsoft/tolerant-php-parser

Like https:/phpactor/phpactor does for IDE autocomplete.
Each is suitable for their own context.

What are you working on anyway?

@muglug
Copy link
Contributor Author

muglug commented Oct 25, 2018

Hey @TomasVotruba - thanks for the heads-up. I'm aware of the other parser, and have decided against using it - it still has a few bugs around handling of valid PHP syntax, and I'd rather deal with these minor parsing-of-invalid-code gripes vs parsing-of-valid-code gripes.

@nikic has also shown a great willingness to indulge those of us who are starting to use PHP Parser for making live updates to code - some recent releases have explicit callouts to improved error recovery in certain situations to allow the rest of the AST to be parsed.

@muglug
Copy link
Contributor Author

muglug commented Oct 25, 2018

Example where the parser recovers from a missing semicolon, allowing Psalm to infer the return type is wrong: https://getpsalm.org/r/bffe1b1f43,

@nikic nikic closed this as completed in 3d0f784 Oct 25, 2018
@muglug
Copy link
Contributor Author

muglug commented Oct 25, 2018

Thanks!

@nikic
Copy link
Owner

nikic commented Oct 25, 2018

I've implemented this with the returnType set to null on error. Is that fine? Expr\Error doesn't really fit here from a typesystem perspective.

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 25, 2018

@muglug I see, I haven't noticed that.

Could you share some links to psalm where this is used?

@nikic
Copy link
Owner

nikic commented Oct 25, 2018

@TomasVotruba Error recovery is a not a big focus of this project, but I try to support it where possible. It's basically just a matter of specifying in the grammar where errors are allowed to occur and what AST should be generated for them. The main larger thing that's not supported is handling of mismatched braces ({ without }). Luckily editors usually insert braces as a pair, so it's not such a big problem (at least that's what I tell myself :P)

@muglug
Copy link
Contributor Author

muglug commented Oct 25, 2018

Could you share some links to psalm where this is used?

Short answer: not yet

Longer answer: The latest releases for Psalm allow you to run a language server to show diagnostics on open and save.

A lot of similar language servers support onChange events too, and I'm working on adding that support to Psalm in this branch.

Part of that effort involves incremental scanning - that is, scanning only the part of the AST that the user has changed, and readjusting file offsets in the resultant AST to match the expected one.

If that partial parsing fails, a hacky fix is applied and parsing re-attempted, and if that fails the whole AST is re-scanned. Whole-file parsing can be expensive for large files, especially as that same failure will occur, resulting in a null AST for the entire file.

Compounding that, while in Language Server mode I also perform AST diffing to ensure that no method gets re-analysed as long as none of its dependents have changed (analysis is much more costly than parsing). Without the above fix that AST diffing would mark every node as having changed, forcing them all to be re-analysed. That's not a problem in a small file, but can be annoying when editing a 5-thousand line file as it means that the illusion of checking-as-you-type is lost.

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