Avoid calculating inferred delegate type unnecessarily in conversions and type inference by cston · Pull Request #58115 · dotnet/roslyn

cston · 2021-12-04T23:00:15Z

AlekseyTs · 2021-12-06T23:32:25Z

src/Compilers/CSharp/Portable/Symbols/FunctionTypeSymbol.cs


 namespace Microsoft.CodeAnalysis.CSharp.Symbols
 {
+    internal sealed class InferredDelegateTypeData


InferredDelegateTypeData

Consider adding a doc comment for the type and its member. #Closed

AlekseyTs · 2021-12-06T23:34:11Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

-                Data = new ConcurrentDictionary<object, NullableWalker.Data>();
-            }
-        }
+        internal object? CompilationData;


CompilationData

Consider reflecting test-only nature of this field in the name. For example, "TestOnlyCompilationData". #Closed

AlekseyTs · 2021-12-06T23:35:16Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

-        /// Nullable analysis data for methods, parameter default values, and attributes.
-        /// The key is a symbol for methods or parameters, and syntax for attributes.
-        /// The data is collected during testing only.
+        /// Optional data collected during testing only.


Optional data collected during testing only.

Consider giving examples of type that can be used as values for this field. #Closed

AlekseyTs · 2021-12-06T23:42:54Z

src/Compilers/CSharp/Portable/Symbols/FunctionTypeSymbol.cs

+            var thisDelegateType = GetInternalDelegateType();
+            var otherDelegateType = otherType.GetInternalDelegateType();
+
+            Debug.Assert(thisDelegateType is { });


Debug.Assert(thisDelegateType is { });

It is not obvious why this is a safe assumption. #Closed

AlekseyTs · 2021-12-07T00:00:58Z

        return HasDelegateVarianceConversion(sourceDelegate, destinationDelegate, ref useSiteInfo);

It doesn't look like this function is going to be happy about null arguments.

In reply to: 987389957

Refers to: src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs:2706 in 1acfcdc. [](commit_id = 1acfcdc, deletion_comment = False)

AlekseyTs · 2021-12-07T00:02:10Z

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

            {
-                if (HasImplicitFunctionTypeConversion(sourceFunctionType, destination, ref useSiteInfo))
+                if (HasImplicitFunctionTypeConversion(sourceFunctionType, destination, ref useSiteInfo) &&
+                    sourceFunctionType.GetInternalDelegateType() is { })


sourceFunctionType.GetInternalDelegateType() is { }

I think it would be better if HasImplicitFunctionTypeConversion checked for that when appropriate. #Closed

AlekseyTs · 2021-12-07T00:04:00Z

src/Compilers/CSharp/Portable/Symbols/FunctionTypeSymbol.cs

+    }
+
    /// <summary>
    /// A <see cref="TypeSymbol"/> implementation that represents the inferred signature of a


that represents the inferred signature

Is this still accurate? #Closed

AlekseyTs · 2021-12-07T00:25:32Z

Done with review pass (commit 2)

333fred · 2021-12-07T01:36:11Z

src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs


+        [Fact]
+        [WorkItem(58106, "https://github.com/dotnet/roslyn/issues/58106")]
+        public void InferDelegateType_01()


Are these enough of a stress test that it will demonstrate the perf problem? Or can we create a pathological test that won't compile in any reasonable amount of time without this change? #Resolved

No, these are not stress tests. I decided to test these cases more directly by tracking the InferredDelegateCount instead. We could test perf although that might require a test that is slow even after the fix.

I like tracking the count here as that is directly tracking the problem we are trying to fix here (avoiding unnecessary work).

My biggest fear is making sure that future developers understand why we are tracking the count though. The only documentation here is the issue and the issue doesn't specifically call out that this is a customer scenario. It only mentions that it's causing us to do unnecessary work. I think we should make the issue more specific here, say it caused customer reported problems and link to the VS feedback (or link to the feedback here).

Want to make it clear as possible to future developers why this count is meaningful.

I've added comments to these tests and links to the related issue.

333fred · 2021-12-07T01:36:43Z

Done review pass (commit 2)

AlekseyTs · 2021-12-07T16:38:33Z

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

            if (destination is FunctionTypeSymbol destinationFunctionType)
            {
-                return HasImplicitFunctionTypeToFunctionTypeConversion(source, destinationFunctionType, ref useSiteInfo);
+                if (!HasImplicitFunctionTypeToFunctionTypeConversion(source, destinationFunctionType, ref useSiteInfo))


if (!HasImplicitFunctionTypeToFunctionTypeConversion(source, destinationFunctionType, ref useSiteInfo))

It looks like we can keep the original logic, i.e.

return HasImplicitFunctionTypeToFunctionTypeConversion(source, destinationFunctionType, ref useSiteInfo); ``` #Closed

AlekseyTs · 2021-12-07T16:40:07Z

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

@@ -2699,6 +2705,11 @@ private bool HasImplicitFunctionTypeToFunctionTypeConversion(FunctionTypeSymbol
            var sourceDelegate = sourceType.GetInternalDelegateType();


GetInternalDelegateType

Were all other call sites of this API reviewed and properly adjusted to handle nulls? #Closed

Yes, I have reviewed all use of GetInternalDelegateType().

AlekseyTs · 2021-12-07T16:53:40Z

src/Compilers/CSharp/Portable/Symbols/FunctionTypeSymbol.cs

-            var otherType = (t2 as FunctionTypeSymbol)?._delegateType;
-            return _delegateType.Equals(otherType, compareKind);
+            return t2 is FunctionTypeSymbol otherType &&
+                Equals(GetInternalDelegateType(), otherType.GetInternalDelegateType(), compareKind);


GetInternalDelegateType(), otherType.GetInternalDelegateType()

It looks like two FunctionTypeSymbols failing inference will be considered equal. Is this intentional? #Closed

AlekseyTs · 2021-12-07T17:09:55Z

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

+            {
+                return false;
+            }
+            return source.GetInternalDelegateType() is { };


return source.GetInternalDelegateType() is { };

How confident are we that all places like this one got fixed up? I mean places that do not explicitly use GetInternalDelegateType API, but assume that it is going to return a non-null value just because we are dealing with FunctionTypeSymbol and before this PR that meant that the signature was inferred successfully. #Closed

I believe I have reviewed all callers that use FunctionTypeSymbol directly or indirectly.

The most interesting cases are in MethodTypeInferrer where the bounds collection was (and still is) including FunctionTypeSymbol instances in the set of lower bounds and which now explicitly drops bounds with null delegate type in MethodTypeInferrer.Fix().

AlekseyTs · 2021-12-07T17:16:06Z

Done with review pass (commit 3).

AlekseyTs

LGTM (commit 4), assuming CI is passing.

333fred

LGTM (commit 5)

…rations * upstream/main: (396 commits) Update several ExpressionCompiler unit tests for inferred delegate types (dotnet#58203) Avoid calculating inferred delegate type unnecessarily in conversions and type inference (dotnet#58115) OmniSharp options (dotnet#58208) Fix generator caching in compiler server (dotnet#57177) Clarify use of null as an initialized value BoundDecisionDag.Rewrite - Avoid capturing the replacement map (dotnet#58137) Address feedback Add regex parser tests Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add missing delegate casts (dotnet#58170) Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report Add additional test for another bug report ...

ghost added the Area-Compilers label Dec 4, 2021

cston added 2 commits December 5, 2021 21:49

Avoid calculating inferred delegate type unnecessarily

f68768c

Add tests

1acfcdc

cston force-pushed the 58106 branch from e3e953b to 1acfcdc Compare December 6, 2021 05:51

cston marked this pull request as ready for review December 6, 2021 07:10

cston requested a review from a team as a code owner December 6, 2021 07:11

cston added the Feature - Lambda Improvements label Dec 6, 2021

AlekseyTs reviewed Dec 6, 2021

View reviewed changes

AlekseyTs reviewed Dec 7, 2021

View reviewed changes

333fred reviewed Dec 7, 2021

View reviewed changes

PR feedback

44d9cd6

runfoapp bot mentioned this pull request Dec 7, 2021

[Flaky Test] CSharp.CSharpCodeActions.AddUsingExactMatchBeforeRenameTracking #57982

Closed

AlekseyTs reviewed Dec 7, 2021

View reviewed changes

PR feedback

217ec28

AlekseyTs approved these changes Dec 7, 2021

View reviewed changes

Add comments and links for tests

6acda31

This was referenced Dec 8, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

333fred approved these changes Dec 9, 2021

View reviewed changes

cston merged commit 2ee4427 into dotnet:main Dec 9, 2021

ghost added this to the Next milestone Dec 9, 2021

cston deleted the 58106 branch December 9, 2021 16:51

cston modified the milestones: Next, 17.1.P3 Dec 10, 2021

		@@ -2699,6 +2705,11 @@ private bool HasImplicitFunctionTypeToFunctionTypeConversion(FunctionTypeSymbol
		var sourceDelegate = sourceType.GetInternalDelegateType();

Conversation

cston commented Dec 4, 2021

Uh oh!

AlekseyTs Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 7, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 7, 2021

Uh oh!

333fred Dec 7, 2021 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

jaredpar Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

cston Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

333fred commented Dec 7, 2021

Uh oh!

AlekseyTs Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 7, 2021

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

AlekseyTs Dec 6, 2021 •

edited

Loading

AlekseyTs Dec 6, 2021 •

edited

Loading

AlekseyTs Dec 6, 2021 •

edited

Loading

AlekseyTs Dec 6, 2021 •

edited

Loading

AlekseyTs commented Dec 7, 2021 •

edited by cston

Loading

AlekseyTs Dec 7, 2021 •

edited

Loading

AlekseyTs Dec 7, 2021 •

edited

Loading

333fred Dec 7, 2021 •

edited by cston

Loading

AlekseyTs Dec 7, 2021 •

edited

Loading

AlekseyTs Dec 7, 2021 •

edited

Loading

AlekseyTs Dec 7, 2021 •

edited

Loading

AlekseyTs Dec 7, 2021 •

edited

Loading