Remove LetOrUseKeyword from SynExprLetOrUseTrivia

[nojaf]

Description

As mentioned on #18825 (comment)
It would be most convenient if a SynBinding holds all the information to print itself on its own.

By extending SynLeadingKeyword we can do exactly that.

• Test cases added

• Performance benchmarks added in case of performance changes

• Release notes entry updated:

  Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

  Release notes files:

  • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
  • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/release-notes/.Language/preview.md
  • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

  Information about the release notes entries format can be found in the documentation.
  Example:

  • More inlines for Result module. (PR #16106)
  • Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363)
    *while! (Language suggestion #1038, PR #14238)

  If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

[github-actions]

✅ No release notes required

[nojaf]

Another issue I saw is the in keyword is not longer preserved correctly in the case of

comp {let!a= b inand! c = d inand! e = f in()}

I made some changes for that as well so that any SynBinding now holds an InKeyword and not SynExpr.LetOrUse. This used to be part of SynExprAndBang.

[nojaf]

Didn't take this into account yet but should the range of in be part of SynBinding?
Thoughts @auduchinok?

[auduchinok]

Didn't take this into account yet but should the range of in be part of SynBinding?

I'd say no. I see it as a part of a let-expression, not a binding. It's what connects a binding (or multiple recursive ones) with the expression where the bound names can be used.

Even though it only makes sense in let-expressions, it can appear in module- or type-level bindings as a terminator, but currently it's effectively ignored in the resulting tree, so I'd care about the expressions case the most.

I'd put it to SynExpr.LetOrUse and (optionally) to SynModuleDecl.Let and SynMemberDefn.LetBindings.

[nojaf]

I'd put it on SynExpr.LetOrUse and (optionally) on SynModuleDecl.Let and SynMemberDefn.LetBindings.

Yeah, makes sense but might be a bit tricky to implement.

As LetOrUse now looks like:

| LetOrUse of// isRecursive: true for 'let rec' and 'use rec' bindings, false for 'let' and 'use' bindings
isRecursive: bool */// isUse: true for 'use' and 'use!' bindings, false for 'let' and 'let!' isUse: bool *// isFromSource: flag indicates whether a binding was explicitly written by the user in the source code versus generated by the compiler during transformations.
isFromSource: bool *// isBang: true for 'let!' and 'use!' bindings, false for 'let' and 'use' bindings
isBang: bool *
bindings: SynBinding list *
body: SynExpr *
range: range

(in my PR here)

Having a list of optional in keyword ranges in the LetOrUseTrivia type that must match the bindings list seems like a bad idea.

A separate list is messy because it forces assuming both lists will have the same length.

So it makes more sense to have a wrapper type that bundles SynBinding with the optional in range.

typeSynBindingAndMaybeAnInKeyword={
Binding:SynBinding;
InKeyword:Range option;}

Once we're on that train of thought, it's not a stretch to put it in the SynBinding trivia like I currently do.

It does not help that the code constructing these values is scattered across the parser.

My take is that including it in SynBinding trivia is the most pragmatic approach.

[auduchinok]

Having a list of optional in keyword ranges in the LetOrUseTrivia type that must match the bindings list seems like a bad idea.

Why would it be a list? There can be at most one in an LetOrUse expression. It's a part of LetOrUse that connects its list of bindings to the in-expression.

[nojaf]

Why would it be a list?

Example:

comp {let!a= b
and! c = d inand! e = f in()}

Becomes LetOrUse with three SynBindings, they can all have the in keyword.

This used to be 1 SynBinding and 2 SynExprAndBang with SynExprAndBangTrivia (which had in keyword as trivia).

After the refactor in F# 10 this information is now lost.
Which is what I'm also trying to solve.

[auduchinok]

@nojaf Interesting, thanks!

I suspect that allowing multiple in in a single let-expression was allowed by a mistake somewhere in #5696 or #7756.

The original RFC mentions the syntax that follows the rules of regular let-expressions (although, it's my interpretation, since it's not that strict):

builder { let! pat1 = e1 and! ... and! patN = eN in ... }

If that's a correct assumption, then the most sound way to proceed would be to disallow the extra in keywords and to model let-expressions as suggested before. There's, however, a chance that some people actually wrote these extra keywords in their projects, but my gut feeling says it should be very low-probability. Disallowing them could be a separate PR if we considered it needing a language version check, but maybe it could qualify as a low-impact bug fix for the and! implementation.

@dsyme May we ask your opinion on the topic, please?

[T-Gro]

@auduchinok :

Do you foresee a way of disallowing that syntax with a reasonable recovery and an error message?
If yes, I would agree to your proposal.

(We have to assume that people don't read release notes and don't like their parsing of "yesterday it worked fine" code fail mid-file)

If not easily doable, a staged approach (i.e. warning on this being a bug first, then remove 1 version later) would work for sure.

[auduchinok]

Do you foresee a way of disallowing that syntax with a reasonable recovery and an error message?

Yes, since it's already parsed properly we can just add an additional error reporting.

[nojaf]

Okay, this is now ready for review!

Future PRs can address the and! + in combo, I'm just going to ignore that here.

[auduchinok]

@nojaf This is fantastic, thanks!

[nojaf]

Hi @T-Gro , I do not get the failures in src\FSharp.Core\async.fs(2161,21): error FS0193: (NETCORE_ENGINEERING_TELEMETRY=Build) Type constraint mismatch. The type � 'Async<unit>' �is not compatible with type� 'unit'
This feels very unrelated.
Any ideas?

[T-Gro]

@nojaf :
I think this is related, most likely the representation of do! has changed and therefore there is a type-checking error stemming from the fact.
It looks like a do! is treated like if it was a do, hence causing the type mismatch.

Maybe best to reproduce with a syntax tree test for some very basic do! usage.

[nojaf]

@T-Gro thanks that did explain it. Green now