Skip to content

Field sharing for [<Struct>] DUs if they have equal name and equal type #15738

Merged
psfinaki merged 34 commits into
dotnet:mainfrom
T-Gro:struct-DUs-field-sharing
Nov 28, 2023
Merged

Field sharing for [<Struct>] DUs if they have equal name and equal type #15738
psfinaki merged 34 commits into
dotnet:mainfrom
T-Gro:struct-DUs-field-sharing

Conversation

@T-Gro

@T-Gro T-Gro commented Aug 2, 2023

Copy link
Copy Markdown
Member

First see what breaks.. (I am basing this on another change which is now awaiting PR review, sorry for larger diff because of that)

This implements part of the approved suggestion outlined here fsharp/fslang-suggestions#699 (comment) .

Implements #15715

Since such code did not compile before, this does not lead to a breaking change.
Even though this isn't the full approved suggestion, it is one that can be brought in without significant risks or breaking changes.

As of now, this allows field reuse as long as the field is equally named and has the same type after erasure.
(therefore e.g. UoM can be reused and therefore take a lot less space - see the added tests with sizeof<> checks added)

(I also personally believe that the full blown solution would be better done with a runtime change to support data overlap even with reference types and generics, and also utilizing the sizing of structs known at JIT time, but not at compile time)

Comment thread src/Compiler/Checking/CheckDeclarations.fs
Comment thread src/Compiler/Checking/CheckDeclarations.fs
Comment thread src/Compiler/CodeGen/EraseUnions.fs
@edgarfgp

Copy link
Copy Markdown
Contributor

@T-Gro Any idea when this will make it to the main? I have a couple of PRs that will improve error reporting around the same area. So do not want to make it difficult for you with the potential conflicts

@vzarytovskii

Copy link
Copy Markdown
Member

Does this change the codegen for all language versions, or only preview? If it's all (I saw some baselines have changed), it's too risky to merge it before November.

@edgarfgp

edgarfgp commented Aug 24, 2023

Copy link
Copy Markdown
Contributor

Does this change the codegen for all language versions, or only preview? If it's all (I saw some baselines have changed), it's too risky to merge it before November.

Maybe I can borrow the diagnostic/error reporting done in here. So this PR will focus in the codegen improvements?

@T-Gro

T-Gro commented Aug 25, 2023

Copy link
Copy Markdown
Member Author

Does this change the codegen for all language versions, or only preview? If it's all (I saw some baselines have changed), it's too risky to merge it before November.

This PR builds on top of #15695, which is a bugfix - and it does change codegen, because the nature of the bug was the fact that codegen was hacky and limiting more cases leading to an unexpected and cryptic runtime failure.

The specific part of this PR only:
It does not, since such definitons (sharing names) were considered an error and not compilable.

@abonie

abonie commented Oct 19, 2023

Copy link
Copy Markdown
Member

So what is the conclusion, can this be merged now or should it wait until November?

@psfinaki

Copy link
Copy Markdown
Contributor

@T-Gro I can review this next week. There are conflicts here though

@T-Gro

T-Gro commented Nov 27, 2023

Copy link
Copy Markdown
Member Author

I will resolve conflicts once this has the approvals.

@psfinaki psfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall LGTM, do you think the changes related to the new functionality can be more isolated in the code?

AFAIU this is just one step on the way to the new feature so it would be helpful for the future contributions to make this more clear.

Comment thread src/Compiler/FSComp.txt Outdated
Co-authored-by: Petr <psfinaki@users.noreply.github.com>
@T-Gro

T-Gro commented Nov 27, 2023

Copy link
Copy Markdown
Member Author

Overall LGTM, do you think the changes related to the new functionality can be more isolated in the code?

AFAIU this is just one step on the way to the new feature so it would be helpful for the future contributions to make this more clear.

The logic is already isolated within EraseUnions.fs, 1 file is already a good level if isolation considering PRs here.
Anything further would not work without redesigning (not just refactoring, but really a risky redesign) of EraseUnions alltogether - therefore causing massive conflicts for nullability, real visibility and other WIP PRs.
=> not worth it.

@T-Gro

T-Gro commented Nov 27, 2023

Copy link
Copy Markdown
Member Author

/run fantomas

  Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

@psfinaki psfinaki merged commit e185f9b into dotnet:main Nov 28, 2023
@xperiandri

Copy link
Copy Markdown
Contributor

Will this change also expose common fields as properties on a base type? So that they can be accessed without matching

@T-Gro

T-Gro commented Dec 12, 2023

Copy link
Copy Markdown
Member Author

For C# consumers, they are exposed.
In F#, pattern matching is to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Implement **part of** approved suggestion: 'Merge fields of the same type **and name** on multi-case union structs'

7 participants