[RFC FS-1079] Make .Is* discriminated union properties visible from F#

[chkn]

This implements RFC FS-1079 - Make .Is* discriminated union properties visible from F#.

I'd love feedback on a couple details:

1. I added the union augmentation stuff into AugmentWithHashCompare so I could re-use the helper functions, and because the code is similar to what was already there. Is that a good place, and if so, should we rename that module because it no longer deals just with HashCompare?

2. I'm not super proud of the fix to make the properties visible in a namespace rec context (https://github.com/dotnet/fsharp/pull/11394/commits/470eb3d8e08b6a6f771a66c8f8b129e2743ff39e). It works, but maybe there is a nicer way to do it

Thanks!

[chkn]

FSC : error FS2014: A problem occurred writing the binary 'D:\a\1\s\artifacts\obj\FSharp.Core\Release\netstandard2.0\FSharp.Core.dll': Error in pass2 for type Microsoft.FSharp.Core.FSharpOption`1, error: duplicate entry 'get_IsNone' in method table [D:\a\1\s\src\fsharp\FSharp.Core\FSharp.Core.fsproj]

Hmm.. I think the proper fix here is to look for [<DefaultAugmentation(false)>] and not generate the properties in that case?

[smoothdeveloper]

if so, should we rename that module because it no longer deals just with HashCompare?

Looking at the history, and given that those refactorings have a risk to break open PR / are, at times, handled by @dsyme or others in the team when they find the time is opportune, I'd say it is safer to not do the change in this PR.

I agree that something like TypeAugmentations.fs or similar would make sense, in the meantime, putting a comment about that realisation at the top of the file won't break anything and will bring it to the attention.

Putting up a separate PR with the refactoring may also get merged if it is the right time.

[auduchinok]

Should this change be a language feature and be in the preview version first?

[cartermp]

@auduchinok Yes, this will require a langversion guard before merging.

[smoothdeveloper]

@chkn / @cartermp could you setup the discussion thread for the RFC?

@chkn, this PR is good place to discuss the implementation details but we need the space to discuss how the RFC will alter the language (@NinoFloris brought something to slack that is worth discussing).

I myself would prefer the feature to be guarded by an attribute (to enable it on case by case basis to F# callers), if you like that too, you can check my PR which introduces an attribute and carries the information in the type checker: #11368

We can discuss more the RFC itself once we have the discussion thread setup.

Thanks!

[chkn]

@smoothdeveloper Thanks for tracking down that discussion thread!

I myself would prefer the feature to be guarded by an attribute (to enable it on case by case basis to F# callers)

My feeling is that we are already paying the metadata price for these members in IL, so we should be able to access them. You can already do [<DefaultAugmentation(false)>] to exclude those helpers entirely, and this PR honors that attribute.

putting a comment about that realisation at the top of the file won't break anything and will bring it to the attention.

Great idea, will add this.

Yes, this will require a langversion guard before merging.

I can add that as well.

[smoothdeveloper]

My feeling is that we are already paying the metadata price for these members in IL, so we should be able to access them.

could you clarify what you mean by paying the metadata price?

To me, making them accessible at cost of an explicit attribute (opt-in) seems like a fair question to evaluate, but if the consensus is that it should just be there and that the original design choice to hide those members needs to be changed, so be it.

There was a conscious choice to hide those early in the language, changing it may affect the idioms significantly.

You can already do [<DefaultAugmentation(false)>] to exclude those helpers entirely, and this PR honors that attribute.

Thanks for this important precision that I overlooked, although I assume it gets rid of all which makes a DU a DU in terms of structural equality.

[dsyme]

I'd like to see more testing of the language as well please, e.g.

• namespace rec where the Is properties are used before the definition
• test the quotation of an Is property
• test generic types
• test an SRTP constraint using an Is property

[vzarytovskii]

@chkn I've added some sample tests for rec namespace, SRTP, and fixed the issue with invoking the entry point in the testing framework. Let me know if you'll need any help with it.

[chkn]

@vzarytovskii Thanks! I also added your fix to the test runner in another spot, but it looks like a couple tests are still failing..not sure if it's related to this PR.

[vzarytovskii]

@chkn The following are failing FSharpQA tests (which look related?):

+++ Conformance\BasicTypeAndModuleDefinitions\UnionTypes (E_Overload_Equals.fs) +++
...
*** The following necessary lines were never matched:
***	\(7,23-7,29\):.+error FS0438:.+Duplicate method\. The method 'Equals' has the same name and signature as another method in type 'DU'\.$

+++ Conformance\BasicTypeAndModuleDefinitions\UnionTypes (E_Overload_GetHashCode.fs) +++
...
*** The following necessary lines were never matched:
***	\(8,25-8,36\):.+error FS0438:.+Duplicate method\. The method 'GetHashCode' has the same name and signature as another method in type 'DU'\.$

+++ Conformance\ObjectOrientedTypeDefinitions\StructTypes (E_Overload_Equals.fs) +++
...
*** The following necessary lines were never matched:
***	\(8,17-8,23\):.+error FS0438:.+Duplicate method\. The method 'Equals' has the same name and signature as another method in type 'S3'\.$

+++ Conformance\ObjectOrientedTypeDefinitions\StructTypes (E_Overload_Equals.fsx) +++
...
*** The following necessary lines were never matched:
***	\(7,6-7,8\):.+error FS0438:.+Duplicate method\. The method 'Equals' has the same name and signature as another method in type 'S3'\.$

+++ Conformance\ObjectOrientedTypeDefinitions\StructTypes (E_Overload_GetHashCode.fs) +++
...
*** The following necessary lines were never matched:
***	\(8,17-8,28\):.+error FS0438:.+Duplicate method\. The method 'GetHashCode' has the same name and signature as another method in type 'S2'\.$

+++ Conformance\ObjectOrientedTypeDefinitions\StructTypes (E_Overload_GetHashCode.fsx) +++
...
*** The following necessary lines were never matched:
***	\(7,6-7,8\):.+error FS0438:.+Duplicate method\. The method 'GetHashCode' has the same name and signature as another method in type 'S2'\.$

I need to look closer to the other suite.

[dsyme]

I fixed the build here, will now review

[dsyme]

I fixed the build here, will now review

Everything looks totally fine with this, I think we should go ahead and merge it into preview once it is green

@KevinRansom @vzarytovskii @TIHan @brettfo Please let me know if you have any concerns - I'm happy for this to be part of F# 6.0

[dsyme]

@chkn An unexpected test failure - the surface area of FSharp.Core has changed, with these unexpectedly missing:

2021-08-09T16:37:19.9718691Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean IsCons
2021-08-09T16:37:19.9719223Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean IsEmpty
2021-08-09T16:37:19.9719739Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean get_IsCons()
2021-08-09T16:37:19.9720282Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean get_IsEmpty()
2021-08-09T16:37:19.9720757Z     Microsoft.FSharp.Core.FSharpChoice`2[T1,T2]: Boolean IsChoice1Of2
2021-08-09T16:37:19.9724712Z     Microsoft.FSharp.Core.FSharpChoice`2[T1,T2]: Boolean IsChoice2Of2
2021-08-09T16:37:19.9725311Z     Microsoft.FSharp.Core.FSharpChoice`2[T1,T2]: Boolean get_IsChoice1Of2()
2021-08-09T16:37:19.9725851Z     Microsoft.FSharp.Core.FSharpChoice`2[T1,T2]: Boolean get_IsChoice2Of2()
2021-08-09T16:37:19.9727996Z     Microsoft.FSharp.Core.FSharpChoice`3[T1,T2,T3]: Boolean IsChoice1Of3
2021-08-09T16:37:19.9728527Z     Microsoft.FSharp.Core.FSharpChoice`3[T1,T2,T3]: Boolean IsChoice2Of3
2021-08-09T16:37:19.9729316Z     Microsoft.FSharp.Core.FSharpChoice`3[T1,T2,T3]: Boolean IsChoice3Of3
2021-08-09T16:37:19.9729784Z     Microsoft.FSharp.Core.FSharpChoice`3[T1,T2,T3]: Boolean get_IsChoice1Of3()
2021-08-09T16:37:19.9730275Z     Microsoft.FSharp.Core.FSharpChoice`3[T1,T2,T3]: Boolean get_IsChoice2Of3()
2021-08-09T16:37:19.9730749Z     Microsoft.FSharp.Core.FSharpChoice`3[T1,T2,T3]: Boolean get_IsChoice3Of3()
2021-08-09T16:37:19.9731300Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean IsChoice1Of4
2021-08-09T16:37:19.9731869Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean IsChoice2Of4
2021-08-09T16:37:19.9732407Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean IsChoice3Of4
2021-08-09T16:37:19.9732965Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean IsChoice4Of4
2021-08-09T16:37:19.9733529Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean get_IsChoice1Of4()
2021-08-09T16:37:19.9734069Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean get_IsChoice2Of4()
2021-08-09T16:37:19.9737009Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean get_IsChoice3Of4()
2021-08-09T16:37:19.9737597Z     Microsoft.FSharp.Core.FSharpChoice`4[T1,T2,T3,T4]: Boolean get_IsChoice4Of4()
2021-08-09T16:37:19.9738058Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean IsChoice1Of5
2021-08-09T16:37:19.9738534Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean IsChoice2Of5
2021-08-09T16:37:19.9739017Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean IsChoice3Of5
2021-08-09T16:37:19.9739474Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean IsChoice4Of5
2021-08-09T16:37:19.9739943Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean IsChoice5Of5
2021-08-09T16:37:19.9740405Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean get_IsChoice1Of5()
2021-08-09T16:37:19.9740895Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean get_IsChoice2Of5()
2021-08-09T16:37:19.9741386Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean get_IsChoice3Of5()
2021-08-09T16:37:19.9741859Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean get_IsChoice4Of5()
2021-08-09T16:37:19.9742344Z     Microsoft.FSharp.Core.FSharpChoice`5[T1,T2,T3,T4,T5]: Boolean get_IsChoice5Of5()
2021-08-09T16:37:19.9742813Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean IsChoice1Of6
2021-08-09T16:37:19.9743296Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean IsChoice2Of6
2021-08-09T16:37:19.9743775Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean IsChoice3Of6
2021-08-09T16:37:19.9744241Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean IsChoice4Of6
2021-08-09T16:37:19.9744718Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean IsChoice5Of6
2021-08-09T16:37:19.9745179Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean IsChoice6Of6
2021-08-09T16:37:19.9745673Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean get_IsChoice1Of6()
2021-08-09T16:37:19.9746240Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean get_IsChoice2Of6()
2021-08-09T16:37:19.9746773Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean get_IsChoice3Of6()
2021-08-09T16:37:19.9747319Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean get_IsChoice4Of6()
2021-08-09T16:37:19.9747969Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean get_IsChoice5Of6()
2021-08-09T16:37:19.9748520Z     Microsoft.FSharp.Core.FSharpChoice`6[T1,T2,T3,T4,T5,T6]: Boolean get_IsChoice6Of6()
2021-08-09T16:37:19.9749065Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean IsChoice1Of7
2021-08-09T16:37:19.9749588Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean IsChoice2Of7
2021-08-09T16:37:19.9750127Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean IsChoice3Of7
2021-08-09T16:37:19.9750738Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean IsChoice4Of7
2021-08-09T16:37:19.9751260Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean IsChoice5Of7
2021-08-09T16:37:19.9751797Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean IsChoice6Of7
2021-08-09T16:37:19.9752321Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean IsChoice7Of7
2021-08-09T16:37:19.9752872Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean get_IsChoice1Of7()
2021-08-09T16:37:19.9753426Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean get_IsChoice2Of7()
2021-08-09T16:37:19.9753969Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean get_IsChoice3Of7()
2021-08-09T16:37:19.9754529Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean get_IsChoice4Of7()
2021-08-09T16:37:19.9755070Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean get_IsChoice5Of7()
2021-08-09T16:37:19.9755627Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean get_IsChoice6Of7()
2021-08-09T16:37:19.9756183Z     Microsoft.FSharp.Core.FSharpChoice`7[T1,T2,T3,T4,T5,T6,T7]: Boolean get_IsChoice7Of7()
2021-08-09T16:37:19.9756691Z     Microsoft.FSharp.Core.FSharpResult`2[T,TError]: Boolean IsError
2021-08-09T16:37:19.9757178Z     Microsoft.FSharp.Core.FSharpResult`2[T,TError]: Boolean IsOk
2021-08-09T16:37:19.9757659Z     Microsoft.FSharp.Core.FSharpResult`2[T,TError]: Boolean get_IsError()
2021-08-09T16:37:19.9758159Z     Microsoft.FSharp.Core.FSharpResult`2[T,TError]: Boolean get_IsOk()
2021-08-09T16:37:19.9758653Z     Microsoft.FSharp.Core.FSharpValueOption`1[T]: Boolean IsValueNone
2021-08-09T16:37:19.9759130Z     Microsoft.FSharp.Core.FSharpValueOption`1[T]: Boolean IsValueSome
2021-08-09T16:37:19.9759627Z     Microsoft.FSharp.Core.FSharpValueOption`1[T]: Boolean get_IsValueNone()
2021-08-09T16:37:19.9760123Z     Microsoft.FSharp.Core.FSharpValueOption`1[T]: Boolean get_IsValueSome()

[dsyme]

The problem here is that the get_Is* declarations are not being added for union definitions that have a signature file (that is, they are being added for the implementation Tycon but not the signature Tycon). I'll look into it

[dsyme]

The problem here is that the get_Is* declarations are not being added for union definitions that have a signature file (that is, they are being added for the implementation Tycon but not the signature Tycon). I'll look into it

@chkn This is now fixed, I'm guessing we'll get further failing tests, let's try to get this green

[dsyme]

Two test failures in fsharpqa

2021-08-10T22:29:40.9394729Z CompilerOptions\fsc\standalone (W_MissingTransitiveRef.fs) -- failed
2021-08-10T22:37:09.3159751Z Libraries\Portable (parse_tests.fs (Desktop)) -- failed

One test failure in fsharp - still a mismatch in surface area

2021-08-10T22:32:20.7553657Z   Failed FSharp.Core.UnitTests.Portable.SurfaceArea.SurfaceAreaTest.VerifyArea [258 ms]
2021-08-10T22:32:20.7555048Z   Error Message:
2021-08-10T22:32:20.7555935Z    System.Exception : Assembly: FSharp.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
2021-08-10T22:32:20.7556726Z 
2021-08-10T22:32:20.7557567Z               Expected and actual surface area don't match. To see the delta, run:
2021-08-10T22:32:20.7558759Z                   windiff D:\workspace\_work\1\s\tests\FSharp.Core.UnitTests\SurfaceArea.fs D:\workspace\_work\1\s\artifacts\bin\FSharp.Core.UnitTests\Release\net472\FSharp.Core.SurfaceArea.coreclr.txt
2021-08-10T22:32:20.7559467Z 
2021-08-10T22:32:20.7559991Z               Unexpectedly missing (expected, not actual):
2021-08-10T22:32:20.7560694Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean IsCons
2021-08-10T22:32:20.7561402Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean IsEmpty
2021-08-10T22:32:20.7562143Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean get_IsCons()
2021-08-10T22:32:20.7562896Z     Microsoft.FSharp.Collections.FSharpList`1[T]: Boolean get_IsEmpty()
2021-08-10T22:32:20.7563344Z 

[chkn]

@dsyme Thanks so much, it looks great! I just did a little minor tidying. If it looks good to you and the tests are green, I think we're good to go

[dsyme]

@dsyme Thanks so much, it looks great! I just did a little minor tidying. If it looks good to you and the tests are green, I think we're good to go

Looks good!

[dsyme]

We do however need some more testing.

• [x] We need to consider whether it's possible to hide the representation of a union by a signature but still reveal the implied IsFoo members. I had originally thought from the implementation that it would be, however on reconsideration it won't be because the IsFoo members get accessibility based on the accessibility of the union cases, which will be private. However it need to be under test

• [ ] I'd like to see some tests across assembly boundaries - a union type in one assembly and the Is* properties accessed from another

• [ ] I'd like to see some tests related to the symbols returned by FSharp.Compiler.Service and it's affect on the IDE - for example, what does "goto definition" do? (should go to the union case) Renaming? (should not be allowed)

[dsyme]

@chkn I did some more work on this.

Union case testers are programmer-visible in intellisense lists so are not marked "compiler generated" - instead there is a new flag for elements that are programmer-visible but "implied" by other constructs.

This allows union case testers to be partially revealed in signatures.

I also added IsUnionCaseTester to the symbols API and made sure that the range of the definition of a union case tester is precisely the range of the union case name

I'd still like to see the additional testing above for FCS (e.g. in tests/service)

We should also consider what previous-version compilers (and compilations using /langversion:some-previous-version) will do when they see DLLs compiled with this new feature that include union types. I guess they will see the union case testers and they will be visible in auto-complete. That seems reasonable.

[vzarytovskii]

I've merged latest main. Will add some cross-project/cross-assembly tests.

[vzarytovskii]

It looks like most of the failed tests are some form of baseline tests. I will try to get to fixing it on weekend.

[chkn]

Thanks @vzarytovskii ! Let me know if there’s anything else I can do to help get this puppy merged

[chkn]

@vzarytovskii Where do we stand on this? Is it just the failing tests and merge conflicts?

[vzarytovskii]

@vzarytovskii Where do we stand on this? Is it just the failing tests and merge conflicts?

Yep, pretty much. I didn't have much time to get back to it. It is still in my backlog.

[edgarfgp]

I’m hopeful that this can be in the next version. Seems just a merge conflict away 😀 Great work @chkn !!

[vzarytovskii]

I’m hopeful that this can be in the next version. Seems just a merge conflict away 😀 Great work @chkn !!

I made 2 tries to merge it with latest main and failed. I will reset it to main and reapply patches manually, it will be easier to do :D

[vzarytovskii]

I've merged it with main. This should be passing now.

@dsyme give it another go when you'll have time.

[vzarytovskii]

Ok, some unrelated test failures which i probably have broken when merging stuff. Will fix it once back from vacation next week.