Use lambda expression and method group signature in type inference #55786

cston · 2021-08-21T07:38:19Z

Use lambda expression and method group signature in method type inference and best common type calculation.

Support implicit conversions of lambda expressions and method groups to object and interfaces of System.Delegate.

static T Identity<T>(T t) => t;

var m = Identity(Main);    // Action<string[]>
var a = new[] { () => 1 }; // Func<int>[]
object o = () => { };      // Action

The implementation adds a Signature property to UnboundLambda and BoundMethodGroup which represents the inferred "function type" of the lambda or method group - a signature although not tied to a particular delegate or Expression type.
The Signature is used in place of Type (which is null) in a few specific code paths - conversions, best common type, and method type inference - to support new conversions and inference from the "function type".

The signature is implemented as a FunctionTypeSymbol which is a new TypeSymbol kind. That is an implementation detail only, to simplify use in the code paths that are already expecting TypeSymbol. Other than those code paths, instances of FunctionTypeSymbol should not be observable.

Proposal: lambda-improvements.md
Test plan: #52192

cston · 2021-08-26T00:45:51Z

@dotnet/roslyn-compiler, please review.

jcouv · 2021-08-26T16:41:58Z

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

+        {
+            if ((object?)_lazyFunctionType == FunctionTypeSymbol.Uninitialized)
+            {
+                var delegateType = _calculateDelegate(_binder!, _expression!);


Should use an assertion for _expression here. (How do we know it's not null?)
I don't think _binder could be null, so suppression could be removed. #Closed

jcouv · 2021-08-26T16:42:23Z

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

+    internal sealed class FunctionSignature
+    {
+        private readonly AssemblySymbol _assembly;
+        private readonly Binder? _binder;


How can _binder be null? #Closed

jcouv · 2021-08-26T16:42:31Z

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

+
+        internal FunctionSignature(Binder binder, Func<Binder, BoundExpression, NamedTypeSymbol?> calculateDelegate)
+        {
+            _assembly = binder.Compilation.Assembly;


We're already storing the binder, seems like we could do without storing the assembly too. #Closed

jcouv · 2021-08-26T16:53:17Z

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

+        internal void SetExpression(BoundExpression expression)
+        {
+            Debug.Assert((object?)_lazyFunctionType == FunctionTypeSymbol.Uninitialized);
+            Debug.Assert(_expression is null);


nit: The expression is either a BoundMethodGroup or an UnboundLambda. Let's add an assertion for clarity. #Closed

jcouv · 2021-08-26T16:56:57Z

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

+    {
+        internal static readonly FunctionTypeSymbol Uninitialized = new FunctionTypeSymbol();
+
+        private const SymbolKind s_SymbolKind = SymbolKind.FunctionPointerType + 1;


Consider using negative values for both enums (SymbolKind and TypeKind), so that we don't have to update them in the future. Also, I'd add comments in those enums to document the existence of internal entries. #Closed

TypeKind has an underlying type of byte so that value cannot be negative unfortunately. I've changed both values to 255.

And rather than adding comments to the public enums, I've added an assert here that the values are not existing values in the enums. That won't catch clashes with other internal values but it seemed like a simple local solution.

Without a (non-doc) comment in the enum I'm afraid it will be a bit difficult to find what values are being used in the codebase.
Alternatively, we could add an internal type next to the public enum type and put those constants there (next to the enum, which is more centralized).

Moving these next to the public enums makes sense, thanks.

jcouv · 2021-08-26T17:01:10Z

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

+        private const SymbolKind s_SymbolKind = SymbolKind.FunctionPointerType + 1;
+        private const TypeKind s_TypeKind = TypeKind.FunctionPointer + 1;
+
+        private readonly AssemblySymbol? _assembly;


Can _assembly be null?
If not, then we also don't need conditional access on usage (line 70 below)

Oh, I see this is possible for Uninitialized... Maybe we should suppress the nullability warning for that instance (which is not supposed to be used) so that the default assumption fits the normal usage. #Closed

I've left _assembly as nullable since it's possible some of these methods or properties may be hit for the Uninitialized instance - in the debugger for instance.

AlekseyTs · 2021-08-26T17:02:23Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -1966,6 +1967,7 @@
    <!-- Type is not significant for this node type; always null -->
    <Field Name="Type" Type="TypeSymbol?" Override="true" Null="always"/>
    <Field Name="Data" Type="UnboundLambdaState" Null="disallow"/>
+    <Field Name="Signature" Type="FunctionSignature?" Null="allow"/>


This change looks suspiciaous. I believe UnboundLambda node is shared across all attempts to bind/convert a lambda expression. Several of them can succeed and succeed differently. #Closed

The inferred signature is based on the lambda expression only. The signature should be independent of any conversions to specific delegate types.

jcouv · 2021-08-26T17:02:41Z

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

+    {
+        internal static readonly FunctionTypeSymbol Uninitialized = new FunctionTypeSymbol();
+
+        private const SymbolKind s_SymbolKind = SymbolKind.FunctionPointerType + 1;


~~Consider adding an assertion in SymbolDisplay that such symbols never reach there.~~
Never mind. SymbolDisplay works on ISymbol and we can get one from a FunctionTypeSymbol (code throws unreachable below) #Closed

jcouv · 2021-08-26T17:06:14Z

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

+        private readonly NamedTypeSymbol _delegateType;
+
+        private FunctionTypeSymbol()
+        {


_assembly = null!;? #Closed

jcouv · 2021-08-26T17:19:37Z

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

+
+        internal override string GetDebuggerDisplay()
+        {
+            return $"DelegateType: {_delegateType.ToDisplayString(s_debuggerDisplayFormat)}";


_delegateType.GetDebuggerDisplay()?
Then s_debuggerDisplayFormat can remain private. #Closed

_delegateType.GetDebuggerDisplay()?

I like it, thanks.

jcouv · 2021-08-26T17:43:45Z

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

+    [DebuggerDisplay("{GetDebuggerDisplay(),nq}")]
+    internal sealed class FunctionTypeSymbol : TypeSymbol
+    {
+        internal static readonly FunctionTypeSymbol Uninitialized = new FunctionTypeSymbol();


nit: Should we also use a singleton for the instance with null _delegateType? #Closed

There are no instances of FunctionTypeSymbol with null _delegateType currently.

Were you suggesting using such a singleton instance rather than null for FunctionSignature._lazyFunctionType? If so, I think that would just change how callers of FunctionSignature.GetSignatureAsTypeSymbol() check for null.

I got confused by this code:

private bool HasImplicitFunctionTypeConversion(FunctionTypeSymbol source, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo) { if (source.GetInternalDelegateType() is null) { return false; } ...

But it's actually not possible for FunctionTypeSymbol.GetInternalDelegateType() to return null.

AlekseyTs · 2021-08-27T16:19:08Z

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

+        internal bool IsAssignableFromMulticastDelegate(TypeSymbol type)
+        {
+            var useSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
+            return ClassifyImplicitConversionFromType(corLibrary.GetSpecialType(SpecialType.System_MulticastDelegate), type, ref useSiteInfo).Exists;


corLibrary.GetSpecialType(SpecialType.System_MulticastDelegate)

If useSiteInfo becomes a parameter, we need to pick up info from this symbol as well, I think. #Closed

AlekseyTs · 2021-08-27T16:21:38Z

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

+            return IsValidFunctionTypeConversionTarget(destination, ref useSiteInfo);
+        }
+
+        internal bool IsValidFunctionTypeConversionTarget(TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)


IsValidFunctionTypeConversionTarget

Echoing Julien's concern about the difference between the implementation and the spec.

AlekseyTs · 2021-08-27T16:35:07Z

src/Compilers/Core/Portable/Symbols/SymbolKind.cs

+        /// Internal Symbol representing the inferred signature of
+        /// a lambda expression or method group.
+        /// </summary>
+        internal const SymbolKind FunctionType = (SymbolKind)255;


FunctionType

Cannot say I am happy about this approach, but cannot think of a "better" alternative at the moment.

AlekseyTs · 2021-08-27T16:52:40Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

@@ -8636,7 +8619,6 @@ static bool isCandidateUnique(ref MethodSymbol? method, MethodSymbol candidate)
                if (wkDelegateType != WellKnownType.Unknown)
                {
                    var delegateType = Compilation.GetWellKnownType(wkDelegateType);
-                    delegateType.AddUseSiteInfo(ref useSiteInfo);


delegateType.AddUseSiteInfo(ref useSiteInfo);

It is not obvious why we don't want to do this. #Closed

The caller of GetMethodGroupOrLambdaDelegateType() is responsible for checking any use-site diagnostics on the return type. Added a comment.

AlekseyTs · 2021-08-27T17:02:06Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                    functionType.GetValue() is null)
+                {
+                    var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
+                    if (targetType.IsExpressionTree() ||


targetType.IsExpressionTree()

This is an expression tree with a target delegate type. Correct? In this case how inferring type from lambda would help? #Closed

AlekseyTs · 2021-08-27T17:02:50Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                    if (targetType.IsExpressionTree() ||
+                        Conversions.IsValidFunctionTypeConversionTarget(targetType, ref discardedUseSiteInfo))
+                    {
+                        Error(diagnostics, ErrorCode.ERR_CannotInferDelegateType, syntax);


Error(diagnostics, ErrorCode.ERR_CannotInferDelegateType, syntax);

Should reporting of this error be conditioned on a language version? #Closed

We only hit this code path if FunctionType is { } which requires language version 10 or higher.

jcouv · 2021-08-27T17:15:24Z

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs

-                BoundMethodGroup methodGroup => GetMethodGroupDelegateType(methodGroup, ref useSiteInfo),
-                _ => throw ExceptionUtilities.UnexpectedValue(expr),
-            };
+            var delegateType = expr.GetInferredDelegateType(ref useSiteInfo);


Consider adding assertion here (only BoundMethodGroup or UnboundLambda) #Closed

AlekseyTs · 2021-08-27T17:21:00Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

-                conversion = Conversions.ClassifyConversionFromExpression(expr, destination, ref useSiteInfo);
-                diagnostics.Add(syntax, useSiteInfo);
-                return CreateConversion(syntax, expr, conversion, isCast, conversionGroup, destination, diagnostics);
+                delegateType = Compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression_T).Construct(delegateType);


Compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression_T)

Why do we assume there is a relationship between the two types? #Closed

Added tests where Expression<T> is not derived from Expression.

AlekseyTs · 2021-08-27T17:21:53Z

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

+                {
+                    return false;
+                }
+                var expressionType = compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression);


var expressionType = compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression);

Helpers in TypeSymbolExtensions do not use well known types to check if a type is an Expression. Doing this here feels inconsistent. #Closed

jcouv · 2021-08-27T17:27:58Z

src/Compilers/CSharp/Portable/BoundTree/BoundExpressionExtensions.cs

+            return delegateType;
+        }
+
+        public static TypeSymbol? GetTypeOrSignature(this BoundExpression expr)


GetTypeOrSignature

nit: maybe GetTypeOrFunctionType #Closed

AlekseyTs · 2021-08-27T17:34:38Z

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

@@ -273,6 +272,7 @@ private enum Dependency
            }

            var inferrer = new MethodTypeInferrer(
+                binder.Compilation,


binder.Compilation

Taking dependency on entire compilation object feels like a big hammer. Is it possible to narrow the dependency? #Closed

After modifying isExpressionType() to use TypeSymbolExtensions.IsGenericOrNonGenericExpressionType(), the only use of CSharpCompilation is the use of compilation.GetWellKnownType(WellKnownType.System_Linq_Expressions_Expression_T) to construct the inferred type. If needed, we could pass in that well-known type to MethodTypeInferrer rather than passing in the CSharpCompilation. For now, I'll leave as-is.

jcouv · 2021-08-27T17:55:38Z

    var a2 = new[] { F1, F2 };

Consider mixing method group and lambda.
nit: Also consider mixing lambda and static lambda (static doesn't matter)

In reply to: 907373662

In reply to: 907373662 #Pending

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs:3418 in 5a2911a. [](commit_id = 5a2911a, deletion_comment = False)

jcouv · 2021-08-27T17:57:38Z

    public void LambdaReturn_01()

nit: In addition to arrays and lambda returns, consider testing ??. It's not relying on best common type if I recall, and it's not very useful, but may be worth covering: () => {} ?? M.
I assume it'll fail.

In reply to: 907374774 #Pending

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs:3540 in 5a2911a. [](commit_id = 5a2911a, deletion_comment = False)

jcouv · 2021-08-27T18:14:09Z

Here's a few exploratory test ideas:

nullability: var x = () => {};, var x = M;, var x = new[] { () => { } };, T M<T>(T t) where T : System.Delegate? called with () => {} or M, then verifying inferred nullability
local functions: use a local function for method group
stackalloc instead of array creation
lambda or method group in various contexts: switch (M) { case Delegate: ... }, var (x, y) = M;, _ = M with { };, using/lock/await/interpolation hole/throw
best common type and method type inference between a function type and null or default #Pending

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DelegateTypeTests.cs:4717 in 5a2911a. [](commit_id = 5a2911a, deletion_comment = False)

jcouv

Done with review pass (iteration 20)

AlekseyTs · 2021-08-27T18:30:16Z

Done with review pass (commit 20), tests are not looked at.

jcouv

LGTM Thanks (iteration 25)

AlekseyTs

LGTM (commit 25)

dotnet-issue-labeler bot added the Area-Compilers label Aug 21, 2021

cston force-pushed the lambdas-type-inference branch 2 times, most recently from fa52250 to 7d295dc Compare August 23, 2021 00:15

Use lambda signatures and method group signatures in type inference

2572169

cston force-pushed the lambdas-type-inference branch from 069d047 to 2572169 Compare August 24, 2021 07:05

Misc.

3b3f294

cston marked this pull request as ready for review August 24, 2021 17:35

cston requested review from a team as code owners August 24, 2021 17:35

cston added the New Feature - Lambda Improvements label Aug 24, 2021

cston added 4 commits August 24, 2021 23:53

Misc.

9424c62

Update method type inference

b3176ff

Variance

5da002f

Resolve PROTOTYPE comments

763f5b5

cston added 3 commits August 25, 2021 17:55

Misc.

7b21e1a

Formatting

3fefaa3

Avoid holding onto lambda cache

dca307d

jcouv reviewed Aug 26, 2021

View reviewed changes

AlekseyTs reviewed Aug 26, 2021

View reviewed changes

jcouv reviewed Aug 26, 2021

View reviewed changes

PR feedback

a0f8caf

jcouv reviewed Aug 26, 2021

View reviewed changes

AlekseyTs reviewed Aug 27, 2021

View reviewed changes

jcouv reviewed Aug 27, 2021

View reviewed changes

AlekseyTs reviewed Aug 27, 2021

View reviewed changes

jcouv reviewed Aug 27, 2021

View reviewed changes

PR feedback

5a2911a

AlekseyTs reviewed Aug 27, 2021

View reviewed changes

jcouv reviewed Aug 27, 2021

View reviewed changes

cston added 5 commits August 27, 2021 11:34

Rename file

df2a2d9

PR feedback

772d7ed

More tests

24e29e9

Test Expression<T> not derived from Expression

890e3f3

Update Breaking Changes document

63f5795

jcouv approved these changes Aug 27, 2021

View reviewed changes

AlekseyTs approved these changes Aug 27, 2021

View reviewed changes

cston merged commit 4b95718 into dotnet:main Aug 28, 2021

ghost added this to the Next milestone Aug 28, 2021

cston deleted the lambdas-type-inference branch August 28, 2021 01:34

cston modified the milestones: Next, 17.0.P4 Aug 28, 2021

cston mentioned this pull request Aug 31, 2021

Avoid calculating the inferred delegate type multiple times #52869

Closed

Use lambda expression and method group signature in type inference #55786

Use lambda expression and method group signature in type inference #55786

Conversation

cston commented Aug 21, 2021 • edited

cston commented Aug 26, 2021

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

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 Aug 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 26, 2021 • edited

Choose a reason for hiding this comment

cston Aug 26, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Aug 26, 2021 • edited

Choose a reason for hiding this comment

cston Aug 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 27, 2021 • edited

Choose a reason for hiding this comment

AlekseyTs Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Aug 27, 2021 • edited

Choose a reason for hiding this comment

AlekseyTs Aug 27, 2021 • edited

Choose a reason for hiding this comment

cston Aug 27, 2021 • edited

Choose a reason for hiding this comment

AlekseyTs Aug 27, 2021 • edited

Choose a reason for hiding this comment

jcouv Aug 27, 2021 • edited

Choose a reason for hiding this comment

AlekseyTs Aug 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Aug 27, 2021 • edited

jcouv commented Aug 27, 2021 • edited

jcouv commented Aug 27, 2021 • edited

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Aug 27, 2021

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Aug 21, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

AlekseyTs Aug 26, 2021 •

edited

cston Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

jcouv Aug 26, 2021 •

edited

cston Aug 26, 2021 •

edited

AlekseyTs Aug 27, 2021 •

edited

AlekseyTs Aug 27, 2021 •

edited

AlekseyTs Aug 27, 2021 •

edited

AlekseyTs Aug 27, 2021 •

edited

jcouv Aug 27, 2021 •

edited

AlekseyTs Aug 27, 2021 •

edited

cston Aug 27, 2021 •

edited

AlekseyTs Aug 27, 2021 •

edited

jcouv Aug 27, 2021 •

edited

AlekseyTs Aug 27, 2021 •

edited

jcouv commented Aug 27, 2021 •

edited

jcouv commented Aug 27, 2021 •

edited

jcouv commented Aug 27, 2021 •

edited