Skip to content

Parser: add recovery for unfinished match clauses #10872

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Aug 30, 2021
Merged

Parser: add recovery for unfinished match clauses #10872

merged 16 commits into from
Aug 30, 2021

Conversation

@auduchinok
Copy link
Member

@auduchinok auduchinok commented Jan 14, 2021 •
edited
Loading

The following examples will now produce match clauses (previously it'd fail and skip them):

match () with
| x
match () with
| x when e
| otherClause -> ()

The first case recovery and diagnostic improvements were done by @dsyme during our parser recovery session.

@kerams kerams mentioned this pull request Apr 22, 2021
Closed
@smoothdeveloper smoothdeveloper mentioned this pull request Apr 24, 2021
Open
15 tasks
@KevinRansom
Copy link
Contributor

@auduchinok , perhaps you could resolve the conflicts, and could we also have some kind of test.

Kevin

@auduchinok
Diagnostics: add production rule index
985b85c
@auduchinok
Parser: fix recovery for unfinished match clause
7bfec2c 
Fixes recovery for missing right hand sides:
```
match () with
| x
```
@auduchinok auduchinok force-pushed the unfinishedMatchClause branch from 110377c to 7bfec2c Compare August 23, 2021 13:45
@auduchinok auduchinok force-pushed the unfinishedMatchClause branch from 1a9f12e to fcef94f Compare August 27, 2021 13:07
@auduchinok auduchinok force-pushed the unfinishedMatchClause branch from 12fe68d to fcb54ab Compare August 27, 2021 14:10
@auduchinok auduchinok force-pushed the unfinishedMatchClause branch from 653b462 to 2b82f1b Compare August 30, 2021 11:11
@auduchinok
Copy link
Member Author

auduchinok commented Aug 30, 2021 •
edited
Loading

I'm not able to reproduce the desired behaviour in the remaining test, VariableIdentifier.AsParameter: I do get the completion items in both main and this branch when running a debug IDE instance, and it fails to get the items when running the test.

I am also unable to debug the test in neither Rider nor Visual Studio 2019, since there're some different issues when trying to build Salsa test project or its dependencies in both IDEs on my machine.

dsyme
dsyme approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments

src/fsharp/pars.fsy
| atomicPatterns
{ SynArgPats.Pats $1 }
{ let mWhole = rhs parseState 1
let m = (mWhole.StartRange, $1) ||> unionRangeWithListBy (fun p -> p.Range)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe call this mAll

tests/service/ParserTests.fs
[<Test>]
let ``Match clause 03 - When`` () =
let parseResults = getParseResults """
match () with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for this, assuming it works?

match () with
| x 
| _ -> ()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see if this recovers:

match () with
| x ->
| _ -> ()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added both cases, both are green. 🙂

tests/service/Common.fs Outdated

let getSingleExprInModule (input: ParsedInput) =
match getSingleModuleLikeDecl input with
| SynModuleOrNamespace (decls = decls) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would normally use

let (SynModuleOrNamespace (decls = decls)) = getSingleModuleLikeDecl input

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call getSingleModuleMemberDecls here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call getSingleModuleMemberDecls here?

I think that's what I originally had wanted and didn't finish while were debugging the tests. Thanks, I'll update it.

@dsyme
Copy link
Contributor

dsyme commented Aug 30, 2021

I'm not able to reproduce the desired behaviour in the remaining test, VariableIdentifier.AsParameter: I do get the completion items in both main and this branch when running a debug IDE instance, and it fails to get the items when running the test.

Given the validation steps you did then we could consider disabling the test. I'll try to reproduce.

@auduchinok
Copy link
Member Author

Yay, all is green, sans the disabled test.

@dsyme
Copy link
Contributor

dsyme commented Aug 30, 2021 •
edited
Loading

Locally I just got this on this branch

C:\GitHub\dsyme\fsharp>.\build -c Release -testFSharpQA

0 tests failed (0.00%)

So it is confusing. The test does however relate to the area that you're changing. Hmmm...

dsyme
dsyme reviewed Aug 30, 2021
View reviewed changes
vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.Completion.fs
list = ["PrivateMethod"])

[<Test>]
// [<Test>]
Copy link
Contributor

@dsyme dsyme Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to use Disabled or Skip or whatever this test suite uses

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should've tried to use some attribute indeed. I looked at the code around and used the same approach.

@dsyme
Copy link
Contributor

dsyme commented Aug 30, 2021

I will merge this, partly because the error recovery implemented is far more useful that the very corner-case test (which may not be a true failure since we can't repro it locally)

@dsyme dsyme merged commit 38f966d into dotnet:main Aug 30, 2021
@auduchinok
Copy link
Member Author

auduchinok commented Aug 30, 2021 •
edited
Loading

Locally I just got this on this branch

It was the VS test suite, not the FSharpQA one. Could you try that one too?

@auduchinok auduchinok deleted the unfinishedMatchClause branch August 31, 2021 08:18
@dsyme dsyme added the Theme-Simple-F# A cross-community initiative called "Simple F#", keeping people in the sweet spot of the language. label Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsyme dsyme dsyme approved these changes

Assignees

@vzarytovskii vzarytovskii

Labels

Theme-Simple-F# A cross-community initiative called "Simple F#", keeping people in the sweet spot of the language.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@auduchinok @KevinRansom @dsyme @vzarytovskii