[WIP] Implement implicit converter for F# optionals

[eiriktsarpalis]

As discussed in #1254, this PR attempts to improve C# consumption of F# optional parameters in two ways:

• Automatically append the OptionalAttribute and DefaultParameterValueAttribute in all optional parameters of F# methods. These will be added in addition to the the already automatically appended FSharp.Core.OptionalArgumentAttribute.
• Implement an implicit converter 'T -> 'T option in FSharp.Core.

[eiriktsarpalis]

Hmm, appveyor is failing to build. Reason seems unrelated to changes introduced here.

[KevinRansom]

@eiriktsarpalis

[KevinRansom]

@eiriktsarpalis

On my machine I synched master, and build net40 proto coreclr notests.
Everything built okay.

I merged your changes and got these errors:

"d:\microsoft\visualfsharp\build-everything.proj" (default target) (1) ->
"d:\microsoft\visualfsharp\src\fsharp-library-build.proj" (Build target) (2) ->
"d:\microsoft\visualfsharp\src\fsharp\FSharp.Core\FSharp.Core.fsproj" (Build target) (3) ->
(CoreCompile target) ->
d:\microsoft\visualfsharp\src\fsharp\FSharp.Core\reflect.fsi(291,19): error FS3188: Type inference cau
sed an inference type variable to escape its scope. Consider adding type annotations to make your code l
ess generic. [d:\microsoft\visualfsharp\src\fsharp\FSharp.Core\FSharp.Core.fsproj]
d:\microsoft\visualfsharp\src\fsharp\FSharp.Core\reflect.fsi(291,19): error FS3188: Type inference cau
sed an inference type variable to escape its scope. Consider adding type annotations to make your code l
ess generic. [d:\microsoft\visualfsharp\src\fsharp\FSharp.Core\FSharp.Core.fsproj]
d:\microsoft\visualfsharp\src\fsharp\FSharp.Core\reflect.fsi(291,19): error FS3188: Type inference ......

Reverted your changes and rebuilt ... everything built fine. So I think the issue is due to your change.

Kevin

[eiriktsarpalis]

@KevinRansom thanks, seeing that too on my machine. Wonder what broke here..

[eiriktsarpalis]

For lack of time I've removed the auto-insertion implementation and simply kept the implicit converter. It would still improve things although API authors would have to manually insert those attributes. What do you think? /cc @dsyme

[dsyme]

@eiriktsarpalis This looks like a fine addition. Update to master and add some test cases?

I'll leave it to @KevinRansom and @otawfik-ms to decide if it fits on the F~ 4.1 schedule.

[eiriktsarpalis]

I'll add those asap. It'd be an immensely useful addition to F# 4.1
On Mon, 8 Aug 2016 at 08:11, Don Syme notifications@github.com wrote:

@eiriktsarpalishttps://github.com/eiriktsarpalis This looks like a
fine addition. Update to master and add some test cases?

I'll leave it to @KevinRansomhttps://github.com/KevinRansom and
@otawfik-ms https://github.com/otawfik-ms to decide if it fits on the
F~ 4.1 schedule.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1386 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACrts8CHKlwi5S5YF6xwjHpXA9XCF3Yqks5qdxzzgaJpZM4JWmKq
.

[eiriktsarpalis]

The tests would probably need to be written in C#. Should I add a C# tests
project?
On Mon, 8 Aug 2016 at 10:02, Eirik George Tsarpalis <
eirik.tsarpalis@gmail.com> wrote:

I'll add those asap. It'd be an immensely useful addition to F# 4.1
On Mon, 8 Aug 2016 at 08:11, Don Syme notifications@github.com wrote:

@eiriktsarpalishttps://github.com/eiriktsarpalis This looks like a
fine addition. Update to master and add some test cases?

I'll leave it to @KevinRansomhttps://github.com/KevinRansom and
@otawfik-ms https://github.com/otawfik-ms to decide if it fits on the
F~ 4.1 schedule.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1386 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACrts8CHKlwi5S5YF6xwjHpXA9XCF3Yqks5qdxzzgaJpZM4JWmKq
.

[dsyme]

@eiriktsarpalis You can probably add tests to "tests/fsharp/core/fsfromfsviacs" (F# from F# via C#). Put the tests in the C# code then trigger them from F# main

[cartermp]

👍 on this making it into F# 4.1, even though we're feature complete. This will be an immediate benefit to a lot of people who are doing plenty of F#/C# interop. I think we should make an exception here.

[KevinRansom]

@cartermp@dsyme For now everything is headed for 4.1. We are feature complete on planned features. However ... opportunistic safe new features are still welcome, subject to concerns about compatability, risk etc ...

Kevin

[dsyme]

Please invoke this method from the actual test.fs?

Also please add further tests for other uses of the implicit conversion (ones that just utilize the conversion independent of optional parameters).

[dsyme]

Off topic now, but IIRC one issue I had with this:

Automatically append the OptionalAttribute and DefaultParameterValueAttribute in all optional parameters of F# methods.

was that OptionalAttribute wasn't available in all portable profiles (e.g. 259). That meant that compilation would change as you move from profile 259 to profile 7 and .NET Standard, which would be unfortunate.

[eiriktsarpalis]

I think it's ready, tests are passing.

[forki]

2,986 additions, 0 deletions in surfacearea for silverlight ?

[dsyme]

@eiriktsarpalis That file can be deleted and the test disabled

[forki]

Much better. Thanks

[dsyme]

Just to mention that over skype I had asked Eirik to add more tests for interop, especially around null, just to check this doesn't break anything for C# consumers.

[eiriktsarpalis]

@dsyme I'll create a second PR that adds those.
On Sat, 13 Aug 2016 at 04:20, Don Syme notifications@github.com wrote:

Just to mention that over skype I had asked Eirik to add more tests for
interop, especially around null, just to check this doesn't break anything
for C# consumers.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1386 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACrts6EWi8FbIVXWmgrfdigS1j6sBbM0ks5qfX5jgaJpZM4JWmKq
.