Skip user-defined conversions when checking extension method 'this' argument #19511

cston · 2017-05-15T18:24:36Z

Customer scenario

Invoke extension methods where the receiver type has user-defined conversion operators. This change affects performance only, not correctness.

Bugs this fixes:

434957

Workarounds, if any

None

Risk

Medium. The change avoids looking for user-defined conversion operators that were already being ignored by the caller for the this argument, although that argument conversion is now handled by a new specific method.

Performance impact

Improves performance

Is this a regression from a previous update?

Yes, a regression from VS2013.

How was the bug found?

Customer reported

gafter

LGTM

cston · 2017-05-17T06:24:32Z

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

@@ -54,67 +52,6 @@ public override Conversion GetMethodGroupConversion(BoundMethodGroup source, Typ
            return conversion;
        }

-        protected override Conversion GetImplicitTupleLiteralConversion(BoundTupleLiteral source, TypeSymbol destination, ref HashSet<DiagnosticInfo> useSiteDiagnostics)


Methods have moved to ConversionsBase since the implementation did not depend on the derived class.

cston · 2017-05-17T06:26:10Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -1340,14 +1391,15 @@ public static bool HasIdentityConversionToAny<T>(T type, ArrayBuilder<T> targetT
        public Conversion ConvertExtensionMethodThisArg(TypeSymbol parameterType, TypeSymbol thisType, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
        {
            Debug.Assert((object)thisType != null);
-            var conversion = this.ClassifyImplicitConversionFromType(thisType, parameterType, ref useSiteDiagnostics);
+            var conversion = this.ClassifyImplicitExtensionMethodThisArgConversion(null, thisType, parameterType, ref useSiteDiagnostics);
            return IsValidExtensionMethodThisArgConversion(conversion) ? conversion : Conversion.NoConversion;
        }

        // Spec 7.6.5.2: "An extension method ... is eligible if ... [an] implicit identity, reference,
        // or boxing conversion exists from expr to the type of the first parameter"
        public static bool IsValidExtensionMethodThisArgConversion(Conversion conversion)


This method should be replaced by an assert.

Yes. I did not notice your comment and was puzzled at why IsValidExtensionMethodThisArgConversion still needs to be called.

cston · 2017-05-17T06:27:01Z

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


-                    if (conversion.IsImplicit)


Implicit conversions are no longer generated for this argument.

cston · 2017-05-17T06:27:51Z

@dotnet/roslyn, @AlekseyTs please review.

VSadov

LGTM

gafter

LGTM

cston · 2017-05-18T01:28:27Z

@MeiChin-Tsai for approval

cston · 2017-05-18T03:39:05Z

@dotnet-bot test windows_release_vs-integration_prtest please

jcouv · 2017-05-18T03:49:46Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+            return Conversion.NoConversion;
+        }
+
+        // It should be possible to remove IsValidExtensionMethodThisArgConversion


File follow-up issue?

Logged #19622

jcouv · 2017-05-18T03:55:58Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+
+            if ((object)sourceType != null)
+            {
+                var tupleConversion = ClassifyTupleConversion(


I didn't quite follow. How do we know that we're looking at a tuple at this point?

We don't know we have a tuple at this point but ClassifyTupleConversion returns NoConversion if either the source or destination is not a tuple. This follows the pattern used elsewhere in ConversionsBase. See the call to ClassifyImplicitTupleConversion in ClassifyStandardImplicitConversion for instance.

jcouv · 2017-05-18T04:08:23Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+            TypeSymbol destination,
+            ref HashSet<DiagnosticInfo> useSiteDiagnostics,
+            ConversionKind kind,
+            ClassifyConversionFromExpressionDelegate classifyConversion,


Would it be simpler to pass options, rather than a delegate? Seems like there are three cases: implicit, explicit and implicit-for-this-in-extension. Maybe two bools, or a small enum?

Unfortunately the code didn't seem any clearer since it added a switch in each method and meant each method took two enum values, one for ConversionKind and one for new enum.

I have changed the delegates to take an additional ConversionsBase argument though, so that the delegate instances are static and cached.

jcouv · 2017-05-18T04:10:32Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+            TypeSymbol destination,
+            ref HashSet<DiagnosticInfo> useSiteDiagnostics,
+            ConversionKind kind,
+            ClassifyConversionFromTypeDelegate classifyConversion,


Same question here.
Thanks for the factoring of common code, btw.

jcouv · 2017-05-18T04:21:37Z

src/Compilers/CSharp/Test/Symbol/Symbols/ConversionTests.cs

@@ -1986,6 +1986,138 @@ static void Main()
                Diagnostic(ErrorCode.ERR_NoImplicitConv, "0").WithArguments("int", "R"));
        }

+        [Fact]
+        [WorkItem(434957, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=434957")]
+        public void SkipUserDefinedConversionsForThisArgument()


Just to confirm: this test (and the following 2 or three) already produced errors (just different error code) before your change?

Yes, these tests also (correctly) produced errors before this change. The tests made more sense with an earlier iteration of this change and are now likely redundant, and I considered removing them.

jcouv

LGTM

MeiChin-Tsai · 2017-05-18T17:18:02Z

Please make sure all checks are successful. Do we want to add more tests given that the risk is medium? Approved since it is a regression. thx.

jasonmalinowski · 2017-05-18T22:49:14Z

@dotnet-bot retest windows_release_vs-integration_prtest please.

cston · 2017-05-19T03:00:33Z

@dotnet-bot retest windows_release_vs-integration_prtest please

…rgument

cston · 2017-05-19T03:32:38Z

@MeiChin-Tsai, added tests for more conversion cases.

cston · 2017-05-19T04:04:45Z

@dotnet-bot retest Ubuntu_16_debug_prtest please

dnfclas added the cla-already-signed label May 15, 2017

gafter approved these changes May 15, 2017

View reviewed changes

gafter added Area-Compilers Tenet-Performance Regression in measured performance of the product from goals. labels May 15, 2017

gafter added this to the 15.3 milestone May 15, 2017

cston force-pushed the skip-udc branch 3 times, most recently from da9be0a to 567f3a0 Compare May 17, 2017 06:15

cston commented May 17, 2017

View reviewed changes

VSadov approved these changes May 17, 2017

View reviewed changes

cston force-pushed the skip-udc branch from 567f3a0 to 7e9492a Compare May 17, 2017 20:50

gafter approved these changes May 18, 2017

View reviewed changes

jcouv reviewed May 18, 2017

View reviewed changes

jcouv approved these changes May 18, 2017

View reviewed changes

cston force-pushed the skip-udc branch from 7e9492a to bd34deb Compare May 18, 2017 17:28

Skip user-defined conversions when checking extension method 'this' a…

7cc216e

…rgument

cston force-pushed the skip-udc branch from bd34deb to 7cc216e Compare May 19, 2017 03:31

cston merged commit 41cdec1 into dotnet:master May 19, 2017

cston deleted the skip-udc branch May 19, 2017 04:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip user-defined conversions when checking extension method 'this' argument #19511

Skip user-defined conversions when checking extension method 'this' argument #19511

cston commented May 15, 2017 •

edited

Loading

gafter left a comment

cston May 17, 2017

cston May 17, 2017

VSadov May 17, 2017

cston May 17, 2017

cston commented May 17, 2017

VSadov left a comment

gafter left a comment

cston commented May 18, 2017

cston commented May 18, 2017

jcouv May 18, 2017

cston May 18, 2017

jcouv May 18, 2017

cston May 18, 2017

jcouv May 18, 2017

cston May 18, 2017 •

edited

Loading

jcouv May 18, 2017

jcouv May 18, 2017

cston May 18, 2017

jcouv left a comment

MeiChin-Tsai commented May 18, 2017

jasonmalinowski commented May 18, 2017

cston commented May 19, 2017

cston commented May 19, 2017

cston commented May 19, 2017

Skip user-defined conversions when checking extension method 'this' argument #19511

Skip user-defined conversions when checking extension method 'this' argument #19511

Conversation

cston commented May 15, 2017 • edited Loading

gafter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented May 17, 2017

VSadov left a comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

cston commented May 18, 2017

cston commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

MeiChin-Tsai commented May 18, 2017

jasonmalinowski commented May 18, 2017

cston commented May 19, 2017

cston commented May 19, 2017

cston commented May 19, 2017

cston commented May 15, 2017 •

edited

Loading

cston May 18, 2017 •

edited

Loading