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

StackOverflow with TryBindType - GetType loop #182

Closed
Thorium opened this issue Dec 11, 2017 · 23 comments
Closed

StackOverflow with TryBindType - GetType loop #182

Thorium opened this issue Dec 11, 2017 · 23 comments

Comments

@Thorium
Copy link
Member

Thorium commented Dec 11, 2017

ProvidedTypesContext class has a function of tryGetTypeFromAssembly
That calls asm.GetType fullName |> function null -> None | x -> Some (x, true)
which calls TargetAssembly's method:

        override x.GetType (nm:string) =
            if nm.Contains("+") then
                let i = nm.LastIndexOf("+")
                let enc,nm2 = nm.[0..i-1], nm.[i+1..]
                match x.GetType(enc) with
                | null -> null
                | t -> t.GetNestedType(nm2,bindAll)
            elif nm.Contains(".") then
                let i = nm.LastIndexOf(".")
                let nsp,nm2 = nm.[0..i-1], nm.[i+1..]
                x.TryBindType(USome nsp, nm2) |> Option.toObj
            else
                x.TryBindType(UNone, nm) |> Option.toObj

which calls TryBindType which contains call back to GetType and the loop is ready

        member __.TryBindType(nsp:string uoption, nm:string): Type option =
            match getReader().ILModuleDef.TypeDefs.TryFindByName(nsp, nm) with
            | Some td -> asm.TxILTypeDef None td |> Some
            | None ->
            match getReader().ILModuleDef.ManifestOfAssembly.ExportedTypes.TryFindByName(nsp, nm) with
            | Some tref ->
                match tref.ScopeRef with
                | ILScopeRef.Assembly aref2 ->
                    let ass2opt = tryBindAssembly(aref2)
                    match ass2opt with
                    | Choice1Of2 ass2 -> 
                        match ass2.GetType(joinILTypeName nsp nm)  with 
                        | null -> None
                        | ty -> Some ty
                    | Choice2Of2 _err -> None
                | _ ->
                    printfn "unexpected non-forwarder during binding"
                    None
            | None -> None

GetType "System.Object"
-> TryBindType Some("System") "Object"
-> GetType "System.Object"
-> TryBindType Some("System") "Object"
-> ...

@Thorium
Copy link
Member Author

Thorium commented Dec 11, 2017

This is mostly valid for .NET Standard libraries using type-providers:
It's not clear if the base types come from mscorlib.dll or netstandard.dll.

@Thorium
Copy link
Member Author

Thorium commented Dec 12, 2017

Adding just

if nm.StartsWith("System.") then
    Type.GetType nm
else

to the override x.GetType (nm:string) of ProvidedTypes.fs will get rid of stackoverflow, but the .NET Standard build still fails:

error FS1109: A reference to the type 'System.Numerics.BigInteger' in assembly 'netstandard' was found, but the type could not be found in that assembly [C:\myproj\myproj.fsproj]
FSC : error FS1109: A reference to the type 'System.Numerics.BigInteger' in assembly 'System.Numerics' was found, but the type could not be found in that assembly [C:\myproj\myproj.fsproj]
FSC : error FS1109: A reference to the type 'System.Numerics.BigInteger' in assembly 'netstandard' was found, but the type could not be found in that assembly [C:\myproj\myproj.fsproj]
C:\Users\tuomas\AppData\Local\Temp\.NETStandard,Version=v2.0.AssemblyAttributes.fs(2,34): error FS0039: The type 'Versioning' is not defined. [C:\myproj\myproj.fsproj]
C:\myproj\Program.fs(17,3): error FS0039: The type 'Literal' is not defined.

@Thorium
Copy link
Member Author

Thorium commented Dec 15, 2017

System.Numerics.BigInteger problem is a bit different: fsc.exe compiler tries to evaluate auto-opens of the reference assemblies and FSharp.Core.dll has some references like Microsoft.FSharp.Core.bigint
What is the library for these in .NET Standard? There is none?

@dsyme
Copy link
Contributor

dsyme commented Jan 9, 2018

@Thorium The library is System.Numerics.dll

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

@Thorium Am I correct that you are seeing this bug when using a type provider to generate code for .NET Standard 2.0? We need to add testing for that scenario, thanks

@Thorium
Copy link
Member Author

Thorium commented Jan 10, 2018

Correct.
I added a PR #185 to test that. There are not many changes.
Don't merge the PR (as it will just break unit tests) but check the changes.

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

@Thorium That's not quite what's need to test targeting .NET Standard 2.0 in code generation. I'll work on testing this today, will send a link

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

@Thorium I haven't been able to reproduce this bug. Could you write out step-by-step instructions for reproducing it? I don't want to pull #185 until I understand what's going on

@dsyme dsyme added needs repro and removed bug labels Jan 10, 2018
@Thorium
Copy link
Member Author

Thorium commented Jan 10, 2018

With the latest changes, targeting to .NET Standard, ProvidedTypes.fs dllInfos is null here:

 [ for dllInfo in dllInfos.GetElements() -> (dllInfo.GetProperty("FileName") :?> string)
   if not (isNull baseObj) then
     for dllInfo in baseObj.GetProperty("Value").GetField("dllInfos").GetElements() -> (dllInfo.GetProperty("FileName") :?> string) ]

Now, this can be skipped with adding a corresponding isNull for dllInfos. But if we skip these dlls the compiler will be in the loop of stack-overflow described in this bug. By adding a check in #183 the stackoverflow is avoided.

@Thorium
Copy link
Member Author

Thorium commented Jan 10, 2018

Maybe the problem is that the tcImports is not working correctly on .NET Standard target and trying to fix the side-effects won't help.

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

@Thorium I see. dllInfos should not be null there. I might need more details of exactly which F# compiler you are using and how you are running it

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

@Thorium #194 adds improved diagnostics if dllInfos is null, thanks

@Thorium
Copy link
Member Author

Thorium commented Jan 10, 2018

Getting the latest #194 and trying to build the library using the type provider, compiling with .NET Standard 2.0 still gives:

C:\Program Files\dotnet\sdk\2.1.2\FSharp\Microsoft.FSharp.Targets(263,9): error MSB6006: "fsc.exe" exited with code -1073741571.

And with more diagnostics:

         Microsoft (R) F# Compiler version 4.1
         Copyright (c) Microsoft Corporation. All Rights Reserved.

         Process is terminated due to StackOverflowException.

I have the definition in the fsproj file:

  <PropertyGroup Condition="'$(IsWindows)' == 'true'">
    <FscToolPath>C:\Program Files (x86)\Microsoft SDKs\F#\4.1\Framework\v4.0</FscToolPath>
    <FscToolExe>fsc.exe</FscToolExe>
  </PropertyGroup>

fsc.exe file version is 2017.11.4.1, product version 4.4.1.0
I have cloned the visualfsharp repo so if I try version 2017.12.11.0 (changeset d19fb00f95c183a4d4ed3f45c7ca74b48a485e13) the result is the same.

I expect that again this is the loop mentioned in the title of this bug.

@Thorium
Copy link
Member Author

Thorium commented Jan 10, 2018

Why is ProvidedTypes.fs line override x.GetType (nm:string) = parameter nm getting values with starting "System." (system types) also?

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

Getting the latest #194 and trying to build the library using the type provider, compiling with .NET Standard 2.0 still gives:

When you say "compiling with .NET Standard 2.0" what do you mean? Compiling SqlProvider? Or one of the samples? Or putting netstandard2.0 in FSharp.TypeProviders.SDK.Tests.fsproj? Or something else? Thanks

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

Why is ProvidedTypes.fs line override x.GetType (nm:string) = parameter nm getting values with starting "System." (system types) also?

That is correct for TargetTypeDefinition. The SDK builds a reflection model of types in the target assemblies. Also other assemblies will be interrogated for these types, returning null if they are not present.

@Thorium
Copy link
Member Author

Thorium commented Jan 10, 2018

When you say "compiling with .NET Standard 2.0" what do you mean? Compiling SqlProvider? Or one of the samples? Or putting netstandard2.0 in FSharp.TypeProviders.SDK.Tests.fsproj? Or something else?

I mean I'm compiling a project that is using a type provider. In my case any of SQLProvider samples at https://github.com/fsprojects/SQLProvider/tree/master/tests/SqlProvider.Core.Tests in a slight modification that their target is netstandard2.0: <TargetFrameworks>netcoreapp2.0;netstandard2.0</TargetFrameworks>

The type provider, SQLProvider, itself compiles nicely. But users of that cannot target to nestandard2.0

So I expected that adding netsrandard2.0 target to FSharp.TypeProviders.SDK.Tests.fsproj would also replicate the same problem, if that project is returning anything from any reference-dll like FSharp.Core. The only modification needed is that .NET Standard is a library, not executable, so there shouldn't be EntryPointAttribute in NET Standard compilation.

@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2018

@Thorium Ah I see, thanks. FSharp.TypeProjects.SDK.Tests.fsproj is not actually using a type provider (i.e. as a compiler addin), but rather directly unit-testing the type provider SDK at the level of provided types and their properties. But perhaps now I can repro the problem by compiling SqlProvider.

@Thorium
Copy link
Member Author

Thorium commented Jan 10, 2018

Those tests do use NuGet reference of SQLProvider (as referencing direct files was broken when I started with .NET Core). I just pushed the latest updated ProvidedTypes.fs version as SQLProvider 1.1.26-alpha1 to NuGet.

@dsyme
Copy link
Contributor

dsyme commented Jan 11, 2018

@Thorium I tried to use the following repro steps and still couldn't repro it:

  1. Install MySql with default settings
  2. git clone https://github.com/dsyme/SQLProvider
  3. msbuild src\SQLProvider\SqlProvider.fsproj
  4. cd C:\GitHub\dsyme\SQLProvider\tests\SqlProvider.Core.Tests\MsSql
  5. Use the freshly-built SQLProvider <Reference Include="FSharp.Data.SqlProvider"><HintPath>../../../bin/net451/FSharp.Data.SqlProvider.dll</HintPath> </Reference> rather than the one from the package
  6. Use a couple of modifications to use a default database that got installed with MySql, notably the code below
  7. .\build.cmd
  8. change to <TargetFramework>netstandard2.0</TargetFramework> in tests\SqlProvider.Core.Tests\MySql\SqlProvider.Core.Tests.fsproj
  9. .\build.cmd

All commands succeeded, when I look at the generated DLL it has generated .NET Standard 2.0 code successfully. This is with the latest TPSDK (which the project uses by default). What am I missing?

image

Thanks!
don

Here's the code adjustment to use the default database:

let connStr =  "Server=localhost;Database=sakila;Uid=root;Pwd=PASSWORD;Convert Zero Datetime=true;"

type HR = SqlDataProvider<Common.DatabaseProviderTypes.MYSQL, connStr,  ResolutionPath = msyqlConnectorPath>

...
            for emp in ctx.Sakila.Staff do
...

@Thorium
Copy link
Member Author

Thorium commented Jan 12, 2018

Hi,
Ok... So the problem was: I used the NuGet package which detected that if my target is .NET Standard 2.0 library, the reference is also netstandard2.0 version rather than net451 version. It seems that you used the net451 compilation of SQLProfiler (src\SQLProvider\SqlProvider.fsproj) rather than the netstandard2.0 (\src\SQLProvider.Standard\SQLProvider.Standard.fsproj), which seems reasonable, now that you again pointed it out, as I know the fsc.exe used by dotnet.exe is the .Net framework version.

Now my question is (documentation related, and maybe stupid, but keep in mind I don't know the details of .NET Core compilation well, I'm just a user):
Will we ever need bin\netstandard2.0\FSharp.Data.SqlProvider.dll (e.g. on runtime on .NET Standard library) or is \bin\net451\FSharp.Data.SqlProvider.dll always enough? In other words: does the fsharp compilation totally embed the erasing type provider library to the compiled dll output, or will it have external references still to the type provider dll?

@dsyme
Copy link
Contributor

dsyme commented Jan 12, 2018

@Thorium Great, thanks for letting me know!

Will we ever need bin\netstandard2.0\FSharp.Data.SqlProvider.dll (e.g. on runtime on .NET Standard library) or is \bin\net451\FSharp.Data.SqlProvider.dll always enough?

Looking at the way your type provider works, you will indeed need bin\netstandard2.0\FSharp.Data.SqlProvider.dll at runtime when people are generating code for .NET Standard 2.0.

For now I think the nuget package should look like this:

lib\net451\FSharp.Data.SqlProvider.dll

lib\netstandard2.0\FSharp.Data.SqlProvider.dll
lib\netstandard2.0\netstandard.dll
lib\netstandard2.0\System.Console.dll
lib\netstandard2.0\System.IO.dll
lib\netstandard2.0\System.Data.SqlClient.dll
lib\netstandard2.0\System.Reflection.dll
lib\netstandard2.0\System.Runtime.dll

Eventually you will need to also make some changes to use the type provider in conjunction with the F# compiler that comes in the .NET SDK (which runs on .NET Core, as opposed to the one you get via setting FscToolPath here). For now don't worry about it though - the F# compiler in the .NET SDK has not yet been updated and there are still ongoing discussions about how exactly a type provider will be packaged to work with both the C:\Program Files (x86)\Microsoft SDKs\F#\4.1\Framework\v4.0 compiler and the .NET SDK compiler simultaneously. For now people will just continue to have to set FscToolPath, though they should now be able to correctly generate .NET Standard 2.0 code.

In other words: does the fsharp compilation totally embed the erasing type provider library to the compiled dll output, or will it have external references still to the type provider dll?

In your cases it has external references. In TP jargon you have your TPRTC and TPDTC in one combined DLL.

@dsyme
Copy link
Contributor

dsyme commented Jan 12, 2018

(@Thorium note I updated the comment above, my first version was inaccurate)

BTW I believe that in the long run you won't need the .NET 4.5.1 version of the type provider at all. The only place I can see where the netstandard2.0 and net451 TPDTCs vary is here:


#if NETSTANDARD
        if conStringName <> "" then
            failwith "ConnectionStringName not supported in .NET Standard. Use a development connection string and then give a runtime connection string as parameter variable to GetDataContext"
#endif

But I suspect this can just be deleted - and that ConnectionStringName might work correctly at design-time and run-time. That would simplify matters - you would then just have a netstandard2.0 TPDTC+TPRTC capable of generating code for either .NET 4.5.1 or .NET Standard 2.0

For now the .NET 4.5.1 TPDTC is still needed for backwards compat, since .NET Standard 2.0 is not supported pre .NET 4.7

@dsyme dsyme closed this as completed Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants