[RFC FS-1079] Make .Is* discriminated union properties visible from F# #11394
Conversation
Hmm.. I think the proper fix here is to look for |
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 Putting up a separate PR with the refactoring may also get merged if it is the right time. |
|
Should this change be a language feature and be in the preview version first? |
|
@auduchinok Yes, this will require a langversion guard before merging. |
|
@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! |
|
@smoothdeveloper Thanks for tracking down that discussion thread!
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
Great idea, will add this.
I can add that as well. |
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.
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. |
src/fsharp/ilx/EraseUnions.fs
Outdated
| @@ -683,6 +683,32 @@ let convAlternativeDef (addMethodGeneratedAttrs, addPropertyGeneratedAttrs, addP | |||
| | SpecialFSharpOptionHelpers | |||
| | SpecialFSharpListHelpers -> | |||
|
|
|||
| let baseTesterMeths, baseTesterProps = | |||
| if cenv.g.langVersion.SupportsFeature LanguageFeature.UnionIsPropertiesVisible then [], [] | |||
There was a problem hiding this comment.
Any idea how to get langVersion here? (cenv is not defined here)
There was a problem hiding this comment.
You should be able to pass in a TcGlobals (g) instead of an ILGlobals
There was a problem hiding this comment.
Hmm I don't think this is possible because TcGlobals is defined later in the file ordering, however adding langVersion to ILGlobals doesn't seem too invasive.
There was a problem hiding this comment.
Hmm I don't think this is possible because TcGlobals is defined later in the file ordering, however adding langVersion to ILGlobals doesn't seem too invasive.
I believe you can move EraseUnions.fs later. It is really part of IlxGen.fs and can be placed right near there if necessary
There was a problem hiding this comment.
I took a quick look at this approach, but it seems that TcGlobals depends on EraseUnions, at least in this one spot: https://github.com/dotnet/fsharp/blob/main/src/fsharp/TcGlobals.fs#L1660.
Perhaps it could be refactored, but I do think the patch to add langVersion to ILGlobals is fairly tidy (6885b0b), and may come in handy in the future. Any specific objections to that approach?
| [<EntryPoint>] | ||
| let main _ = | ||
| let foo = Foo.Foo "hi" | ||
| printfn "IsFoo: %b / IsBar: %b" foo.IsFoo foo.IsBar |
There was a problem hiding this comment.
It would be good to have tests
- that these are present in autocomplete lists
- what the results of "go to definition" on these is
- what happens when you try to rename this symbol (I guess it shouldn't be allowed)
I suspect (1) and (2) will be ok but (3) will not be - though I think we don't mind that much what happens with (3)
Ideally (1) and (2) should have automated tests
There was a problem hiding this comment.
It would be good to have tests
that these are present in autocomplete lists
what the results of "go to definition" on these is
what happens when you try to rename this symbol (I guess it shouldn't be allowed)
I suspect (1) and (2) will be ok but (3) will not be - though I think we don't mind that much what happens with (3)
Ideally (1) and (2) should have automated tests
|
I'd like to see more testing of the language as well please, e.g.
|
|
@chkn I've added some sample tests for rec namespace, SRTP, and fixed the issue with invoking the entry point in the testing framework. |
src/fsharp/CheckDeclarations.fs
Outdated
| @@ -2443,7 +2574,7 @@ let TcMutRecDefns_Phase2 (cenv: cenv) envInitial bindsm scopem mutRecNSInfo (env | |||
| // The rest should have been removed by splitting, they belong to "core" (they are "shape" of type, not implementation) | |||
| | _ -> error(Error(FSComp.SR.tcDeclarationElementNotPermittedInAugmentation(), mem.Range)))) | |||
|
|
|||
|
|
|||
| let extraBindings = Dictionary(HashIdentity.Reference) | |||
There was a problem hiding this comment.
I'm a bit confused why the dictionary and two-phase approach is needed. Can't we make the PublishValueDefnMaybeInclCompilerGenerated calls in the first phase (the part with Some tycon when declKind = DeclKind.ModuleOrMemberBinding), and the AugmentWithHashCompare.MakeBindingsForUnionAugmentation in the second phase (the part that comes after MutRecBindingChecking.TcMutRecDefns_Phase2_Bindings)?
There was a problem hiding this comment.
I mean, what you have is not wrong, and I can see that in order to do the second part you need to have the vals available from th first part anyway. I guess ideally they would be stored in the MutRecShape. However what you have is ok, maybe put a comment in like this:
/// In the first phase of establishing the type definitions we create and publish the values for the `Is*` members. For convencience we also create the associated bindings. The bindings aren't actually needed until the second phase, after `MutRecBindingChecking.TcMutRecDefns_Phase2_Bindings`, so we store them away in a dictionary until they are needed.
Also, the dictionary should really be indexed by stamp (tycon.Stamp). What is here is not wrong, it's just a little unusual in the compiler codebase to use object reference identity of Tycon/Entity as a key in a dictionary. Not a big deal but might be best to adjust it.
There was a problem hiding this comment.
Yeah, as I recall, the tradeoff I was considering was stashing the vals away (e.g. in TyconAugmentation), but that would mean adding an extra pointer to every type that's only needed for unions. Given how many non-union types you might find in a compilation, that could add up to a lot of wasted memory.
I agree the dictionary isn't the most elegant solution and I'm open to other ideas. For now, I added the comment you suggested and modified the dictionary to use tycon.Stamp
|
@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. |
|
@chkn I need to look closer to the other suite. |
There was a problem hiding this comment.
@chkn
The following are failing FSharpQA tests (which look related?):
@vzarytovskii Thanks for digging into that. I think I was able to fix that by simply reverting b2b3c2c-- not sure why I wasn't seeing FS0023 before, but I am now even with that reverted. The only thing is now you get both errors:
error FS0438: Duplicate method. The method 'get_IsA' has the same name and signature as another method in type 'DU'.
error FS0438: Duplicate method. The method 'get_IsA' has the same name and signature as another method in type 'DU'.
error FS0023: The member 'IsA' can not be defined because the name 'IsA' clashes with the default augmentation of the union case 'A' in this type or module
Perhaps that's not so bad though?
src/fsharp/CheckDeclarations.fs
Outdated
| @@ -2443,7 +2574,7 @@ let TcMutRecDefns_Phase2 (cenv: cenv) envInitial bindsm scopem mutRecNSInfo (env | |||
| // The rest should have been removed by splitting, they belong to "core" (they are "shape" of type, not implementation) | |||
| | _ -> error(Error(FSComp.SR.tcDeclarationElementNotPermittedInAugmentation(), mem.Range)))) | |||
|
|
|||
|
|
|||
| let extraBindings = Dictionary(HashIdentity.Reference) | |||
There was a problem hiding this comment.
Yeah, as I recall, the tradeoff I was considering was stashing the vals away (e.g. in TyconAugmentation), but that would mean adding an extra pointer to every type that's only needed for unions. Given how many non-union types you might find in a compilation, that could add up to a lot of wasted memory.
I agree the dictionary isn't the most elegant solution and I'm open to other ideas. For now, I added the comment you suggested and modified the dictionary to use tycon.Stamp
|
I fixed the build here, will now review |
dsyme
changed the title
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 |
|
@chkn An unexpected test failure - the surface area of FSharp.Core has changed, with these unexpectedly missing: |
|
I've merged it with @dsyme give it another go when you'll have time. |
|
Ok, some unrelated test failures which i probably have broken when merging stuff. Will fix it once back from vacation next week. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
PEVerify is failing on one of the dlls (invalid IL for generated methods?): Error running command 'D:\a_work\1\s\tests\FSharp.Test.Utilities....\artifacts\bin\PEVerify\Release\net472\PEVerify.exe' with args 'test--optimize-lib.dll /nologo' in directory 'D:\a_work\1\s\tests\fsharp\core\members\basics'.
---- stdout below ---
[D:\a_work\1\s\tests\fsharp\core\members\basics\test--optimize-lib.dll : Global+button::get_IsButtpon][mdToken=0x6000061][offset 0x00000002] Stack must contain only the return value.
[D:\a_work\1\s\tests\fsharp\core\members\basics\test--optimize-lib.dll : Global+callconv::get_IsAA][mdToken=0x600008e][offset 0x00000002] Stack must contain only the return value.
---- stderr below ---
. ERRORLEVEL 2Not sure what to do with it. |
|
Some secondary thoughts on this
I'll add these to the discussion for the RFC |
I can imagine this been problematic if for example if we have a [<RequireQualifiedAccess>]
type X =
| A
| a
where we would have IsA and Isa |
Yes. Though equally the feature is at least "honest" as we do codegen these properties and they can be used from C#.... |
|
Wondering how is it interacting with backticked DU cases. |
Certainly should be under test. I'd imagine it's just |
|
@chkn, do you still want to pursue this PR, it has been almost a year without any action? it seems to need some conflict resolution, and perhaps tests. |
|
@KevinRansom Hmm just reading back through the comments. Seems like we need to:
I'll try to find some time to get that done; it would be nice to finally land this :p Note that I've been using this branch locally for ages to build a project without any issue. |
|
I can't run the tests locally- they are crashing. Does anyone have any tips? |
You should probably run them in Release mode. We run Release in CI. To fix it in Debug, probably worth looking at what's crashing and use StackGuard. |
|
Pingerino. Would be a shame not to have this in F# 8. |
|
@chkn, I've brought this up-to-date locally and added a backtick test. If you give me write permissions to your fork, I can push the changes here and see what happens. |
This implements RFC FS-1079 - Make .Is* discriminated union properties visible from F#.
I'd love feedback on a couple details:
I added the union augmentation stuff into
AugmentWithHashCompareso 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?I'm not super proud of the fix to make the properties visible in a
namespace reccontext (470eb3d). It works, but maybe there is a nicer way to do itThanks!