Parse lambda attributes #52038

cston · 2021-03-21T23:08:28Z

Parse attributes on lambda expressions and lambda parameters.

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

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs

src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

333fred · 2021-03-22T22:38:43Z

    private bool IsPossibleLambdaExpression(Precedence precedence)

This will need to be updated. #Closed

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:11311 in 6d72d30. [](commit_id = 6d72d30, deletion_comment = False)

333fred · 2021-03-22T22:39:30Z

                return IsPossibleAnonymousMethodExpression() || IsPossibleLambdaExpression(Precedence.Expression);

Likely needs a new case for open brackets. #Closed

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:9680 in 6d72d30. [](commit_id = 6d72d30, deletion_comment = False)

cston · 2021-03-22T22:46:10Z

    private bool IsPossibleLambdaExpression(Precedence precedence)

This will need to be updated.

I didn't think so. This method is only called after scanning static or an identifier.

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

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:11311 in 6d72d30. [](commit_id = 6d72d30, deletion_comment = False)

333fred · 2021-03-22T22:48:23Z

    private bool IsPossibleLambdaExpression(Precedence precedence)

This method is only called after scanning static or an identifier.

Currently true. Which is why [Test] static () => {} isn't going to parse correctly currently in cases that call this :).

In reply to: 804445349 [](ancestors = 804445349,804442264)

Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:11311 in 6d72d30. [](commit_id = 6d72d30, deletion_comment = False)

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs

src/Compilers/CSharp/Test/Syntax/Parsing/SuppressNullableWarningExpressionParsingTests.cs

333fred · 2021-03-22T23:09:00Z

Done review pass (commit 2) #Closed

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs

333fred · 2021-03-24T21:39:46Z

Done review pass (commit 5) #Closed

halter73 · 2021-03-24T21:43:02Z

Proposed changes:

Allow lambdas with attributes

Allow lambdas with explicit return type

Infer a natural delegate type for lambdas and method groups

Of the proposed changes in lambda-improvements.md, does this only cover item 1? If so, is item 3 next? Between 2 and 3, 3 is a lot more important for the usability of an API taking an arbitrary Delegate. Right now, it requires a lot of ugly casting.

333fred · 2021-03-24T21:45:52Z

This is just the parsing changes for 1. 3 requires no parsing changes and will come later.

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

cston · 2021-03-24T21:48:44Z

@halter73

Of the proposed changes in lambda-improvements.md, does this only cover item 1?

This PR covers a part of 1: parsing the attribute syntax. Changes will be needed to bind and emit attributes as well. After that, I'll focus on 3.

333fred · 2021-03-25T18:11:56Z

Done review pass (commit 6)

333fred

LGTM (commit 8)

cston · 2021-03-29T21:43:09Z

@dotnet/roslyn-compiler, please review, thanks.

jaredpar · 2021-03-29T22:20:28Z

@RikkiGibson

CyrusNajmabadi · 2021-03-30T23:31:54Z

Looking now.

CyrusNajmabadi · 2021-03-30T23:32:15Z

src/Compilers/CSharp/Portable/Errors/MessageID.cs

@@ -216,6 +216,7 @@ internal enum MessageID
        IDS_FeatureVarianceSafetyForStaticInterfaceMembers = MessageBase + 12791,
        IDS_FeatureConstantInterpolatedStrings = MessageBase + 12792,
        IDS_FeatureMixedDeclarationsAndExpressionsInDeconstruction = MessageBase + 12793,
+        IDS_FeatureLambdaAttributes = MessageBase + 12798,


curious why the +5 here. #Resolved

curious why the +5 here.

To reduce the chance of conflict with other features.

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

CyrusNajmabadi · 2021-03-30T23:36:26Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -11056,6 +11072,8 @@ private bool ScanExplicitlyTypedLambda(Precedence precedence)
                    // Advance past the open paren or comma.
                    this.EatToken();

+                    _ = ParseAttributeDeclarations();


this is definitely so weird. i don't expect you to change this in this pr, but at this point, i think we need to rethink the whole 'scan then parse' approach we take. Now that scanning is effectively just running the parser to make its determination, we should think about just swithcing to a "just parse" approach. We'd still have rewind points, we just wouldn't be trying to duplicate the logic between these guys.

CyrusNajmabadi · 2021-03-30T23:37:39Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                    default:
+                        result = false;
+                        break;
+                }


could just be a switch expression. #Resolved

CyrusNajmabadi · 2021-03-30T23:38:08Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                        result = IsPossibleLambdaExpressionCore(precedence);
+                        break;
+                    case SyntaxKind.OpenParenToken:
+                        result = ScanParenthesizedImplicitlyTypedLambda(precedence) || ScanExplicitlyTypedLambda(precedence);


should this go into a single ScanParenthesizedLambda helper? #Resolved

CyrusNajmabadi · 2021-03-30T23:38:33Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                }
+
+                this.Reset(ref resetPoint);
+                this.Release(ref resetPoint);


we generally put these in finallys. is it ok that these aren't? #Resolved

also, if you do a try/finally, the body of this helper becomes much simpler,a nd less likely to have a problem later on (for example, if someone errantly returns). #Resolved

CyrusNajmabadi · 2021-03-30T23:39:54Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                {
+                    case SyntaxKind.StaticKeyword:
+                    case SyntaxKind.IdentifierToken:
+                        result = IsPossibleLambdaExpressionCore(precedence);


i don't love the assymetry here with the check above (though i grant it is correct). i would prefer another case of "== Static || IsTrueIdentifier" to make it clear tehse chunks are in sync, and aren't dependent on an internal detail of IsTrueIdentifier implying that this case is sufficient.

CyrusNajmabadi · 2021-03-30T23:43:23Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -12233,6 +12291,7 @@ private SyntaxList<SyntaxToken> ParseAnonymousFunctionModifiers()

        private LambdaExpressionSyntax ParseLambdaExpression()
        {
+            var attributes = ParseAttributeDeclarations(checkLambdaAttributesFeature: true);


so this is technically a bit unclean, but hopefully not a problem. THe main issue is that this makes attribute parsing context sensitive. We generally like avoiding that as it makes incremental difficult. Specifically, it means we couldn't jsut say "ah, we can reuse the attributes from a previous parse that are available in this location".

In your case, i think you're ok. that's bacause you'll have one of the two cases:

the attribute decl is on a lambda before. so it will have the feature diagnostic on it, so it will crumble if we try to reparse it.

the attribute decl is not on a lambda. it is reused in ParseAttributeDecl, but we still add the diagnostic on afterwards.

This is subtle though and easy to get wrong. in general, it can be preferable to just do the parse normally and have the caller (i.e. this method) do the check.

this makes attribute parsing context sensitive. We generally like avoiding that as it makes incremental difficult.

I'm interested to understand how to handle the feature check in this method. Specifically, what would the resulting error be attached to that would simplify incremental parsing? (Perhaps I can address this in a subsequent PR.)

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

CyrusNajmabadi

Parsing looks correct. I couldn't think of any cases that would be a problem. I do have some suggestions on potential simplification/safety. But as they are not necessary, it's up to you if you want to do any of them.

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

Parse lambda attributes

6d72d30

cston force-pushed the parse-attributes branch from 10648fb to 6d72d30 Compare March 22, 2021 01:32

cston marked this pull request as ready for review March 22, 2021 01:34

cston requested a review from a team as a code owner March 22, 2021 01:34

RikkiGibson reviewed Mar 22, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs Outdated Show resolved Hide resolved

333fred reviewed Mar 22, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4 Show resolved Hide resolved

Fix tests

9919a1a

333fred reviewed Mar 22, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

333fred reviewed Mar 22, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs Show resolved Hide resolved

333fred reviewed Mar 22, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/SuppressNullableWarningExpressionParsingTests.cs Outdated Show resolved Hide resolved

cston added 3 commits March 22, 2021 17:51

Update PublicAPI.unshipped.txt

36960b1

PR feedback

9a9c3fa

Misc.

5fbccbb

333fred reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

333fred reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs Show resolved Hide resolved

333fred reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs Show resolved Hide resolved

333fred reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs Show resolved Hide resolved

333fred reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs Outdated Show resolved Hide resolved

333fred reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs Outdated Show resolved Hide resolved

333fred reviewed Mar 24, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Show resolved Hide resolved

PR feedback

01d7b6a

cston added 2 commits March 25, 2021 11:27

Add test

2dced75

Fix test

c35c94f

This was referenced Mar 25, 2021

Agents canceling during dotnet builds jaredpar/runfo#47

Closed

Azure Agent timeout #48030

Closed

333fred approved these changes Mar 25, 2021

View reviewed changes

cston requested a review from a team March 26, 2021 02:37

cston requested a review from CyrusNajmabadi March 30, 2021 19:49

CyrusNajmabadi reviewed Mar 30, 2021

View reviewed changes

CyrusNajmabadi approved these changes Mar 30, 2021

View reviewed changes

RikkiGibson self-assigned this Mar 31, 2021

PR feedback

c4b1b68

RikkiGibson approved these changes Mar 31, 2021

View reviewed changes

cston merged commit 4463994 into dotnet:features/lambdas Mar 31, 2021

cston deleted the parse-attributes branch March 31, 2021 15:02

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse lambda attributes #52038

Parse lambda attributes #52038

cston commented Mar 21, 2021 •

edited by jcouv

Loading

333fred commented Mar 22, 2021 •

edited

Loading

333fred commented Mar 22, 2021 •

edited

Loading

cston commented Mar 22, 2021

333fred commented Mar 22, 2021

333fred commented Mar 22, 2021 •

edited

Loading

333fred commented Mar 24, 2021 •

edited

Loading

halter73 commented Mar 24, 2021 •

edited

Loading

333fred commented Mar 24, 2021

cston commented Mar 24, 2021

333fred commented Mar 25, 2021

333fred left a comment

cston commented Mar 29, 2021

jaredpar commented Mar 29, 2021

CyrusNajmabadi commented Mar 30, 2021

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

cston Mar 30, 2021

CyrusNajmabadi Mar 30, 2021

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021

CyrusNajmabadi Mar 30, 2021

cston Mar 31, 2021

CyrusNajmabadi left a comment

Parse lambda attributes #52038

Parse lambda attributes #52038

Conversation

cston commented Mar 21, 2021 • edited by jcouv Loading

333fred commented Mar 22, 2021 • edited Loading

333fred commented Mar 22, 2021 • edited Loading

cston commented Mar 22, 2021

333fred commented Mar 22, 2021

333fred commented Mar 22, 2021 • edited Loading

333fred commented Mar 24, 2021 • edited Loading

halter73 commented Mar 24, 2021 • edited Loading

333fred commented Mar 24, 2021

cston commented Mar 24, 2021

333fred commented Mar 25, 2021

333fred left a comment

Choose a reason for hiding this comment

cston commented Mar 29, 2021

jaredpar commented Mar 29, 2021

CyrusNajmabadi commented Mar 30, 2021

CyrusNajmabadi Mar 30, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Mar 30, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 30, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 30, 2021 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 30, 2021 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 30, 2021 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 30, 2021 • edited by cston Loading

Choose a reason for hiding this comment

CyrusNajmabadi Mar 30, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Mar 30, 2021

Choose a reason for hiding this comment

cston Mar 31, 2021

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

cston commented Mar 21, 2021 •

edited by jcouv

Loading

333fred commented Mar 22, 2021 •

edited

Loading

333fred commented Mar 22, 2021 •

edited

Loading

333fred commented Mar 22, 2021 •

edited

Loading

333fred commented Mar 24, 2021 •

edited

Loading

halter73 commented Mar 24, 2021 •

edited

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading

CyrusNajmabadi Mar 30, 2021 •

edited by cston

Loading