Skip to content

Commit

Permalink
Prefer by-val methods over in methods in overload resolution (#23122)
Browse files Browse the repository at this point in the history 
* Prefer by-val methods over in methdos in overload resolution

* More tests

* Moved check to the end of OR chain

* Address PR comments

* Operators overloading

* Address PR Comments

* Clean up
  • Loading branch information
@OmarTawfik
OmarTawfik authored Dec 8, 2017
1 parent cd90b27 commit 919b606
Show file tree
Hide file tree
Showing 6 changed files with 1,201 additions and 101 deletions.
41 changes: 40 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorOverloadResolution.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1003,7 +1003,46 @@ private BetterResult BetterOperator(BinaryOperatorSignature op1, BinaryOperatorS
}
}

return BetterResult.Neither;
// Always prefer operators with val parameters over operators with in parameters:
BetterResult valOverInPreference;

if (op1.LeftRefKind == RefKind.None && op2.LeftRefKind == RefKind.In)
{
valOverInPreference = BetterResult.Left;
}
else if (op2.LeftRefKind == RefKind.None && op1.LeftRefKind == RefKind.In)
{
valOverInPreference = BetterResult.Right;
}
else
{
valOverInPreference = BetterResult.Neither;
}

if (op1.RightRefKind == RefKind.None && op2.RightRefKind == RefKind.In)
{
if (valOverInPreference == BetterResult.Right)
{
return BetterResult.Neither;
}
else
{
valOverInPreference = BetterResult.Left;
}
}
else if (op2.RightRefKind == RefKind.None && op1.RightRefKind == RefKind.In)
{
if (valOverInPreference == BetterResult.Left)
{
return BetterResult.Neither;
}
else
{
valOverInPreference = BetterResult.Right;
}
}

return valOverInPreference;
}

private BetterResult MoreSpecificOperator(BinaryOperatorSignature op1, BinaryOperatorSignature op2, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
Expand Down
45 changes: 43 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Operators/BinaryOperatorSignature.cs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.CodeAnalysis.CSharp.Symbols;
using System;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
Expand Down Expand Up @@ -35,7 +36,7 @@ public BinaryOperatorSignature(BinaryOperatorKind kind, TypeSymbol leftType, Typ

public override string ToString()
{
return $"kind: {this.Kind} left: {this.LeftType} right: {this.RightType} return: {this.ReturnType}";
return $"kind: {this.Kind} leftType: {this.LeftType} leftRefKind: {this.LeftRefKind} rightType: {this.RightType} rightRefKind: {this.RightRefKind} return: {this.ReturnType}";
}

public bool Equals(BinaryOperatorSignature other)
Expand Down Expand Up @@ -70,5 +71,45 @@ public override int GetHashCode()
Hash.Combine(RightType,
Hash.Combine(Method, (int)Kind))));
}

public RefKind LeftRefKind
{
get
{
if ((object)Method != null)
{
Debug.Assert(Method.ParameterCount == 2);

if (!Method.ParameterRefKinds.IsDefaultOrEmpty)
{
Debug.Assert(Method.ParameterRefKinds.Length == 2);

return Method.ParameterRefKinds[0];
}
}

return RefKind.None;
}
}

public RefKind RightRefKind
{
get
{
if ((object)Method != null)
{
Debug.Assert(Method.ParameterCount == 2);

if (!Method.ParameterRefKinds.IsDefaultOrEmpty)
{
Debug.Assert(Method.ParameterRefKinds.Length == 2);

return Method.ParameterRefKinds[1];
}
}

return RefKind.None;
}
}
}
}
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorOverloadResolution.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,16 @@ private BetterResult BetterOperator(UnaryOperatorSignature op1, UnaryOperatorSig
}
}

// Always prefer operators with val parameters over operators with in parameters:
if (op1.RefKind == RefKind.None && op2.RefKind == RefKind.In)
{
return BetterResult.Left;
}
else if (op2.RefKind == RefKind.None && op1.RefKind == RefKind.In)
{
return BetterResult.Right;
}

return BetterResult.Neither;
}

Expand Down
26 changes: 23 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Operators/UnaryOperatorSignature.cs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp
{
Expand All @@ -25,7 +25,27 @@ public UnaryOperatorSignature(UnaryOperatorKind kind, TypeSymbol operandType, Ty

public override string ToString()
{
return $"kind: {this.Kind} operand: {this.OperandType} return: {this.ReturnType}";
return $"kind: {this.Kind} operandType: {this.OperandType} operandRefKind: {this.RefKind} return: {this.ReturnType}";
}

public RefKind RefKind
{
get
{
if ((object)Method != null)
{
Debug.Assert(Method.ParameterCount == 1);

if (!Method.ParameterRefKinds.IsDefaultOrEmpty)
{
Debug.Assert(Method.ParameterRefKinds.Length == 1);

return Method.ParameterRefKinds.Single();
}
}

return RefKind.None;
}
}
}
}
112 changes: 83 additions & 29 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1234,20 +1234,11 @@ private void RemoveWorseMembers<TMember>(ArrayBuilder<MemberResolutionResult<TMe
worse.Free();
}

// Return the parameter type corresponding to the given argument index.
private static TypeSymbol GetParameterType(int argIndex, MemberAnalysisResult result, ImmutableArray<ParameterSymbol> parameters)
/// <summary>
/// Returns the parameter type (considering params).
/// </summary>
private static TypeSymbol GetParameterType(ParameterSymbol parameter, MemberAnalysisResult result)
{
RefKind discarded;
return GetParameterType(argIndex, result, parameters, out discarded);
}

// Return the parameter type corresponding to the given argument index.
private static TypeSymbol GetParameterType(int argIndex, MemberAnalysisResult result, ImmutableArray<ParameterSymbol> parameters, out RefKind refKind)
{
int paramIndex = result.ParameterFromArgument(argIndex);
ParameterSymbol parameter = parameters[paramIndex];
refKind = parameter.RefKind;

if (result.Kind == MemberResolutionKind.ApplicableInExpandedForm &&
parameter.IsParams && parameter.Type.IsSZArray())
{
Expand All @@ -1259,6 +1250,15 @@ private static TypeSymbol GetParameterType(int argIndex, MemberAnalysisResult re
}
}

/// <summary>
/// Returns the parameter corresponding to the given argument index.
/// </summary>
private static ParameterSymbol GetParameter(int argIndex, MemberAnalysisResult result, ImmutableArray<ParameterSymbol> parameters)
{
int paramIndex = result.ParameterFromArgument(argIndex);
return parameters[paramIndex];
}

private BetterResult BetterFunctionMember<TMember>(
MemberResolutionResult<TMember> m1,
MemberResolutionResult<TMember> m2,
Expand Down Expand Up @@ -1320,6 +1320,9 @@ private BetterResult BetterFunctionMember<TMember>(
// implicit conversion from EX to PX, and for at least one argument, the conversion from
// EX to PX is better than the conversion from EX to QX.

var m1LeastOverridenParameters = m1.LeastOverriddenMember.GetParameters();
var m2LeastOverridenParameters = m2.LeastOverriddenMember.GetParameters();

bool allSame = true; // Are all parameter types equivalent by identify conversions, ignoring Task-like differences?
int i;
for (i = 0; i < arguments.Count; ++i)
Expand All @@ -1335,20 +1338,22 @@ private BetterResult BetterFunctionMember<TMember>(
continue;
}

RefKind refKind1, refKind2;
var type1 = GetParameterType(i, m1.Result, m1.LeastOverriddenMember.GetParameters(), out refKind1);
var type2 = GetParameterType(i, m2.Result, m2.LeastOverriddenMember.GetParameters(), out refKind2);
var parameter1 = GetParameter(i, m1.Result, m1LeastOverridenParameters);
var type1 = GetParameterType(parameter1, m1.Result);

var parameter2 = GetParameter(i, m2.Result, m2LeastOverridenParameters);
var type2 = GetParameterType(parameter2, m2.Result);

bool okToDowngradeToNeither;
BetterResult r;

r = BetterConversionFromExpression(arguments[i],
type1,
m1.Result.ConversionForArg(i),
refKind1,
parameter1.RefKind,
type2,
m2.Result.ConversionForArg(i),
refKind2,
parameter2.RefKind,
considerRefKinds,
ref useSiteDiagnostics,
out okToDowngradeToNeither);
Expand Down Expand Up @@ -1466,11 +1471,12 @@ private BetterResult BetterFunctionMember<TMember>(
continue;
}

RefKind refKind1, refKind2;
var type1 = GetParameterType(i, m1.Result, m1.LeastOverriddenMember.GetParameters(), out refKind1);
var type2 = GetParameterType(i, m2.Result, m2.LeastOverriddenMember.GetParameters(), out refKind2);

var parameter1 = GetParameter(i, m1.Result, m1LeastOverridenParameters);
var type1 = GetParameterType(parameter1, m1.Result);
var type1Normalized = type1.NormalizeTaskTypes(Compilation);

var parameter2 = GetParameter(i, m2.Result, m2LeastOverridenParameters);
var type2 = GetParameterType(parameter2, m2.Result);
var type2Normalized = type2.NormalizeTaskTypes(Compilation);

if (Conversions.ClassifyImplicitConversionFromType(type1Normalized, type2Normalized, ref useSiteDiagnostics).Kind != ConversionKind.Identity)
Expand Down Expand Up @@ -1527,7 +1533,7 @@ private BetterResult BetterFunctionMember<TMember>(
}
}

return BetterResult.Neither;
return PreferValOverInParameters(arguments, m1, m1LeastOverridenParameters, m2, m2LeastOverridenParameters);
}

// If MP is a non-generic method and MQ is a generic method, then MP is better than MQ.
Expand Down Expand Up @@ -1620,8 +1626,11 @@ private BetterResult BetterFunctionMember<TMember>(
continue;
}

uninst1.Add(GetParameterType(i, m1.Result, m1Original));
uninst2.Add(GetParameterType(i, m2.Result, m2Original));
var parameter1 = GetParameter(i, m1.Result, m1Original);
uninst1.Add(GetParameterType(parameter1, m1.Result));

var parameter2 = GetParameter(i, m2.Result, m2Original);
uninst2.Add(GetParameterType(parameter2, m2.Result));
}

result = MoreSpecificType(uninst1, uninst2, ref useSiteDiagnostics);
Expand All @@ -1636,7 +1645,7 @@ private BetterResult BetterFunctionMember<TMember>(
// UNDONE: Otherwise if one member is a non-lifted operator and the other is a lifted
// operator, the non-lifted one is better.

// The penultimate rule: Position in interactive submission chain. The last definition wins.
// Otherwise: Position in interactive submission chain. The last definition wins.
if (m1.Member.ContainingType.TypeKind == TypeKind.Submission && m2.Member.ContainingType.TypeKind == TypeKind.Submission)
{
// script class is always defined in source:
Expand All @@ -1656,16 +1665,61 @@ private BetterResult BetterFunctionMember<TMember>(
}
}

// Finally, if one has fewer custom modifiers, that is better
// Otherwise, if one has fewer custom modifiers, that is better
int m1ModifierCount = m1.LeastOverriddenMember.CustomModifierCount();
int m2ModifierCount = m2.LeastOverriddenMember.CustomModifierCount();
if (m1ModifierCount != m2ModifierCount)
{
return (m1ModifierCount < m2ModifierCount) ? BetterResult.Left : BetterResult.Right;
}

// Otherwise, neither function member is better.
return BetterResult.Neither;
// Otherwise, prefer methods with 'val' parameters over 'in' parameters.
return PreferValOverInParameters(arguments, m1, m1LeastOverridenParameters, m2, m2LeastOverridenParameters);
}

private static BetterResult PreferValOverInParameters<TMember>(
ArrayBuilder<BoundExpression> arguments,
MemberResolutionResult<TMember> m1,
ImmutableArray<ParameterSymbol> parameters1,
MemberResolutionResult<TMember> m2,
ImmutableArray<ParameterSymbol> parameters2)
where TMember : Symbol
{
BetterResult valOverInPreference = BetterResult.Neither;

for (int i = 0; i < arguments.Count; ++i)
{
if (arguments[i].Kind != BoundKind.ArgListOperator)
{
var p1 = GetParameter(i, m1.Result, parameters1);
var p2 = GetParameter(i, m2.Result, parameters2);

if (p1.RefKind == RefKind.None && p2.RefKind == RefKind.In)
{
if (valOverInPreference == BetterResult.Right)
{
return BetterResult.Neither;
}
else
{
valOverInPreference = BetterResult.Left;
}
}
else if (p2.RefKind == RefKind.None && p1.RefKind == RefKind.In)
{
if (valOverInPreference == BetterResult.Left)
{
return BetterResult.Neither;
}
else
{
valOverInPreference = BetterResult.Right;
}
}
}
}

return valOverInPreference;
}

private static void GetParameterCounts<TMember>(MemberResolutionResult<TMember> m, ArrayBuilder<BoundExpression> arguments, out int declaredParameterCount, out int parametersUsedIncludingExpansionAndOptional) where TMember : Symbol
Expand Down
Loading

0 comments on commit 919b606

Please sign in to comment.