Parsing Using Var#28315
Conversation
| else if (node.UsingKeyword != default) | ||
| { | ||
| kind = LocalDeclarationKind.UsingVariable; | ||
| } |
There was a problem hiding this comment.
this looks to be going beyond just a parsing change (as the title of the PR implies).
| { | ||
| kind = LocalDeclarationKind.RegularVariable; | ||
| } | ||
| foreach (var vdecl in decl.Declaration.Variables) |
There was a problem hiding this comment.
- in general we like a blank like after a }.
- this is indented improperly.
- avoid abbrs when possible. you can just say "var variableDeclaration" #Resolved
There was a problem hiding this comment.
I just accidentally indented a line or two due to copying and pasting code between branches to isolate parsing from the rest of the feature. Anything including vdecl is not my code. Should I change it regardless? #Resolved
There was a problem hiding this comment.
No. Once you fix the indentation, just leave the code alone if it isn't necessary to change for your PR. Thanks! :) #Resolved
| Assert.NotNull(us.Declaration); | ||
| Assert.Equal("f ? x = a", us.Declaration.ToString()); | ||
|
|
||
| } |
There was a problem hiding this comment.
Nit: bunch of unnecessary empty lines here and elsewhere.
Also, for testing syntax, have you tried tests that use this format: TernaryVersusDeclaration_05?
Those will show and validate the structure of the parse tree. A small trick to help writing such tests is to uncomment //#define PARSING_TESTS_DUMP in the base file (ParsingTests) so that the test output gives you the expected code. #Closed
|
|
||
| internal sealed partial class LocalDeclarationStatementSyntax : StatementSyntax | ||
| { | ||
| internal readonly SyntaxToken usingKeyword; |
There was a problem hiding this comment.
i'm not s huge fan of repurposing this node in this manner. It feels more appropriate as a new sort of node.
But i could be convinced otherwise. #Closed
There was a problem hiding this comment.
This is usually what we do when we augment nodes. For instance, when we added ref locals we didn't add new types of local declarations, we just added optional ref modifiers. I think it's the same situation here. #Resolved
There was a problem hiding this comment.
Yeah, i'm on the fence. Something feels... 'wonky' here. Perhaps because 'using' bridges between two things (using-statements, and local-var-statements). That's unlike 'ref'.
That said, this may just need some time on my part to get used to it. So this is not at all strong pushback. If the team feels like this is "good", then i have no objection. #Resolved
There was a problem hiding this comment.
Could it be parsed into Modifiers on LocalDeclaration? Currently Modifiers is only used for const.
| public LocalDeclarationStatementSyntax Update(SyntaxTokenList modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken) | ||
| { | ||
| return Update(default(SyntaxToken), modifiers, declaration, semicolonToken); | ||
| } |
| @@ -21,5 +29,10 @@ public static StackAllocArrayCreationExpressionSyntax StackAllocArrayCreationExp | |||
| { | |||
| return StackAllocArrayCreationExpression(stackAllocKeyword, type, default(InitializerExpressionSyntax)); | |||
| } | |||
There was a problem hiding this comment.
newline after this }
| return StackAllocArrayCreationExpression(stackAllocKeyword, type, default(InitializerExpressionSyntax)); | ||
| } | ||
| public static LocalDeclarationStatementSyntax LocalDeclarationStatement(SyntaxTokenList modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken) | ||
| { |
| { | ||
| return LocalDeclarationStatement(default(SyntaxToken), modifiers, declaration, semicolonToken); | ||
| } | ||
|
|
There was a problem hiding this comment.
reomve this blank line.
| { | ||
| public LocalDeclarationStatementSyntax Update(SyntaxTokenList modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken) | ||
| { | ||
| return Update(default(SyntaxToken), modifiers, declaration, semicolonToken); |
There was a problem hiding this comment.
you should be able to just use 'default' instead of 'default(SyntaxToken)' right?
|
|
||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
too many blank lines.
| Assert.Equal(SyntaxKind.UsingKeyword, us.UsingKeyword.Kind()); | ||
| Assert.NotNull(us.Declaration); | ||
| Assert.Equal("f ? x = a", us.Declaration.ToString()); | ||
|
|
There was a problem hiding this comment.
unnecessary blank line.
| <Field Name="UsingKeyword" Type="SyntaxToken" Optional="true"> | ||
| <Kind Name="UsingKeyword"/> | ||
| </Field> | ||
| <Field Name="Modifiers" Type="SyntaxList<SyntaxToken>"> |
There was a problem hiding this comment.
Modifiers [](start = 17, length = 9)
I see that the parsing logic tolerates putting modifiers on local declarations. Can you test such scenarios with using locals?
I assume all modifiers in this position should be errors. Is that correct? #Closed
There was a problem hiding this comment.
Well, I would assume they would be binding errors, not syntax errors.
There was a problem hiding this comment.
Looks like we still need a test for this case.
I think we should test all of the following cases:
using ref int x = ref y;
using ref readonly int x = ref y;
using ref var x = ref y;
using ref var x = y;
using readonly var x, y = ref z;| var text = "using T a = b;"; | ||
| var statement = this.ParseStatement(text); | ||
|
|
||
| Assert.NotNull(statement); |
There was a problem hiding this comment.
I think these tests would be far more readable if you use the UsingTree helper. I think most of the existing tests are written in this style because they existed before that helper did.
There was a problem hiding this comment.
Done and pushed; have both versions for now.
|
|
||
| internal sealed partial class LocalDeclarationStatementSyntax : StatementSyntax | ||
| { | ||
| internal readonly SyntaxToken usingKeyword; |
There was a problem hiding this comment.
This is usually what we do when we augment nodes. For instance, when we added ref locals we didn't add new types of local declarations, we just added optional ref modifiers. I think it's the same situation here. #Resolved
| var decl = (LocalDeclarationStatementSyntax)innerStatement; | ||
| LocalDeclarationKind kind = decl.IsConst ? LocalDeclarationKind.Constant : LocalDeclarationKind.RegularVariable; | ||
| foreach (var vdecl in decl.Declaration.Variables) | ||
| LocalDeclarationKind kind; |
There was a problem hiding this comment.
I don't think these binder changes should be included. Right now we're only testing parsing.
|
|
||
| public static LocalDeclarationStatementSyntax LocalDeclarationStatement(SyntaxTokenList modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken) | ||
| => LocalDeclarationStatement(default(SyntaxToken), modifiers, declaration, semicolonToken); | ||
|
|
| { | ||
| kind = LocalDeclarationKind.Constant; | ||
| } | ||
| else if (decl.UsingKeyword != default(SyntaxToken)) |
There was a problem hiding this comment.
SyntaxToken [](start = 66, length = 11)
~~Nit: you could use decl.UsingKeyword != default here (omit the type)~~Never mind, the binder changes should be undone for this PR.
#Resolved
| var decl = (LocalDeclarationStatementSyntax)innerStatement; | ||
| LocalDeclarationKind kind = decl.IsConst ? LocalDeclarationKind.Constant : LocalDeclarationKind.RegularVariable; | ||
| LocalDeclarationKind kind; | ||
| if (decl.IsConst) |
There was a problem hiding this comment.
Could you test the syntax tree when you have both Never mind, the binder changes should be undone for this PR. #Resolvedconst and using?
| N(SyntaxKind.IdentifierName); | ||
| { | ||
| N(SyntaxKind.IdentifierToken); | ||
| N(SyntaxKind.VariableDeclarator); |
There was a problem hiding this comment.
Indentation (nesting) looks wrong. I think the variable declarator should be directly beneath VariableDeclaration.
| { | ||
| N(SyntaxKind.IdentifierName); | ||
| { | ||
| N(SyntaxKind.IdentifierToken); |
There was a problem hiding this comment.
N( takes a second optional parameter here so you can verify the correct IdentifierToken (should be T I think). You should use it for all the IdentifierTokens.
| N(SyntaxKind.IdentifierName); | ||
| { | ||
| N(SyntaxKind.IdentifierToken); | ||
| N(SyntaxKind.VariableDeclarator); |
There was a problem hiding this comment.
Indentation also looks wrong here.
| N(SyntaxKind.IdentifierName); | ||
| { | ||
| N(SyntaxKind.IdentifierToken); | ||
| N(SyntaxKind.VariableDeclarator); |
| } | ||
|
|
||
| } | ||
| N(SyntaxKind.QuestionToken); |
There was a problem hiding this comment.
It looks like QuestionToken should be inside the NullableType
| N(SyntaxKind.IdentifierToken); | ||
| } | ||
| } | ||
| N(SyntaxKind.QuestionToken); |
There was a problem hiding this comment.
QuestionToken is part of NullableType
| <Field Name="UsingKeyword" Type="SyntaxToken" Optional="true"> | ||
| <Kind Name="UsingKeyword"/> | ||
| </Field> | ||
| <Field Name="Modifiers" Type="SyntaxList<SyntaxToken>"> |
There was a problem hiding this comment.
Looks like we still need a test for this case.
I think we should test all of the following cases:
using ref int x = ref y;
using ref readonly int x = ref y;
using ref var x = ref y;
using ref var x = y;
using readonly var x, y = ref z;| private StatementSyntax ParseLocalDeclarationStatement() | ||
| { | ||
| var mods = _pool.Allocate(); | ||
| var usingKeyword = this.CurrentToken.Kind == SyntaxKind.UsingKeyword ? this.EatToken() : null; |
There was a problem hiding this comment.
Might as well put this at the top and keep the mods declaration and ParseDeclarationModifiers together.
| { | ||
| kind = LocalDeclarationKind.Constant; | ||
| } | ||
|
|
| N(SyntaxKind.UsingKeyword); | ||
| N(SyntaxKind.VariableDeclaration); | ||
| { | ||
| N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Missing strings for the IdentifierNames
| N(SyntaxKind.UsingKeyword); | ||
| N(SyntaxKind.VariableDeclaration); | ||
| { | ||
| N(SyntaxKind.IdentifierName); |
| N(SyntaxKind.UsingKeyword); | ||
| N(SyntaxKind.VariableDeclaration); | ||
| { | ||
| N(SyntaxKind.IdentifierName); |
| } | ||
| N(SyntaxKind.VariableDeclarator); | ||
| { | ||
| N(SyntaxKind.IdentifierToken); |
| N(SyntaxKind.RefExpression); | ||
| { | ||
| N(SyntaxKind.RefKeyword); | ||
| N(SyntaxKind.IdentifierName); |
| N(SyntaxKind.RefType); | ||
| { | ||
| N(SyntaxKind.RefKeyword); | ||
| N(SyntaxKind.IdentifierName); |
| N(SyntaxKind.RefType); | ||
| { | ||
| N(SyntaxKind.RefKeyword); | ||
| N(SyntaxKind.IdentifierName); |
| N(SyntaxKind.ReadOnlyKeyword); | ||
| N(SyntaxKind.VariableDeclaration); | ||
| { | ||
| N(SyntaxKind.IdentifierName); |
agocke
left a comment
There was a problem hiding this comment.
LGTM aside from nesting problem.
| } | ||
| } | ||
| } | ||
| N(SyntaxKind.CommaToken); |
There was a problem hiding this comment.
I believe both the comma and variable declarator below are part of the variable declaration
| @@ -8243,6 +8245,7 @@ private StatementSyntax ParseLocalDeclarationStatement() | |||
|
|
|||
| var semicolon = this.EatToken(SyntaxKind.SemicolonToken); | |||
| return _syntaxFactory.LocalDeclarationStatement( | |||
There was a problem hiding this comment.
We should add a PROTOTYPE comment here to make sure we're okay with using on a local declaration being handled differently than other modifiers like ref, const, etc ... Fine for this PR but should have a comment tracking with the API design.
| [Fact] | ||
| public void TestUsingVarSpecialCase3() | ||
| { | ||
| var text = "using f ? x, y;"; |
There was a problem hiding this comment.
What is this doing? I'm unsure what exactly is happening here.
There was a problem hiding this comment.
Ah I see. It's a type f? there is just a space before the ?.
In reply to: 201878919 [](ancestors = 201878919)
jaredpar
left a comment
There was a problem hiding this comment.
We should add a PROTOTYPE comment to track the API decision but looks good otherwise.
| } | ||
| } | ||
|
|
||
| // PROTOTYPE |
There was a problem hiding this comment.
PROTOTYPE [](start = 19, length = 9)
For some reason, we typically use the branch name in PROTOTYPE markers. But, it's more convenient without. @jaredpar, OK with simple "PROTOTYPE"?
Also, can you expand the comment to say the purpose of the marker? What is the issue?
| [Fact] | ||
| public void TestUsingVarReadonlyMultipleDeclarations() | ||
| { | ||
| UsingStatement("using readonly var x, y = ref z;", CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.CSharp7_2), |
There was a problem hiding this comment.
using readonly [](start = 28, length = 14)
Could you also test with reverse order? readonly using ... (this shows that using is not just parsed as a modifier)
| "; | ||
| CreateCompilation(source).VerifyDiagnostics( | ||
| // (6,8): error CS1003: Syntax error, '(' expected | ||
| // (6,8): error CS1031: Type expected |
There was a problem hiding this comment.
// (6,8): error CS1031: Type expected [](start = 16, length = 37)
The diagnostic quality here isn't the best. You could also type a parenthesis. Maybe add a PROTOTYPE marker to revisit.
| @@ -7,9 +7,13 @@ namespace Microsoft.CodeAnalysis.CSharp.Syntax | |||
| public partial class StackAllocArrayCreationExpressionSyntax | |||
There was a problem hiding this comment.
I see that the stackalloc syntax derives from the local declaration syntax. Could you add a corresponding parsing test (using and stackalloc)? And also take a note to do a full test on that scenario (not just parsing).
Can you or @agocke start a test plan issue for this feature work, if you don't have one already? It is useful to collect test ideas.
jcouv
left a comment
There was a problem hiding this comment.
LGTM with some test suggestions and nits (can be addressed in next PR). Thanks
Parsing changes for the
using varfeature ahead of LDM Monday.