-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parsing Using Var #28315
Parsing Using Var #28315
Conversation
else if (node.UsingKeyword != default) | ||
{ | ||
kind = LocalDeclarationKind.UsingVariable; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -12284,14 +12284,20 @@ static LocalFunctionStatementSyntax() | |||
|
|||
internal sealed partial class LocalDeclarationStatementSyntax : StatementSyntax | |||
{ | |||
internal readonly SyntaxToken usingKeyword; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. use =>
@@ -21,5 +29,10 @@ public static StackAllocArrayCreationExpressionSyntax StackAllocArrayCreationExp | |||
{ | |||
return StackAllocArrayCreationExpression(stackAllocKeyword, type, default(InitializerExpressionSyntax)); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline after this }
@@ -21,5 +29,10 @@ public static StackAllocArrayCreationExpressionSyntax StackAllocArrayCreationExp | |||
{ | |||
return StackAllocArrayCreationExpression(stackAllocKeyword, type, default(InitializerExpressionSyntax)); | |||
} | |||
public static LocalDeclarationStatementSyntax LocalDeclarationStatement(SyntaxTokenList modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider =>
{ | ||
return LocalDeclarationStatement(default(SyntaxToken), modifiers, declaration, semicolonToken); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to just use 'default' instead of 'default(SyntaxToken)' right?
|
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary blank line.
@@ -1853,6 +1853,9 @@ | |||
|
|||
<Node Name="LocalDeclarationStatementSyntax" Base="StatementSyntax"> | |||
<Kind Name="LocalDeclarationStatement"/> | |||
<Field Name="UsingKeyword" Type="SyntaxToken" Optional="true"> | |||
<Kind Name="UsingKeyword"/> | |||
</Field> | |||
<Field Name="Modifiers" Type="SyntaxList<SyntaxToken>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I would assume they would be binding errors, not syntax errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and pushed; have both versions for now.
@@ -12284,14 +12284,20 @@ static LocalFunctionStatementSyntax() | |||
|
|||
internal sealed partial class LocalDeclarationStatementSyntax : StatementSyntax | |||
{ | |||
internal readonly SyntaxToken usingKeyword; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -204,8 +204,20 @@ protected ImmutableArray<LocalSymbol> BuildLocals(SyntaxList<StatementSyntax> st | |||
{ | |||
Binder localDeclarationBinder = enclosingBinder.GetBinder(innerStatement) ?? enclosingBinder; | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra newline.
{ | ||
kind = LocalDeclarationKind.Constant; | ||
} | ||
else if (decl.UsingKeyword != default(SyntaxToken)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -204,7 +204,20 @@ protected ImmutableArray<LocalSymbol> BuildLocals(SyntaxList<StatementSyntax> st | |||
{ | |||
Binder localDeclarationBinder = enclosingBinder.GetBinder(innerStatement) ?? enclosingBinder; | |||
var decl = (LocalDeclarationStatementSyntax)innerStatement; | |||
LocalDeclarationKind kind = decl.IsConst ? LocalDeclarationKind.Constant : LocalDeclarationKind.RegularVariable; | |||
LocalDeclarationKind kind; | |||
if (decl.IsConst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 IdentifierToken
s.
N(SyntaxKind.IdentifierName); | ||
{ | ||
N(SyntaxKind.IdentifierToken); | ||
N(SyntaxKind.VariableDeclarator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation also looks wrong here.
N(SyntaxKind.IdentifierName); | ||
{ | ||
N(SyntaxKind.IdentifierToken); | ||
N(SyntaxKind.VariableDeclarator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation looks wrong.
} | ||
|
||
} | ||
N(SyntaxKind.QuestionToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like QuestionToken
should be inside the NullableType
N(SyntaxKind.IdentifierToken); | ||
} | ||
} | ||
N(SyntaxKind.QuestionToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QuestionToken
is part of NullableType
@@ -1853,6 +1853,9 @@ | |||
|
|||
<Node Name="LocalDeclarationStatementSyntax" Base="StatementSyntax"> | |||
<Kind Name="LocalDeclarationStatement"/> | |||
<Field Name="UsingKeyword" Type="SyntaxToken" Optional="true"> | |||
<Kind Name="UsingKeyword"/> | |||
</Field> | |||
<Field Name="Modifiers" Type="SyntaxList<SyntaxToken>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
@@ -8204,13 +8204,15 @@ private LabeledStatementSyntax ParseLabeledStatement() | |||
private StatementSyntax ParseLocalDeclarationStatement() | |||
{ | |||
var mods = _pool.Allocate(); | |||
var usingKeyword = this.CurrentToken.Kind == SyntaxKind.UsingKeyword ? this.EatToken() : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well put this at the top and keep the mods
declaration and ParseDeclarationModifiers
together.
@@ -618,7 +618,6 @@ private BoundStatement BindDeclarationStatementParts(LocalDeclarationStatementSy | |||
{ | |||
kind = LocalDeclarationKind.Constant; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look necessary.
N(SyntaxKind.UsingKeyword); | ||
N(SyntaxKind.VariableDeclaration); | ||
{ | ||
N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing strings for the IdentifierNames
N(SyntaxKind.UsingKeyword); | ||
N(SyntaxKind.VariableDeclaration); | ||
{ | ||
N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
N(SyntaxKind.UsingKeyword); | ||
N(SyntaxKind.VariableDeclaration); | ||
{ | ||
N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
} | ||
N(SyntaxKind.VariableDeclarator); | ||
{ | ||
N(SyntaxKind.IdentifierToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
N(SyntaxKind.RefExpression); | ||
{ | ||
N(SyntaxKind.RefKeyword); | ||
N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
N(SyntaxKind.RefType); | ||
{ | ||
N(SyntaxKind.RefKeyword); | ||
N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
N(SyntaxKind.RefType); | ||
{ | ||
N(SyntaxKind.RefKeyword); | ||
N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
N(SyntaxKind.ReadOnlyKeyword); | ||
N(SyntaxKind.VariableDeclaration); | ||
{ | ||
N(SyntaxKind.IdentifierName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from nesting problem.
} | ||
} | ||
} | ||
N(SyntaxKind.CommaToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this doing? I'm unsure what exactly is happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. It's a type f?
there is just a space before the ?
.
In reply to: 201878919 [](ancestors = 201878919)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a PROTOTYPE comment to track the API decision but looks good otherwise.
@@ -8240,9 +8242,10 @@ private StatementSyntax ParseLocalDeclarationStatement() | |||
mods[i] = this.AddError(mod, ErrorCode.ERR_BadMemberFlag, mod.Text); | |||
} | |||
} | |||
|
|||
// PROTOTYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -2533,21 +2533,15 @@ void B() | |||
} | |||
"; | |||
CreateCompilation(source).VerifyDiagnostics( | |||
// (6,8): error CS1003: Syntax error, '(' expected | |||
// (6,8): error CS1031: Type expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some test suggestions and nits (can be addressed in next PR). Thanks
Parsing changes for the
using var
feature ahead of LDM Monday.