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

[panesofglass]

This is an update to #4949

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

cc @KevinRansom and @Nhowka

[dnfadmin]

All CLA requirements met.

[cartermp]

Thanks for picking this up @panesofglass! In terms of tests, the best place to put them for now would be in the FSharpSuite here: https://github.com/dotnet/fsharp/tree/master/tests/fsharp/Compiler/Conformance

[panesofglass]

Thanks for the tip, @cartermp. I was planning to take a gander at that tonight.

[panesofglass]

Looks like the [<ParamArray>] doesn't quite work correctly, though I cannot tell whether that is my implementation or something else. Also, external extensions do not appear to work with overloading, but that may again be related to the [<ParamArray>].

  X OverloadLib accepts overloaded internal and external extensions [829ms]
  Error Message:
   Compile had warnings and/or errors: [|C:\Users\ryanr\AppData\Local\Temp\tmpD9E9.fs (43,14)-(43,20) typecheck error This expression was expected to have type
    'ArraySegment<byte>'
but here has type
    'string'    ;
  C:\Users\ryanr\AppData\Local\Temp\tmpD9E9.fs (46,14)-(46,27) typecheck error This expression was expected to have type
    'byte []'
but here has type
    'string'    ;
  C:\Users\ryanr\AppData\Local\Temp\tmpD9E9.fs (46,28)-(46,32) typecheck error This expression was expected to have type
    'int'
but here has type
    'string'    ;
  C:\Users\ryanr\AppData\Local\Temp\tmpD9E9.fs (46,33)-(46,42) typecheck error This expression was expected to have type
    'int'
but here has type
    'string'    |]
  Stack Trace:
     at <StartupCode$FSharp-TestHelpers>[email protected](Tuple`2 tupledArg) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 525
   at FSharp.TestHelpers.CompilerAssert.compileAux(Boolean isExe, String[] options, String source, FSharpFunc`2 f) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 221
   at <StartupCode$FSharp-TestHelpers>[email protected](Unit _arg2) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 274
   at FSharp.TestHelpers.CompilerAssert.compile(Boolean isExe, String[] options, String source, FSharpFunc`2 f)
   at FSharp.TestHelpers.CompilerAssert.CompileExeAndRunWithOptions(String[] options, String source) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 522
   at FSharp.Compiler.UnitTests.ComputationExpressions.OverloadLibTest(Boolean inclInternalExt, Boolean inclExternalExt, String source) in C:\Users\ryanr\Code\OSS\fsharp\tests\fsharp\Compiler\Conformance\DataExpressions\ComputationExpressions.fs:line 770
   at FSharp.Compiler.UnitTests.ComputationExpressions.OverloadLib accepts overloaded internal and external extensions() in C:\Users\ryanr\Code\OSS\fsharp\tests\fsharp\Compiler\Conformance\DataExpressions\ComputationExpressions.fs:line 802

  X OverloadLib accepts overloaded internal extension methods [1s 538ms]
  Error Message:
   System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
  ----> System.Exception : FAILED TmFtZVxyXG5FbWF2, expected [|78uy; 97uy; 109uy; 101uy; 13uy; 10uy; 69uy; 109uy; 97uy; 105uy; 108uy; 13uy;
  10uy; 115uy; 115uy; 119uy; 111uy; 13uy; 10uy; 68uy; 101uy; 115uy; 99uy; 114uy;
  105uy; 112uy; 116uy; 105uy; 111uy; 110uy; 13uy; 10uy; 111uy; 102uy; 13uy; 10uy;
  99uy; 111uy; 110uy; 116uy; 101uy; 110uy; 116uy; 13uy; 10uy|], got [|78uy; 97uy; 109uy; 101uy; 13uy; 10uy; 69uy; 109uy; 97uy; 105uy; 108uy; 13uy;
  10uy; 115uy; 115uy; 119uy; 111uy; 13uy; 10uy|]
  Stack Trace:
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at FSharp.TestHelpers.Worker.ExecuteTestCase(String assemblyPath, String[] deps) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 48
   at FSharp.TestHelpers.Worker.ExecuteTestCase(String assemblyPath, String[] deps)
   at FSharp.TestHelpers.CompilerAssert.executeBuiltApp(String assembly, FSharpList`1 deps) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 182
   at <StartupCode$FSharp-TestHelpers>[email protected](Tuple`2 tupledArg) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 527
   at FSharp.TestHelpers.CompilerAssert.compileAux(Boolean isExe, String[] options, String source, FSharpFunc`2 f) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 221
   at <StartupCode$FSharp-TestHelpers>[email protected](Unit _arg2) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 274
   at FSharp.TestHelpers.CompilerAssert.compile(Boolean isExe, String[] options, String source, FSharpFunc`2 f)
   at FSharp.TestHelpers.CompilerAssert.CompileExeAndRunWithOptions(String[] options, String source) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 522
   at FSharp.Compiler.UnitTests.ComputationExpressions.OverloadLibTest(Boolean inclInternalExt, Boolean inclExternalExt, String source) in C:\Users\ryanr\Code\OSS\fsharp\tests\fsharp\Compiler\Conformance\DataExpressions\ComputationExpressions.fs:line 770
   at FSharp.Compiler.UnitTests.ComputationExpressions.OverloadLib accepts overloaded internal extension methods() in C:\Users\ryanr\Code\OSS\fsharp\tests\fsharp\Compiler\Conformance\DataExpressions\ComputationExpressions.fs:line 788
--Exception
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1639.Invoke(String message) in C:\Users\ryanr\Code\OSS\fsharp\src\fsharp\FSharp.Core\printf.fs:line 1639
   at TmpDD27.check[a](String msg, a actual, a expected)
   at <StartupCode$tmpDD28>.$TmpDD27.main@()
  X OverloadLib accepts overloaded methods [1s 226ms]
  Error Message:
   System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
  ----> System.Exception : FAILED TmFtZVxyXG5FbWF1, expected [|78uy; 97uy; 109uy; 101uy; 13uy; 10uy; 69uy; 109uy; 97uy; 105uy; 108uy; 13uy;
  10uy; 115uy; 115uy; 119uy; 111uy; 13uy; 10uy; 68uy; 101uy; 115uy; 99uy; 114uy;
  105uy; 112uy; 116uy; 105uy; 111uy; 110uy; 13uy; 10uy; 111uy; 102uy; 13uy; 10uy;
  99uy; 111uy; 110uy; 116uy; 101uy; 110uy; 116uy; 13uy; 10uy|], got [|78uy; 97uy; 109uy; 101uy; 13uy; 10uy; 69uy; 109uy; 97uy; 105uy; 108uy; 13uy;
  10uy; 115uy; 115uy; 119uy; 111uy; 13uy; 10uy|]
  Stack Trace:
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at FSharp.TestHelpers.Worker.ExecuteTestCase(String assemblyPath, String[] deps) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 48
   at FSharp.TestHelpers.Worker.ExecuteTestCase(String assemblyPath, String[] deps)
   at FSharp.TestHelpers.CompilerAssert.executeBuiltApp(String assembly, FSharpList`1 deps) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 182
   at <StartupCode$FSharp-TestHelpers>[email protected](Tuple`2 tupledArg) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 527
   at FSharp.TestHelpers.CompilerAssert.compileAux(Boolean isExe, String[] options, String source, FSharpFunc`2 f) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 221
   at <StartupCode$FSharp-TestHelpers>[email protected](Unit _arg2) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 274
   at FSharp.TestHelpers.CompilerAssert.compile(Boolean isExe, String[] options, String source, FSharpFunc`2 f)
   at FSharp.TestHelpers.CompilerAssert.CompileExeAndRunWithOptions(String[] options, String source) in C:\Users\ryanr\Code\OSS\fsharp\tests\FSharp.TestHelpers\CompilerAssert.fs:line 522
   at FSharp.Compiler.UnitTests.ComputationExpressions.OverloadLibTest(Boolean inclInternalExt, Boolean inclExternalExt, String source) in C:\Users\ryanr\Code\OSS\fsharp\tests\fsharp\Compiler\Conformance\DataExpressions\ComputationExpressions.fs:line 770
   at FSharp.Compiler.UnitTests.ComputationExpressions.OverloadLib accepts overloaded methods() in C:\Users\ryanr\Code\OSS\fsharp\tests\fsharp\Compiler\Conformance\DataExpressions\ComputationExpressions.fs:line 774
--Exception
   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1639.Invoke(String message) in C:\Users\ryanr\Code\OSS\fsharp\src\fsharp\FSharp.Core\printf.fs:line 1639
   at TmpE324.check[a](String msg, a actual, a expected)
   at <StartupCode$tmpE325>.$TmpE324.main@()

[panesofglass]

@isaacabraham this would really help with the things we discussed re: https://github.com/CompositionalIT/farmer

[isaacabraham]

Mmmm. It feels like overloads in CEs is kind of a hidden feature at the moment that doesn't quite work. Supporting overloads would definitely be useful.

[cartermp]

@dsyme what do you think of some of these test failures? There's some oddities in this space in general, but I'm not sure if these would be expected or if they're indicative of a different issue with overload resolution.

[NinoFloris]

Beyond the ParamArray issue I found overloads with different arities aren't picked up when using @panesofglass workaround. Though maybe this will * just work * once all methods can be attributed? Worth a test, anyway.

[Nhowka]

When I tried to remove the artificial limitations in the past, the [<ParamArray>] and other overloads worked just fine. Maybe something changed between then and now if this became an issue...

[panesofglass]

@Nhowka are my tests incorrect? Is there a better way to go about this?

[Nhowka]

I think it's probably just missing an open Extensions, so it's not getting the overload that accepts string, making it fail. Got the same error in fsi but worked after adding the open Extensions. Not sure how to run the same test locally to be honest, I don't know if it's just that.

[Nhowka]

The changes look good, however, it needs to be protected with a language version switch.

While I understand the reason, I'm not sure if I agree. This is an unbreaking change, in the sense that code that used to fail to compile when using a computation expression that had that kind of overload will now just work. Note that the computation expression builder itself used to compile with no problem, the error to compile were just when using the custom keywords.

In the RFC I added an example of that happening, the library was compiled with no warnings and then when the client tried to use it, the error happened. I think that making the code that failed to compile suddenly working wouldn't be that bad, but I don't know if there an existing policy to treat these cases...

[cartermp]

Note that the langversion switch isn't about breaking changes or not. It's a means of separating preview features from non-preview features. Any new feature that is merged is preview, and we deliberately "promote" them to a language version when it makes sense. My mistake for approving right away, this needs a new case here: https://github.com/dotnet/fsharp/blob/master/src/fsharp/LanguageFeatures.fs#L36 and the associated check in both places in the typechecker.

Testing appears to use the preview flag already outside of the fsharpQA cases - @KevinRansom how should the fsharpqa cases be handled?

[Nhowka]

I understand it better now, makes sense!

[Nhowka]

For the feature string would it just need a new line on FSComp.txt or is something extra needed?

[Nhowka]

I believe line around 1514 would be better to group the features together

[panesofglass]

I moved it further up next to the line that says overloads are not allowed.

[panesofglass]

@cartermp or @KevinRansom how do I set up the tests to pass now?

[dsyme]

@panesofglass For the desktop_Release failure, you have baseline differences in neg60.fs, neg61,fs and neg87.fs.

There's a way to update the baselines on your local machine automatically (I need to look it up), or you can manually run and adjust and throw it back at CI. Here are three command lines yo ucan use to recreate the error files and replace the baseline files:

2020-08-08T02:44:16.5143188Z  D:\a\1\s\tests\FSharp.Test.Utilities\..\..\artifacts\bin\fsc\Release\net472\fsc.exe -r:System.Core.dll --nowarn:20 --define:COMPILED  --define:NEGATIVE --vserrors --warnaserror --nologo --maxerrors:10000 -a -o:neg60.dll neg60.fs 2>neg60.err


2020-08-08T02:44:18.0656712Z  D:\a\1\s\tests\FSharp.Test.Utilities\..\..\artifacts\bin\fsc\Release\net472\fsc.exe -r:System.Core.dll --nowarn:20 --define:COMPILED  --define:NEGATIVE --vserrors --warnaserror --nologo --maxerrors:10000 -a -o:neg61.dll neg61.fs 2>neg61.err

2020-08-08T02:44:48.1234110Z  D:\a\1\s\tests\FSharp.Test.Utilities\..\..\artifacts\bin\fsc\Release\net472\fsc.exe -r:System.Core.dll --nowarn:20 --define:COMPILED  --define:NEGATIVE --vserrors --warnaserror --nologo --maxerrors:10000 -a -o:neg87.dll neg87.fsx 2>neg87.err

You also have three failures in "fsharpqa"

2020-08-08T01:59:58.0125197Z Conformance\Expressions\DataExpressions\QueryExpressions (E_WhitespaceErrors01.fs) -- failed
2020-08-08T01:59:58.0130028Z Conformance\Expressions\DataExpressions\QueryExpressions (E_BadGroupValBy01.fs) -- failed
2020-08-08T01:59:58.0131689Z Conformance\Expressions\DataExpressions\QueryExpressions (E_BadGroupValBy02.fs) -- failed

Unfortunately they aren't showing here so you'll have to manually try to work out what the relevant command line is from env.lst in that directory, satisfy yourself there is no signficant regression (I doubt there is) and try to patch them up as well as you can

https://dev.azure.com/dnceng/public/_build/results?buildId=762398&view=ms.vss-test-web.build-test-results-tab

[cartermp]

Sorry for all the hassle, especially the fsharpqa tests. They're really quite awful to adjust, but we're planning on moving them to be normal tests that are easy to run and diagnose

[panesofglass]

@cartermp do you need me to rebase and/or merge commits to clean up the commit history?

[cartermp]

@panesofglass we tend to use squash commits here, so it should be good to go.

@KevinRansom - want to re-review?

[cartermp]

@KevinRansom this now correctly guards against the flag and all tests are updated. Looking good? I'd like to get this one in soon.

[cartermp]

Thanks @panesofglass and @Nhowka!

[realvictorprm]

This is absolutely great! Thank you all!

[mastoj]

Awesome stuff. If this was in the box a month ago the pulumiprovider that I helped out on would be in a much better shape: https://github.com/UnoSD/Pulumi.FSharp.Extensions. Great to see this happen!