Prefer by-val methods over in methods in overload resolution #23122

OmarTawfik · 2017-11-10T21:39:16Z

Customer scenario

Given two methods with parameters that only differ by ref kind:

void M(int x) { }
void M(in int x) { }

The call M(int) would result in (C# 7.2, shipped with VS 15.5):
error CS0121: The call is ambiguous between the following methods or properties: 'C.M(int)' and 'C.M(in int)'

This PR proposes to change that behavior, and always prefer value methods over in methods. This is not a breaking change, because it should only affect cases where the compiler produced an error, to turn it into valid invocations.

[Note from jcouv:] This change would ship in 15.6 preview 2 as a bug fix for C# 7.2 (without introducing a new language version).

Bugs this fixes

Contributes to dotnet/csharplang#945 (comment)

Workarounds, if any

None needed

Risk

Low. This should be a new tie-breaker rule that is introduced to C# overload resolution.

Performance impact

None.

Is this a regression from a previous update?

No. This is a new feature.

Test documentation updated?

No.

OmarTawfik · 2017-11-13T17:18:41Z

@dotnet-bot test this please #Closed

OmarTawfik · 2017-11-13T19:26:14Z

@dotnet/roslyn-compiler #Closed

jcouv · 2017-11-13T19:28:41Z

I'm wondering if we should move this to C# 7.3 and gate by language version. We don't want different people on a team seeing two different behaviors for this. #Resolved

jaredpar · 2017-11-13T19:30:57Z

@jcouv every release of the compiler changes overload resolution: some more explicitly than others. So far we've avoided using the language version to toggle how overload resolution works. Mostly because it adds layers to the complexity matrix of an already incredibly hard problem. #Resolved

VSadov · 2017-11-13T23:50:14Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

                {
                    return BetterResult.Neither;
                }
            }

+            // If an ambiguity exists between 'by-val' and 'in' parameter, choose the 'by-val' one.
+            // Except if it was params. Subsequent betterness analysis will always prefer the non-params one.
+            if (!p1.IsParams && p1.RefKind == RefKind.None && p2.RefKind == RefKind.In)


This is confusing.

If we have

M1(params int[] arr) . . .

and

M1(in int[] arr) ...

And I call with M1( new int[] {1,2}) - i.e. first is applicable in normal (not expanded) form
I would expect the byval method to be selected, regardless if it was declared params or not. The comment seems to say the opposite.

We need some tests for this scenario - with params overload matching in normal or expanded form. (in expanded form I expect it to not be selected) #Resolved

@VSadov We already have a test that tests params expansion with in: PassingInArgumentsOverloadedOnInParams.
For the array variation, we are still reporting ambiguity (I prefer this over selecting the by-val one, as it can break the caller). Will add another test for that variation and explain in the comment. #Closed

AlekseyTs · 2017-11-15T00:31:19Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

-                    var type1 = GetParameterType(i, m1.Result, m1.LeastOverriddenMember.GetParameters(), out refKind1);
-                    var type2 = GetParameterType(i, m2.Result, m2.LeastOverriddenMember.GetParameters(), out refKind2);
+                    var type1 = GetParameterType(i, m1.Result, m1.LeastOverriddenMember.GetParameters(), out ParameterSymbol parameter1);
+                    var type2 = GetParameterType(i, m2.Result, m2.LeastOverriddenMember.GetParameters(), out ParameterSymbol parameter2);


ParameterSymbol parameter2 [](start = 109, length = 26)

Consider using discard. #Closed

AlekseyTs · 2017-11-15T00:51:38Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

                {
                    return BetterResult.Neither;
                }
            }

+            // If an ambiguity exists between 'by-val' and 'in' parameter, choose the 'by-val' one, except if it was params,
+            // for later betterness analysis will decide on whether the expanded/non-expanded form can be used, or if it is ambiguous.
+            if (!p1.IsParams && p1.RefKind == RefKind.None && p2.RefKind == RefKind.In)


p1.IsParams [](start = 17, length = 11)

See IsValidParams helper how to check for valid params parameter. Also, why should this matter if candidate is not using expanded form? #Closed

AlekseyTs · 2017-11-15T01:07:08Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

                {
                    return BetterResult.Neither;
                }
            }

+            // If an ambiguity exists between 'by-val' and 'in' parameter, choose the 'by-val' one, except if it was params,


// If an ambiguity exists between 'by-val' and 'in' parameter, choose the 'by-val' one, except if it was params, [](start = 12, length = 112)

It feels suspicious that we do this check before we checked which candidate uses BetterConversionFromExpression. Even if we do that immediately after, we might still introduce a breaking change as the tie breaking rules that are executed today after this function can resolve the ambiguity differently. #Closed

AlekseyTs · 2017-11-15T01:12:36Z

Done with review pass (iteration 2). It feels like we need to figure out exact rules and where do they fit in the current overload resolution specification. Then we can review the change. #Closed

OmarTawfik · 2017-11-15T23:27:21Z

@AlekseyTs I addressed the comments. Will follow up on the spec change in the mean time. #Closed

VSadov · 2017-11-16T15:40:12Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

+            // Otherwise, prefer methods with 'val' parameters over 'in' parameters.
+            Debug.Assert(m1Original.Length == m2Original.Length, "Unequal parameters counts are handled in an earlier stage");
+            for (i = 0; i < m1Original.Length; i++)
+            {


It seems the order of paremeters matters.
I.E. we can resolve M(1, 1) when
M(in int x, int y)
M(int x, in int y)

Second wins. Is that intentional? #Resolved

AlekseyTs · 2017-11-16T20:05:52Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

            int m1ModifierCount = m1.LeastOverriddenMember.CustomModifierCount();
            int m2ModifierCount = m2.LeastOverriddenMember.CustomModifierCount();
            if (m1ModifierCount != m2ModifierCount)
            {
                return (m1ModifierCount < m2ModifierCount) ? BetterResult.Left : BetterResult.Right;
            }

+            // Otherwise, prefer methods with 'val' parameters over 'in' parameters.
+            Debug.Assert(m1Original.Length == m2Original.Length, "Unequal parameters counts are handled in an earlier stage");


"Unequal parameters counts are handled in an earlier stage" [](start = 65, length = 59)

Could you please point to the place that does that? #Closed

Check lines 1441-1519. They should handle these cases where we end up with mismatching parameter count because of unused optional or expanded ones. I'll add a test as well.

In reply to: 151523306 [](ancestors = 151523306)

I see that code comparing the number of parameters used, but not the number of parameters declared.

In reply to: 151544214 [](ancestors = 151544214,151523306)

AlekseyTs · 2017-11-16T20:06:39Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

            int m1ModifierCount = m1.LeastOverriddenMember.CustomModifierCount();
            int m2ModifierCount = m2.LeastOverriddenMember.CustomModifierCount();
            if (m1ModifierCount != m2ModifierCount)
            {
                return (m1ModifierCount < m2ModifierCount) ? BetterResult.Left : BetterResult.Right;
            }

+            // Otherwise, prefer methods with 'val' parameters over 'in' parameters.
+            Debug.Assert(m1Original.Length == m2Original.Length, "Unequal parameters counts are handled in an earlier stage");
+            for (i = 0; i < m1Original.Length; i++)


for (i = 0; i < m1Original.Length; i++) [](start = 12, length = 39)

I think we should only look at parameters that have arguments. #Closed

AlekseyTs · 2017-11-16T20:08:17Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

+            for (i = 0; i < m1Original.Length; i++)
+            {
+                var p1 = m1Original[i];
+                var p2 = m2Original[i];


var p2 = m2Original[i]; [](start = 16, length = 23)

It looks like these parameters may correspond to different arguments. Why would we look at them in this case? #Closed

AlekseyTs · 2017-11-16T20:09:55Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

+
+                if (p1.RefKind == RefKind.None && p2.RefKind == RefKind.In)
+                {
+                    return BetterResult.Left;


return BetterResult.Left; [](start = 20, length = 25)

Returning too early? Please add tests for multiple arguments including cases when different arguments point to a different winners. #Closed

AlekseyTs · 2017-11-16T20:11:05Z

            return BetterResult.Neither;

Should we try to disambiguate here as well? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs:1518 in d095bd7. [](commit_id = d095bd7, deletion_comment = False)

AlekseyTs · 2017-11-16T20:12:08Z

Done with review pass (iteration 3) #Closed

OmarTawfik · 2017-11-29T23:14:18Z

@VSadov @AlekseyTs comments addressed. #Closed

VSadov

LGTM

AlekseyTs · 2017-12-05T17:35:39Z

    public override string ToString()

Should this method be adjusted? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorSignature.cs:25 in c06fca6. [](commit_id = c06fca6, deletion_comment = False)

AlekseyTs · 2017-12-05T17:36:37Z

    public bool Equals(BinaryOperatorSignature other)

Should this method be adjusted? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorSignature.cs:42 in c06fca6. [](commit_id = c06fca6, deletion_comment = False)

AlekseyTs · 2017-12-05T17:54:57Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorOverloadResolution.cs

+            // Always prefer operators with val parameters over operators with in parameters:
+            BetterResult valOverInPreference;
+
+            if (op1.LeftRefKind == RefKind.None && op2.LeftRefKind == RefKind.In)


if (op1.LeftRefKind == RefKind.None && op2.LeftRefKind == RefKind.In) [](start = 12, length = 69)

It feels like the logic here could be simplified if we add a helper that calculates the best among two ref kinds. Then we call it for both sides and then reason about final answer.
Something like this:

BetterResult fromLeft = better(op1.LeftRefKind, op2.LeftRefKind); BetterResult fromRight = better(op1.RightRefKind, op2.RightRefKind); if (fromLeft == BetterResult.Neither || fromLeft == fromRight) return fromRight; if (fromRight == BetterResult.Neither) return fromLeft; return BetterResult.Neither; ``` #Resolved

To me, the existing implementation is easier to read/trace. However, I don't feel strongly about this. I can implement your suggestions if you think it is the better alternative.

In reply to: 155026276 [](ancestors = 155026276)

AlekseyTs · 2017-12-05T17:55:46Z

Done with review pass (iteration 5). #Closed

OmarTawfik · 2017-12-05T22:48:35Z

    public bool Equals(BinaryOperatorSignature other)

I'm assuming you mean ToString()?

In reply to: 349380594 [](ancestors = 349380594)

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorSignature.cs:42 in c06fca6. [](commit_id = c06fca6, deletion_comment = False)

OmarTawfik · 2017-12-06T00:27:13Z

@AlekseyTs comments addressed. #Closed

AlekseyTs · 2017-12-06T23:35:42Z

    public bool Equals(BinaryOperatorSignature other)

I'm assuming you mean ToString()?

I meant "Equals" implementation, but, given how LeftRefKind/RightRefKind are implemented, the current implementation should be fine.

In reply to: 349467602 [](ancestors = 349467602,349380594)

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorSignature.cs:42 in c06fca6. [](commit_id = c06fca6, deletion_comment = False)

AlekseyTs · 2017-12-06T23:37:09Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorSignature.cs

+        {
+            get
+            {
+                if (Method != null)


Method [](start = 20, length = 6)

Please add cast to object. #Resolved

AlekseyTs · 2017-12-06T23:37:17Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorSignature.cs

+        {
+            get
+            {
+                if (Method != null)


Method [](start = 20, length = 6)

Please add cast to object. #Resolved

AlekseyTs · 2017-12-06T23:38:41Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorSignature.cs

+        {
+            get
+            {
+                if (Method != null)


Method [](start = 20, length = 6)

Please add cast to object. #Resolved

AlekseyTs · 2017-12-06T23:39:57Z

src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorSignature.cs

+                    {
+                        Debug.Assert(Method.ParameterRefKinds.Length == 1);
+
+                        return Method.ParameterRefKinds.Single();


.Single() [](start = 55, length = 9)

Is Single() better than just [0]? #Resolved

It will throw a more descriptive exception if something went wrong.

In reply to: 155394341 [](ancestors = 155394341)

AlekseyTs

LGTM (iteration 6)

…-tie-breaker

OmarTawfik · 2017-12-07T20:34:22Z

@MeiChin-Tsai for ask mode approval.

jcouv · 2017-12-08T18:34:31Z

Ping for @MeiChin-Tsai for ask-mode approval (15.6). Thanks

MeiChin-Tsai · 2017-12-08T18:38:54Z

Approved.

OmarTawfik · 2017-12-08T19:47:26Z

@dotnet-bot test windows_release_unit32_prtest

OmarTawfik · 2017-12-08T21:51:12Z

@dotnet-bot test this please

* dotnet/master: (39 commits) Prefer by-val methods over in methods in overload resolution (dotnet#23122) Update build to account for additional Mono.Cecil assemblies Fix build break Enable multicore JIT in the compilers (dotnet#23173) Separate debugging workspace service and EnC service (dotnet#23630) UseInferredMemberName: use one code style option and share more code (dotnet#23506) Add negative test cases Remove unnecessary Mono.Cecil reference Updated formatting of decompiler license notice Use sentence case for the decompiler legal notice Use ConcurrentDictionary to avoid locks in IsGeneratedCode and HasHiddenRegions Recognize condition with logical negation Refactor so that GetSymbolInfo can be called last Change the code fix to find the expression so that we don't need to unwrap the argument Add check for null argument syntax Fix argument names Formatting adjustments Add VB tests for omitted arguments Add WorkItem attributes Pool the Stopwatch instance used in the analyzer driver ...

Prefer by-val methods over in methdos in overload resolution

2237b17

gafter added the Area-Compilers label Nov 12, 2017

OmarTawfik added the Feature - Readonly References Readonly References label Nov 13, 2017

jcouv requested a review from VSadov November 13, 2017 20:12

VSadov reviewed Nov 13, 2017

View reviewed changes

jcouv changed the title ~~Prefer by-val methods over in methdos in overload resolution~~ Prefer by-val methods over in methods in overload resolution Nov 14, 2017

More tests

8f3cae7

AlekseyTs reviewed Nov 15, 2017

View reviewed changes

Moved check to the end of OR chain

d095bd7

VSadov reviewed Nov 16, 2017

View reviewed changes

AlekseyTs reviewed Nov 16, 2017

View reviewed changes

shyamnamboodiripad changed the base branch from post-dev15.5-contrib to master November 17, 2017 20:03

Address PR comments

7b98d7e

VSadov approved these changes Nov 29, 2017

View reviewed changes

Operators overloading

c06fca6

AlekseyTs reviewed Dec 5, 2017

View reviewed changes

Address PR Comments

0a2d3fe

OmarTawfik mentioned this pull request Dec 6, 2017

Accept PE Operator Symbols having in parameters #23508

Merged

AlekseyTs reviewed Dec 6, 2017

View reviewed changes

AlekseyTs approved these changes Dec 6, 2017

View reviewed changes

OmarTawfik added 2 commits December 6, 2017 22:44

Clean up

047a57c

Merge branch 'master' of https://github.com/dotnet/roslyn into val-in…

97a9b90

…-tie-breaker

MeiChin-Tsai added the Approved to merge label Dec 8, 2017

OmarTawfik merged commit 919b606 into dotnet:master Dec 8, 2017

OmarTawfik deleted the val-in-tie-breaker branch December 8, 2017 22:56

jcouv added this to the 15.6 milestone Dec 8, 2017

jcouv mentioned this pull request Dec 10, 2017

Use in-parameter operator from different assembly #23689

Closed

OmarTawfik mentioned this pull request Jan 23, 2018

C# in by val tiebreaker dotnet/docs#3949

Closed

Prefer by-val methods over in methods in overload resolution #23122

Prefer by-val methods over in methods in overload resolution #23122

Conversation

OmarTawfik commented Nov 10, 2017 • edited by jcouv Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Test documentation updated?

OmarTawfik commented Nov 13, 2017 • edited Loading

OmarTawfik commented Nov 13, 2017 • edited Loading

jcouv commented Nov 13, 2017 • edited by OmarTawfik Loading

jaredpar commented Nov 13, 2017 • edited by OmarTawfik Loading

VSadov Nov 13, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 15, 2017 • edited Loading

OmarTawfik commented Nov 15, 2017 • edited Loading

VSadov Nov 16, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

OmarTawfik Nov 16, 2017

Choose a reason for hiding this comment

AlekseyTs Nov 16, 2017

Choose a reason for hiding this comment

AlekseyTs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 16, 2017 • edited Loading

AlekseyTs commented Nov 16, 2017 • edited by OmarTawfik Loading

OmarTawfik commented Nov 29, 2017 • edited Loading

VSadov left a comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 5, 2017 • edited Loading

AlekseyTs commented Dec 5, 2017 • edited Loading

AlekseyTs Dec 5, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Dec 5, 2017

Choose a reason for hiding this comment

AlekseyTs commented Dec 5, 2017 • edited by OmarTawfik Loading

OmarTawfik commented Dec 5, 2017

OmarTawfik commented Dec 6, 2017 • edited Loading

AlekseyTs commented Dec 6, 2017

AlekseyTs Dec 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs Dec 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs Dec 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

AlekseyTs Dec 6, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Dec 7, 2017

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

OmarTawfik commented Dec 7, 2017

jcouv commented Dec 8, 2017

MeiChin-Tsai commented Dec 8, 2017

OmarTawfik commented Dec 8, 2017

OmarTawfik commented Dec 8, 2017

OmarTawfik commented Nov 10, 2017 •

edited by jcouv

Loading

OmarTawfik commented Nov 13, 2017 •

edited

Loading

OmarTawfik commented Nov 13, 2017 •

edited

Loading

jcouv commented Nov 13, 2017 •

edited by OmarTawfik

Loading

jaredpar commented Nov 13, 2017 •

edited by OmarTawfik

Loading

VSadov Nov 13, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Nov 14, 2017 •

edited

Loading

AlekseyTs Nov 15, 2017 •

edited

Loading

AlekseyTs Nov 15, 2017 •

edited

Loading

AlekseyTs Nov 15, 2017 •

edited

Loading

AlekseyTs commented Nov 15, 2017 •

edited

Loading

OmarTawfik commented Nov 15, 2017 •

edited

Loading

VSadov Nov 16, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Nov 16, 2017 •

edited

Loading

AlekseyTs Nov 16, 2017 •

edited

Loading

AlekseyTs Nov 16, 2017 •

edited

Loading

AlekseyTs Nov 16, 2017 •

edited

Loading

AlekseyTs commented Nov 16, 2017 •

edited

Loading

AlekseyTs commented Nov 16, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Nov 29, 2017 •

edited

Loading

AlekseyTs commented Dec 5, 2017 •

edited

Loading

AlekseyTs commented Dec 5, 2017 •

edited

Loading

AlekseyTs Dec 5, 2017 •

edited by OmarTawfik

Loading

AlekseyTs commented Dec 5, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Dec 6, 2017 •

edited

Loading

AlekseyTs Dec 6, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Dec 6, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Dec 6, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Dec 6, 2017 •

edited by OmarTawfik

Loading