Skip to content

[RFC FS-1075] Improve interop to C# nullable-typed optionals #7989

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

Merged
merged 30 commits into from
Feb 5, 2020

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Dec 16, 2019

Implementation of F# RFC FS-1075 - Interop with C# nullable-typed optional parameters

  • use feature branch
  • language version protection

Continues #7296

@dsyme
Copy link
Contributor Author

dsyme commented Dec 16, 2019

I added the language version protection and tests for it, so I think this is actually all ready for review

@TIHan
Copy link
Contributor

TIHan commented Dec 16, 2019

Great! Will look at it.

@abelbraaksma
Copy link
Contributor

@dsyme, the discussion link on the spec returns a 404. Do we have a dedicated discussion thread for this RFC? Could you update?

@dsyme
Copy link
Contributor Author

dsyme commented Jan 10, 2020

@cartermp This feature is ready and I think can be integrated into preview releases

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a couple of minor questions.

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment about a comment.

AdjustCalledArgTypeForOptionals is where 99% of everything happens, that looks fine.

I think I understand enforceNullableOptionalsKnownTypes, it's set to true when there are multiple candidates for overload resolution which would mean that we need to be more explicit about the types due to ambiguity.

After testing the feature out, there is existing F# code that can break once this feature is enabled, ex:

using System;

namespace ClassLibrary1
{
    public class Class1
    {
        public static void Print(int? x = 3)
        {
            Console.WriteLine(x);
        }

        public static void Print(int x = 3)
        {
            Console.WriteLine(x);
        }
    }
}
open ClassLibrary1

let test1 (xArg: int) =
    Class1.Print(x = xArg)

let test2 (xArg: int) =
    Class1.Print(xArg)

The F# code above will compile without the feature enabled. With the feature enabled, it will fail with the message of unable to find a unique overload on both calls to Class1.Print.
test2 should definitely compile as we are explicit of the type.

Whether test1 should fail, depends on what the design should be at that point. In C#, a call to Class1.Print ->

        public static void Test(int n)
        {
            Class1.Print(x: n);
        }

Will compile and the method chosen is one that is non-nullable -> Print(int x). Perhaps we should do the same? Yea, I think so. Ex: Class1.Print(3) and Class1.Print(x = 3) - both should always point to the same method, in this case Print(int x).


(removed comment here as it was inaccurate)

@dsyme
Copy link
Contributor Author

dsyme commented Jan 29, 2020

@TIHan

After testing the feature out, there is existing F# code that can break once this feature is enabled, ex:

I've fixed this problem, thanks for finding it!

This requires the addition of two new rules to the RFC

  • when comparing overloads, and overload argument of type X is preferred to one of type Nullable<X> if they otherwise both match

  • when comparing overloads, we were only comparing unnamed arguments. Now, if two overloads were considered equal priority by previous rules (i.e. comparison returned zero), then their entire argument lists are compared including both unnamed and named arguments, using the same rules as already used for unnamed arguments. We use this as a last-resort comparison.

I've updated the PR and will update the RFC

@dsyme
Copy link
Contributor Author

dsyme commented Jan 29, 2020

The update to the RFC is here (I did it via direct commit)

fsharp/fslang-design@43785dd#diff-67995b3eec7592c0a05c96bff39a7a83

That resolves all issues with the PR as it stands, please re-review? @TIHan Thanks!

@TIHan
Copy link
Contributor

TIHan commented Feb 5, 2020

Ok, will look at this again. Thank you!

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for making the changes :)

@TIHan TIHan merged commit 0f2bc13 into master Feb 5, 2020
@cartermp cartermp deleted the feature/nullable-optionals branch February 6, 2020 00:07
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
…7989)

* add test cases we need to make work

* cleanup method call argument processing

* update for new cases

* update tests

* compat Nullable allowed

* reduce diff

* fix mistake with byrefs

* update error message

* trigger CI

* add version tests

* mnimize diff

* mnimize diff

* remove assert

* minor updates for code review

* fix overloading change of behaviour

Co-authored-by: Phillip Carter <[email protected]>
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…7989)

* add test cases we need to make work

* cleanup method call argument processing

* update for new cases

* update tests

* compat Nullable allowed

* reduce diff

* fix mistake with byrefs

* update error message

* trigger CI

* add version tests

* mnimize diff

* mnimize diff

* remove assert

* minor updates for code review

* fix overloading change of behaviour

Co-authored-by: Phillip Carter <[email protected]>
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…7989)

* add test cases we need to make work

* cleanup method call argument processing

* update for new cases

* update tests

* compat Nullable allowed

* reduce diff

* fix mistake with byrefs

* update error message

* trigger CI

* add version tests

* mnimize diff

* mnimize diff

* remove assert

* minor updates for code review

* fix overloading change of behaviour

Co-authored-by: Phillip Carter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants