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

[WIP, RFC FS-1043] Extension members visible to trait constraints #6286

Open
wants to merge 64 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@dsyme
Copy link
Contributor

dsyme commented Feb 26, 2019

RFC https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1043-extension-members-for-operators-and-srtp-constraints.md

This is work by @TobyShaw and myself to implement RFC FS-1043. This PR brings #3582 up-to-date with master

TODO items from the code

  • TastOps.fs extSlns // TODO: do we need to remap here???
  • TastPickle.fs extSlns starts empty. TODO: check the ramifications of this when inlining solved trait calls from other assemblies
  • Optimizer.fs // TODO: consider what happens when the expression refers to extSlns that have become hidden
  • Check use of None for TraitFreshener, e.g. https://github.com/Microsoft/visualfsharp/pull/3582/files#diff-5b9ab9dd9d7133aaf23add1048742031R576
  • ConstraintSolver.fs // TODO: check the use of 'allPairs' - not all these extensions apply to each type variable.

Things to test

  • explicit instantiations of all the different possible generic things
  • Using C#-style extension members defined in F# to satisfy constraints
  • More uses of extension constraints

Things from previous PR

Lab User and others added some commits Aug 17, 2017

dsyme dsyme
dsyme dsyme
dsyme dsyme
dsyme dsyme
dsyme dsyme
dsyme dsyme

dsyme added some commits Mar 11, 2019

@dsyme dsyme changed the title [WIP, FS-1043] Extension members visible to trait constraints [WIP, RFC FS-1043] Extension members visible to trait constraints Mar 12, 2019

@alfonsogarciacaro

This comment has been minimized.

Copy link
Contributor

alfonsogarciacaro commented Mar 14, 2019

How would this work with the Expression Tree? Right now the TraitCall pattern only gives you the method name and little information else (like whether it is an instance or a static member). Fable has to resolve the exact overload by itself, for example, which can be tricky sometimes. I'm not sure we could locate an extension member if the TraitCall doesn't gives you a reference to it.

@dsyme

This comment has been minimized.

Copy link
Contributor Author

dsyme commented Mar 14, 2019

How would this work with the Expression Tree? Right now the TraitCall pattern only gives you the method name and little information else (like whether it is an instance or a static member). Fable has to resolve the exact overload by itself, for example, which can be tricky sometimes. I'm not sure we could locate an extension member if the TraitCall doesn't gives you a reference to it.

@alfonsogarciacaro It is critical that we update the TAST (and the FCS API) to include solutions (witnesses) for trait constraints. This has long been a flaw in the TAST and our code generation, leading to horrors where we rediscover the solution during code generation and likewise where we rediscover solutions using reflection in FSHarp.Core.

I need to finally address this problem and record solutions to trait constraints in the TAST itself, and possibly pass them as witnesses in the underlying generic code used for reflection calls and quotation evaluation. This is very tricky and awkward as we can't break compat, I need to prototype carefully. Much of the compiler cleanup and documentation I've been doing has been preparing to storm this particular castle, or die doing it.

@realvictorprm

This comment has been minimized.

Copy link
Contributor

realvictorprm commented Mar 14, 2019

@dsyme @cartermp can we discuss that point about easy accessability to experimental compiler features?

@kevmal

This comment has been minimized.

Copy link

kevmal commented Mar 14, 2019

testfiles\test.fs does not compile:

> .\artifacts\bin\fsc\Release\net46\fsc.exe .\testfiles\test.fs
Microsoft (R) F# Compiler version 10.2.3 for F# 4.5
Copyright (c) Microsoft Corporation. All Rights Reserved.

testfiles\test.fs(11,20): error FS0043: The type 'int' does not support the operator '++'

testfiles\test.fs(32,20): error FS0043: The type 'int' does not support the operator '++'

testfiles\test.fs(40,20): error FS0043: The type 'int' does not support the operator '++'

testfiles\test.fs(80,23): error FS0001: The type 'MyType' does not support the operator '+'

testfiles\test.fs(80,21): error FS0043: The type 'MyType' does not support the operator '+'

....
@dsyme

This comment has been minimized.

Copy link
Contributor Author

dsyme commented Mar 14, 2019

@dsyme @cartermp can we discuss that point about easy accessability to experimental compiler features?

My reply was "In this case, compile the branch and use it" :)

(We could also make the links to the VSIX artifacts from the CI builds more apparent)

@kevmal

This comment has been minimized.

Copy link

kevmal commented Mar 15, 2019

@dsyme Not sure what the best way is to help with this one. So I'll just outline what I found here:

In ConstraintSolver.fs GetRelevantExtensionMethodsForTrait is never called. Looks like GetRelevantMethodsForTrait should be

/// Only consider overload resolution if canonicalizing or all the types are now nominal. 
/// That is, don't perform resolution if more nominal information may influence the set of available overloads 
and GetRelevantMethodsForTrait (csenv:ConstraintSolverEnv) permitWeakResolution nm (TTrait(tys, _, memFlags, argtys, rty, soln, extSlns, ad) as traitInfo) : MethInfo list =
    let results = 
        if permitWeakResolution || MemberConstraintSupportIsReadyForDeterminingOverloads csenv traitInfo then
            let m = csenv.m
            let minfos = 
                match memFlags.MemberKind with
                | MemberKind.Constructor ->
                    tys |> List.map (GetIntrinsicConstructorInfosOfType csenv.SolverState.InfoReader m)
                | _ ->
                    tys |> List.map (GetIntrinsicMethInfosOfType csenv.SolverState.InfoReader (Some nm, AccessibleFromSomeFSharpCode, AllowMultiIntfInstantiations.Yes) IgnoreOverrides m) 

            // Merge the sets so we don't get the same minfo from each side 
            // We merge based on whether minfos use identical metadata or not. 
            let minfos = List.reduce (ListSet.unionFavourLeft MethInfo.MethInfosUseIdenticalDefinitions) minfos
            
            // Get the extension method that may be relevant to solving the constraint as MethInfo objects.
            // Extension members are not used when canonicalizing prior to generalization (permitWeakResolution=true)
            let extMInfos = 
                if MemberConstraintSupportIsReadyForDeterminingOverloads csenv traitInfo then 
                    GetRelevantExtensionMethodsForTrait csenv.m csenv.amap traitInfo
                else []

            let extMInfos = extMInfos |> ListSet.setify MethInfo.MethInfosUseIdenticalDefinitions 

            let minfos = minfos @ extMInfos

            /// Check that the available members aren't hiding a member from the parent (depth 1 only)
            let relevantMinfos = minfos |> List.filter(fun minfo -> not minfo.IsDispatchSlot && not minfo.IsVirtual && minfo.IsInstance)
            minfos
            |> List.filter(fun minfo1 ->
                not(minfo1.IsDispatchSlot && 
                    relevantMinfos
                    |> List.exists (fun minfo2 -> MethInfosEquivByNameAndSig EraseAll true csenv.g csenv.amap m minfo2 minfo1)))
        else 
            []

    // The trait name "op_Explicit" also covers "op_Implicit", so look for that one too.
    if nm = "op_Explicit" then 
        results @ GetRelevantMethodsForTrait csenv permitWeakResolution "op_Implicit" (TTrait(tys, "op_Implicit", memFlags, argtys, rty, soln, extSlns, ad))
    else
        results 

With this change the testfile now has three errors:

testfiles\test.fs(66,63): error FS0001: Type constraint mismatch. The type
    'int'
is not compatible with type
    'MyType'


testfiles\test.fs(67,63): error FS0001: Type constraint mismatch. The type
    'int'
is not compatible with type
    'MyType'


testfiles\test.fs(68,63): error FS0001: Type constraint mismatch. The type
    'int'
is not compatible with type
    'MyType'
@kevmal

This comment has been minimized.

Copy link

kevmal commented Mar 15, 2019

The three remaining errors can be reproduced by

type MyType =
    | MyType of int
/// Locally extending an F# type with a wide range of standard operators
module FSharpTypeWithExtrinsicOperators = 

    [<AutoOpen>]
    module Extensions = 
        type MyType with
            static member (|||)(MyType (x : int), MyType (y : int)) = MyType (x ||| y)
            static member (&&&)(MyType x, MyType y) = MyType (x &&& y)
            static member (^^^)(MyType x, MyType y) = MyType (x ^^^ y)

Here, SolveMemberConstraint matches on minfos, tys, memFlags.IsInstance, nm, argtys and though some branches have a wildcard match on minfos (for example | _, _, false, ("op_Addition" | "op_Subtraction" | "op_Modulus"), [argty1;argty2]) the branch of interest matches on minfos being empty | [], _, false, ("op_BitwiseAnd" | "op_BitwiseOr" | "op_ExclusiveOr"), [argty1;argty2] which it is not due to the extension method. Perhaps this could also just be a wildcard match which allows the above to compile.

Going back to the test file this leaves one last error on


/// Extending a .NET primitive type with new operator
module DotNetPrimtiveWithAmbiguousNewOperator = 
    [<AutoOpen>]
    module Extensions = 
        type System.Int32 with
            static member (++)(a: int, b: int) = a 

    [<AutoOpen>]
    module Extensions2 = 
        type System.Int32 with
            static member (++)(a: int, b: string) = a 

    let f x = 1 ++ x

Where x (last line) is inferred to have type obj producing the error

testfiles\test.fs(25,17): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. internal error: Exception of type 'FSharp.Compiler.ConstraintSolver+LocallyAbortOperationThatFailsToResolveOverload' was thrown.

With a type annotation (let f (x : string) = 1 ++ x) the test file compiles.

@kevmal

This comment has been minimized.

Copy link

kevmal commented Mar 15, 2019

Using Extension attribute does not work:

open System.Runtime.CompilerServices
[<Extension>]
type Ext2() = 
    [<Extension>]
    static member inline (!@)(a : string) = a.Length

!@"324234"

Error FS0043 op_BangAt is not a static method

@dsyme

This comment has been minimized.

Copy link
Contributor Author

dsyme commented Mar 15, 2019

@kevmal Super work, thanks. Could you submit the first change as a PR to my branch? Thanks

kevmal and others added some commits Mar 15, 2019

Merge pull request #13 from kevmal/ext1
append extension methods in GetRelevantMethodsForTrait

@cartermp cartermp added the Needs RFC label Mar 18, 2019

@kevmal

This comment has been minimized.

Copy link

kevmal commented Mar 20, 2019

ExtensionAttribute members work fine. For example

open System.Runtime.CompilerServices
[<Extension>]
type Ext2() = 
    [<Extension>]
    static member Bleh(a : string) = a.Length

let inline bleh s = (^a : (member Bleh : unit -> int) s)

works. The last operator example, as the error states, is an instance member and shouldn't work. Currently I think that's okay because

type System.Int32 with 
    static member (!@)(a:string) = a.Length

!@"32423"

also works. My only concern is it would make sense to restrict the above (you must extend string with string operators). But it's important to me to be able to do something like (not exactly this, just staying on the same theme)

type System.Int32 with 
    static member inline (!@)(a) = (^a : (member Length : int) a)

where, in similar cases, I'd usually have to use the ExtensionAttribute.

With that said there seems to be an issue:

type System.Int32 with 
    static member inline (+)(a, b) = Array.map2 (+) a b

[|1;2;3|] + [|2;3;4|] //Okay
[|TimeSpan.Zero|] + [|TimeSpan.Zero|] //Okay
[|1m|] + [|2m|] //Okay
[|1uy|] + [|2uy|] //Okay
[|1L|] + [|2L|] //Okay
[|1I|] + [|2I|] //Okay
[| [|1 ; 1|]; [|2|] |] + [| [|2; 2|]; [|3|] |] //Okay
[|"1"|] + [|"2"|] //error FS0001
[|1.f|] + [|2.f|] //error FS0001
[|1.0|] + [|2.0|] //error FS0001

Where the errors (for float/single/string) are:

error FS0001: Type constraint mismatch. The type
    'float'
is not compatible with type
    ''a []

@dsyme dsyme removed the Needs RFC label Mar 21, 2019

@dsyme

This comment has been minimized.

Copy link
Contributor Author

dsyme commented Mar 22, 2019

Here, SolveMemberConstraint matches on minfos, tys, memFlags.IsInstance, nm, argtys and though some branches have a wildcard match on minfos (for example | _, _, false, ("op_Addition" | "op_Subtraction" | "op_Modulus"), [argty1;argty2]) the branch of interest matches on minfos being empty | [], _, false, ("op_BitwiseAnd" | "op_BitwiseOr" | "op_ExclusiveOr"), [argty1;argty2] which it is not due to the extension method. Perhaps this could also just be a wildcard match which allows the above to compile.

Yes, I remember reverting this during integration, thanks, I will fix it now

@dsyme

This comment has been minimized.

Copy link
Contributor Author

dsyme commented Mar 22, 2019

Re this:

[|"1"|] + [|"2"|] //error FS0001

I think this should be an error. + constrains to ( ^a or ^b) : (static member ( + ) : ^a * ^b -> ^c). This means that the type providing the witness must be either the left or right hand operand type.

@dsyme

This comment has been minimized.

Copy link
Contributor Author

dsyme commented Mar 22, 2019

@kevmal Thanks for your help! I've pushed the required fixes to the branch and testfiles\test.fs now compiles.

If you'd like to contribute a bit of code, then moving test.fs into a new tests\fsharp\core\... test file would be great, and adding execution tests that check the right values are computed.

@kevmal

This comment has been minimized.

Copy link

kevmal commented Mar 22, 2019

@dsyme With that change (wildcard match on minfos) all solution tests passed. I think the overall issue relates to the last TODO you listed. GetRelevantExtensionMethodsForTrait is returning extension methods that are not actually valid (in this case the one that's being defined).

So in the context of the previous issue I brought up... With integertypes we hit this branch

      | _, _, false, ("op_Addition" | "op_Subtraction" | "op_Modulus"), [argty1;argty2]
          when // Ignore any explicit +/- overloads from any basic integral types
               (minfos |> List.forall (fun minfo -> isIntegerTy g minfo.ApparentEnclosingType ) &&

Since the check minfos passes for integer types it's fine but for double/float/string the invalid method (array extension op) causes this branch to skip and error on the available method. In the case of TimeSpan there's a valid method in minfos and it's fine.

Though with that said I'm not sure why the above would not be List.forall (fun minfo -> IsNumericOrIntegralEnumType g minfo.ApparentEnclosingType )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.