fsprojects / FSharp.TypeProviders.SDK

The SDK for creating F# type providers
https://fsprojects.github.io/FSharp.TypeProviders.SDK/
MIT License
300 stars 94 forks source link

Erasing TP, Lazy types evaluated too early? #164

Closed Thorium closed 7 years ago

Thorium commented 7 years ago

Some of the type provider types (ProvidedTypeDefinition) can be generated lazily on-the-fly in the get .AddMembersDelayed(fun () -> (e.g. scenario of any large datasource)

A problem with the latest ProvidedTypes.fs (at least Erasing TP):

FSharp.Compiler.Private calls very early TypeProvider.fs method: override this.GetMethods bindingFlags

...which calls getMembers() which will cause this clause to execute:

Some ((fun () -> x.GetMembersInternal() |> Array.map (convMemberDefToTgt xT)) ...and roll through... (why it needs this internal? Will there ever be all?) goes:

        and convType toTgt (t:Type) =
            let table = (if toTgt then typeTableFwd else typeTableBwd)
            match table.TryGetValue(t) with

Now, TryGetValue uses object-equality-comparison which is using a definition of the hashcode for a ProvidedTypeDefinition:

override x.GetHashCode() = x.Namespace.GetHashCode() ^^^ className.GetHashCode()

But this is a lazy type and erased type, the namespace is not there!

        override __.Namespace = 
            match container with
            | TypeContainer.Namespace (_,nsp) -> nsp
            | TypeContainer.Type t           -> t.Namespace
            | TypeContainer.TypeToBeDecided -> failwithf "type '%s' was not added as a member to a declaring type" className

I could try to move all the dynamic types to the root namespace (that would solve resolving namespaces), but that would make using the typeprovider quite painful.

Similar problem is convProvidedTypeDefToTgt which calls isNested which calls DeclaringType which is not there.

Thorium commented 7 years ago

Usage code is basically this (pseudo):

let generateMethod (container:ProvidedTypeDefinition) 
    let rt = ProvidedTypeDefinition("subItem1", None, true)
    let resultType = ProvidedTypeDefinition("subItem2",None, true)
    let empty = fun (_:Expr list) -> <@@ () @@>
    resultType.AddMember(ProvidedConstructor([ProvidedParameter("context", typeof<IMyType>)], empty))
    rt.AddMember resultType
    container.AddMember(rt)
    ProvidedProperty("subItem", resultType, getterCode = (fun args -> <@@ ((%%args.[0] : obj) :?>IMyType) @@>) ) 

let typ = ProvidedTypeDefinition("MyBaseType", None, true)
[1 .. 5] |> Seq.iter(fun _ -> typ.AddMembersDelayed(fun () -> generateMethod typ))
Thorium commented 7 years ago

This maybe due to going through the whole TranslateQuotationToCode procedure even if I just would want an empty type to help categorising type hierarchy for a developer, so not actual execution....

Is there a way to skip the invokeCode totally?

Thorium commented 7 years ago

Another thing that did work before these SwaggerProvider commits, gives me now The type 'myType' does not have 'null' as a proper value. So has some default value generation modified from a class to a struct, and how can I go back to class, or what is happening there?

Thorium commented 7 years ago

To summarize:

No problems: e0436b11faed9e7938b3c90d874a535f94311b87
Problems: 27232ac95fa6f91f136e01b23f38dda34d0f3dc4 
Still problems: e75f056b5a675d04445649d872aa020439a6442a

For now, I think I'll revert, that should solve it.

dsyme commented 7 years ago

@Thorium Thanks for the detailed analysis - it will help find this problem

Thorium commented 7 years ago

No panic on this, I went back to e0436b1... for now. SQLProvider seems to have so many users that we get the feedback fast even if the unit test coverage is not perfect (although increasing).

Thorium commented 7 years ago

I can provide a repo also: https://github.com/fsprojects/SQLProvider/tree/tp-sdk-problem Steps to reproduce:

  1. Clone the repo https://github.com/fsprojects/SQLProvider and git checkout tp-sdk-problem
  2. Run build.cmd to restore package. It will fail to unit tests...
  3. Open in Visual Studio: SQLProvider.sln and build. Works.
  4. Problem 1: From Solution Explorer, open file doc\core\crud.fsx and observe that isNull foundEmployee is underlined by compiler, saying it cannot be null. But it can, as there is no searched ID in the database. You can run the file in FSI and observe that foundEmployee is null.
  5. Problem 2: Then open SQLProvider.Tests.sln and try to build, fails. Reason: The last test of QueryTests.fs "'Result' was not added as a member to a declaring type". This comes from the SQLProvider.sln file SqlDesignTime.fs lines 291-297 and 312-314. Did work earlier and I cannot figure out what is not ok.

I tried also different combinations of IsTypeInitializer and IsImplicitConstructor but it didn't help. But they did affect to the performance characteristics a bit. But performance optimization is not target for this issue.

ProvidedTypes.fs is the latest and located at paket-files\sourcefiles\fsprojects\FSharp.TypeProviders.SDK\src

dsyme commented 7 years ago

@Thorium I haven't yet checked your exact repro, but from looking at your description I believe the PR above will fix your problem

dsyme commented 7 years ago

Problem 1: From Solution Explorer, open file doc\core\crud.fsx and observe that isNull foundEmployee is underlined by compiler, saying it cannot be null. But it can, as there is no searched ID in the database. You can run the file in FSI and observe that foundEmployee is null.

@Thorium I'm not sure the PR solves this problem, could you check? Thanks

Thorium commented 7 years ago
dsyme commented 7 years ago

Problem 1: From Solution Explorer, open file doc\core\crud.fsx and observe that isNull foundEmployee is underlined by compiler, saying it cannot be null. But it can, as there is no searched ID in the database. You can run the file in FSI and observe that foundEmployee is null.

I don't understand this problem yet. It's very odd, since you never set nonNullable=true for any provided types. I've added some extra testing to the TPSDK and there doesn't seem to be an error here.

Could you describe to me the actual provided type that appears to get non-nullable behaviour and perhaps point me to the line of source code in the TP where it is specified with a ProvidedTypeDefinition constructor? thanks

dsyme commented 7 years ago

@Thorium Actually I think this is by design. You would need the AllowNullLiteral(true) attribute on type Employee2:

type Employee2 = {
    Id:int
    FirstName:string
    LastName:string
}
dsyme commented 7 years ago

That's assuming the offending line is

    if not (isNull foundEmployee) then
dsyme commented 7 years ago

I'll close this as Problem 2 has been resolved - please reopen a new issue if you disagree about problem 1 - thanks!

Thorium commented 7 years ago

That's fine, problem 2 is probably my error. There seems to be quite many properties already in this SDK. A little documentation to e.g. these github-site wiki-pages would be nice... :-)

dsyme commented 7 years ago

@Thorium You mean problem 1?

Doc issue covered by https://github.com/fsprojects/FSharp.TypeProviders.SDK/issues/170

Thorium commented 7 years ago

Yes, sorry, I mean one. This is ok now, thanks.

Thorium commented 7 years ago

I tried to verify this but couldn't, there was so many moving parts. type Employee2 is not used at all in the query: the IQueryable returns type of TP generated entity. When I tried to compare the generated type with the old and new TP, they did look the same to me. No difference in attributes.

I don't know if there is an easy way to add AllowNullLiteralAttribute to the TP generated type, and do I even want that.

Maybe the current TP SDK does one boxing less or something...? Anyway, I don't know if this is a problem or not. It seems that isNull operator comes from FSharp core operators, so maybe VS2017 fsx file and fsi-interactive loads a different core?

dsyme commented 7 years ago

@Thorium You are right, I don't know what I was thinking. Relevant line of code is here https://github.com/fsprojects/SQLProvider/blob/tp-sdk-problem/docs/content/core/crud.fsx#L140. In that case I don't know what the problem is

You should not need to add AllowNullLiteralAttribute in any form - allowing nulls is the default for provided types

dsyme commented 7 years ago

@Thorium I've recorded the problem as a separate issue here: https://github.com/fsprojects/FSharp.TypeProviders.SDK/issues/172