Skip to content

F# 3.1 / Profile 259: <@ System.Exception() @> causes AmbiguousMatchException at runtime #68

Closed

Description

@stephen-swensen

A Universal App or unit test executing code like <@ System.Exception() @> in an F# 3.1 / Profile 259 portable class library will result in an AmbiguousMatchException at runtime. Stacktrace looks something like

at Microsoft.FSharp.Core.ReflectionAdapters.commit[a](a[] results)
at Microsoft.FSharp.Core.ReflectionAdapters.Type.GetConstructor(Type this, Type[] parameterTypes)
at Microsoft.FSharp.Quotations.PatternsModule.bindCtor(Type tc, FSharpFunc2 argTypes, FSharpList1 tyargs)
at Microsoft.FSharp.Quotations.PatternsModule.u_constSpec@1424-40.Invoke(FSharpList1 tyargs) at Microsoft.FSharp.Quotations.PatternsModule.u_Expr@1305-2.Invoke(BindingEnv env) at Microsoft.FSharp.Quotations.PatternsModule.u_Expr@1327-7.Invoke(BindingEnv env) at Microsoft.FSharp.Quotations.PatternsModule.deserialize(Type localAssembly, FSharpList1 types, FSharpList1 splices, Byte[] bytes) at Microsoft.FSharp.Quotations.FSharpExpr.Deserialize(Type qualifyingType, FSharpList1 spliceTypes, FSharpList`1 spliceExprs, Byte[] bytes)
at ...

Activity

  1. dsyme commented on Jan 20, 2015

    @dsyme
    Contributor

    Hi @stephen-swensen - do you know why this is? Here is the code for ReflectionAdapters - any idea why multiple constructors are being returned here?

    I suppose the problem goes away when you add a binding redirect for FSharp.Core from the profile 259 FSharp.Core to 4.3.1.0.

    It would be good to know if/why our existing unit tests don't catch this when run on profile259.

    let commit (results : _[]) = 
        match results with
        | [||] -> null
        | [| m |] -> m
        | _ -> raise (AmbiguousMatchException())
    

    ...

        member this.GetConstructor(parameterTypes : Type[]) = 
            this.GetTypeInfo().DeclaredConstructors
            |> Seq.filter (fun ci ->
                let parameters = ci.GetParameters()
                (parameters.Length = parameterTypes.Length) &&
                (parameterTypes, parameters) ||> Array.forall2 (fun ty pi -> pi.ParameterType.Equals ty) 
            )
            |> Seq.toArray
            |> commit
    
  2. dsyme commented on Jan 20, 2015

    @dsyme
    Contributor

    @stephen-swensen - please attach a link to repro solution as well, thanks.

  3. stephen-swensen commented on Jan 20, 2015

    @stephen-swensen
    ContributorAuthor

    Hi @dsyme -

    I created a repo solution at http://swensen.googlecode.com/svn/AmbiguousMatchRepo/. Run the Repo.repo xUnit test using VS2013 Test Explorer (I am using xUnit pre-release NuGet packages that support testing portable profiles and Test Explorer integration).

    I did some further digging: evidently System.Exception in profile 259 has a static no-arg class constructor in addition to the expected instance no-arg constructor whereas the full .net 45 only has the instance no-arg constructor.

    In the following screen-shot you can see the two ConstructorInfo matches being returned: .ctor() and .cctor() where the later is the unexpected static no-arg class constructor.

    why-debug

    I think the fix for this issue is just to filter out static class constructors.

    As to why your unit tests don't catch this issue, possibly you simply don't have a test case that covers this scenario, or else possibly your test runner is not actually bound to the profile 259 framework.

  4. stephen-swensen commented on Jan 21, 2015

    @stephen-swensen
    ContributorAuthor

    I'd be willing to attempt a pull-request fixing this bug if that sounds alright with you.

  5. dsyme commented on Jan 21, 2015

    @dsyme
    Contributor

    Ah!!! Yes, that's the problem. Great work to find the root cause.

    Re PR - yes please, that would be great. Let us know if you need a hand. You can just add this exact quotation as a unit test here: https://github.com/Microsoft/visualfsharp/blob/fsharp4/src/fsharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Quotations/FSharpQuotations.fs. (We will do a separate check to see if we are running the unit tests bound to the profile259 framework - you don't have to fix that if we're not)

  6. latkin commented on Jan 21, 2015

    @latkin
    Contributor

    I am fairly confident that the unit tests (and the core portable tests) bind to the correct portable runtimes. This is indeed fraught with peril, but it seems stable for now. There is similar danger of tests accidentally binding to shipped F# bits on your machine, rather than private-built open source bits, but again, I believe I got this all rooted out.

    e.g. running 'coreunitportable259' with fusion logging on reveals proper binding:

    image

    Similarly, logging all ReadFile operations on anything called FSharp.Core.dll using ProcMon shows that the right guy is being used:

    image

  7. brodyberg commented on Jan 21, 2015

    @brodyberg

    @latkin This is great - something that might be a useful extension to our bug filing requirements/background information and/or repro.

  8. dsyme commented on Jan 29, 2015

    @dsyme
    Contributor

    @stephen-swensen - any progress on a PR for this? Thanks

  9. stephen-swensen commented on Jan 29, 2015

    @stephen-swensen
    ContributorAuthor

    @dsyme per the guidelines at https://github.com/Microsoft/visualfsharp/blob/fsharp4/CONTRIBUTING.md, I am currently in the process of seeking appropriate signatures from my employer for the CLA so that I can contribute. I hope to know the outcome within a week or so, but no definite timeline. Once that is all sorted out, I will get started on the PR right away.

  10. dsyme commented on Jan 29, 2015

    @dsyme
    Contributor

    OK, thanks!

  11. stephen-swensen commented on Feb 5, 2015

    @stephen-swensen
    ContributorAuthor

    @dsyme good news - both myself and my employer have signed the CLA so I am now free to contribute! I plan to submit a pull request for this issue some time next week, thanks.

  12. dsyme commented on Feb 5, 2015

    @dsyme
    Contributor

    Great news! :)

  13. added a commit that references this issue on Feb 16, 2015
    df0c8c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions