Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Oct 9, 2017

This is a substantial set of changes to the ProvidedTypes SDK

  1. We require that all authors of type providers now use a ProvidedTypesContext, e.g.

    let ctxt = ProvidedTypesContext.Create(config, isForGenerated=false)
    ...
    let myType = ctxt.ProvidedTypeDefinition(asm, ns, "MyType", Some typeof<obj>)
    ...
    

    There are no more direct constructors for ProvidedTypeDefinition etc.

  2. ProvidedTypesContext.Create now takes a flag isForGenerated. It should be set to true for generative type providers

  3. IsStaticMethod becomes IsStatic and some other similar naming changes

  4. Direct setters such as prop.GetterCode <- ... are removed in favour of optional parameters. You must specify GetterCode as a parameter

  5. Enables use as part of .NET Core execution of the F# compiler by extending TypeDelegator instead of Type. This needs to be more fully tested but the repo itself now compiles as both .NET Standard 2.0 and .NET Framework, and passes tests as both .NET CoreApp 2.0 and .NET Framework 4.6.1

  6. Puts everything into one file ProvidedTypes.fs and ProvidedTypes.fsi. This file is large but this approach makes it very easy for people writing existing type providers to update (after accounting for changes in the ProvidedTypes API)

The one major thing that's remaining is cross-targeting for generative type providers. A lot of the code restructuring here is to enable us to implement that cleanly and accurately (we basically need an IL binary generator that is not dependent on .NET reflection emit, as discussed elsewhere)

@dsyme
Copy link
Contributor Author

dsyme commented Oct 9, 2017

@7sharp9 This brings cross-targeting generative type providers much closer

@isaacabraham @sergey-tihon All erasing type providers should be upgraded to use this version of the API.

@KevinRansom This does a lot of the ground work necessary to have type providers we can execute when the compiler executes with .NET Core.

@Thorium I'd love to get early feedback on upgrading SQLProvider to use this

@dsyme dsyme merged commit 7b789ca into fsprojects:master Oct 9, 2017
|> (fun arr -> arr.[n - 1])

let adaptMethod = getFastFuncType args resultType

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for GetterCode instead of getterCode here? Actually, I'm a bit confused why a lot of parameters are CamelCase. Did I miss a guideline somewhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm asking because from the changes it seems to be "less" source code breaking with lower case but that could be completely off)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just reverting that

@dsyme
Copy link
Contributor Author

dsyme commented Oct 9, 2017

Example of upgrading a TP that already uses ProvidedTypesContext:

@Thorium
Copy link
Member

Thorium commented Oct 9, 2017

Yes, I think the major issue was, that using named parameters did actually call non-ProvidedTypesContext-setters

ctxt.ProvidedMethod("MyMethod", providerParams, serviceType, someinvoke, IsStaticMethod = true) // Wrong
ctxt.ProvidedMethod("MyMethod", providerParams, serviceType, someinvoke, true) // Correct

I already removed named parameters, but on some level it decreases code readability.

The main problem now is that type providers need sometimes reflection, and using reflection in .NET Standard is hard, because every dll has so many other dependencies which have to be also near.

The answer should be Microsoft.Extensions.DependencyModel's DependencyContext.Default libraries, but
sadly Microsoft.Extensions.DependencyModel 2.0.0 version is having more dependencies to other
libraries, and it's not pure netstandard2.0 version, as the library content is just netstandard1.6 and net451 versions. They do conflict the versions I try to load. So I end-up in a situation where the type provider
should have compile-time ( =runtime for TP) assembly-binding-redirects with AssemblyResolve-event (ReflectionOnlyAssemblyResolve was not enough), and still it's hard to figure out what is missing (as the console output may not be visible).

@isaacabraham
Copy link

isaacabraham commented Oct 9, 2017

Sounds promising! I'll give this a bash against the Azure TP.

@Thorium
Copy link
Member

Thorium commented Oct 9, 2017

I still suspect that named parameters are not working and calling a method like

ctxt.ProvidedMethod("GetDataContext", providerParams, serviceType, invokeCode=invoker)

is wrong because it will call IL constructor for the ProvidedMethod without named parameter and then doing prop.invokeCode <- invoker behind the curtains.

For that reason the parameters shouldn't be optional at all.

@dsyme
Copy link
Contributor Author

dsyme commented Oct 9, 2017

I still suspect that named parameters are not working

They are working. I'm not sure what you're seeing but with that you will call https://github.com/fsprojects/FSharp.TypeProviders.SDK/blob/master/src/ProvidedTypes.fsi#L433 with invokerCode = Some...

@dsyme
Copy link
Contributor Author

dsyme commented Oct 9, 2017

That will pass the value through as an argument here https://github.com/fsprojects/FSharp.TypeProviders.SDK/blob/master/src/ProvidedTypes.fs#L6909

@Thorium
Copy link
Member

Thorium commented Oct 9, 2017

The previous version did allow to rule out some functionality with conditional compilation. The current version enforces to use the new model. The new model doesn't support F# function lambda calls in the expression tree as the old one allowed.

Edit: Example that generates above:

let myThingRuntime = 
   <@@ match myToolsCall param1 with "a" -> 1 | "b" -> 2 | _ -> failwith "unknown" @@>

ctxt.ProvidedMethod("MyMethod", providerParams, serviceType, 
    invokeCode = fun (_:Expr list) -> <@@ MyType(%%myThingRuntime) @@>)

@dsyme
Copy link
Contributor Author

dsyme commented Oct 10, 2017

@Thorium Thanks? Could you provide a full repro for that? We need to fix that.

@dsyme
Copy link
Contributor Author

dsyme commented Oct 10, 2017

The previous version did allow to rule out some functionality with conditional compilation.

Did you have a specific #define in mind? FX_NO_LOCAL_FILESYSTEM?

The current version enforces to use the new model.

Yes - you have to use ProvidedTypeContext. That's required now.

The new model doesn't support F# function lambda calls in the expression tree as the old one allowed.

This is the part I've like a specific repro for

@Thorium
Copy link
Member

Thorium commented Oct 10, 2017

Yes. To reproduce this problem, there is a branch: https://github.com/fsprojects/SQLProvider/tree/providedtypes-expr

SQLProvider has multiple database connections to different databases.
So there is an utility function to fetch runtime connection string from cache.

  • Previous commit uses the old API for this expression, run build.cmd and it works.
  • The last commit uses the new API for this expression, run build.cmd and it fails.

We are using directly this project's files, they are located after a build at:
paket-files\sourcefiles\fsprojects\FSharp.TypeProviders.SDK\src\

It fails on runtime while running test from solution SQLProvider.Tests.sln file QueryTests.fs.
Those can be also debugged with F# interactive.

The error: Exception of type 'System.Collections.Generic.KeyNotFoundException' was thrown. from

gtd.Metadata.Methods.FindByNameAndArity(name, types.Length)
 
|> Array.find (fun md -> eqTypesAndILTypesWithInst types args md.ParameterTypes)

...but the real problem is at recursive expression tree parsing, where it tries to parse different kind of logics to Microsoft.FSharp.Quotations.FSharpExpr.
This time it fails to PrintFormat which comes from source code node "failwithf" but if the quotation would be different, it would fail to something else.

For SQLProvider this was a minor problem, I just excluded those overrides from .NET Standard version as their usage is so minor anyways, and kind of anti-pattern, but on the other hand I would like to maintain the old features for backward-compatibility.

@dsyme
Copy link
Contributor Author

dsyme commented Oct 10, 2017

@Thorium Lovely, thanks for the repro

@dsyme
Copy link
Contributor Author

dsyme commented Oct 10, 2017

@Thorium Tracking the bug with #138

@dsyme
Copy link
Contributor Author

dsyme commented Oct 12, 2017

@Thorium Could you open a pull request to SQLProvider master from https://github.com/fsprojects/SQLProvider/tree/providedtypes-expr please - I will help iterate on the pull request (e.g. updating it with fixed TPSDKs) until it is working and passing all tests, thanks

@dsyme
Copy link
Contributor Author

dsyme commented Oct 12, 2017

@Thorium You might also want to paket-update to the latest TPSDK and see if we get a bit further (I pushed a number of fixes in today)

@Thorium
Copy link
Member

Thorium commented Oct 13, 2017

Working!

@dsyme
Copy link
Contributor Author

dsyme commented Oct 14, 2017

@Thorium That's great to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants