(Empty/NoEmpty)-bodied named computation expression should produce the same AST

[edgarfgp]

Description

Investigating if it is feasible to produce the same AST for (Empty/NoEmpty)-bodied named computation expression

Fantomas

Builder () {  }

// Produces
SynExpr.App(
      flag = ExprAtomicFlag.NonAtomic,
      isInfix = false,
      funcExpr =
          SynExpr.App(
              flag = ExprAtomicFlag.NonAtomic,
              isInfix = false,
              funcExpr = SynExpr.Ident(Ident("Builder", R("(1,0--1,7)"))),
              argExpr = SynExpr.Const(constant = SynConst.Unit, range = R("(1,8--1,10)")),
              range = R("(1,0--1,10)")
          ),
      argExpr = SynExpr.Record(baseInfo = None, copyInfo = None, recordFields = [], range = R("(1,11--1,15)")),
      range = R("(1,0--1,15)")
  )

Builder () { 0 }

// Produces
SynExpr.App(
  flag = ExprAtomicFlag.NonAtomic,
  isInfix = false,
  funcExpr =
      SynExpr.App(
          flag = ExprAtomicFlag.NonAtomic,
          isInfix = false,
          funcExpr = SynExpr.Ident(Ident("Builder", R("(3,0--3,7)"))),
          argExpr = SynExpr.Const(constant = SynConst.Unit, range = R("(3,8--3,10)")),
          range = R("(3,0--3,10)")
      ),
  argExpr = SynExpr.ComputationExpr(hasSeqBuilder = false, expr = SynExpr.Const(constant = SynConst.Int32(0), range = R("(3,13--3,14)")), range = R("(3,11--3,16)")),
  range = R("(3,0--3,16)")
)

Fixes # (issue, if applicable)

Checklist

• Test cases added
• Performance benchmarks added in case of performance changes
• Release notes entry updated

[brianrourkeboll]

Yeah, the only downsides to this approach I noted in the RFC were:

Cons

• The AST no longer represents what the user wrote. That is, the user wrote builder { }, not builder { () }, but the distinction between these is no longer possible to represent in the AST.
• It may be difficult to foresee all potential effects on and interactions with existing parsing behavior. At a glance, the parsing of expressions involving curly braces { … } is already rather complex and involves multiple layers of non-obvious fallthroughs, etc.

[edgarfgp]

Thanks @brianrourkeboll. Im thinking that if we are already re-writing the Untyped AST during type checking . We can do that in the parser so we avoid the rewrite part ?.

[brianrourkeboll]

Im thinking that if we are already re-writing the Untyped AST during type checking . We can do that in the parser so we avoid the rewrite part ?.

Yeah sorry, I didn't mean to imply that doing it in the parser was categorically bad. Those were just the reasons why I chose not to modify the parser in the RFC.

I see that Florian has a potential solution to fsprojects/fantomas#3140 in fsprojects/fantomas#3141, but if we wanted to be more explicit — and since it looks like AST consumers need to update their handling of this scenario regardless — another alternative would be this:

Update the AST

In the untyped abstract syntax tree, update

SynExpr.ComputationExpr of hasSeqBuilder: bool * expr: SynExpr * range: range

to

SynExpr.ComputationExpr of hasSeqBuilder: bool * expr: SynExpr option * range: range

and update the parser to parse

builder { }

as

SynExpr.App
   (ExprAtomicFlag.NonAtomic,
     false,
     SynExpr.Ident (Ident ("builder", m)),
     SynExpr.ComputationExpr (false, None, m),
     m)

Pros

• The AST actually more closely represents what the user wrote — if their intent was to write the application of a computation expression builder value to an empty computation expression body. (See also cons below.)
• Minimal changes needed in the typechecker — just treat

SynExpr.ComputationExpr (false, None, m)

the same as

SynExpr.ComputationExpr (false, Some (SynExpr.Const (SynConst.Unit, range0)), m)

Cons

• In the absence of a body and additional type information, it is theoretically just as likely that the user is attempting to apply a function to an incomplete record construction expression. In this case, the AST now diverges from the user's intent.
• Both parsing and typechecking must be updated.
• Represents a breaking change to the untyped AST.
• As in the previous alternative, there is risk and complexity in making changes to the parser's treatment of curly-braced expressions.

For maximum accuracy, we could even add something like

SynExpr.EmptyRecordOrComputationExpr of range: range

:)

[github-actions]

❗ Release notes required

@edgarfgp,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

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

You can open this PR in browser to add release notes: open in github.dev

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.200.md	No release notes found or release notes format is not correct

[edgarfgp]

Thanks Brian. Yeah updating SynExpr.ComputationExpr to SynExpr.ComputationExpr of hasSeqBuilder: bool * expr: SynExpr option * range: range and adding an extra SynExpr.EmptyRecordOrComputationExpr of range: range it makes the AST close to the user will write. See my last commit.