Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behavior with struct multicase unions #1678

Closed
cartermp opened this issue Oct 28, 2016 · 21 comments
Closed

Unexpected behavior with struct multicase unions #1678

cartermp opened this issue Oct 28, 2016 · 21 comments
Labels
Bug Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code.
Milestone

Comments

@cartermp
Copy link
Contributor

cartermp commented Oct 28, 2016

The latest F# 4.1 compiler fails to compile either of the following code programs on .NET Core:

[<Struct>]
type Foo = Bar | Baz

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    0 // return an integer exit code
[<Struct>]
type Foo = 
    | Bar of int
    | Baz of int

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    0 // return an integer exit code

With this error:

FSC : error FS2014: A problem occurred writing the binary '/Users/phillip/scratch/test/obj/Debug/netcoreapp2.0/test.dll': Error in pass2 for type Program, error: Error in pass2 for type Foo, error: duplicate entry '.ctor' in method table [/Users/phillip/scratch/test/test.fsproj]

This is one rel/2.0.0 of the .NET CLI.

--- Old Issue ---

In VS "15" Preview 5, if you have the following:

[<Struct>]
type Shape =
    | Circle of radius: float
    | Square of side: double
    | Rectangle of sides: double*double

The following error occurs:

Error in pass2 for type Program, error: Error in pass2 for type Shape, error: duplicate entry '.ctor' in method table

If I remove the Square case , or change it to be of type int, it compiles.

@cartermp cartermp changed the title Unexpected behavior with multicase union structs Unexpected behavior with struct multicase unions Oct 31, 2016
@dsyme dsyme added Bug Area-Compiler Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. and removed Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. labels Nov 8, 2016
@neildanson
Copy link

neildanson commented Nov 24, 2016

You also get the same in the case with something like this

[<Struct>]
type DU = A | B

I've not taken a look at the implementation of struct multi case unions, but I'm guessing that a unique constructor is built per signature of each case? That would explain why it fails for cases with duplicate signatures ?

@cloudRoutine
Copy link
Contributor

I just ran into this issue too.

While it shows up in the build output, it does not in the error list

@amongonz
Copy link

I just installed F# support for the latest VS2017 RC and hit this issue. It fails every time two or more cases have the same type.

Inspecting the IL emitted for struct unions I see a private constructor for each case. I think the easiest fix would be having a single constructor, with a parameter for every field.

@dsyme
Copy link
Contributor

dsyme commented Jan 28, 2017

@mistiara Yes, we need to document this limitation, it is a bug in F# 4.1. We will fix it.

@dsyme dsyme added this to the VS 2017 Updates milestone Jan 28, 2017
@dsyme dsyme added Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. and removed Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. labels Mar 14, 2017
@dsyme dsyme added the Ready label Apr 7, 2017
@et1975
Copy link

et1975 commented May 1, 2017

Is there any way to tell if it's in the fsharp/fsharp yet?

@vasily-kirichenko
Copy link
Contributor

@et1975 I believe fsharp/fsharp is dead. Build the compiler from this repo.

@dsyme
Copy link
Contributor

dsyme commented May 2, 2017

@et1975 I believe fsharp/fsharp is dead. Build the compiler from this repo.

No, not dead - just aligned. TBH it will never be "dead" because its existence forms part of the mission statement of the FSSF.

Right now, all the Mono distributions, FSharp.Compiler.Tools and FSharp.Compiler.Service packages are still packaged via integrations and tagging in that repo and/or fsharp/FSharp.Compiler.Service and other downstream packaging repos. The details are in https://github.com/fsharp/fsharp/blob/master/README.md

So fsharp/fsharp is really just a downstream packaging repo.

Unless absolutely critical (e.g. in preparing Mono releases) all contributions should go via this repo.

@et1975
Copy link

et1975 commented Jul 12, 2017

Is there any way to tell if this (or any other fix) has been released, in MS and/or Open distributions?

@neildanson
Copy link

@et1975 I'm not sure if there's an official way to tell, but I confirm this particular fix is in the latest f# compiler nuget package.

@zpodlovics
Copy link

I just checked it with .NET Core 2.0 preview2 SDK (2.0.0-preview2-006497) and still hit the "duplicate entry '.ctor' in method table" issue with this example:

[<Struct>]
type DU = Case1 | Case2

@cartermp
Copy link
Contributor Author

I can confirm this with Preview 3 as well:

screen shot 2017-07-29 at 9 52 58 am

@KevinRansom does the compiler in the .NET Core SDK not contain the fix in #2811?

@KevinRansom
Copy link
Member

@cartermp yes, that code is in both master and vs2017-rtm. It is in both the desktop and coreclr builds.

@KevinRansom
Copy link
Member

@et1975 Everything that was in master before 7/27/2017 is in the release branch for VS2017.3.

Merge PR: 4a312d3

The latest dotnet/cli compiler and nuget released Microsoft.FSharp.Compiler was built including those changes. The VS2017.3 rtm branch contain cherry-picks of the most important recent changes to master.

We don't have a great process for identifying when something goes from master to vs2017-rtm, that is usually controlled by the VS release cadence and the level of risk that can be allowed against the VS release. There is also the bar for the dotnet-cli to take into account, although we release to that out of VS2017-RTM via the nuget Microsoft.FSharp.Compiler.nuget package.

I'm sorry it's so difficult to figure out, releasing code to millions of users is a black-art and I am but a cog in the machine.

The rule of thumb I use is this ---
master branch --- will be released for the next + 1 VS and corresponding dotnet cli release.
vs2017-rtm --- will be released in the next VS and corresponding dotnet-cli release.

So far we have included code in VS2017, VS2017.2 and VS2017.3.

After a release has occurred, master will be merged into vs2017-rtm and relevant vs2017-rtm changes will be merged back into master.

All of the above is dependent on approvals from the VS release team and their goals for each release.

@cartermp
Copy link
Contributor Author

cartermp commented Jul 29, 2017

Hmmmm. Then it appears that @dsyme's PR didn't fix this duplicate ctor bug.

@KevinRansom
Copy link
Member

It's all fixed in VS2017.3
Desktop
image

Coreclr

C:\temp\acme>dotnet --info
.NET Command Line Tools (1.1.0)

Product Information:
 Version:            1.1.0
 Commit SHA-1 hash:  d6f4336106

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.14393
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\1.1.0

C:\temp\acme>type Program.fs
// Learn more about F# at http://fsharp.org

open System

[<Struct>]
type Shape =
    | Circle of radius: float
    | Square of side: double
    | Rectangle of sides: double*double

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    0 // return an integer exit code

C:\temp\acme>dotnet restore
  Restore completed in 43.16 ms for C:\temp\acme\acme.fsproj.

C:\temp\acme>dotnet build
Microsoft (R) Build Engine version 15.3.409.57025 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  acme -> C:\temp\acme\bin\Debug\netcoreapp1.1\acme.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.05

C:\temp\acme>

@cartermp
Copy link
Contributor Author

Does that work cross platform? I tested with the latest build of SDK 2.0 on macOS and repro'd.

@KevinRansom
Copy link
Member

Should work fine cross platform.

@KevinRansom
Copy link
Member

Works on MAC-OS with the latest bits.

You need to use release/2.0.0 branch or rel/1.1.0 for attest bits. master is a bit whacky on dotnet/cli

MacBook-Pro:/Users/kevinr/temp/one> cat Program.fs
// Learn more about F# at http://fsharp.org

open System

[<Struct>]
type Shape =
    | Circle of radius: float
    | Square of side: double
    | Rectangle of sides: double*double

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    0 // return an integer exit code
MacBook-Pro:/Users/kevinr/temp/one> dotnet build
Microsoft (R) Build Engine version 15.3.409.57025 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  one -> /Users/kevinr/temp/one/bin/Debug/netcoreapp2.0/one.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:01.35
MacBook-Pro:/Users/kevinr/temp/one> ````

@zpodlovics
Copy link

@KevinRansom The example you posted (struct du with different unique type for each case) works fine with .NET Core preview2 (2.0.0-preview2-006497) running on Ubuntu 16.04. However it seems that the struct du the typeless/parameterless cases are not handled properly in the preview2 version of the sdk.

The compiler for this example at least provide an error why the struct du is not compilable (current struct du limitation):

open System

[<Struct>]
type DU =
    | Case1 of int
    | Case2 of int

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    0 // return an integer exit code
dotnet run -c Release
/tmp/du/Program.fs(4,6): error FS3204: If a union type has more than one case and is a struct, then all fields within the union type must be given unique names. [/tmp/du/du.fsproj]

The build failed. Please fix the build errors and run again.

However for the following case, the compiler does not give any information why the compilation fails (current struct du limitations):

open System

[<Struct>]
type DU =
    | Case1
    | Case2

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    0 // return an integer exit code
dotnet run -c Release
FSC : error FS2014: A problem occurred writing the binary '/tmp/du/obj/Release/netcoreapp2.0/du.dll': Error in pass2 for type Program, error: Error in pass2 for type DU, error: duplicate entry '.ctor' in method table [/tmp/du/du.fsproj]

The build failed. Please fix the build errors and run again.

@cartermp
Copy link
Contributor Author

cartermp commented Jul 30, 2017

@KevinRansom

You need to use release/2.0.0 branch or rel/1.1.0 for attest bits

That's what I'm using. On rel/2.0.0:

[<Struct>]
type Foo = Bar | Baz

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    0 // return an integer exit code

Fails:

FSC : error FS2014: A problem occurred writing the binary '/Users/phillip/scratch/test/obj/Debug/netcoreapp2.0/test.dll': Error in pass2 for type Program, error: Error in pass2 for type Foo, error: duplicate entry '.ctor' in method table [/Users/phillip/scratch/test/test.fsproj]

@cartermp cartermp reopened this Jul 30, 2017
@dsyme dsyme removed the Ready label Aug 5, 2017
@dsyme
Copy link
Contributor

dsyme commented Aug 30, 2017

@cartermp I tried

[<Struct>]
type Foo = Bar | Baz

on master and it works correctly for me

@dsyme dsyme closed this as completed Aug 30, 2017
@cartermp cartermp modified the milestones: VS 2017 Updates, .NET Core 2.0, 15.5 Jun 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code.
Projects
None yet
Development

No branches or pull requests

9 participants