[WIP] [FS-1056] Allow overloads of custom keywords in computation expressions

[Nhowka]

Implementation for fsharp/fslang-suggestions#69.
RFC here
tests in progress.

[msftclas]

All CLA requirements met.

[cartermp]

@Nhowka I added [WIP] to the title FYI

[NinoFloris]

@Nhowka Are you still working on this btw, can we help to speed it along? I would really like to see this in vnext!

[Nhowka]

Code is already working, or at least was. Now it should be create some tests and discuss implementation details.

I never wrote enough tests in my life to fill a hand, so I need help on that.

[pirrmann]

I may be able to help you in order to add tests, as I'd really like this to be part of vNext. Do you still need help?

[cartermp]

Just as a pointer, some tests could be added in this file (example).

I think having regressions tests that exhaust each way you can overload one of these members would be great. @Nhowka anything to add to that?

[Nhowka]

@pirrmann Thanks! To be honest, I'm useless about anything involving that kind of tests. Mostly of the use I thought for this feature is for simplifying some Saturn expressions that kinda need an entire new keyword to do the same thing because the types are different.

So, nothing to add. Probably regression tests are enough, but is the discussion empty because the RFC is good enough or because the feature is not that interesting?

[cartermp]

I think the RFC is sufficiently detailed to not warrant discussion on my part.

[Nhowka]

cc @gerardtoconnor
Just read your post, maybe what I did here could be measured. Maybe the benchmarks will be very motivating!

[gerardtoconnor]

@Nhowka That's awesome thanks! Will integrate this soon as merged PR released.

I had looked at doing this PR myself but when I traced the branches, It seemed that the operation was really inefficient and was getting dragged down a rabbit hole. I think tryGetDataForCustomOperation , and the customOperationMethodsIndexedByKeyword and customOperationMethodsIndexedByMethodName lookups it uses can be optimised, do we want to do in this PR?

We start with a list of tuples but then start using Seq functions (groupBy & Distinct) that use temporary Dictionaries/Sets, and then put them into a new Dict (readonly layer over dictionary) … this should be possible in one iteration into a final Dictionary<>, safely controlling access in tryGetDataForCustomOperation. Given tryGetDataForCustomOperation is only matching to one list item in the Dict, does this still work with overloads given I thought there would be multiple methodinfo Logical (name with types) names for an Ident, or is this Logical name just simply the method name without parameter types? Either way, it feels like a code smell that the there are two dicts of both keyword and LogicalName, it seems that possible Logical names would be a subset of the keywords, allowing for subsequent smaller refined hash space lookup on LogicalName. Don't want to side-track current PR, just want to make sure overloads handled correctly and take easy speed-up while we're at it?

[Nhowka]

@gerardtoconnor I agree that it can be a lot better. My understanding of the original code is that it was made to create an artificial limitation and made to fail the compilation if the computation expression was used in a way that it was not intended.

My original use-case was that I wanted to be able to give more than a keyword for the same method for reasons of backward compatibility and consistency. I was using an with_ prefix in a method and use_ in another. That made exploring the api harder, so I wanted to lift the one keyword per method limitation. That feature was rejected and quite frankly wouldn't make any impact in doing something better, but by accident the overloads started working.

Most of the changes I did are on code made just for that limitation. The place where it solves what overload will be called for each call depending of the number of arguments and their types is elsewhere and works really well in the testing I did. Even [<ParamArray>] with variable number of arguments were well supported out of the box.

So I made the bare minimum while keeping some sort of limitation: You can't have myKeyword as the name for calling MyMethodA or myMethodB. The same for MyMethod being exposed by myKeywordA or myKeywordB. My lack of familiarity is what made me keep it almost as I found it. I don't know if it is that way because it's not hot enough to deserve an optimization or if it has a hidden purpose that made it that way.

[panesofglass]

@Nhowka, did you already enable this in some way? I discovered today that the following works to overload a CustomOperation:

type Microsoft.FSharp.Control.AsyncBuilder with
    [<CustomOperation("and!", IsLikeZip=true)>]
    member __.Merge(x, y, [<ProjectionParameter>] resultSelector) =
        async {
            let x' = Async.StartAsTask x
            let y' = Async.StartAsTask y
            do System.Threading.Tasks.Task.WaitAll(x',y')
            return resultSelector x'.Result y'.Result
        }
    member __.Merge(x:Async<'a>, y:System.Threading.Tasks.Task<'b>, [<ProjectionParameter>] resultSelector) =
        async {
            let x' = Async.StartAsTask x
            do System.Threading.Tasks.Task.WaitAll(x',y)
            return resultSelector x'.Result y.Result
        }
    member __.Merge(x:System.Threading.Tasks.Task<'a>, y:Async<'b>, [<ProjectionParameter>] resultSelector) =
        async {
            let y' = Async.StartAsTask y
            do System.Threading.Tasks.Task.WaitAll(x,y')
            return resultSelector x.Result y'.Result
        }
    member __.Merge(x:System.Threading.Tasks.Task<'a>, y:System.Threading.Tasks.Task<'b>, [<ProjectionParameter>] resultSelector) =
        async {
            do System.Threading.Tasks.Task.WaitAll(x,y)
            return resultSelector x.Result y.Result
        }

[<Tests>]
let tests =
    testList "and! tests" [
        test "concurrent async execution" {
            let expected = 3
            let concurrent =
                async {
                    for a in async.Return(1) do
                    ``and!`` b in async.Return(2)
                    return a + b
                }
            let actual = Async.RunSynchronously concurrent
            Expect.equal actual expected "Expected actual to equal 3"
        }

        test "concurrent task execution within async" {
            let expected = 3
            let concurrent =
                async {
                    for a in System.Threading.Tasks.Task.FromResult(1) do
                    ``and!`` b in System.Threading.Tasks.Task.FromResult(2)
                    return a + b
                }
            let actual = Async.RunSynchronously concurrent
            Expect.equal actual expected "Expected actual to equal 3"
        }

        test "concurrent async & task execution within async" {
            let expected = 3
            let concurrent =
                async {
                    for a in async.Return(1) do
                    ``and!`` b in System.Threading.Tasks.Task.FromResult(2)
                    return a + b
                }
            let actual = Async.RunSynchronously concurrent
            Expect.equal actual expected "Expected actual to equal 3"
        }

        test "concurrent task & async execution within async" {
            let expected = 3
            let concurrent =
                async {
                    for a in System.Threading.Tasks.Task.FromResult(1) do
                    ``and!`` b in async.Return(2)
                    return a + b
                }
            let actual = Async.RunSynchronously concurrent
            Expect.equal actual expected "Expected actual to equal 3"
        }
    ]

If you only define the attribute with the name "and!" once, you can set it on any of the overloads, and method dispatch appears to pick up the attribute from one and then choose the correct method.

[Nhowka]

@panesofglass That feature feels like a bug... I couldn't expoit it to enable multiple arguments when curried, but tupled it went fine. Nice find!

[gerardtoconnor]

@panesofglass & @Nhowka looks like a bug to me also,.. but nice to know there is a workaround for the functionality though.

@Nhowka I think you're right that we should look to do the bare minimum changes for now so that it works, tidying/optimisation is a less critical job.

[KevinRansom]

@Nhowka, It's a wip and nothing has happened for more than 18 months, should I close this?

Thanks

Kevin

[Nhowka]

Sorry, I moved to another city and didn't have my computer with me until last month. It was working when I last touched it, but I don't know how much or if anything changed.

I probably have more knowledge now to write some tests and try to bring it up to date. I'll try to redo those changes tomorrow on the latest master and see if they are still working.

[KevinRansom]

@Nhowka , hey I am trying to clean up our repo of orphaned PR,s it makes our job with current PR's hard, because of all of the orphaned PR's in the tree.

Please feel free to reactivate this PR when you are ready to work on it some more.

Thanks for this contribution

Kevin