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: rewrite tuple expr recovery to allow better items recovery #15227

Merged
merged 1 commit into from
May 23, 2023

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented May 16, 2023

Rewrites tuple expression recovery in a way that allows having better recovery for tuple item expressions. It also changes recovery for = binary expressions, which allows a significantly better recovery for named arguments analysis (and improved Parameter Info feature quality), if the type checker is also updated later.

Due to how precedences between tuple and binary expressions are aligned in F# (and how there's no separate parser rules for things like argument lists), it's difficult to have great recovery for both at the same time. This PR worsens the tuple recovery a bit, by removing rules for missing first item in the most cases:

let _ = , a

but adds a special rule to still allow that recovery inside parens, e.g. in a method call:

obj.Method(, b)
obj.Method(, b, c)

This rule implementation is more ad-hoc than other rules, but it may be a good compromise in this particular situation.
Recovery for missing items between and after other items is unchanged and is still good.

At the same time it adds recovery for previously failed cases like this:

M(a = 1, b = )
M(a =  , b = 2)

The balance seems to be good, since it's probably more common to write an additional named arg in an existing argument list, than to prepend an expression with a comma and then with another expression.

This PR also makes tuple expr recovery tests use the newer tree dumps checks instead of manually checking parts of the tree.

If this PR works out well, I'll likely try to expand the change into tuple patterns and other binary expressions, as it currently covers only = expressions and tuple expressions to make it more manageable.

@auduchinok auduchinok requested a review from a team as a code owner May 16, 2023 15:08
@auduchinok auduchinok changed the title Parser: rewrite tuple expr recover to allow named args recovery Parser: rewrite tuple expr recovery to allow better items recovery May 16, 2023
@auduchinok
Copy link
Member Author

Yay, all is green! I wasn't expecting that 😅

@T-Gro
Copy link
Member

T-Gro commented May 17, 2023

What do we now get in the "regressed" scenario? I did not see it in the tests (or missed it)

let _ = , a

@auduchinok
Copy link
Member Author

auduchinok commented May 17, 2023

What do we now get in the "regressed" scenario? I did not see it in the tests (or missed it)

It's covered here:
https://github.com/dotnet/fsharp/pull/15227/files#diff-defca677f09ea14864047eefb5081844627ef68e3e4216daefb17079197fadb5

Not a good output, but I consider it to be a much less critical case than the ones that got working now, like these:

M(a = 1, b = , c = 3)
M(a =, b = 2)

We can probably add more ad-hoc rules in some contexts like it was done for parens in M(, b, c) in future, if we find other places where this regression may worsen the actual usage during editing.

The removed rule was also made by me, and now I think it wasn't the best way to do it, despite fixing the parsing. Why not? Because its precedence is too high, and it would try to parse things as tuples too aggressively when seeing , in some cases where it could be better to have recovery in the outer nodes that have lower precedence. If there's another way to handle it, I'd be happy to know about it.

The problem with the outer nodes recovery was that in this case:

a =, b = 2 // from M(a =, b = 2)

it would be parsed as

binaryApp
  a
  =
  tuple
    error
    ,
    binaryApp
      b
      =
      2

instead of more expected

tuple
  binaryApp
    a
    =
    error
  ,
  binaryApp
    b
    =
    2

This is the main kind of cases that this PR fixes.

@T-Gro
Copy link
Member

T-Gro commented May 18, 2023

That makes sense, thanks.
Approved now.

@auduchinok
Copy link
Member Author

This is ready.

@vzarytovskii vzarytovskii merged commit 024b98d into dotnet:main May 23, 2023
@auduchinok auduchinok deleted the parser-namedArgs branch May 23, 2023 14:27
vzarytovskii added a commit that referenced this pull request May 31, 2023
* LexFilter: cleanup whitespaces (#15250)

* Parser: rewrite tuple expr recovery to allow better items recovery (#15227)

* Checker: recover on unknown record fields (#15214)

* Make anycpu work correctly on Arm64 (#15234)

* Makeanuycpu work correctly on arm64

* Update Microsoft.FSharp.Targets

* Fix15254 (#15257)

* Deploy only compressed metadata for dotnet sdk implementation (#15230)

* compress fsharp for sdk

* Update FSharp.DependencyManager.Nuget.fsproj

* Parser: more binary expressions recovery (#15255)

* Use background CancellableTask in VS instead of async & asyncMaybe (#15187)

* wip

* iteration

* iteration: quickinfo, help context

* fantomas

* todo

* moved tasks to editor project, fixed comment colouring bug

* fantomas

* Fantomas + PR feedback

* Update vsintegration/src/FSharp.Editor/Hints/HintService.fs

Co-authored-by: Andrii Chebukin <xperiandri@live.ru>

* Revert "Update vsintegration/src/FSharp.Editor/Hints/HintService.fs"

This reverts commit bf51b31.

---------

Co-authored-by: Andrii Chebukin <xperiandri@live.ru>

* Name resolution: actually add reported item when trying to replace (#14772)

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>

* Move flatErrors tests from fsharpqa (#15251)

* temp

* tests

* flaterrors

* update tests

* preserve ranges in result of UnsolvedTyparsOfModuleDef to help with warnings (#15243)

* preserve ranges in result of UnsolvedTyparsOfModuleDef to help with warnings

* use fallback range only for range0

* pattern match instead of Option.isSome

* Add test

* Revert "Add test"

This reverts commit e05e808.

* Make `FSharpReferencedProject` representation public (#15266)

* Make FSharpReferencedProject representation public

* Update surface area

* Fantomas

* Fantomas

---------

Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>

* Fix navigation for external enums, DUs and name resultion for members (#15270)

* Update FSharp.Compiler.Service.SurfaceArea.netstandard20.debug.bsl

* Add warning when compiler selects among multiple record type candidates, fslang-suggestion 1091 (#15256)

* Protect assembly exploration for C# extension members (#15271)

* Compute ValInline.Never for externs (#15274)

* Compute ValInline.Never for externs

---------

Co-authored-by: Eugene Auduchinok <eugene.auduchinok@jetbrains.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
Co-authored-by: Andrii Chebukin <xperiandri@live.ru>
Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com>
Co-authored-by: dawe <dawedawe@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants