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

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

Closed
wants to merge 2 commits into from

Conversation

71
Copy link
Contributor

@71 71 commented Jul 27, 2018

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
Copy link
Contributor

dsyme commented Jul 30, 2018

@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
Copy link
Contributor Author

71 commented Jul 30, 2018

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
Copy link
Member

7sharp9 commented Aug 2, 2018

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
Copy link
Contributor Author

71 commented Aug 3, 2018

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
Copy link
Contributor

dsyme commented Aug 23, 2018

@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
Copy link
Contributor Author

71 commented Aug 23, 2018

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
Copy link
Contributor

dsyme commented Sep 6, 2018

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
Copy link
Contributor Author

71 commented Sep 7, 2018

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
Copy link
Contributor

dsyme commented Sep 10, 2018

@6a I'll look now, thanks

@dsyme
Copy link
Contributor

dsyme commented Sep 11, 2018

@6a I've cherry-picked the support for try/finally into #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
Copy link
Contributor Author

71 commented Sep 11, 2018

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

@dsyme
Copy link
Contributor

dsyme commented Sep 12, 2018

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

@dsyme dsyme closed this Sep 12, 2018
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.

3 participants