fsprojects / FSharp.TypeProviders.SDK

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

Added support for try-with and try-finally (fixes #241). #242

Closed 71 closed 5 years ago

71 commented 6 years ago

This adds support for Expr.TryFinally and Expr.TryWith, as well as support for exception blocks in the ILGenerator.

The following provider was used to test the try blocks:

namespace FSharp.TypeProviders.SDK.Testing

open ProviderImplementation.ProvidedTypes
open FSharp.Core.CompilerServices

open System
open System.Reflection

[<TypeProvider>]
type ComboErasingProvider(config : TypeProviderConfig) as this =
    inherit TypeProviderForNamespaces(config)

    let ns = "Tests"
    let asm = Assembly.GetExecutingAssembly()

    let provider = ProvidedTypeDefinition(asm, ns, "Provider", Some typeof<obj>, hideObjectMethods = true)

    do provider.DefineStaticParameters([ProvidedStaticParameter("Host", typeof<string>)], fun name _ ->
        let provided = ProvidedTypeDefinition(asm, ns, name, Some typeof<obj>, hideObjectMethods = true)

        let fn = ProvidedMethod("Test", [ ProvidedParameter("disp", typeof<IDisposable>) ], typeof<System.Void>, fun [ arg ] ->
            <@@
                use __ = (%%arg : IDisposable)

                try
                    failwith "This will throw anyway, don't mind it."

                    printfn "[-] Should not get here."
                finally
                    printfn "[+] Caught try-finally, nice."

                    try
                        failwith "It failed again."

                        printf "[-] Should not get here."
                    with
                    | _ ->
                        printfn "[+] Caught try-with, nice."

                    try
                        printfn "[?] Gonna go to finally without throwing..."
                    finally
                        printfn "[+] Yup, it worked totally."
            @@>
        , isStatic = true)

        provided.AddMember fn
        provided
    )

    do this.AddNamespace(ns, [provider])

[<assembly: TypeProviderAssembly>]
do ()

The following script was used to test the provider:

#r "bin/Debug/netstandard2.0/FSharp.TypeProviders.SDK.Testing.dll"
open System

type TestType = Tests.Provider<"Doesn't matter">

let notifyOnDeath = { new IDisposable with
    member __.Dispose() = printfn "[+] Disposable was killed in 'use' block."
}

TestType.Test(notifyOnDeath)

Output:

[+] Caught try-finally, nice.
[+] Caught try-with, nice.
[?] Gonna go to finally without throwing...
[+] Yup, it worked totally.
[+] Disposable was killed in 'use' block.
System.Exception: This will throw anyway, don't mind it.
   at <StartupCode$FSI_0001>.$FSI_0001.main@() in D:\Media\OneDrive\Code\FSharp.TypeProviders.SDK.Testing\Test.fsx:line 10
Stopped due to error

Edit: Two questions:

  1. Should I add the tests somewhere in this repository? I haven't seen much tests in the codebase which is why I used a separate project to test my stuff.
  2. My tests don't cover every possible case, do you want me to take care of that?
dsyme commented 6 years ago

@6A Great work! Yes, add a test please, I think in https://github.com/fsprojects/FSharp.TypeProviders.SDK/blob/master/tests/BasicGenerativeProvisionTests.fs

My tests don't cover every possible case, do you want me to take care of that?

As comprehensive as possible please, always better.

71 commented 6 years ago

I just had a look at BasicGenerativeProvisionTests.fs, and it looks like they mostly test for support of specific platforms. There are no tests that ensure the code generation is correct, which is what the snippet I included above does.

To me, the best thing we could do for testing, is add more tests that actually use the provider (sort of like we have in examples, but strictly for testing. Are you ok with me setting up a separate project for this, or will it get refused no matter what?

For example, a test about a type provider could contain a Test.Name.fsproj with a single file Test.Name.fs, and finally a Test.Name.fsx script that can be ran and ensures everything works.

We can even use MSBuild and after-build targets to run the scripts automatically after compilation in order to ensure everything works fine.

Edit: GenerativeEnumsProvisionTests does something closer to what I had imagined, but there is still a lot of boilerplate every time a provider must be invoked. I think we should at least make a function, ie makeTypeProvider, which could be used in the following way:

[<Fact>]
let ``some type provider works``() =
    let provider = makeTypeProvider <| fun config -> MyTypeProvider config
    let providedType = provider.Namespaces.[0].GetTypes().[0] // could make a util function for this too
    let typ = provider.ApplyStaticArguments(providedType, "TypeName", [| ... |])

    let asm = buildAssembly provider type.Assembly // returns an Assembly

    // do your tests
    Assert.NotEqual(asm, null)

Update: I can't get any test loading. For some reason, IDisposable and ArgumentException won't load when using an actual type provider for tests (it cannot find System.Private.CoreLib), and executing tests like the GenerativeEnumsProvisionTests does not work either, with the Container type not appearing in the target assembly.

7sharp9 commented 6 years ago

Also note the current tests are not instantiating the correct type providers.

GenerativePropertyProviderWithStaticParams is used in bothGenerativeProviderWithRecursiveReferencesToGeneratedTypes and GenerativeProviderWithRecursiveReferencesToGeneratedTypes when it should be using the GenerativeProviderWithRecursiveReferencesToGeneratedTypes type provider.

Theres a PR to fix this here also.

71 commented 6 years ago

Alright, I created a repository that specifically tests for the TryWith and TryFinally expressions, and I think this should be merged without adding tests to the main repo (since no other expression is particularly tested here, it would be strange to have tests only for this PR).

dsyme commented 6 years ago

@6A Sorry for the delay, I've been on vacation

Alright, I created a repository that specifically tests for the TryWith and TryFinally expressions, and I think this should be merged without adding tests to the main repo (since no other expression is particularly tested here, it would be strange to have tests only for this PR).

We do need to add some kind of testing with this PR, even if it means it's the first testing of expressions

71 commented 6 years ago

No problem.

If I do add tests for expressions, are you fine with me adding a new project similar to the one I linked above? The existing tests focus on the correctness of the inner workings of the ProvidedTypes module, rather than on the correctness of the generated code, and if more tests like mine are to be added in the future, I think it would be easier to set them up like I did.

It's a personal opinion though, so if you believe it is better to keep going as is, then I'll try to get new tests working in the existing test project.

dsyme commented 6 years ago

If I do add tests for expressions, are you fine with me adding a new project similar to the one I linked above? The existing tests focus on the correctness of the inner workings of the ProvidedTypes module, rather than on the correctness of the generated code, and if more tests like mine are to be added in the future, I think it would be easier to set them up like I did.

Yes, adding an additional test project like the one above would be ok, thanks!

71 commented 6 years ago

Looks like the tests cannot run, probably because of #244. I don't really know what to do at this point. I tried a few different fixes I found, but none worked (using dotnet 2.1.302 with both net461 and netcoreapp2.0).

dsyme commented 6 years ago

@6A I'll look now, thanks

dsyme commented 6 years ago

@6A I've cherry-picked the support for try/finally into https://github.com/fsprojects/FSharp.TypeProviders.SDK/pull/258 since I hit the need while working on "for" (which has an implicit try/finally)

I've also included some fixes regarding labels (needed to use DefineLabel/MarkLabel) so there will be conflicts when we integrate back here.

I've not included your testing yet: we need to sort through that, but I have added a test that generates code (as part of a "for") though doesn't actually run it.

71 commented 5 years ago

Alright, I'm glad it got merged one way or another! I'll keep an eye out for updates.

dsyme commented 5 years ago

@6A I lifted your tests in #259 and moved them into the aptly named "StressProvider". It's under "examples" but effectively is a test