✅ No release notes required

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

comp {
    let! a = b in
    and! c = d in
    and! 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.

Didn't take this into account yet but should the range of in be part of SynBinding?
Thoughts @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.

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.

type SynBindingAndMaybeAnInKeyword = {
  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.

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.

Why would it be a list?

Example:

comp {
    let! a = b
    and! c = d in
    and! 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.

@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?

@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.

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.

Okay, this is now ready for review!

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

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?

@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.

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