Skip to content

Conversation

@T-Gro
Copy link
Member

@T-Gro T-Gro commented Jun 8, 2023

This revives #14132 and implements fsharp/fslang-suggestions#1182 .

TODOs:

  • Order of let static init - tests

  • Active pattern - tests

  • DU cases within 'static let' without the type/module being recursive - tests

  • Plain enum behavior - tests

  • Hide this behind language feature, and keep producing an error for non-preview versions

  • Make this fail at typecheck time for plain enums (type E = | A = 0 | B = 1 ). As of now it fails at IL generation first.

  • Check behavior in recursive situations with circular dependencies between static members depending on static lets

  • Check behavior in generic types keeping per-TyparInstantion data in a static let binding

  • Fix regression for losing ' FS0912: This declaration element is not permitted in an augmentation '

  • Disallow 'static let' for extrinsic augmentations (to types in other assemblies)

  • Regression - "type Bad4 = member val X = 1 + 1" leads to internal error after a diagnostic is reported .

  • Regression - nonstatic extensions to records must remain an error (a "val" added, abstract member added)

  • Multi-file test , static init in not-last file (IlxGen has special behavior for last files)

  • Multi-file test, static init cascade trough multiple modules

@T-Gro T-Gro changed the title [WIP] To see CI errors [WIP] Allow static let bindings in union, record, struct, non-incremental-class types Jun 13, 2023
@edgarfgp
Copy link
Contributor

edgarfgp commented Jun 13, 2023

@T-Gro Would it make sense to add some tests for lowercase DUs? https://github.com/fsharp/fslang-design/blob/main/FSharp-7.0/FS-1126-allow-lower-case-du-cases%20when-require-qualified-access-is%20specified.md

[<RequireQualifiedAccess>]
type DU =
     | an of int
     | B of string
     | C
     | ``D`` of bool
     | ``d``

Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

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

We need to (manually) test the following scenario:

  1. Build the library with compiler from this branch
  2. Load it in project with older compiler, which does not have this feature.
  3. See that we generate IL + pickled info, which is loadable and accessible by earlier compilers.

@T-Gro
Copy link
Member Author

T-Gro commented Jun 21, 2023

I added a change that will make Fsharp.Core compilation fail if a feature requiring new pickling codes is used. We have discussed this with @dsyme and @vzarytovskii and Fsharp.Core is the only library where we want to support compilation on older tooling - it will fail if new constructs are used in it. Otherwise, it should not be a reason to stop language evolution as library authors can suggest to update tooling.

Methodology:
I have added a Discriminated Union to SI.fs in Fsharp.Core that was using "static let" fields and actions, marked under

#if !PROTO

I have updated Fsharp.Core to use preview version of the language.

I ran the build.cmd script.

The build has failed with the following two errors when pickling FSharp.Core:

D:\fsharp\src\FSharp.Core\SI.fs(236,16): error FS3569: Newly added pickle state cannot be used in FSharp.Core, since it must be working in older compilers+tooling as well. The time window is at least 3 years after feature introduction. Violation: fields in union . Context: � sizeOfTCached�perTyparInstMutableCounter [D:\fsharp\src\FSharp.
Core\FSharp.Core.fsproj::TargetFramework=netstandard2.0]
D:\fsharp\src\FSharp.Core\SI.fs(236,16): error FS3569: Newly added pickle state cannot be used in FSharp.Core, since it must be working in older compilers+tooling as well. The time window is at least 3 years after feature introduction. Violation: fields in union . Context: � sizeOfTCached�perTyparInstMutableCounter [D:\fsharp\src\FSharp.
Core\FSharp.Core.fsproj::TargetFramework=netstandard2.1]

sizeOfTCached and perTyparInstMutableCounter were names of the offending fields - they are part of the message to make the violation easier to find in X years in the future.

(error reported once for NS2.0, and once for NS2.1)

I am now keeping the check itself in the code, and rolling back the changes for making Fsharp.Core fail for demonstrative reasons.

cc: @auduchinok @vzarytovskii

Thanks for spotting this, kindly asking for reviewing the new bits.

@kerams
Copy link
Contributor

kerams commented Jun 22, 2023

What about a test in EmittedIL to verify what initialization takes place under the hood?

@T-Gro
Copy link
Member Author

T-Gro commented Jun 26, 2023

What about a test in EmittedIL to verify what initialization takes place under the hood?

Will add, one single file and one multi file (compilation strategy differs)

@T-Gro
Copy link
Member Author

T-Gro commented Jun 26, 2023

This is ready now

@edgarfgp
Copy link
Contributor

edgarfgp commented Jul 5, 2023

@T-Gro I'm curious to see if these changes will help with #11929 ?

@T-Gro
Copy link
Member Author

T-Gro commented Jul 7, 2023

@T-Gro I'm curious to see if these changes will help with #11929 ?

I am afraid this is orthogonal @edgarfgp , the issue still remains.
The workaround is to pull the code which needs to access "base" into a member, and then call that member.

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.

8 participants