Skip to content

Unify anonymous record fields - #18910

Closed
edgarfgp wants to merge 9 commits into
dotnet:mainfrom
edgarfgp:unify-syn-expr-records
Closed

Unify anonymous record fields#18910
edgarfgp wants to merge 9 commits into
dotnet:mainfrom
edgarfgp:unify-syn-expr-records

Conversation

@edgarfgp

@edgarfgpedgarfgp commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

Description

Fixes # (issue, if applicable)

Checklist

  • Test cases added
  • Release notes entry updated

@github-actions

github-actionsBot commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

❗ 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 pathRelease notes pathDescription
src/Compilerdocs/release-notes/.FSharp.Compiler.Service/10.0.100.mdNo release notes found or release notes format is not correct

@brianrourkeboll

brianrourkeboll commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

I selfishly hope that this doesn't get merged till after I open my spread PR (soon!), since there would be some pretty big merge conflicts :)

@vzarytovskii

vzarytovskii commented Sep 16, 2025

Copy link
Copy Markdown
Member

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@edgarfgp

edgarfgp commented Sep 17, 2025

Copy link
Copy Markdown
ContributorAuthor

I selfishly hope that this doesn't get merged till after I open my spread PR (soon!), since there would be some pretty big merge conflicts :)

Will happily wait for the spread operator PR then :)

@edgarfgp

edgarfgp commented Sep 17, 2025

Copy link
Copy Markdown
ContributorAuthor

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@vzarytovskii Is there a way to add test to ensure the backwards compatibility?

@vzarytovskii

vzarytovskii commented Sep 17, 2025

Copy link
Copy Markdown
Member

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@vzarytovskii Is there a way to add test to ensure the backwards compatibility?

Good question. I don't think we have harness specifically for testing that. I guess manual testing should be enough.

@edgarfgp

edgarfgp commented Sep 17, 2025

Copy link
Copy Markdown
ContributorAuthor

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@vzarytovskii Is there a way to add test to ensure the backwards compatibility?

Good question. I don't think we have harness specifically for testing that. I guess manual testing should be enough.

I guess best l I can do is to write an ilverify test before and after the changes and compare the generated code

@vzarytovskii

vzarytovskii commented Sep 17, 2025

Copy link
Copy Markdown
Member

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@vzarytovskii Is there a way to add test to ensure the backwards compatibility?

Good question. I don't think we have harness specifically for testing that. I guess manual testing should be enough.

I guess best l I can do is to write an ilverify test before and after the changes and compare the generated code

That won't necessarily show all changes, as they can be in the sigdata as well I think.

@T-Gro

T-Gro commented Sep 17, 2025

Copy link
Copy Markdown
Member

I selfishly hope that this doesn't get merged till after I open my spread PR (soon!), since there would be some pretty big merge conflicts :)

We did postpone refactorings/code reorgs in the past when big work was in progress, and spreading justifies that IMO.

@T-Gro

T-Gro commented Sep 17, 2025

Copy link
Copy Markdown
Member

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@vzarytovskii Is there a way to add test to ensure the backwards compatibility?

Not as one of the existing test suites.
We could create a pair of projects, let's say a lib and a consumer.

And then have a test matrix:
Old + New
New + Old
New + New

Driven by global.json and msbuild properties to indicate which project should be build by which SDK//F# compiler respectively.

It's a suite which does not exist ATM.

T-Gro
T-Gro reviewed Sep 17, 2025
View reviewed changes
Comment threadsrc/Compiler/pars.fsy
braceBarExprCore:
| LBRACE_BAR recdExprCore bar_rbrace
{ let orig, flds = $2
let flds =

@T-GroT-GroSep 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right level for motivation here, i.e. unifying the parser code for anons vs records (and reflecting that in SyntaxTree as well) ?

Since the motivation for a refactoring should be to make future changes easier, I would also like to use @brianrourkeboll judgement (since spreading will do work across records and anons) if this change is a change that brings benefits.

@edgarfgpedgarfgpSep 17, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and reuse the new BlockSeparator type so we can have a better error recovery when using the wrong separator.

e.g. instead of showing error FS0039: The value or constructor 'B' is not defined. we should say that the wrong separator is used and guide users to use ; instead of comma.

{| A =3; B =4|}{| A =3, B =4|}
stdin(5,11): error FS0039: The value or constructor 'B' is not defined.

@edgarfgp

edgarfgp commented Sep 17, 2025

Copy link
Copy Markdown
ContributorAuthor

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@vzarytovskii Is there a way to add test to ensure the backwards compatibility?

Not as one of the existing test suites. We could create a pair of projects, let's say a lib and a consumer.

And then have a test matrix: Old + New New + Old New + New

Driven by global.json and msbuild properties to indicate which project should be build by which SDK//F# compiler respectively.

It's a suite which does not exist ATM.

@T-Gro I think that would be valuable for us moving forward. Even something that Copilot can do for us :)

auduchinok
auduchinok reviewed Sep 17, 2025
View reviewed changes
Comment threadsrc/Compiler/Service/ServiceParseTreeWalk.fs Outdated
Comment threadsrc/Compiler/Checking/Expressions/CheckExpressions.fs Outdated
@T-Gro

T-Gro commented Sep 17, 2025

Copy link
Copy Markdown
Member

One should be very careful with anonymous records, because it's very easy to break backwards compatibility for them by changing how they're emitted. At level when new compiler (with change) won't be able to consume assemblies produced by the old compiler (and probably vice versa).

@vzarytovskii Is there a way to add test to ensure the backwards compatibility?

Not as one of the existing test suites. We could create a pair of projects, let's say a lib and a consumer.
And then have a test matrix: Old + New New + Old New + New
Driven by global.json and msbuild properties to indicate which project should be build by which SDK//F# compiler respectively.
It's a suite which does not exist ATM.

@T-Gro I think that would be valuable for us moving forward. Even something that Copilot can do for us :)

Everyone, feel free to add comments and ask for more use cases at #18913 (comment) . I will act as a secretary repeating them to copilot...

@edgarfgp
edgarfgpforce-pushed the unify-syn-expr-records branch from 52bb958 to 5f52cc2CompareSeptember 17, 2025 18:31
edgarfgpand others added 8 commits September 17, 2025 19:31
@edgarfgp
Merge branch 'main' into unify-syn-expr-records
6223950
@edgarfgp
Unify anonymous record fields
5ce8cdc
@edgarfgp
Merge branch 'unify-syn-expr-records' of github.com:edgarfgp/fsharp i…
893bd93
…nto unify-syn-expr-records
# Conflicts:
#	src/Compiler/Driver/GraphChecking/FileContentMapping.fs
#	src/Compiler/Service/FSharpParseFileResults.fs
#	src/Compiler/Service/SynExpr.fs
#	src/Compiler/SyntaxTree/SyntaxTreeOps.fs
@edgarfgp
Reduce diff
e5265b1
@edgarfgp
update baselines
99a8c87
@edgarfgp
Merge branch 'main' into unify-syn-expr-records
2c44af1
# Conflicts:
#	tests/ILVerify/ilverify_FSharp.Compiler.Service_Debug_netstandard2.0.bsl
#	tests/ILVerify/ilverify_FSharp.Compiler.Service_Release_netstandard2.0.bsl
@edgarfgp
update baselines
7d052de
@edgarfgp
Merge branch 'main' into unify-syn-expr-records
0803809
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@T-GroT-GroT-Gro left review comments
+1 more reviewer
@auduchinokauduchinokauduchinok left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

F# Compiler and Tooling
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@edgarfgp@brianrourkeboll@vzarytovskii@T-Gro@auduchinok