Treat { new Foo() } as SynExpr.ObjExpr

[edgarfgp]

Description

I propose we unify { new Foo }, { new Foo() } and { New Foo with ... } as SynExpr.ObjExpr

Before

fantomas tools

{ new Foo  }   // Parsed as SynExpr.ObjExpr

{ new Foo() }  // Parsed as SynExpr.ComputationExpr

{ new Foo() with member this.Bar() = () }  // Parsed as SynExpr.ObjExpr

After

{ new Foo() } will still be parsed as SynExpr.ComputationExpr but instead we will re-write this to SynExpr.ObjExpr e.g.

{ new Foo  }   // Parsed as SynExpr.ObjExpr

{ new Foo() }  // Still Parsed as SynExpr.ComputationExpr 

{ new Foo() with member this.Bar() = () }  // Parsed as SynExpr.ObjExpr

// We are re-writing to  SynExpr.ObjExpr  If a SynExpr.ComputationExpr body consists of a single SynExpr.New, we should convert it to a SynExpr.ObjExpr/
match .. with
 | SynExpr.ComputationExpr(false, SynExpr.New(_, targetType, expr, m), _) ->       
    false, SynExpr.ObjExpr(targetType, Some(expr, None), None, [], [], [], m, rhsExpr.Range), overallTy, overallTy

Motivation

• Better error reporting
• Consistent compiler analysis.
• Prior work for Allow object expression without overrides #17387

[<AbstractClass>]
type Foo() = class end

let foo = { new Foo() }

The above snippet reports 3 different errors.

• Invalid object expression. Objects without overrides or interfaces should use the expression form 'new Type(args)' without braces.
• Invalid record, sequence or computation expression. Sequence expressions should be of the form 'seq { ... }'
• Instances of this type cannot be created since it has been marked abstract or not all methods have been given implementations. Consider using an object expression '{ new ... with ... }' instead.

After this PR it will only report 1 error.

• Invalid object expression. Objects without overrides or interfaces should use the expression form 'new Type(args)' without braces.

Checklist

• Test cases added
• Release notes entry updated

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.100.md

[psfinaki]

Naive question, how hard would it be to fix the problem in the parser, as in - stop parsing { new Foo() } as ComputationExpr? Or is it just about avoiding breaking changes here?

[edgarfgp]

Naive question, how hard would it be to fix the problem in the parser, as in - stop parsing { new Foo() } as ComputationExpr? Or is it just about avoiding breaking changes here?

Yeah. That was first attempt(Update the parser and some how fix the described conflict here. But then realized the described rule conflict might out of date now that there are implicit yields.

If an expression like { new Foo() } were in function argument position, like expr { new Foo() } , and if the function expression's type were a computation expression builder type, then { new Foo() } should be parsed as a SynExpr.ComputationExpr with an implicit yield.

So I think our best bet for now is to re-write during type checking like proposed in this PR.

[edgarfgp]

@vzarytovskii Do we also need to update the 9.0 release notes ?

[vzarytovskii]

@vzarytovskii Do we also need to update the 9.0 release notes ?

only 9.0.1xx, not 8.0.xxx as it's this time of the year when I locked the repo to bump version and change a bunch of defaults, so we can set up insertions in net9 only and bump TFM and language version

[edgarfgp]

Would love some feedback. Thanks

[psfinaki]

@edgarfgp we've discussed this in the team briefly recently - the motivation of the PR is good here, we need to think if there is any better approach. This one is reasonable, just not traditional. If we cannot think of anything better, we can probably get this in like it is now.

I understand this is a prior work and we appreciate you doing things in small chunks! That said, currently we are directing a lot of our time and energy into reviewing the nullness PR - we want to merge it ASAP to properly battletest it for .NET 9. You will help us a lot if you take a glance there also, since you've touched some of the code areas in your work :)

[edgarfgp]

@edgarfgp we've discussed this in the team briefly recently - the motivation of the PR is good here, we need to think if there is any better approach. This one is reasonable, just not traditional. If we cannot think of anything better, we can probably get this in like it is now.

I understand this is a prior work and we appreciate you doing things in small chunks! That said, currently we are directing a lot of our time and energy into reviewing the nullness PR - we want to merge it ASAP to properly battletest it for .NET 9. You will help us a lot if you take a glance there also, since you've touched some of the code areas in your work :)

Thanks @psfinaki. My approach here is similar to what @brianrourkeboll took in #17352

[brianrourkeboll]

@edgarfgp we've discussed this in the team briefly recently - the motivation of the PR is good here, we need to think if there is any better approach. This one is reasonable, just not traditional. If we cannot think of anything better, we can probably get this in like it is now.
I understand this is a prior work and we appreciate you doing things in small chunks! That said, currently we are directing a lot of our time and energy into reviewing the nullness PR - we want to merge it ASAP to properly battletest it for .NET 9. You will help us a lot if you take a glance there also, since you've touched some of the code areas in your work :)

Thanks @psfinaki. My approach here is similar to what @brianrourkeboll took in #17352

Right. In fact, if we updated the parser to treat { new Foo() } as SynExpr.ObjExpr instead of SynExpr.ComputationExpr, we'd still need to rewrite that to SynExpr.ComputationExpr in a scenario like builder { new Foo() } anyway. So I'm not sure there's an easy way to avoid AST adjustments during typechecking altogether.

[edgarfgp]

This is a prior work to implement a lang suggestion see PR description. Let me know if this PR is clear or you need something else for me to address.

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[psfinaki]

Hey @edgarfgp, sorry this takes time, we were busy with nullness PR for a while. As for this PR, @T-Gro wanted to take a deep look but is off now for a few days. We keep track of this.

I personally now cannot see any better way to do what you're trying to achieve and @brianrourkeboll's argumentation seems convincing to me. But please allow us some more time here.

[edgarfgp]

Patience is not passive, on the contrary, it is concentrated strength. Bruce Lee