Skip to content

Conversation

@T-Gro
Copy link
Member

@T-Gro T-Gro commented Jul 27, 2023

Fixes: #5342
Fixes: #9767

Before:
https://sharplab.io/#v2:DYLgZgzgPgsAUAbQDwDkD2BhNBbADgQwCcBLCNAOwG50BRARwFd9hiAXAT0oGVXCGBjVgD4AuvA64ApgAIAKhAjSAvNOkBBaVGkAhTdIx6AInpp6AYnoDiegBJ6AknoBSegNJ6AMnoCyelPHggA=

Have a look at the generated constructors, artificial arguments were added to tell them apart.
This lead to unexpected runtime crashes when constructing a case from a DU with > 49 cases.

After:

  • Nullary cases created plain .ctor setting the _tag field (just like new StructType(_tag = 13))
  • Non-nullary cases created using "maker method", which:
    • initializes the struct to 0 ( I am open to suggestions about cases where we could in theory avoid it?)
    • assigns the constant _tag field for this case
    • assigns fields needed by the case

The public-API - mainly properties for creationg, "IsXXX" properties etc. all remain the same, as does the struct layout in memory (at least for this PR).

@T-Gro T-Gro marked this pull request as ready for review July 31, 2023 09:41
@T-Gro T-Gro requested a review from a team as a code owner July 31, 2023 09:41
@T-Gro T-Gro changed the title [WIP] [<Struct>] DUs - eliminate dummy .ctor args, fix bug > 49 cases, simplify IL - see what fails [<Struct>] DUs - eliminate dummy .ctor args, fix bug > 49 cases, simplify IL Jul 31, 2023
@T-Gro T-Gro added this to the July-2023 milestone Jul 31, 2023
@T-Gro T-Gro self-assigned this Jul 31, 2023
@T-Gro T-Gro modified the milestones: July-2023, August-2023 Jul 31, 2023
@vzarytovskii
Copy link
Member

Important question - do we merge it, so it gets to 8, or is it too risky?

@T-Gro
Copy link
Member Author

T-Gro commented Aug 25, 2023

For me the risk outweighs the existing error System.InvalidProgramException: Common Language Runtime detected an invalid program. which this fixes.

A downside is that we are not dogfooding large struct DUs ourselves that much, only a few small ones in FSharp.Core.

Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

LGTM, I guess this can go?

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

5 participants