Bind and emit lambda attributes by cston · Pull Request #52178 · dotnet/roslyn

cston · 2021-03-26T21:27:18Z

Bind and emit attributes on lambda expressions and lambda parameters.

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

AlekseyTs · 2021-03-29T14:48:26Z

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

LambdaSymbol [](start = 26, length = 12)

It looks like this type should override AddDeclarationDiagnostics so that it could collect errors that are reported during attribute binding and other similar errors that are normally treated as declaratio errors. #Resolved

AlekseyTs · 2021-03-29T14:50:34Z

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

return false; [](start = 17, length = 14)

Isn't this based on attributes? Same for other similar properties. #Resolved

333fred · 2021-04-01T17:37:49Z

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

+            return OneOrMany.Create(attributeLists);
+        }
+
+        internal void GetDeclarationDiagnostics(BindingDiagnosticBag addTo)


I'm not certain why we need both GetDeclarationDiagnostics and AddDeclarationDiagnostics? #Closed

AddDeclarationDiagnostics() is an override of a method called in several places (such as from SourceParameterSymbol) where declaration diagnostics may be calculated. GetDeclarationDiagnostics() provides access to the declaration diagnostics which is necessary from UnboundLambdaState.ReallyBind() to ensure the diagnostics that are calculated are actually reported.

In reply to: 605833109 [](ancestors = 605833109)

333fred · 2021-04-01T17:38:21Z

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

            }

+            // PROTOTYPE: Test that we're reporting diagnostics even in the case where we re-used the lambda above.
+            lambdaSymbol.GetDeclarationDiagnostics(diagnostics);


Why are we not using AddDeclarationDiagnostics here, which will ensure diagnostics are actually calculated? #ByDesign

Why are we not using AddDeclarationDiagnostics here, which will ensure diagnostics are actually calculated?

AddDeclarationDiagnostics() only adds diagnostics that have already been calculated. GetDeclarationDiagnostics() will ensure diagnostics are added to _declarationDiagnostics.

In reply to: 605833428 [](ancestors = 605833428)

333fred · 2021-04-01T17:52:08Z

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

+    static void Main()
+    {
+        Action<object, object> a;
+        a = ([A] _, y) => { };


Consider verifying that these attributes actually show up in the symbol model. #Closed

Added verification here and in LambdaTests.LambdaAttributes_01.

In reply to: 605841651 [](ancestors = 605841651)

333fred · 2021-04-01T17:58:49Z

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

+        Func<bool> a4 = [MemberNotNull(""x"")][MemberNotNullWhen(false, ""y"")][MemberNotNullWhen(true, ""z"")] () => true;
+    }
+}";
+            var comp = CreateCompilation(


Prototype comment to make sure these actually do the things they're supposed to? #Closed

333fred · 2021-04-01T18:00:25Z

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

+            var lambdas = exprs.SelectAsArray(e => GetLambdaSymbol(model, e));
+            Assert.Equal(FlowAnalysisAnnotations.AllowNull | FlowAnalysisAnnotations.MaybeNullWhenFalse, lambdas[0].Parameters[0].FlowAnalysisAnnotations);
+            Assert.Equal(new[] { "x" }, lambdas[1].Parameters[1].NotNullIfParameterNotNull);
+        }


Consider an error test that uses an attribute recursively, something like

[OtherAttr([Attr] () => {})] class Attr : Attribute {} class OtherAttr : Attribute { public OtherAttr(object o) {} } ``` #Closed

333fred · 2021-04-01T18:00:50Z

Done review pass (commit 1) #Closed

jcouv · 2021-04-06T02:18:03Z

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

@@ -38,25 +35,30 @@ internal sealed class LambdaSymbol : SourceMethodSymbol

        private static readonly TypeWithAnnotations UnknownReturnType = TypeWithAnnotations.Create(ErrorTypeSymbol.UnknownResultType);


UnknownReturnType [](start = 52, length = 17)

It looks like UnknownReturnType is unused #Closed

jcouv · 2021-04-06T02:26:43Z

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

            get { return _returnType; }
        }

-        public override FlowAnalysisAnnotations ReturnTypeFlowAnalysisAnnotations => FlowAnalysisAnnotations.None;


~~Please include a test for nullable return type and nullability attributes (Maybe/NotNull/NotNullIfNotNull).~~ Do nullability checks on returned values work properly? #Closed

Do nullability checks on returned values work properly?

Added tests with PROTOTYPE comment. #Closed

jcouv · 2021-04-06T02:44:07Z

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

            var builder = ArrayBuilder<ParameterSymbol>.GetInstance(unboundLambda.ParameterCount);
            var hasExplicitlyTypedParameterList = unboundLambda.HasExplicitlyTypedParameterList;
            var numDelegateParameters = parameterTypes.Length;
+            var parenthesizedParameters = (Syntax as ParenthesizedLambdaExpressionSyntax)?.ParameterList;


parenthesizedParameters appears unused #Closed

jcouv · 2021-04-06T03:07:31Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Conditional.cs

+@"using System.Diagnostics;
+class Program
+{
+    [Conditional(""A"")]


Can Conditional be applied to a lambda? (I'd think it should not)
We should review other attributes listed in SourceMethodSymbolWithAttributes.DecodeWellKnownAttributeAppliedToMethod as well.
~~SkipLocalsInitAttribute~~, EnumeratorCancellationAttribute (probably requires explicit return types on lambda first), ExcludeFromCodeCoverageAttribute, OptionalAttribute should be observable.
There's also MarshalAsAttribute and PrincipalPermission
Let's test disallowed attributes on lambda and on lambda parameters.

I don't know if we care, but Action a = [Obsolete]() => {}; should probably produce a diagnostic. #Closed

ConditionalAttribute and ObsoleteAttribute are not handled. Let me know if you think there is a scenario with either that needs to be addressed.

The other attributes should be handled. #Closed

ObsoleteAttribute: I agree (ie. don't care). But we should have a test

ConditionalAttribute: I think that should be disallowed (as it is on local functions)

In reply to: 612020197 [](ancestors = 612020197)

I was incorrect. ConditionalAttribute is allowed on local functions. We should do the same.
Example

In reply to: 612132673 [](ancestors = 612132673,612020197)

jcouv · 2021-04-06T03:41:57Z

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

+                        if (syntax.Kind() == SyntaxKind.ParenthesizedLambdaExpression)
+                        {
+                            // PROTOTYPE: Report error if parameter syntax is missing the type (using implicit parameter type syntax).
+                            MessageID.IDS_FeatureLambdaAttributes.CheckFeatureAvailability(diagnostics, attributeList);


~~Do we do a similar check on attribute on the lambda too?~~
I see this is checked during parsing. Is there a reason both shouldn't be semantic checks? #Closed

You may have missed this question (should we only use semantic checks for LangVer checks?)
Could you also check if IDS_FeatureLambdaAttributes has the appropriate comment (syntax vs. semantic check)?

In reply to: 607473773 [](ancestors = 607473773)

jcouv · 2021-04-06T03:56:06Z

src/Compilers/CSharp/Portable/Lowering/ClosureConversion/SynthesizedClosureMethod.cs

        internal int ExtraSynthesizedParameterCount => this._structEnvironments.IsDefault ? 0 : this._structEnvironments.Length;

-        internal override bool InheritsBaseMethodAttributes => BaseMethod is LocalFunctionSymbol;
+        internal override bool InheritsBaseMethodAttributes => true;


I'm not very familiar. What's a scenario affected by this change? #Closed

What's a scenario affected by this change?

For rewritten lambdas or local functions (rewritten during lowering by ClosureConversion), this property ensures any parameters in the rewritten method refer back to the original lambda or local function property for attributes. That is required to emit the attributes.

In reply to: 607479101 [](ancestors = 607479101)

jcouv · 2021-04-06T04:03:52Z

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

+        private static LambdaSymbol GetLambdaSymbol(SemanticModel model, LambdaExpressionSyntax syntax)
+        {
+            return model.GetSymbolInfo(syntax).Symbol.GetSymbol<LambdaSymbol>();
+        }


Some test ideas, if not covered already:

Test semantic model on attributes (like LocalFunctionAttribute_Argument_SemanticModel)

Enforcement of attribute targets (like LocalFunctionAttribute_BadAttributeLocation)

Disallowed attributes (like LocalFunctionDisallowedAttributes and LocalFunctionDisallowedSecurityAttributes)

Scoping rules on attributes (lookup tests like LookupInsideLocalFunctionAttribute)

Incremental parsing tests (like Statement_EditAttributeList_01)
#Closed

Added tests adapted from the corresponding attribute tests in LocalFunctionTests. Thanks for the suggestions (and thanks @RikkiGibson for the local function tests). #Closed

jcouv

Done with review pass (iteration 1)

jcouv · 2021-04-06T04:07:02Z

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

+        {
+            foreach (var parameter in _parameters)
+            {
+                parameter.ForceComplete(null, default);


nit: consider argument names #Closed

333fred · 2021-04-09T21:36:10Z

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

+    public class IsReadOnlyAttribute : Attribute { }
+    public class IsUnmanagedAttribute : Attribute { }
+    public class IsByRefLikeAttribute : Attribute { }
+    public class NullableContextAttribute : Attribute { public NullableContextAttribute(byte b) { } }


Probably UnmanagedCallersOnlyAttribute as well #Closed

I don't see a response to this.

In reply to: 610916427 [](ancestors = 610916427)

333fred · 2021-04-09T21:38:33Z

Done review pass (commit 4) #Closed

333fred · 2021-04-12T23:46:57Z

Done review pass (commit 5)

jcouv · 2021-04-13T05:00:41Z

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

+    }
+}";
+            var comp = CreateCompilation(new[] { source, MaybeNullAttributeDefinition, NotNullAttributeDefinition }, parseOptions: TestOptions.RegularPreview);
+            // PROTOTYPE: Report error for a2, no error for a1.


I think both declarations will end up having a warning. The first one in the conversion/assignment, and the other in the return value of the lambda.

Confirmed with similar scenario with local functions:
sharplab #Closed

jcouv · 2021-04-13T05:27:37Z

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

+    static void Main()
+    {
+        Action a1 = [DoesNotReturn] () => { };
+        Action a2 = [DoesNotReturn] () => throw new Exception();


Please add a case where the lambda does return (we'll enforce/warn) #Closed

Should warn on a1 too

In reply to: 612139330 [](ancestors = 612139330)

From offline discussion, let's test NotNullWhen enforcement too (should likely fall out when DoesNotReturn works)

In reply to: 612140847 [](ancestors = 612140847,612139330)

jcouv · 2021-04-13T05:29:53Z

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

+    static void Main()
+    {
+        Action<object> a1 = ([AllowNull] x) => { x.ToString(); };
+        Action<object?> a2 = ([DisallowNull] x) => { x.ToString(); };


Ditto here. We should have two warnings, just like with local functions. A dereference warning inside the first lambda, and a warning on conversion of the second lambda.
sharplab #Closed

Do you want to leave a PROTOTYPE marker here?

In reply to: 612140116 [](ancestors = 612140116)

jcouv

Done with review pass (iteration 5) with comments and some test suggestions on nullability attributes (can be PROTOTYPED)

cston · 2021-04-13T18:15:35Z

@333fred, @jcouv, thanks for reviewing. All comments have been addressed.

jcouv · 2021-04-13T19:15:05Z

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

+                    }
+                    else
+                    {
+                        Error(diagnostics, ErrorCode.ERR_AttributesNotAllowed, attributeList);


nit: consider using a dedicated error message suggesting to parenthesize the lambda's parameter list #Closed

jcouv · 2021-04-13T19:19:19Z

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

+    static void Main()
+    {
+        Action a1 = [DoesNotReturn] () => { };
+        Action a2 = [DoesNotReturn] () => throw new Exception();


Do you want to leave a PROTOTYPE marker here too (test isn't behaving as expected) #Closed

jcouv

Done with review pass (iteration 8). Just missing a couple PROTOTYPE markers. One nit to consider

jcouv · 2021-04-13T20:24:47Z

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

+            {
+                // (8,56): error CS8916: Attributes require a lambda expression with a parenthesized parameter list.
+                //         Action<object, object> a = delegate (object x, [A][B] object y) { };
+                Diagnostic(ErrorCode.ERR_AttributesRequireParenthesizedLambdaExpression, "[A]").WithLocation(8, 56),


Here the message doesn't make sense, as the parameter list is already parenthesized :-/ #Resolved

Yes, the parameter list is parenthesized, but the message also indicates the expression should be a lambda expression. #Closed

I've reverted the error message for anonymous methods. #Closed

jcouv

LGTM THanks (iteration 9) with nit on diagnostic message for attribute used in delegate syntax

333fred · 2021-04-13T22:16:39Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <value>Attributes are not valid in this context.</value>
  </data>
+  <data name="ERR_AttributesRequireParenthesizedLambdaExpression" xml:space="preserve">
+    <value>Attributes require a lambda expression with a parenthesized parameter list.</value>


Wording of this is a bit awkward. Consider "Using attributes on a lambda expression requires a parenthesized parameter list", to avoid implying that all attributes require parenthesized lambda expressions. #Closed

Updated to "Attributes on lambda expressions require a parenthesized parameter list." #Closed

333fred · 2021-04-13T22:23:43Z

src/Compilers/CSharp/Test/Syntax/IncrementalParsing/IncrementalParsingTests.cs

        }

+        [Fact]
+        public void Lambda_EditAttributeList()


Consider tests where the parameter list isn't parenthesized, where there wasn't an attribute to start with, and where the attribute is applied to a parameter, not the lambda. #Closed

333fred

LGTM (commit 11)

ghost added the Area-Compilers label Mar 26, 2021

cston changed the base branch from main to features/lambdas March 26, 2021 22:21

cston force-pushed the bind-attributes branch from 0daa818 to 90ed0ca Compare March 26, 2021 23:47

AlekseyTs reviewed Mar 29, 2021

View reviewed changes

cston force-pushed the bind-attributes branch 3 times, most recently from 7cf1d33 to 2cbe75a Compare April 1, 2021 00:17

Bind and emit lambda attributes

3208b96

cston force-pushed the bind-attributes branch from 2cbe75a to 3208b96 Compare April 1, 2021 04:42

cston marked this pull request as ready for review April 1, 2021 04:48

cston requested a review from a team as a code owner April 1, 2021 04:48

cston requested review from halter73 and removed request for halter73 April 1, 2021 05:38

333fred reviewed Apr 1, 2021

View reviewed changes

jcouv reviewed Apr 6, 2021

View reviewed changes

jcouv added this to the C# 10 milestone Apr 6, 2021

jcouv self-assigned this Apr 6, 2021

cston added 3 commits April 8, 2021 10:21

PR feedback

a5cf1a5

Adapt tests from LocalFunctionTests

92b2e81

SpeculativeSemanticModel

e2ead64

333fred reviewed Apr 9, 2021

View reviewed changes

PR feedback

4be2d4e

jcouv reviewed Apr 13, 2021

View reviewed changes

cston added 3 commits April 12, 2021 23:36

PR feedback

d5bc775

Update PublicAPI

f0c579d

More tests

a8803c8

jcouv reviewed Apr 13, 2021

View reviewed changes

PR feedback

1d7214c

jcouv reviewed Apr 13, 2021

View reviewed changes

jcouv approved these changes Apr 13, 2021

View reviewed changes

PR feedback

a1d3e89

333fred reviewed Apr 13, 2021

View reviewed changes

PR feedback

8953b0b

333fred approved these changes Apr 13, 2021

View reviewed changes

cston merged commit 9a800bf into dotnet:features/lambdas Apr 14, 2021

cston deleted the bind-attributes branch April 14, 2021 01:02

Youssef1313 mentioned this pull request Dec 10, 2021

Fix regression of LambdaSymbol.GetHashCode #58247

Merged

		@@ -38,25 +35,30 @@ internal sealed class LambdaSymbol : SourceMethodSymbol

		private static readonly TypeWithAnnotations UnknownReturnType = TypeWithAnnotations.Create(ErrorTypeSymbol.UnknownResultType);

Conversation

cston commented Mar 26, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Mar 29, 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!

AlekseyTs Mar 29, 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!

333fred Apr 1, 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!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 1, 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!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 1, 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!

Choose a reason for hiding this comment

Uh oh!

333fred Apr 1, 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!

333fred Apr 1, 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!

333fred commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Apr 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!

jcouv Apr 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!

cston Apr 12, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 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!

jcouv Apr 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!

cston Apr 12, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 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!

Choose a reason for hiding this comment

cston commented Mar 26, 2021 •

edited by jcouv

Loading

AlekseyTs Mar 29, 2021 •

edited by cston

Loading

AlekseyTs Mar 29, 2021 •

edited by cston

Loading

333fred Apr 1, 2021 •

edited

Loading

333fred Apr 1, 2021 •

edited by cston

Loading

333fred Apr 1, 2021 •

edited

Loading

333fred Apr 1, 2021 •

edited

Loading

333fred Apr 1, 2021 •

edited

Loading

333fred commented Apr 1, 2021 •

edited

Loading

jcouv Apr 6, 2021 •

edited

Loading

jcouv Apr 6, 2021 •

edited

Loading

cston Apr 12, 2021 •

edited by jcouv

Loading

jcouv Apr 6, 2021 •

edited

Loading

jcouv Apr 6, 2021 •

edited

Loading

cston Apr 12, 2021 •

edited by jcouv

Loading

jcouv Apr 6, 2021 •

edited

Loading

jcouv Apr 6, 2021 •

edited

Loading

jcouv Apr 6, 2021 •

edited

Loading

cston Apr 9, 2021 •

edited by jcouv

Loading

jcouv Apr 6, 2021 •

edited

Loading

333fred Apr 9, 2021 •

edited

Loading

333fred commented Apr 9, 2021 •

edited

Loading

jcouv Apr 13, 2021 •

edited

Loading

jcouv Apr 13, 2021 •

edited

Loading

jcouv Apr 13, 2021 •

edited

Loading

jcouv Apr 13, 2021 •

edited

Loading