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

Parser: more unfinished type recovery #15585

Merged
merged 5 commits into from
Jul 18, 2023
Merged

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Jul 11, 2023

Continues #15410 and adds recovery for a missed case:

type T() =

@auduchinok auduchinok requested a review from a team as a code owner July 11, 2023 20:40
@auduchinok
Copy link
Member Author

Please don't merge it yet, I'll continue some work here tomorrow.

@T-Gro T-Gro marked this pull request as draft July 12, 2023 11:30
@auduchinok
Copy link
Member Author

auduchinok commented Jul 12, 2023

How much do we care about parser recovery for missing = in the following case? It doesn't seem like a popular mistake people make to me.

type MissingEquals A of int

There was a special case added for it some time ago, and it makes it very tricky to have any progress here. I've spent many hours today, trying to workaround it, but for now my conclusion is it seems it doesn't work well with precedence of typ rule that is used inside type abbreviation declarations:

type Abbr = int

Changing the precedence of that rule seems too dangerous, because typ is used everywhere in the grammar. Besides that, there may be other affecting precedences specified somewhere in the grammar, that we'd uncover later.

If the special case for missing = is removed, the parser starts to produce the usual syntax error about the unexpected token instead of =. That seems sufficient for the Visual Studio quick fix. The downside is subsequent nodes aren't parsed properly. The good thing is that the much more common unfinished type T() = is parsed properly and subsequent code is not broken during typing.

I'll look into it once more tomorrow.

@smoothdeveloper
Copy link
Contributor

It doesn't seem like a popular mistake people make to me.

For DU & enums, it is not frequent, but when switching from other languages to F#, I sometimes miss it on records and classes.

But since the error message is giving what the end user needs:

Unexpected token in type definition. Expected '=' after the type 'A'.

I feel that the recovery for this particular case doesn't make it a big issue, especially if it comes with a code fix.

I'm wondering also, if FCS wouldn't benefit from a using stale type checking results, whenever the syntax tree comes broken, if there was a way to split the "dirty" segment and recompose the parts?

@auduchinok
Copy link
Member Author

For DU & enums, it is not frequent, but when switching from other languages to F#, I sometimes miss it on records and classes.

My guess is they would also type { in, and I doubt that the parser is ready for that. Or is there some popular syntax where neither = nor { is required?

@T-Gro
Copy link
Member

T-Gro commented Jul 13, 2023

I think it is a good tradeoff to bring in your change as it is.
Unfinished typing is a daily scenario for every user.

@auduchinok auduchinok force-pushed the parser-type2 branch 3 times, most recently from f883bc0 to ac36b8c Compare July 14, 2023 12:55
@auduchinok
Copy link
Member Author

I think it is a good tradeoff to bring in your change as it is.

Thanks for your reply! Yes, I've dropped that rule, and it simplified things considerably. It has also allowed improving errors and ranges in other places.

@auduchinok auduchinok marked this pull request as ready for review July 14, 2023 13:23
@auduchinok
Copy link
Member Author

This is ready. @psfinaki Thanks a lot for helping with the change!

src/Compiler/pars.fsy Show resolved Hide resolved
@psfinaki psfinaki linked an issue Jul 18, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Error number 3360 is reused for two errors
4 participants