FSharp 4.1 support

[varon]

I'm making pretty decent headway here, so I might as well open this up now.

I had to adjust the TypeShape library, which I've submitted a PR for: https://github.com/eiriktsarpalis/TypeShape/pull/6

With the updated TypeShape library running locally, I've got the tests (and new ones) passing with the EMIT_IL flag off.

[dsyme]

I'll update the dependencies in a different PR, #87

[varon]

I'm seeing some really wierd behaviour on the serialisation of array struct tuples (with EMIT_IL off). I'm not quite sure what's going on or what's causing this at the moment:

System.Exception : Falsifiable, after 13 tests (0 shrinks) (StdGen (1959872520,296312272)):
Original:
[]
with exception:
NUnit.Framework.AssertionException:   Expected: [], but was []

Any ideas on that?

[dsyme]

Any ideas on that?

Make sure the "expected" has the appropriate type annotation, that could be it

[dsyme]

@varon Got a line number?

[varon]

SerializerTests.fs#201

Looking carefully at the debugger, it seems as though it might be getting the array rank wrong for the empty 2D array, but that's supremely wierd because the types are identical.

[dsyme]

You'll need to resolve conflicts now, apologies

[varon]

That's no problem - there's always the wonderful checkout --theirs!

[varon]

Figured out the why the AppDomain tests with value tuples were failing:

1. BinaryFormatterSerializer fails with ValueTuple is not marked as serializable
2. Which defaults to Json.Net
3. Which represents all empty N-dimensional arrays as [], which loses the rank information of higher dimensions if the lower ones are 0.

Basically, Json.Net can't round trip 2D arrays while preserving rank information in this specific case.

We've got basically two options to address this:

either

ensure that all dimensions generated in our test arrays don't trigger the degenerate cases. This is pretty easily done with a Gen modification.

or

Adjust the equality checking to work around this.

I'd be in favour of (A) here.

[varon]

Turns out running struct records and unions through the existing struct pickler generation code passes all InMemory and Generic tests with or without EMIT_IL.

I'll add the Gen adjustments to fix those AppDomain tests.

[varon]

I'm not quite sure why CI is failing here. Seems to be a mono versioning issue. running git clean -fdx followed by ./build.sh on my PC passes everything.

Tests run: 1588, Errors: 0, Failures: 0, Inconclusive: 0, Time: 77.5807500742408 seconds
  Not run: 0, Invalid: 0, Ignored: 0, Skipped: 0

Finished Target: RunTests
Starting Target: Default (==> RunTests)
Finished Target: Default

---------------------------------------------------------------------
Build Time Report
---------------------------------------------------------------------
Target          Duration
------          --------
Clean           00:00:00.1927603
AssemblyInfo    00:00:00.0258366
Prepare         00:00:00.0000799
Build.Default   00:00:07.8997495
Build.NoEmit    00:00:07.7865849
Build           00:00:00.0000635
RunTests        00:02:35.0607391
Default         00:00:00.0000355
Total:          00:02:51.0348473
---------------------------------------------------------------------
Status:         Ok
---------------------------------------------------------------------

Ideas?

[varon]

AppVeyor build seems to fail due to F# 4.1 compiler not being available, I'm not sure how you guys want to best address that.

The travis-ci build also seems to fail for a similar reason. Perhaps a binding redirect or something is missing here: TypeShape error: struct unions not supported for runtime FSharp.Core versions lower than 4.1

[eiriktsarpalis]

That typeshape error is thrown intentionally. Using TS with struct unions and FSharp.Core < 4.4.1.0 is dangerous because FSharp.Reflection is broken for those types. I would try just upgrading the test version to use 4.4.1.0

[varon]

From the Paket.lock file, we're already using FSharp.Core 4.2.1 in the tests group.

The FSharp.Core.dll is being referenced from the tests group as well: <HintPath>..\..\packages\test\FSharp.Core\lib\net45\FSharp.Core.dll</HintPath>

We have the binding redirect set up in the tests app.config as follows:

<dependentAssembly>
  <Paket>True</Paket>
  <assemblyIdentity name="FSharp.Core" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
  <bindingRedirect oldVersion="0.0.0.0-65535.65535.65535.65535" newVersion="4.4.1.0" />
</dependentAssembly>

This is copied to the output folder and referenced as follows:

<Content Include="app.config">
  <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>

It's running and passing all tests on my windows box.

[dsyme]

@varon What does fsharpCoreRuntimeVersion evaluate to before that check fails?

[varon]

@dsyme - It's not failing locally on my computer, even from a command line build. As it's not printed out at any point on the CI server, I'm not sure how to check what it's evaluating to.

I've made a PR here to display it as part of that error: https://github.com/eiriktsarpalis/TypeShape/pull/7 , which should help us diagnose this

[varon]

@dsyme - Got that update in. It's magically running 4.4.0.0 somehow.

TypeShape error: FSharp.Core Runtime 4.4.0.0 does not support struct unions. 4.4.1.0 or later is required

[varon]

I tried adding an explicit FSharp.Core version to the build group. Still getting the same error.

I'm not sure how to resolve this. Help would be great here.

[dsyme]

Looking at this afresh, the appveyor tests are failing because F# 4.1 is not being used. You should add

image: Visual Studio 2017

to appveyor.yml

I will look at the Travis failure in a bit

[dsyme]

That's looking better on AppVeyor at least :)

[varon]

Magic! Thanks for the help on finding those. Guess we're ready for review. @dsyme @eiriktsarpalis

[dsyme]

This totally looks ready to me! Fabulous work!

[dsyme]

@varon Thanks for the contribution!

[varon]

Cheers

[eiriktsarpalis]

@dsyme There are unresolved issues I would like to see addressed before this gets merged.

[varon]

@eiriktsarpalis - The major goal was to just avoid the horrible crashing on the F# 4.1 struct types.

We definitely would benefit from more efficient serialization using the union cases, but that's significantly less critical than having our serializer die completely when encountering anything from 4.1.

[eiriktsarpalis]

It's not a question of efficiency, it's a question of deciding on a proper serialization format.

[dsyme]

@eiriktsarpalis OK, my apologies. We should address the remaining issue.

@varon A better approach to dealing with this for now is to fall back to reflection-based serialization for structs, which should work without alteration.

Can that be done just for struct unions?

[eiriktsarpalis]

@dsyme I believe so, it relies on FSharp.Reflection for most of the work