Relax the shape of lambdas in our syntax model to allow for a list of modifiers. by CyrusNajmabadi · Pull Request #39118 · dotnet/roslyn

CyrusNajmabadi · 2019-10-07T21:56:25Z

~~WIP: personal review.~~ Now ready for review.

This is desirable if we add static lambdas (dotnet/csharplang#275) as it means we don't put unnecessary syntax model restriction on people writing static async a => ... or async static a => .... Note: the language may choose that only one of these is legal. But we won't restrict it in the model to make sure that parser resilience and modeling is high quality.

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

…ol.cs

into staticLambdas

CyrusNajmabadi · 2019-10-07T23:13:58Z

Tagging @jcouv . A precursor PR to adding support for static lambdas.

CyrusNajmabadi · 2019-10-07T23:17:40Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

        {
            // first if there is a single list, then choose the list because it would not have been optional
-            int listCount = nd.Fields.Count(f => IsAnyNodeList(f.Type));
+            int listCount = nd.Fields.Count(f => IsAnyNodeList(f.Type) && !IsAttributeOrModifiersList(f));


the changes in SourceWriter are so that we don't generate this syntax factory overload:

public ParenthesizedLambdaExpression ParethesizedLambda(SyntaxTokenList modifiers = default) { ... }

We already have

public ParenthesizedLambdaExpression ParethesizedLambda() { ... }

Which this would conflict with. Note: this change was already made in the local-function-attributes branch as well. this just cherry-picks that bac to here.

I'm confused as to why there would be a = default optional value for the SyntaxTokenList when it is never optional. Is that just a special way that SyntaxTokenList gets handled in the generator? Sorry if this was already addressed when we pulled this into the other feature branch.

I'm confused as to why there would be a = default optional value for the SyntaxTokenList when it is never optional

So, first, Lists are always optional. That's because they can be 'empty' and thus can be elided.

We then have extra logic in our source generator to say: let me generate a bunch of helper factory methods that should try to balance determining which nodes/tokens we will generate on their behalf, and which nodes they need to provide.

Let's look at an example of that: BlockSyntax.

So a Block (prior to my attributes-on-statement work) was always: { statement* }.

So, the generator saw that and said: ah, you have a single list, and some required tokens. I will generate:

public BlockSyntax Block(SyntaxList<StatementSyntax> statements = default)

That's a really useful factory method. You can trivially make an empty block with Block() and you can pass some amount of statements into that to generate a block out of those statements.

The logic for this code however, was that we would do this for the first list we found (as long as there was only one), otherwise we would use the first optional node.

When I added attributes to statements (in the other PR), and now that i've added 'modifiers' to lambdas (in this PR), the source generator says: ah, i've found a list that i think should be the one to focus these helper factory functions around. For something like Block that would mean providing an entrypoint like:

public BlockSyntax Block(SyntaxList<AttributeListSyntax> attributeLists = default)

That's just literally not helpful. No one is going to say "i want an easy way to make a block, optionally deciding on some attributes for it.

In a very real sense, attributes and modifiers are low relevance. They can be on a lot of nodes, but they're never the primary concern people have when constructing them simply. So having the helper methods be targetted around them is not something we want.

Makes total sense, thanks, and this seems like a reasonable enough one-off way of handling it instead of adding an extra attribute to the AttributeLists and Modifiers elements to express this case.

CyrusNajmabadi · 2019-10-08T05:17:52Z

@jcouv This should likely go in a feature branch. I don't have permission to do that. Could you maybe make one off of master called features/static-lambdas. I can then rebase this PR (and #39121) to that branch. Thanks! :)

jcouv · 2019-10-08T17:36:11Z

@CyrusNajmabadi Branch features/static-lambdas created.

RikkiGibson · 2019-10-08T18:10:54Z

added auto-merge config in dotnet/roslyn-tools@d1b2c49 😄

CyrusNajmabadi · 2019-10-08T18:38:22Z

@jcouv This is ready for review. Thanks!

CyrusNajmabadi · 2019-10-08T18:38:59Z

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

+        public new LambdaExpressionSyntax WithBody(CSharpSyntaxNode body)
+            => body is BlockSyntax block
+                ? WithBlock(block).WithExpressionBody(null)
+                : WithExpressionBody((ExpressionSyntax)body).WithBlock(null);


this is just a move.

CyrusNajmabadi · 2019-10-08T18:39:14Z

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

-        public new LambdaExpressionSyntax WithBody(CSharpSyntaxNode body)
-            => body is BlockSyntax block
-                ? WithBlock(block).WithExpressionBody(null)
-                : WithExpressionBody((ExpressionSyntax)body).WithBlock(null);


this moved to its own file.

CyrusNajmabadi · 2019-10-08T18:40:35Z

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


 anonymous_method_expression
-  : 'async'? 'delegate' parameter_list? block expression?
+  : modifier* 'delegate' parameter_list? block expression?


in terms of grammar generator, we could consider adding information to Syntax.xml stating what subset of modifiers are legal. but no need to do that any time soon.

CyrusNajmabadi · 2019-10-08T18:41:09Z

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

+        public AnonymousFunctionExpressionSyntax WithAsyncKeyword(SyntaxToken asyncKeyword)
+            => WithAsyncKeywordCore(asyncKeyword);
+
+        internal abstract AnonymousFunctionExpressionSyntax WithAsyncKeywordCore(SyntaxToken asyncKeyword);


a lot of these methods are to preserve the previous public API shape.

CyrusNajmabadi · 2019-10-08T19:47:56Z

Thanks @RikkiGibson

CyrusNajmabadi · 2019-10-14T17:19:23Z

@jcouv can you ptal for this feature-branch pr? Thanks! :)

jcouv · 2019-10-14T22:26:13Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

            }
        }

+        private static bool IsAttributeOrModifiersList(Field f)


IsAttributeOrModifiersList [](start = 28, length = 26)

could be local function?

It will prevent conflicts with features/local-function-attributes to leave it as is. Maybe we can take care of it once one or the other feature branch is merged to master?

jcouv · 2019-10-14T22:32:39Z

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

+        public override SyntaxToken AsyncKeyword
+            => this.Modifiers.FirstOrDefault(SyntaxKind.AsyncKeyword);
+
+        internal override AnonymousFunctionExpressionSyntax WithAsyncKeywordCore(SyntaxToken asyncKeyword) => WithAsyncKeyword(asyncKeyword);


=> [](start = 107, length = 2)

nit: line break before => and also before next member?

These were actually copied out of the generated code. But sure, i don't mind tweaking this :)

jcouv

LGTM Thanks (iteration 17)

CyrusNajmabadi · 2019-10-14T23:34:56Z

Would you be willing to merge in? the overhead on waiting for CI is so high :)

RikkiGibson · 2019-10-14T23:45:21Z

Thanks @CyrusNajmabadi

CyrusNajmabadi · 2019-10-15T00:11:55Z

Thank you!

CyrusNajmabadi added 4 commits October 7, 2019 14:24

Move over from a token to a modifiers list

84b2169

Don't add overload that takes default modifiers list.

5785158

Restore overloads

9acbc7a

Merge remote-tracking branch 'upstream/master' into staticLambdas

f00739a

CyrusNajmabadi commented Oct 7, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 3 commits October 7, 2019 14:58

Update src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymb…

506504d

…ol.cs

Update public API

41f1ddf

Merge branch 'staticLambdas' of https://github.com/CyrusNajmabadi/roslyn

a49a09f

into staticLambdas

CyrusNajmabadi mentioned this pull request Oct 7, 2019

Add support for static lambdas. #39121

Merged

4 tasks

CyrusNajmabadi commented Oct 7, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into staticLambdas

ff55d28

Merge branch 'features/static-lambdas' into staticLambdas

42e4000

CyrusNajmabadi changed the base branch from master to features/static-lambdas October 8, 2019 17:53

CyrusNajmabadi commented Oct 8, 2019

View reviewed changes

CyrusNajmabadi marked this pull request as ready for review October 8, 2019 18:51

CyrusNajmabadi requested a review from a team as a code owner October 8, 2019 18:51

jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 8, 2019

RikkiGibson added this to the Compiler.Next milestone Oct 8, 2019

RikkiGibson approved these changes Oct 8, 2019

View reviewed changes

RikkiGibson requested a review from a team October 8, 2019 21:22

jcouv reviewed Oct 14, 2019

View reviewed changes

jcouv approved these changes Oct 14, 2019

View reviewed changes

jcouv self-assigned this Oct 14, 2019

RikkiGibson merged commit 4dfb5a5 into dotnet:features/static-lambdas Oct 14, 2019

CyrusNajmabadi deleted the staticLambdas branch October 15, 2019 00:11

Conversation

CyrusNajmabadi commented Oct 7, 2019 • edited by RikkiGibson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Oct 7, 2019

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!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Oct 8, 2019

Uh oh!

jcouv commented Oct 8, 2019

Uh oh!

RikkiGibson commented Oct 8, 2019

Uh oh!

CyrusNajmabadi commented Oct 8, 2019

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!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Oct 8, 2019

Uh oh!

CyrusNajmabadi commented Oct 14, 2019

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!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Oct 14, 2019

Uh oh!

RikkiGibson commented Oct 14, 2019

Uh oh!

CyrusNajmabadi commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Oct 7, 2019 •

edited by RikkiGibson

Loading