Skip to content

[WIP] RFC-TBD Ease restrictions on static members and let in union and records - #14132

Closed
dsyme wants to merge 9 commits into
dotnet:mainfrom
dsyme:ease1
Closed

[WIP] RFC-TBD Ease restrictions on static members and let in union and records#14132
dsyme wants to merge 9 commits into
dotnet:mainfrom
dsyme:ease1

Conversation

@dsyme

@dsymedsyme commented Oct 18, 2022

Copy link
Copy Markdown
Contributor

This is the beginning of fsharp/fslang-suggestions#1182

Basically, allow static members and let in a wider range of type definitions

typeU=| Case1
| Case2 ofint
static do printfn "init U 1"static letu1= Case2 1static memberU1= u1
static do printfn "init U 2"static letu2= Case2 2static member valU2= B 2
static do printfn "init U 3"
printfn "%A" U.U1
printfn "%A" U.U2
typeR={
F1:int
F2:int}
static do printfn "init R 1"static letr1={ F1 =1; F2 =1}static memberR1= r1
static do printfn "init R 2"static letr2={ F1 =1; F2 =2}static member valR2= r2
printfn "%A" R.R1
printfn "%A" R.R2

@vzarytovskiivzarytovskii added this to the November-2022 milestone Oct 19, 2022
@edgarfgp

Copy link
Copy Markdown
Contributor

@vzarytovskii will be this inserted in VS and sdk for 17.5 ?

@T-Gro

Copy link
Copy Markdown
Member

Hi @dsyme , would you mind if I attempt to revive this, learn something new and continue where you ended + cover it with tests? Or are you planning to continue yourself?

@dsyme

Copy link
Copy Markdown
ContributorAuthor

@T-Gro Please do! I'll review and leave further notes

@dsyme

Copy link
Copy Markdown
ContributorAuthor

I think my only concerns were about initialization for union types, but I'll take a look and write notes for testing too

@T-GroT-Gro self-assigned this Apr 4, 2023
@T-GroT-Gro modified the milestones: Backlog, .NET 8May 4, 2023
@T-GroT-Gro modified the milestones: .NET 8, June-2023Jun 1, 2023
@vzarytovskiivzarytovskii removed their assignment Jun 5, 2023
@T-Gro

T-Gro commented Jun 8, 2023

Copy link
Copy Markdown
Member

/run fantomas

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

Copy link
Copy Markdown
Contributor

@T-Gro

Copy link
Copy Markdown
Member

@dsyme I revived the branch and continued here.
Can you please let me know which kind of union-types initilization issues I had in mind so that I can include them in the tests I have created so far?

https://github.com/dotnet/fsharp/pull/15343/files#diff-b66164fe40caedb46112c6330b8a2515feeefe867428cd33a389028cfa324731

@dsyme

Copy link
Copy Markdown
ContributorAuthor

@dsyme I revived the branch and continued here.

@T-Gro This is great!

Can you please let me know which kind of union-types initilization issues I had in mind so that I can include them in the tests I have created so far?

The slight concern is that for union types we emit beforefieldinit initialization flag. I can't actually think through a scenario where this matters however.

Context:

  • Initialization of static content in F# is basically on-demand, at the granularity of a file - if any static initialization is triggered, it's triggered for the whole file.

  • The static content is all stored in one per-file class with an initializer to ensure all its content gets initialized together if any is triggered

  • What constitutes a trigger is explained in the F# spec - some simple declarations like accessing x of let x = 1 do not cause a trigger. I've actually forgotten if this applies to accessing static let x = 1 in a class, via a member, I'd need to check.

  • The rules also change slightly for static content in generic classes because these get initialized "per-instantiation". In these cases the static content is stored directly in the class as it is type-indexed.

  • For unions there is also a unique singleton static stored for each union case that takes no arguments, e.g. look at the compilation of

    typeX= A | B ofint

    Here he static field is stored in the type X.

I think covering a range of common cases of static init will be enough. Check particularly the singleton cases, e.g.

type T =
| A
| B of int
static let x = [A; B 3]
static member X = x

Here accessing T.X from another file should obv trigger. I guess the important thing here is that initialization of the file content needs to access initialization of the static content of T (but not vice-versa)

With variations

  • mutual recursion between two types
  • mutable let
  • T becomes generic

On a quick glance I think I've seen these in the tests and it all looks ok tbh

@T-Gro

T-Gro commented Jun 14, 2023

Copy link
Copy Markdown
Member

Thanks @dsyme for the insight.

I know do remember that from the IlxGen file-level init code, and is actually something I have missed in the tests - I should have at least some scenario for multi-file test cases, of static init not being in the final file. (because IlxGen has different logic for the very last file)

(i.e. an action in main triggering static init of another file later, not immediately)

==> I will add those ( I will reuse some of the existing test code I already wrote, but just call it from another file added to the compilation)

@T-Gro

Copy link
Copy Markdown
Member

I have now covered the multi-file scenario as well:

https://github.com/dotnet/fsharp/pull/15343/files#diff-612f0ce4418f76f1f79c4bd03811dde6e6424025ecb489c4a3d6a104ebeb7303R316

It does work as expected:

  • neither type's nor module's init is called if nothing from the file is referenced by main program
  • non-main content is initialized on first access to anything in that file (following the existing behavior on what constitutes an access or doesn't)
    • The file's static init contents are accessed in order:
      1. All module-level bindings/do's which happened before the type definition
      2. Then the actual static content of the type
      3. Then module-level content which was declared below the type definition

@T-GroT-Gro removed their assignment Jun 26, 2023
@vzarytovskii

Copy link
Copy Markdown
Member

@T-Gro feel free to close if everything is covered in your PR.

@T-GroT-Gro closed this Jul 24, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants

@dsyme@edgarfgp@T-Gro@vzarytovskii