Implement ToString on unions (RFC FS-1022)

[colinbull]

This is an implementation of RFC FS-1022.

The current implementation implements by default an override of ToString on Unions, only if no override has been defined previously. The implementation is somewhat naive at the minute and just uses sprintf. As the RFC states it is possible to create a more optimised implementation of this since do, know the structure of the types within the union case, so we could avoid the reflection cost. However, I'm currently of the opinion that the extra complexity of this might not be worth it. As I would expect this to emit representations that where equivalent to sprintf so my gut feeling is that we would just be re-implementing a lot of the reflection logic, just in this case statically using the type definition. Moreover people can simply override ToString themselves this if they wish to provide a more performant implementation

This is currently work in progress as there are some tests still failing in core04.

[msftclas]

Hi @colinbull, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[msftclas]

@colinbull, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[dsyme]

Normally we now try to generate augmentations in TypeChecker.fs/AugmentWithHashCompare. Doing this (generating TAST nodes) usually has benefits, because to F# the thing looks like a "normal" method for all other checks, and everything works just as if the user had written the code manually.

However in this particular case I can't think of a specific reason why that would bring a benefit. But if you wanted to take a look at doing that in AugmentWithHashCompare it may end up being be better

[colinbull]

Yep, I certainly can do. I was initially looking at that, but was slightly
put off putting logic not to do with HashCompare because of the name and I
had noticed the DebugDisplayAttribute augmentations for records in
IlxGen.fs.

On Tue, Oct 11, 2016 at 9:41 AM, Don Syme notifications@github.com wrote:

Normally we now try to generate augmentations in TypeChecker.fs/AugmentWithHashCompare.
Doing this (generating TAST nodes) usually has benefits, because to F# the
thing looks like a "normal" method for all other checks, and everything
works just as if the user had written the code manually.

However in this particular case I can't think of a specific reason why
that would bring a benefit. But if you wanted to take a look at doing that
in AugmentWithHashCompare it may end up being be better

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1589 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjvSsQTg_qpHC-kT345jqcki3fPLrveks5qy0vEgaJpZM4KTD2O
.

[dsyme]

@colinbull Yeah. Since you have this in place already I'm inclined to think you should just keep it as is unless we can think of a specific reason why moving to generating TASTs is better for this case

[colinbull]

Apologies for the sort of newbie nature to this question, but I'm slightly confused by these failing tests. Basically I have added 3 modules to core\members\basics that represent some simple test cases for this change. If I remove these tests then the full suite passes so I know it is definitely something to do with these but I'm abit lost as to what. Looking in tests results I can see that the tests I have added pass for the GENERATED_SIGNATURE permutation.

However when these tests are run for FSI_FILE, FS_OPT_PLUS_DEBUG and AS_DLL permutations fail with ERRORLEVEL -1073741571, 255 and 255 respectively which seems to be due to a StackOverflow exception.

The thing I'm finding strange is this seems to work perfectly from both fsi when the type is entered and in a compiled assembly, I cannot replicate the error that the tests are showing. Now I'm sure there is something obviously wrong, but looking at the IL it looks pretty sound to my untrained eye,

.method public virtual strict instance string 
      ToString() cil managed 
    {
      .maxstack 8

      IL_0000: ldstr        "%A"
      IL_0005: newobj       instance void class [FSharp.Core]Microsoft.FSharp.Core.PrintfFormat`5<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class Global.ToStringOnUnionTest/MyUnion, string>, class [FSharp.Core]Microsoft.FSharp.Core.Unit, string, string, class Global.ToStringOnUnionTest/MyUnion>::.ctor(string)
      IL_000a: call         !!0/*class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class Global.ToStringOnUnionTest/MyUnion, string>*/ [FSharp.Core]Microsoft.FSharp.Core.ExtraTopLevelOperators::PrintFormatToString<class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class Global.ToStringOnUnionTest/MyUnion, string>>(class [FSharp.Core]Microsoft.FSharp.Core.PrintfFormat`4<!!0/*class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class Global.ToStringOnUnionTest/MyUnion, string>*/, class [FSharp.Core]Microsoft.FSharp.Core.Unit, string, string>)
      IL_000f: ldarg.0      // this
      IL_0010: callvirt     instance !1/*string*/ class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2<class Global.ToStringOnUnionTest/MyUnion, string>::Invoke(!0/*class Global.ToStringOnUnionTest/MyUnion*/)
      IL_0015: ret          

    } // end of method MyUnion::ToString  

and the C# representation of the decompiled sources looks O.K as well,

      public override string ToString()
      {
        return ExtraTopLevelOperators.PrintFormatToString<FSharpFunc<ToStringOnUnionTest.MyUnion, string>>((PrintfFormat<FSharpFunc<ToStringOnUnionTest.MyUnion, string>, Unit, string, string>) new PrintfFormat<FSharpFunc<ToStringOnUnionTest.MyUnion, string>, Unit, string, string, ToStringOnUnionTest.MyUnion>("%A")).Invoke(this);
      }

Any pointers on the best way to go about debugging this?

[liboz]

@colinbull I did some poking around for this. I think the issue is that when it is compiled for FSI_FILE, FS_OPT_PLUS_DEBUG, and AS_DLL the union class is compiled with [CompilationMapping((SourceConstructFlags)33), DebuggerDisplay("{__DebugDisplay(),nq}")] instead of [CompilationMapping(SourceConstructFlags.SumType), DebuggerDisplay("{__DebugDisplay(),nq}")].

Because of this difference in the CompilationMapping attribute, the call to FSharpType.IsUnion for your test type results in false when only using public bindings. And then it treats the union type as a generic .NET object, which it then attempts to call ToString() on to get it's string evaluation. However, since this all started because it is using ToString(), this results in an infinite loop and the StackOverFlow exception.

I'm not sure why the CompilationMapping is different when compiled for for those modes, though they are equivalent when using KindMask, but maybe @dsyme can shed some light on that.

Edit: That said modifying the IL and changing the CompilationMapping to the SumType, it still crashes, but with a visibility issue. I imagine that means that the CompilationMapping is just a symptom of something else...

[colinbull]

OK, so the fix to this is quiet simple, just declare the types in the test.fsi file. However, I'm not sure why this would change the CompilationMapping as @liboz points out and cause the Stack overflow I was seeing. Having checked the language specification, I don't see anything obvious in there that would point to why this behaviour exists.

@dsyme is this by design or is something else going on here?

[liboz]

@colinbull I think you might want to try rebasing onto master instead of merging. Merging caused a lot of commits that weren't yours to show up in the changes.

Also I don't think you need to commit the TestLibrary.dll.

[forki]

what's the status? I'm REALLY looking forward to this

[colinbull]

Since the merge of the Roslyn stuff to the master I cannot build it anymore as I have been unable to get the install of Dev 15 to complete successfully on any machine at the minute. SO I can't investigate why the CI builds are failing. So a little blocked until I can get this working.

[forki]

Sigh. Ok then let's try something different. Can you please try to rebase your stuff on very old commit that still builds for you? Should not influence anything in this feature branch.

On a different note: @cartermp we should always make sure that compiler build/edit experience works with current VS. VS Vnext is something that really only Microsoft cares about. Unfortunately we have discussion every single time before new VS release.

[liboz]

Odd. I'm using Visual Studio 2015 and not having problems working on master...

[dsyme]

@colinbull Are you saying you can't build build.cmd net40? That shouldn't need anything related to Visual Studio.

[0x53A]

I had a similar issue, a git clean and rebuild of proto compiler helped in my case

[colinbull]

I haven't tried rebuilding the proto compiler. I'll give that a try, although I suspect it won't help looking at the build log (attached), apparently it is missing some references.

See here.
build.txt

EDIT: Obviously I can add these references, but wondering whether I actually should

[dsyme]

@colinbull I thought I'd fixed that issue. Are you sync'd with master?

thanks

[colinbull]

@dsyme I have just cloned a brand new repository and this error still persists.

[dsyme]

@colinbull Thanks. I can definitely see the problem. There is this incredibly annoying issue here: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Compiler.Server.Shared/AssemblyInfo.fs#L12

If you want to work around it you can just remove that attribute (it is only used for Visual Studio setup installing this DLL) or define CROSS_PLATFORM_COMPILER for that DLL

It seems we don't do a CI build on a Windows machine with Visual Studio installed, I see if I can enable that and fix this issue by getting that DLL from some nuget package.

  Primary reference "Microsoft.VisualStudio.Shell.Immutable.10.0.dll".
C:\Program Files (x86)\MSBuild\14.0\bin\Microsoft.Common.CurrentVersion.targets(1820,5): warning MSB3245: Could not resolve this reference. Could not locate the assembly "Microsoft.VisualStudio.Shell.Immutable.10.0.dll". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors. [C:\Appdev\visualfsharp\src\fsharp\FSharp.Compiler.Server.Shared\FSharp.Compiler

[colinbull]

@dsyme I tried to get this to build based on your suggestion but it still didn't work since the VS SDK assemblies aren't conditionally referenced. So to fix this, I have added these as conditional references; but to make sure the current build still works when those assemblies are present, I have made some other changes to the build. However having the assmeblies NuGetted is alot more transparent so if that is possible then I'll keep the changes I have local. Either way I have pushed the changes that fixed the build for me to colinbull@4e0a848 if they look good to you and NuGet-ing the assmeblies isn't possible. then I'll create an issue and corresponding PR with the fix. I'm just concerned I might mess up something even more subtle with this type of change. But the build now works for me :)

[dsyme]

@colinbull Can you try #1723 please and see it it builds for you with that fix. It uses nuget in the obvious way

[forki]

Uh so it's failing more than one single time? That's weird. Then it should be investigated

[liboz]

@KevinRansom @dsyme I think this is ready for review now that it's all green.

[liboz]

@colinbull Now that #2189 has been merged, to get rid of the conflict, you need to reset to 1220c65240f827fa451c08f4ef2a9c88612bc5d3 and then rebase from master.

[KevinRansom]

@colinbull I will look at this over the weekend. a 34 file PR will require some time.

Thanks

Kevin

[colinbull]

OK cool, the majority of files are due to the impact the tostring augmentation had on the IL generation and thus the tests covering those.

[KevinRansom]

@dotnet-bot test this please

[KevinRansom]

This looks good to me.

[KevinRansom]

@dsyme Can you run your eyes over this please, it looks fine to me.

Kevin

[cartermp]

🎉

[KevinRansom]

@colinbull

Thanks for this work

Kevin

[abelbraaksma]

New location of RFC: https://github.com/fsharp/fslang-design/blob/main/FSharp-4.1b/FS-1022-override-ToString-for-discriminated-unions-and-records.md