Fix stack overflow on assembly resolution

[KevinRansom]

Well .... this is embarrassing ... as I have said before our build is dependent on a very old build of the coreclr for testing.

We had a bug based on a badly implemented LoadContext (Sorry about that ... all my fault), that works on this very old coreclr but was fixed soon after ... only .... well we couldn't update at that time. On a fixed coreclr this old bad but working code becomes a stack overflow. Because I believed it only impacted fsi.exe, and there is a lot more I want to do for that tool I wasn't in any rush to fix it --- bad descision because it also impacts TypeProviders which I forgot about.

Anyway ... because we still need to use the old coreclr for a bit longer ... only for testing of course ... the fix here tests to see if the mscorlib is of a very specific version ... the one we use for testing. We do the old thing ... if instead it is any other version in existence we do the right thing.

Anyway please don't hate on me ... I am doing plenty of that myself. I wish that I was smart enough to come up with a good excuse that blamed others but this one was all on me ...

Fixes: #3658

[KevinRansom]

@dsyme @xenocons , do either of you have a repro to test. This fixes the issue that I know of but that one doesn't use a TP.

Thanks

Kevin

[ghost]

Hi Kevin, We all have bugs :)

I have difficulties to compile fsharp on netcore 2 with debian at the moment, however as a repo:

dotnet /usr/share/dotnet/sdk/2.0.0/FSharp/fsc.exe --noframework -r:/usr/share/dotnet/sdk/NuGetFallbackFolder/netstandard.library/2.0.0/build/netstandard2.0/ref/mscorlib.dll -r:/usr/share/dotnet/sdk/NuGetFallbackFolder/netstandard.library/2.0.0/build/netstandard2.0/ref/System.Runtime.dll -r:/usr/share/dotnet/sdk/NuGetFallbackFolder/netstandard.library/2.0.0/build/netstandard2.0/ref/netstandard.dll -r:/home/xenocons/.nuget/packages/sqlprovider/1.1.11/lib/FSharp.Data.SqlProvider.dll test.fs

test.fs can be anything, and the bug occurs with the inclusion of sqlprovider (you will need to modify the path in the above command). For anyone else reading this links to #3654

[forki]

let attributes = typeof.GetTypeInfo().Assembly.GetCustomAttributes()
match Seq.tryHead attributes with
| Some a -> a.Version = "4.6.24410.01"
| _ -> false

[forki]

also I assume it should be moved down next to usage

[forki]

and the the bool you can remove the temporary bool completely ....

[dsyme]

Great to have this fixed. If it unblocks development of type providers for .NET Core I'll be very very happy

[KevinRansom]

Okay this addresses the stack overflow report. However it moves the cheese down the line a bit:

There are two further issues from this:

1. when building f# with the dotnet/cli f# somehow ... I don't know how picks the dotnet/cli from the path.

c:\temp\repro>c:\microsoft\visualfsharp\Tools\dotnet20\dotnet.exe build /v:normal

   ... a ton of logging elided

         C:\Program Files\dotnet\sdk\2.0.2-vspre-006949\FSharp\RunFsc.cmd -o:obj\Debug\netcoreapp2.0\repro.dll
      

2. ProvidedType definition doesn't implement IReflect.

C:\microsoft\visualfsharp\tests\testbin\release\coreclr\FSC>C:\microsoft\visualfsharp\Tools\dotnet20\dotnet C:\microsoft\visualfsharp\Tools\dotnet20\sdk\2.0.0-preview2-006502\FSharp\fsc.exe @c:\temp\repro\repro.rsp
Microsoft (R) F# Compiler version 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

error FS3033: The type provider 'FSharp.Data.Sql.SqlTypeProvider' reported an error: Unable to cast object of type 'ProviderImplementation.ProvidedTypes.ProvidedTypeDefinition' to type 'System.Reflection.IReflectableType'.

error FS3020: One or more errors seen during provided type setup

[KevinRansom]

1. is a dotnet sdk issue not ours :-)

[KevinRansom]

@dotnet-bot test Windows_NT Release_ci_part2 Build please

[ghost]

@KevinRansom could you confirm if the point 2. that you raised above would be remediated by implementing IReflectableType on https://github.com/fsprojects/FSharp.TypeProviders.StarterPack ?

Edit: Possibly related #1496

[KevinRansom]

@xenocons

I have been looking at it and am kind of confused. It is possible to GetTypeinfo() from a Type despite Type not implementing IReflectableType. So I need to understand why the runtime is failing to do it.

On CoreClr type doesn't implement IReclectableType.

CoreClr type

Anyway ... working on it ...

[dsyme]

I’m trying to work out this business with IReflectableType when implementing F# type providers and running them within the .NET Core hosted compiler

Basically, we need to implement IReflectedType because FSharp.Core uses GetTypeInfo (and possibly for other reasons TBD). But we can’t implement IReflectedType within the .NET Standard or .NET Framework programming models

What’s the right way to go here?

1. Is it expected that it be possible to implement TypeInfo within the .NET Standard programming model? Is this an oversight or by design?

2. Should we be avoiding the use of GetTypeInfo in FSharp.Core.dll? If so we will need to quickly prioritize pushing out a new FSharp.Core suitable for this.

*Observation: We can’t implement IReflectedType

The central purpose of the Type Provider SDK is to provide an implementation of System.Type:

type ProvidedTypeDefinition(…) =
    inherit Type()
    …

However it seems this means we must also implement IReflectableType (See below for why)

abstract GetTypeInfo : unit -> TypeInfo

But AFAICS there is no public or protected way to construct a TypeInfo in either the .NET Standard 2.0 or .NET Framework 4.6.1. programming model. Trying this:

type ProvidedTypeDefinition(…) =
    inherit TypeInfo()

gives

C:\GitHub\dsyme\FSharp.TypeProviders.StarterPack\src\ProvidedTypes.fs(1973,5): error FS0509: Method or object constructor 'TypeInfo' not found 
[C:\GitHub\dsyme\FSharp.TypeProviders.StarterPack\src\FSharp.TypeProviders.SDK.fsproj]

Observation: We need to implement IReflectedType because FSharp.Core uses it

It is possible we could get away with just not implementing IReflectedType, or providing a dummy implementation of it. However, it turns out FSharp.Core uses it, e.g.

Microsoft.FSharp.Core.ReflectionAdapters.Type.GetCustomAttributes(Type this, Type attrTy, Boolean inherits)

Thanks
Don

[KevinRansom]

@dsyme we can't implement IReflectableType because TypeInfo does not have a public constructor. I think this may be a reference assemblies thing. But I am still worrying about p2p refs right now.

[KevinRansom]

@dsyme, @brettfo, @cartermp, @Pilchie

I had another chat with Immo. This is clearly a bug in the framework and it is unclear how it can be fixed. Type extensibility by deriving from Type is incompatible with reflection using TypeInfo.

• The most correct fix would be to make the TypeInfo() constructor protected or public, allowing us to implement a TypeInfo() which would then delegate to the generated type. However, there exists no dotnet runtime anywhere with that public, so it would be somewhat limited in terms of reach.

• What we will do is this ... build the compiler for netcoreapp2.0 and ensuring that we don't use GetTypeInfo(). This will mean that either ...
  we enable build for both netcoreapp2.0 and netcoreapp1.1 or we stop shipping with dotnet/cli 1.1.*

Given that VS now has a dotnet core 2.0 install, I doubt the dotnet core 1.0 release matters much to us now.

Obviously all of the usual Kevin is embarrassed about this excuses spring to the fore here. I don't believe we use GetTypeInfo in FSharp.Core so I think we can stick with a NetStandard 1.6 FSharp.Core.dll.

Nope I was wrong we have to not use GetTypeInfo on generated types and there are a couple of places in FSharp.Core.dll where we use GetTypeInfo, especially in quotations so we may need a netstandard 2.0 fsharp.core.dll too.