Skip to content
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

Merged
merged 12 commits into from
Jul 16, 2018
Merged

Conversation

fayrose
Copy link
Contributor

@fayrose fayrose commented Jul 5, 2018

Parsing changes for the using var feature ahead of LDM Monday.

@fayrose fayrose requested a review from a team as a code owner July 5, 2018 22:19
@jcouv jcouv added this to the 16.0 milestone Jul 5, 2018
else if (node.UsingKeyword != default)
{
kind = LocalDeclarationKind.UsingVariable;
}
Copy link
Member

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)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. in general we like a blank like after a }.
  2. this is indented improperly.
  3. avoid abbrs when possible. you can just say "var variableDeclaration" #Resolved

Copy link
Contributor Author

@fayrose fayrose Jul 5, 2018

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 5, 2018

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());

}
Copy link
Member

@jcouv jcouv Jul 5, 2018

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;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 5, 2018

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather like it. Tagging @gafter for advice on the shape of the syntax tree (extend local declarations, or create a dedicated kind of node?).


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

Copy link
Member

@agocke agocke Jul 5, 2018

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 5, 2018

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

Copy link
Contributor

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);
}
Copy link
Member

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));
}
Copy link
Member

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)
{
Copy link
Member

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);
}

Copy link
Member

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);
Copy link
Member

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?


}


Copy link
Member

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());

Copy link
Member

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&lt;SyntaxToken&gt;">
Copy link
Member

@jcouv jcouv Jul 5, 2018

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

Copy link
Member

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.

Copy link
Member

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);
Copy link
Member

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.

Copy link
Contributor Author

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;
Copy link
Member

@agocke agocke Jul 5, 2018

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;
Copy link
Member

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);

Copy link
Member

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))
Copy link
Member

@jcouv jcouv Jul 6, 2018

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)
Copy link
Member

@jcouv jcouv Jul 6, 2018

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 const and using? Never mind, the binder changes should be undone for this PR. #Resolved

N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken);
N(SyntaxKind.VariableDeclarator);
Copy link
Member

@agocke agocke Jul 10, 2018

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);
Copy link
Member

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 IdentifierTokens.

N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken);
N(SyntaxKind.VariableDeclarator);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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&lt;SyntaxToken&gt;">
Copy link
Member

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;
Copy link
Member

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;
}

Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Member

@agocke agocke left a 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);
Copy link
Member

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(
Copy link
Member

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;";
Copy link
Member

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.

Copy link
Member

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)

Copy link
Member

@jaredpar jaredpar left a 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
Copy link
Member

@jcouv jcouv Jul 13, 2018

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),
Copy link
Member

@jcouv jcouv Jul 13, 2018

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
Copy link
Member

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
Copy link
Member

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.

Copy link
Member

@jcouv jcouv left a 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

@fayrose fayrose merged commit 98e5398 into dotnet:features/enhanced-using Jul 16, 2018
@fayrose fayrose deleted the using-var-parsing branch July 16, 2018 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants