Lambda expression explicit return type: binding #54075

cston · 2021-06-14T05:39:48Z

Support optional explicit return type for lambda expressions with parenthesized parameter list:

Func<T> f = T () => default;
Action<T> a = static void (T t) => { };

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

jcouv · 2021-06-15T05:24:20Z

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

+        }
+
+        [Fact]
+        public void LambdaReturnType_02()


Could you add a similar test, but with one of the type parameters constrained by the other type parameter (where T : U for example)? #Closed

jcouv · 2021-06-15T05:27:20Z

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

-            // At this point we know that we have either a delegate type or an expression type for the target.
+            if (reason == LambdaConversionResult.MismatchedReturnType)
+            {
+                Error(diagnostics, ErrorCode.ERR_CantConvAnonMethReturnType, syntax, id, targetType);


nit: The name of the error code suggests that conversions have something to do, but the logic and message are that the types have to match. Consider renaming the error code and diagnostic. #Closed

Never mind. I see that we're using same naming convention for parameter types (ERR_CantConvAnonMethParams). Fine as-is then

jcouv · 2021-06-15T05:39:28Z

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

+            if (anonymousFunction.HasExplicitReturnType(out var refKind, out var returnType))
+            {
+                if (invokeMethod.RefKind != refKind ||
+                    !invokeMethod.ReturnType.Equals(returnType.Type, TypeCompareKind.AllIgnoreOptions))


Are we sure this is the rule we want? Does this fall out of existing spec rules, or needs to be spec'ed?

On one hand, Func<int> f = localFunctionReturningShort; an error (with short localFunctionReturningShort()). So Func<int> f = short () => 1; makes sense to error as well.
On the other hand, I think Func<object, int> f = (o) => 1; involves a conversion from expression which gives o its object type, so why wouldn't that conversion also handle Func<object, int> f = short (o) => 1? #Closed

Also, is AllIgnoreOptions the right option? ~~Can we include tests for types differing by small differences (object vs. dynamic, nullability, custom modifiers)?~~

Update: I see LambdaReturnType_04 covers most of those. Is there a way to get into a situation with a custom modifiers difference as well?

Are we sure this is the rule we want? Does this fall out of existing spec rules, or needs to be spec'ed?

This should be confirmed with LDM, but I believe it corresponds to the behavior for parameters. For instance, Action<string> a = (object o) => { }; results in a conversion error (see sharplab.io).

Can we include tests for types differing by small differences (object vs. dynamic, nullability, custom modifiers)?

There are tests for object and dynamic, tuple element names, nint and IntPtr, and nullable differences.

You're correct, this is the behavior for parameters: https://github.com/dotnet/csharplang/blob/main/spec/conversions.md#anonymous-function-conversions
"If F has an explicitly typed parameter list, each parameter in D has the same type and modifiers as the corresponding parameter in F."

We probably need to add a similar rule to the spec for the return type.

These things should just fall out. Variance only allows for implicit reference conversions, nothing else, because they need to be representation-preserving.

Is there a way to get into a situation with a custom modifiers difference as well?

Added tests for custom modifiers.

jcouv · 2021-06-15T05:49:37Z

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

+            MessageID.IDS_FeatureLambdaReturnType.CheckFeatureAvailability(diagnostics, syntax);
+
+            RefKind refKind = RefKind.None;
+            if (syntax is RefTypeSyntax refTypeSyntax)


nit: There's a helper that might be re-used here: syntax = syntax.ReturnType.SkipRef(out RefKind refKind);
#Closed

Updated, thanks.

jcouv · 2021-06-15T05:56:19Z

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

+    static void Main()
+    {
+        Delegate d;
+        d = (ref void () => { });


Let's add a passing test involving a void-returning lambda too. #Closed

I couldn't find the test you added to address this comment

I updated DelegateTypeTests.GetLambdaData() which is used for tests that compile and report the signature of the lambdas.

jcouv · 2021-06-15T06:04:50Z

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

-                        declarationMap.Add(s.Name, s);
+                        var name = parameter.Name;
+                        this.parameterMap.Add(name, parameter);
+                        if (!_definitionMap.ContainsKey(name))


We don't need the IsDiscard check after all (either on parameterMap or on _definitionMap)? #Closed

Oops, missed it above ;-)

jcouv · 2021-06-15T06:26:50Z

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

@@ -165,6 +165,7 @@ public TypeWithAnnotations GetInferredReturnType(ConversionsBase? conversions, N
        internal static InferredLambdaReturnType InferReturnType(ArrayBuilder<(BoundReturnStatement, TypeWithAnnotations)> returnTypes,
            UnboundLambda node, Binder binder, TypeSymbol? delegateType, bool isAsync, ConversionsBase conversions)
        {
+            Debug.Assert(!node.HasExplicitReturnType(out _, out _));


Consider moving assertion into InferReturnTypeImpl so that it also covers the InferReturnType overload a few lines above (line 159) #Resolved

Ping in case you missed this suggestion

Thanks. There's more work required to move this assert. I'll investigate.

jcouv · 2021-06-15T06:49:00Z

    public BoundLambda BindForReturnTypeInference(NamedTypeSymbol delegateType)

I assume BindForReturnTypeInference is never called when the return type is explicit, so we never populate _returnInferenceCache in that case. Is that correct? If yes, is there an assertion we could include here?

In reply to: 861226670

Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:858 in f8f6c7a. [](commit_id = f8f6c7a, deletion_comment = False)

jcouv · 2021-06-15T06:56:32Z

            SecurityWellKnownAttributeData securityData = wellKnownData.SecurityInformation;

nit: since you enabled nullability in this section of the file, this local should probably be annotated since we're null-testing it below. #Resolved

In reply to: 861231753

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs:1208 in f8f6c7a. [](commit_id = f8f6c7a, deletion_comment = False)

jcouv · 2021-06-15T06:57:18Z

src/Compilers/CSharp/Portable/Syntax/SyntaxFacts.cs

@@ -183,6 +183,9 @@ public static bool IsInTypeOnlyContext(ExpressionSyntax node)
                    case LocalFunctionStatement:
                        return ((LocalFunctionStatementSyntax)parent).ReturnType == node;

+                    case ParenthesizedLambdaExpression:
+                        return ((ParenthesizedLambdaExpressionSyntax)parent).ReturnType == node;


What scenario did this affect? #Closed

This is required to bind dynamic as an explicit return type since BindNonGenericSimpleNamespaceOrTypeOrAliasSymbol() requires dynamic to be in a type-only context.

jcouv · 2021-06-15T07:04:15Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -2524,7 +2540,7 @@ private bool TryGetReturnType(out TypeWithAnnotations type, out FlowAnalysisAnno
                return false;
            }

-            var delegateOrMethod = _delegateInvokeMethod ?? method;
+            var delegateOrMethod = _useDelegateInvokeReturnType ? _delegateInvokeMethod! : method;


nit: we should probably also have an assertion (not just a suppression) since this is enforced elsewhere #Closed

We could although we're dereferencing the result immediately.

Ah, had missed that. Sounds okay then

jcouv · 2021-06-15T07:06:17Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -6566,6 +6590,15 @@ private void ReportNullabilityMismatchWithTargetDelegate(Location location, Name
            }

            Debug.Assert(delegateType is object);
+
+            if (unboundLambda.HasExplicitReturnType(out _, out var returnType) &&


Should we take the refKind into account too? #Closed

We're already requiring an identity conversion so I don't think ref kind will have any effect. Added a test in case the identity conversion requirement is relaxed.

jcouv · 2021-06-15T07:13:12Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -6566,6 +6590,15 @@ private void ReportNullabilityMismatchWithTargetDelegate(Location location, Name
            }

            Debug.Assert(delegateType is object);
+
+            if (unboundLambda.HasExplicitReturnType(out _, out var returnType) &&
+                IsNullabilityMismatch(invoke.ReturnTypeWithAnnotations, returnType, requireIdentity: true))


Should this be requireIdentity: false instead? #Closed

Requiring the return type nullability to match exactly corresponds to the existing requirement for parameters (see sharplab.io).

We could relax this requirement but we should probably change the requirement for both parameters and return types at the same time.

Thanks. It makes sense to follow what we do for parameters

jcouv · 2021-06-15T07:17:03Z

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

+}";
+            var comp = CreateCompilation(sourceB, references: new[] { refA }, parseOptions: TestOptions.RegularPreview);
+            comp.VerifyDiagnostics(
+                // (7,11): error CS0648: 'A' is a type not supported by the language


This isn't the diagnostic I expected for a use-site issue, this looks more like bad PE. Should the test name be adjusted?
If so, could you add a use-site error scenario? #Closed

This is a use-site diagnostic reported in PENamedTypeSymbol.GetUseSiteDiagnosticImpl().

jcouv · 2021-06-15T07:21:50Z

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

+                // (9,4): error CS0182: An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type
+                // [A(F(async () => await Task.Yield()))]
+                Diagnostic(ErrorCode.ERR_BadAttributeArgument, "F(async () => await Task.Yield())").WithLocation(9, 4));
+        }


Consider testing a type inference scenario with an explicit return type.
M(int (i) => i) with M<T>(Func<T, T> f) would be interesting.
Or something more basic like: M(int (o) => 1) with M<T>(Func<object, T> f). #Closed

Added those tests, although currently type inference makes explicit type inferences from anonymous method parameter types but not return type.

jcouv · 2021-06-15T07:23:25Z

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

+{
+    static void F<T>()
+    {
+        Func<T> f1 = T () => default;


Consider testing some best type scenarios involving multiple lambdas: b ? (ExplicitType1 () => null) : (ExplicitType2 () => null) #Closed

jcouv

Done with review pass (iteration 3)

jaredpar · 2021-06-15T15:24:43Z

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

+        {
+            var source =
+@"delegate T D1<T>(ref T t);
+delegate ref T D2<T>(ref T t);


Consider adding a test to ensure that ref readonly returns work as well. #Resolved

cston · 2021-06-15T23:00:20Z

    public BoundLambda BindForReturnTypeInference(NamedTypeSymbol delegateType)

This method is still used when the return type is explicit. (It is the call to ReallyInferReturnType() below that avoids calculating the return type if explicit.) And we're binding the lambda body and caching the resulting BoundLambda rather than the just the return type here.

That said, it's possible we could avoid this code path altogether for explicit return types. #54125

In reply to: 861226670

Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:858 in f8f6c7a. [](commit_id = f8f6c7a, deletion_comment = False)

jcouv · 2021-06-15T23:18:43Z

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

+        }
+
+        // Should method type inference make an explicit type inference
+        // from the lambda expression return type?


FYI, I added a note to test plan to track this: "LDM: Should the explicit return type contribute to type inference (see TypeInference_02) "
nit: I'd probably also leave a PROTOTYPE marker here #Closed

jcouv

LGTM Thanks (iteration 4) with a minor suggestion to move an assertion

333fred · 2021-06-17T17:08:58Z

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

+            {
+                diagnostics.Add(ErrorFacts.GetStaticClassReturnCode(useWarning: false), syntax.Location, type);
+            }
+            else if (returnType.IsRestrictedType(ignoreSpanLikeTypes: true))


Theoretically, when we start inferring delegate types, we could encounter an async function with an __arglist parameter. Do we need to check that here? Or perhaps assert that we'll never encounter such a scenario? SourceOrdinaryMethodSymbolBase checks this case in its equivalent method, so I think we should do something about it, even if it's just to assert we'll never hit it. #Closed

Is this an issue introduced by explicit return types or an existing issue with lambda parameters?

It looks like the parser rejects __arglist in IsPossibleLambdaParameter(). I've added some parsing tests.

333fred · 2021-06-17T18:56:40Z

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

+                var returnTypes = ArrayBuilder<(BoundReturnStatement, TypeWithAnnotations)>.GetInstance();
+                BoundLambda.BlockReturns.GetReturnTypes(returnTypes, block);
+                inferredReturnType = BoundLambda.InferReturnType(returnTypes, _unboundLambda, lambdaBodyBinder, delegateType, lambdaSymbol.IsAsync, lambdaBodyBinder.Conversions);
+                // TODO: Should InferredReturnType.UseSiteDiagnostics be merged into BoundLambda.Diagnostics?


I would think so. #ByDesign

I'll leave this as is since this is an existing comment.

333fred · 2021-06-17T18:59:16Z

src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs

@@ -235,6 +238,8 @@ internal Location DiagnosticLocation
            }
        }

+        private bool HasExplicitReturnType => (Syntax as ParenthesizedLambdaExpressionSyntax)?.ReturnType is not null;


(Syntax as ParenthesizedLambdaExpressionSyntax)?.ReturnType is not null

Nit: could be Syntax is ParenthesizedLambdaExpressionSyntax { ReturnType: not null } #Closed

333fred · 2021-06-17T19:01:37Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

                }

-                for (NamedTypeSymbol curr = this.ContainingType; (object)curr != null; curr = curr.ContainingType)
+                // Avoid checking attributes on containing types to avoid a potential cycle when a lambda
+                // is used in an attribute argument - see  https://github.com/dotnet/roslyn/issues/54074.


This issue is closed as won't fix: consider updating this comment to indicate that since partial trust isn't supported any more, we don't want to make the effort to plumb through understanding whether we're currently in an attribute here. #Closed

333fred · 2021-06-17T19:07:47Z

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

+    static void Main()
+    {
+        Func<dynamic> f1 = object () => default!;
+        Func<(int, int)> f2 = (int X, int Y) () => default;


(int X, int Y) () => default

Conceptually, this feels similar to an override changing the names of a tuple. Do we want to consider a warning/error? #ByDesign

We don't report diagnostics for simple tuple assignments or for method group conversions involving tuple return types, and the lambda cases seem similar to those (see sharplab.io).

333fred · 2021-06-17T19:08:44Z

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

+    static void Main()
+    {
+        Expression<Func<object>> e1 = dynamic () => default!;
+        Expression<Func<(int X, int Y)>> e2 = (int, int) () => default;


Same comment #ByDesign

333fred · 2021-06-17T19:15:04Z

Done review pass (commit 5)

cston · 2021-06-19T21:03:47Z

@jcouv, @333fred, all feedback has been addressed, thanks.

jcouv · 2021-06-19T21:47:44Z

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

            }

-            return null;
+            return Binder.GetMethodGroupOrLambdaDelegateType(


GetMethodGroupOrLambdaDelegateType

It looks like the other caller of GetMethodGroupOrLambdaDelegateType has some similar logic for handling void. Should we push this logic down into GetMethodGroupOrLambdaDelegateType? #Resolved

Should we push this logic down into GetMethodGroupOrLambdaDelegateType?

I considered that but decided against it because that would require GetMethodGroupOrLambdaDelegateType to allow two ways to represent System.Void - either null or an actual TypeSymbol - because InferReturnType() will return null for that case.

jcouv

LGTM Thanks (iteration 8)

dotnet-issue-labeler bot added the Area-Compilers label Jun 14, 2021

cston force-pushed the bind-type branch from 2946f7c to dc8e63a Compare June 14, 2021 19:36

Lambda expression explicit return type: binding

1102544

cston force-pushed the bind-type branch from dc8e63a to 1102544 Compare June 14, 2021 21:00

cston marked this pull request as ready for review June 14, 2021 21:00

cston requested a review from a team as a code owner June 14, 2021 21:00

cston added 2 commits June 14, 2021 21:38

Update NullableWalker

9b2d9ed

Fix warning

f8f6c7a

jcouv self-assigned this Jun 15, 2021

jcouv added this to the C# 10 milestone Jun 15, 2021

jcouv reviewed Jun 15, 2021

View reviewed changes

jaredpar reviewed Jun 15, 2021

View reviewed changes

PR feedback

08d21a9

cston mentioned this pull request Jun 15, 2021

Avoid using UnboundLambda.BindForReturnTypeInference() if the lambda expression has an explicit return type #54125

Open

jcouv reviewed Jun 15, 2021

View reviewed changes

jcouv approved these changes Jun 15, 2021

View reviewed changes

Add tests

a752010

333fred reviewed Jun 17, 2021

View reviewed changes

cston added 3 commits June 18, 2021 22:35

Fix tests

6c05d62

PR feedback

ed9af13

PR feedback

20a262b

jcouv reviewed Jun 19, 2021

View reviewed changes

jcouv approved these changes Jun 19, 2021

View reviewed changes

333fred approved these changes Jun 21, 2021

View reviewed changes

cston merged commit 56c3369 into dotnet:features/lambdas Jun 21, 2021

cston deleted the bind-type branch June 21, 2021 21:35

cston mentioned this pull request Jun 21, 2021

Lambda explicit return type: IDE tests #54250

Merged

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

Lambda expression explicit return type: binding #54075

Lambda expression explicit return type: binding #54075

Conversation

cston commented Jun 14, 2021 • edited Loading

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

cston Jun 15, 2021 • 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 Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 15, 2021 • edited Loading

jcouv commented Jun 15, 2021 • edited by cston Loading

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

cston Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

cston Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jaredpar Jun 15, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston commented Jun 15, 2021 • edited Loading

jcouv Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

333fred Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Jun 17, 2021 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

333fred Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

333fred Jun 17, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston commented Jun 14, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

cston Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

cston Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited by cston

Loading

jcouv commented Jun 15, 2021 •

edited

Loading

jcouv commented Jun 15, 2021 •

edited by cston

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

cston Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

cston Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

jaredpar Jun 15, 2021 •

edited by cston

Loading

cston commented Jun 15, 2021 •

edited

Loading

jcouv Jun 15, 2021 •

edited

Loading

333fred Jun 17, 2021 •

edited

Loading

333fred Jun 17, 2021 •

edited by cston

Loading

333fred Jun 17, 2021 •

edited

Loading

333fred Jun 17, 2021 •

edited

Loading

333fred Jun 17, 2021 •

edited by cston

Loading

cston Jun 19, 2021 •

edited

Loading

333fred Jun 17, 2021 •

edited by cston

Loading

jcouv Jun 19, 2021 •

edited by cston

Loading