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.

Sign up for GitHub

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

Parsing Using Var #28315

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7fb24dc
Parsing
Jul 5, 2018
b4608e6
Requested changes
Jul 5, 2018
81632c1
Most of statement parsing tests
Jul 5, 2018
50779d4
Special case tree tests
Jul 6, 2018
3d73621
Reverts binder changes
Jul 6, 2018
686f245
Minor fix
Jul 6, 2018
249b490
Test fix
Jul 10, 2018
68426cf
More tests and bug fix
Jul 10, 2018
208e8db
Differentiated IdentifierNames
Jul 11, 2018
a2b5d0f
Added IdentifierToken differentiators
Jul 11, 2018
70bfd1f
Indenting fix
Jul 11, 2018
81d4440
added prototype comment
Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,10 @@ 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.

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

var variableList = node.Declaration.Variables;
int variableCount = variableList.Count;

Expand Down Expand Up @@ -817,7 +820,7 @@ protected BoundLocalDeclaration BindVariableDeclaration(
{
Debug.Assert(declarator != null);

return BindVariableDeclaration(LocateDeclaredVariableSymbol(declarator, typeSyntax),
return BindVariableDeclaration(LocateDeclaredVariableSymbol(declarator, typeSyntax, kind),
kind,
isVar,
declarator,
Expand Down Expand Up @@ -972,7 +975,7 @@ protected BoundLocalDeclaration BindVariableDeclaration(

ImmutableArray<BoundExpression> arguments = BindDeclaratorArguments(declarator, localDiagnostics);

if (kind == LocalDeclarationKind.FixedVariable || kind == LocalDeclarationKind.UsingVariable)
if (kind == LocalDeclarationKind.FixedVariable)
{
// CONSIDER: The error message is "you must provide an initializer in a fixed
// CONSIDER: or using declaration". The error message could be targetted to
Expand Down Expand Up @@ -1025,12 +1028,13 @@ internal ImmutableArray<BoundExpression> BindDeclaratorArguments(VariableDeclara
return arguments;
}

private SourceLocalSymbol LocateDeclaredVariableSymbol(VariableDeclaratorSyntax declarator, TypeSyntax typeSyntax)
private SourceLocalSymbol LocateDeclaredVariableSymbol(VariableDeclaratorSyntax declarator, TypeSyntax typeSyntax, LocalDeclarationKind outerKind)
{
return LocateDeclaredVariableSymbol(declarator.Identifier, typeSyntax, declarator.Initializer);
LocalDeclarationKind kind = outerKind == LocalDeclarationKind.UsingVariable ? LocalDeclarationKind.UsingVariable : LocalDeclarationKind.RegularVariable;
return LocateDeclaredVariableSymbol(declarator.Identifier, typeSyntax, declarator.Initializer, kind);
}

private SourceLocalSymbol LocateDeclaredVariableSymbol(SyntaxToken identifier, TypeSyntax typeSyntax, EqualsValueClauseSyntax equalsValue)
private SourceLocalSymbol LocateDeclaredVariableSymbol(SyntaxToken identifier, TypeSyntax typeSyntax, EqualsValueClauseSyntax equalsValue, LocalDeclarationKind kind)
{
SourceLocalSymbol localSymbol = this.LookupLocal(identifier);

Expand All @@ -1044,7 +1048,7 @@ private SourceLocalSymbol LocateDeclaredVariableSymbol(SyntaxToken identifier, T
false, // do not allow ref
typeSyntax,
identifier,
LocalDeclarationKind.RegularVariable,
kind,
equalsValue);
}

Expand Down
16 changes: 14 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/LocalScopeBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

if (decl.IsConst)
Copy link
Member

@jcouv jcouv Jul 6, 2018 •
edited
Loading

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

{
kind = LocalDeclarationKind.Constant;
}
else if (decl.UsingKeyword != default(SyntaxToken))
Copy link
Member

@jcouv jcouv Jul 6, 2018 •
edited
Loading

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

{
kind = LocalDeclarationKind.UsingVariable;
}
else
{
kind = LocalDeclarationKind.RegularVariable;
}
foreach (var vdecl in decl.Declaration.Variables)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 5, 2018 •
edited by jcouv
Loading

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 •
edited by jcouv
Loading

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 •
edited by jcouv
Loading

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

{
var localSymbol = MakeLocal(decl.Declaration, vdecl, kind, localDeclarationBinder);
locals.Add(localSymbol);
Expand Down
108 changes: 68 additions & 40 deletions src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12284,14 +12284,20 @@ static LocalFunctionStatementSyntax()

internal sealed partial class LocalDeclarationStatementSyntax : StatementSyntax
{
internal readonly SyntaxToken usingKeyword;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jul 5, 2018 •
edited by jcouv
Loading

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 •
edited by jcouv
Loading

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 •
edited by jcouv
Loading

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
Member

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.

internal readonly GreenNode modifiers;
internal readonly VariableDeclarationSyntax declaration;
internal readonly SyntaxToken semicolonToken;

internal LocalDeclarationStatementSyntax(SyntaxKind kind, GreenNode modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken, DiagnosticInfo[] diagnostics, SyntaxAnnotation[] annotations)
internal LocalDeclarationStatementSyntax(SyntaxKind kind, SyntaxToken usingKeyword, GreenNode modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken, DiagnosticInfo[] diagnostics, SyntaxAnnotation[] annotations)
: base(kind, diagnostics, annotations)
{
this.SlotCount = 3;
this.SlotCount = 4;
if (usingKeyword != null)
{
this.AdjustFlagsAndWidth(usingKeyword);
this.usingKeyword = usingKeyword;
}
if (modifiers != null)
{
this.AdjustFlagsAndWidth(modifiers);
Expand All @@ -12304,11 +12310,16 @@ internal LocalDeclarationStatementSyntax(SyntaxKind kind, GreenNode modifiers, V
}


internal LocalDeclarationStatementSyntax(SyntaxKind kind, GreenNode modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken, SyntaxFactoryContext context)
internal LocalDeclarationStatementSyntax(SyntaxKind kind, SyntaxToken usingKeyword, GreenNode modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken, SyntaxFactoryContext context)
: base(kind)
{
this.SetFactoryContext(context);
this.SlotCount = 3;
this.SlotCount = 4;
if (usingKeyword != null)
{
this.AdjustFlagsAndWidth(usingKeyword);
this.usingKeyword = usingKeyword;
}
if (modifiers != null)
{
this.AdjustFlagsAndWidth(modifiers);
Expand All @@ -12321,10 +12332,15 @@ internal LocalDeclarationStatementSyntax(SyntaxKind kind, GreenNode modifiers, V
}


internal LocalDeclarationStatementSyntax(SyntaxKind kind, GreenNode modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
internal LocalDeclarationStatementSyntax(SyntaxKind kind, SyntaxToken usingKeyword, GreenNode modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
: base(kind)
{
this.SlotCount = 3;
this.SlotCount = 4;
if (usingKeyword != null)
{
this.AdjustFlagsAndWidth(usingKeyword);
this.usingKeyword = usingKeyword;
}
if (modifiers != null)
{
this.AdjustFlagsAndWidth(modifiers);
Expand All @@ -12336,6 +12352,7 @@ internal LocalDeclarationStatementSyntax(SyntaxKind kind, GreenNode modifiers, V
this.semicolonToken = semicolonToken;
}

public SyntaxToken UsingKeyword { get { return this.usingKeyword; } }
/// <summary>Gets the modifier list.</summary>
public Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken> Modifiers { get { return new Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken>(this.modifiers); } }
public VariableDeclarationSyntax Declaration { get { return this.declaration; } }
Expand All @@ -12345,9 +12362,10 @@ internal override GreenNode GetSlot(int index)
{
switch (index)
{
case 0: return this.modifiers;
case 1: return this.declaration;
case 2: return this.semicolonToken;
case 0: return this.usingKeyword;
case 1: return this.modifiers;
case 2: return this.declaration;
case 3: return this.semicolonToken;
default: return null;
}
}
Expand All @@ -12367,11 +12385,11 @@ public override void Accept(CSharpSyntaxVisitor visitor)
visitor.VisitLocalDeclarationStatement(this);
}

public LocalDeclarationStatementSyntax Update(Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken> modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
public LocalDeclarationStatementSyntax Update(SyntaxToken usingKeyword, Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken> modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
{
if (modifiers != this.Modifiers || declaration != this.Declaration || semicolonToken != this.SemicolonToken)
if (usingKeyword != this.UsingKeyword || modifiers != this.Modifiers || declaration != this.Declaration || semicolonToken != this.SemicolonToken)
{
var newNode = SyntaxFactory.LocalDeclarationStatement(modifiers, declaration, semicolonToken);
var newNode = SyntaxFactory.LocalDeclarationStatement(usingKeyword, modifiers, declaration, semicolonToken);
var diags = this.GetDiagnostics();
if (diags != null && diags.Length > 0)
newNode = newNode.WithDiagnosticsGreen(diags);
Expand All @@ -12386,18 +12404,24 @@ public LocalDeclarationStatementSyntax Update(Microsoft.CodeAnalysis.Syntax.Inte

internal override GreenNode SetDiagnostics(DiagnosticInfo[] diagnostics)
{
return new LocalDeclarationStatementSyntax(this.Kind, this.modifiers, this.declaration, this.semicolonToken, diagnostics, GetAnnotations());
return new LocalDeclarationStatementSyntax(this.Kind, this.usingKeyword, this.modifiers, this.declaration, this.semicolonToken, diagnostics, GetAnnotations());
}

internal override GreenNode SetAnnotations(SyntaxAnnotation[] annotations)
{
return new LocalDeclarationStatementSyntax(this.Kind, this.modifiers, this.declaration, this.semicolonToken, GetDiagnostics(), annotations);
return new LocalDeclarationStatementSyntax(this.Kind, this.usingKeyword, this.modifiers, this.declaration, this.semicolonToken, GetDiagnostics(), annotations);
}

internal LocalDeclarationStatementSyntax(ObjectReader reader)
: base(reader)
{
this.SlotCount = 3;
this.SlotCount = 4;
var usingKeyword = (SyntaxToken)reader.ReadValue();
if (usingKeyword != null)
{
AdjustFlagsAndWidth(usingKeyword);
this.usingKeyword = usingKeyword;
}
var modifiers = (GreenNode)reader.ReadValue();
if (modifiers != null)
{
Expand All @@ -12421,6 +12445,7 @@ internal LocalDeclarationStatementSyntax(ObjectReader reader)
internal override void WriteTo(ObjectWriter writer)
{
base.WriteTo(writer);
writer.WriteValue(this.usingKeyword);
writer.WriteValue(this.modifiers);
writer.WriteValue(this.declaration);
writer.WriteValue(this.semicolonToken);
Expand Down Expand Up @@ -36512,10 +36537,11 @@ public override CSharpSyntaxNode VisitLocalFunctionStatement(LocalFunctionStatem

public override CSharpSyntaxNode VisitLocalDeclarationStatement(LocalDeclarationStatementSyntax node)
{
var usingKeyword = (SyntaxToken)this.Visit(node.UsingKeyword);
var modifiers = this.VisitList(node.Modifiers);
var declaration = (VariableDeclarationSyntax)this.Visit(node.Declaration);
var semicolonToken = (SyntaxToken)this.Visit(node.SemicolonToken);
return node.Update(modifiers, declaration, semicolonToken);
return node.Update(usingKeyword, modifiers, declaration, semicolonToken);
}

public override CSharpSyntaxNode VisitVariableDeclaration(VariableDeclarationSyntax node)
Expand Down Expand Up @@ -40348,9 +40374,20 @@ public LocalFunctionStatementSyntax LocalFunctionStatement(Microsoft.CodeAnalysi
return new LocalFunctionStatementSyntax(SyntaxKind.LocalFunctionStatement, modifiers.Node, returnType, identifier, typeParameterList, parameterList, constraintClauses.Node, body, expressionBody, semicolonToken, this.context);
}

public LocalDeclarationStatementSyntax LocalDeclarationStatement(Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken> modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
public LocalDeclarationStatementSyntax LocalDeclarationStatement(SyntaxToken usingKeyword, Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken> modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
{
#if DEBUG
if (usingKeyword != null)
{
switch (usingKeyword.Kind)
{
case SyntaxKind.UsingKeyword:
case SyntaxKind.None:
break;
default:
throw new ArgumentException("usingKeyword");
}
}
if (declaration == null)
throw new ArgumentNullException(nameof(declaration));
if (semicolonToken == null)
Expand All @@ -40364,17 +40401,7 @@ public LocalDeclarationStatementSyntax LocalDeclarationStatement(Microsoft.CodeA
}
#endif

int hash;
var cached = CSharpSyntaxNodeCache.TryGetNode((int)SyntaxKind.LocalDeclarationStatement, modifiers.Node, declaration, semicolonToken, this.context, out hash);
if (cached != null) return (LocalDeclarationStatementSyntax)cached;

var result = new LocalDeclarationStatementSyntax(SyntaxKind.LocalDeclarationStatement, modifiers.Node, declaration, semicolonToken, this.context);
if (hash >= 0)
{
SyntaxNodeCache.AddNode(result, hash);
}

return result;
return new LocalDeclarationStatementSyntax(SyntaxKind.LocalDeclarationStatement, usingKeyword, modifiers.Node, declaration, semicolonToken, this.context);
}

public VariableDeclarationSyntax VariableDeclaration(TypeSyntax type, Microsoft.CodeAnalysis.Syntax.InternalSyntax.SeparatedSyntaxList<VariableDeclaratorSyntax> variables)
Expand Down Expand Up @@ -47311,9 +47338,20 @@ public static LocalFunctionStatementSyntax LocalFunctionStatement(Microsoft.Code
return new LocalFunctionStatementSyntax(SyntaxKind.LocalFunctionStatement, modifiers.Node, returnType, identifier, typeParameterList, parameterList, constraintClauses.Node, body, expressionBody, semicolonToken);
}

public static LocalDeclarationStatementSyntax LocalDeclarationStatement(Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken> modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
public static LocalDeclarationStatementSyntax LocalDeclarationStatement(SyntaxToken usingKeyword, Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList<SyntaxToken> modifiers, VariableDeclarationSyntax declaration, SyntaxToken semicolonToken)
{
#if DEBUG
if (usingKeyword != null)
{
switch (usingKeyword.Kind)
{
case SyntaxKind.UsingKeyword:
case SyntaxKind.None:
break;
default:
throw new ArgumentException("usingKeyword");
}
}
if (declaration == null)
throw new ArgumentNullException(nameof(declaration));
if (semicolonToken == null)
Expand All @@ -47327,17 +47365,7 @@ public static LocalDeclarationStatementSyntax LocalDeclarationStatement(Microsof
}
#endif

int hash;
var cached = SyntaxNodeCache.TryGetNode((int)SyntaxKind.LocalDeclarationStatement, modifiers.Node, declaration, semicolonToken, out hash);
if (cached != null) return (LocalDeclarationStatementSyntax)cached;

var result = new LocalDeclarationStatementSyntax(SyntaxKind.LocalDeclarationStatement, modifiers.Node, declaration, semicolonToken);
if (hash >= 0)
{
SyntaxNodeCache.AddNode(result, hash);
}

return result;
return new LocalDeclarationStatementSyntax(SyntaxKind.LocalDeclarationStatement, usingKeyword, modifiers.Node, declaration, semicolonToken);
}

public static VariableDeclarationSyntax VariableDeclaration(TypeSyntax type, Microsoft.CodeAnalysis.Syntax.InternalSyntax.SeparatedSyntaxList<VariableDeclaratorSyntax> variables)
Expand Down
Loading