Skip to content

Extended Keyword Parsing to cover member lookup expressions. #29233

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

Closed
wants to merge 5 commits into from
Closed

Extended Keyword Parsing to cover member lookup expressions. #29233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
577c6f0
Extend the parsing for keywords, to look ahead at the next token to s…
AdamSpeight2008 Aug 12, 2018
0071d44
Initial feedback work.
AdamSpeight2008 Aug 13, 2018
0710328
Further Feedback changes.
AdamSpeight2008 Aug 13, 2018
5e675c5
Revert Changes
AdamSpeight2008 Aug 13, 2018
6b8dc2e
Additional Unit Tests
AdamSpeight2008 Aug 14, 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
32 changes: 23 additions & 9 deletions src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Imports InternalSyntaxFactory = Microsoft.CodeAnalysis.VisualBasic.Syntax.Intern

Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax

Friend Partial Class Parser
Partial Friend Class Parser
Copy link
Member

Choose a reason for hiding this comment

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

Please revert

Copy link
Contributor Author

@AdamSpeight2008 AdamSpeight2008 Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jaredpar I think this is caused by an something in the IDE, possible pretty lister? Or Code Style enforcement?
Writing Friend Partial is converted to Partial Friend automatically. Even happens when copy and pasting in the original code.

See Issue #27893


Friend Function ParseExpression(
Optional pendingPrecedence As OperatorPrecedence = OperatorPrecedence.PrecedenceNone,
Expand Down Expand Up @@ -46,7 +46,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
' bool BailIfFirstTokenRejected // bail (return NULL) if the first token isn't a valid expression-starter, rather than reporting an error or setting ErrorInConstruct
Private Function ParseExpressionCore(
Optional pendingPrecedence As OperatorPrecedence = OperatorPrecedence.PrecedenceNone,
Optional bailIfFirstTokenRejected As Boolean = False
Optional bailIfFirstTokenRejected As Boolean = False,
Optional PrevTerm As ExpressionSyntax = Nothing
Copy link
Member

Choose a reason for hiding this comment

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

Please use camelCased names.

) As ExpressionSyntax

Try
Expand Down Expand Up @@ -107,7 +108,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
expression = SyntaxFactory.AddressOfExpression(startToken, Operand)

Case Else
expression = ParseTerm(bailIfFirstTokenRejected)
expression = ParseTerm(bailIfFirstTokenRejected, PrevTerm:=PrevTerm)

If expression Is Nothing Then
Return Nothing
Expand Down Expand Up @@ -157,7 +158,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
'Binary.Opcode = Opcode

'Binary.Left = Expr
Dim rightOperand As ExpressionSyntax = ParseExpressionCore(precedence)
Dim rightOperand As ExpressionSyntax = ParseExpressionCore(precedence, PrevTerm:=PrevTerm)

expression = SyntaxFactory.BinaryExpression(GetBinaryOperatorHelper([operator]), expression, [operator], rightOperand)

Expand All @@ -175,7 +176,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax

Private Function ParseTerm(
Optional BailIfFirstTokenRejected As Boolean = False,
Optional RedimOrNewParent As Boolean = False
Optional RedimOrNewParent As Boolean = False,
Optional PrevTerm As ExpressionSyntax = Nothing
) As ExpressionSyntax

'// Note: this function will only ever return NULL if the flag "BailIfFirstTokenIsRejected" is set,
Expand Down Expand Up @@ -232,10 +234,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
term = ParseSimpleNameExpressionAllowingKeywordAndTypeArguments()

Case SyntaxKind.ExclamationToken
term = ParseQualifiedExpr(Nothing)
term = ParseQualifiedExpr(PrevTerm)

Case SyntaxKind.DotToken
term = ParseQualifiedExpr(Nothing)
term = ParseQualifiedExpr(PrevTerm)

Case SyntaxKind.GlobalKeyword
' NB. GetNextToken has the side-effect of advancing CurrentToken.
Expand Down Expand Up @@ -410,9 +412,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
Case SyntaxKind.DollarSignDoubleQuoteToken

term = ParseInterpolatedStringExpression()

Case Else

If start.Kind = SyntaxKind.QuestionToken AndAlso CanStartConsequenceExpression(Me.PeekToken(1).Kind, qualified:=False) Then
' This looks like ?. or ?!

Expand All @@ -421,7 +421,21 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax

GetNextToken()
term = SyntaxFactory.ConditionalAccessExpression(term, qToken, ParsePostFixExpression(RedimOrNewParent, term:=Nothing))
ElseIf start.IsKeyword Then
' Is it in the form : [keyword] [qualifier] [identifier]
' If so treat the keyword as an identifier and continue to parse.
Dim _NextToken_ = PeekNextToken()
If _NextToken_.Kind = SyntaxKind.DotToken OrElse _NextToken_.Kind = SyntaxKind.QuestionToken Then
Dim _KeywordAsIdentifier_ = ParseSimpleNameExpressionAllowingKeywordAndTypeArguments()
_KeywordAsIdentifier_ = ReportSyntaxError(_KeywordAsIdentifier_, ERRID.ERR_InvalidUseOfKeyword)
Dim _InvocationExpression_ = ParseExpressionCore(PrevTerm:=_KeywordAsIdentifier_)
If _InvocationExpression_.Kind = SyntaxKind.InvocationExpression Then
Return _InvocationExpression_
End If
End If
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 unnecessary. though you can potentially put a comment there saying it is intentional that you are falling through.

GoTo MissingExpression
Else
MissingExpression:
Copy link
Member

Choose a reason for hiding this comment

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

a goto here seems unnecessary. why not just replace the else with an "End If"?

If BailIfFirstTokenRejected Then
Return Nothing
End If
Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/VisualBasic/Portable/Parser/ParseInterpolatedString.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ Imports InternalSyntaxFactory = Microsoft.CodeAnalysis.VisualBasic.Syntax.Intern

Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax

Friend Partial Class Parser
Partial Friend Class Parser
Copy link
Member

Choose a reason for hiding this comment

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

Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See Issue #27893


Private Function ParseInterpolatedStringExpression() As InterpolatedStringExpressionSyntax
Debug.Assert(CurrentToken.Kind = SyntaxKind.DollarSignDoubleQuoteToken, "ParseInterpolatedStringExpression called on the wrong token.")
Debug.Assert(CurrentToken.Kind = SyntaxKind.DollarSignDoubleQuoteToken, $"{NameOf(ParseInterpolatedStringExpression)} called on the wrong token.")

ResetCurrentToken(ScannerState.InterpolatedStringPunctuation)

Expand Down Expand Up @@ -96,7 +96,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
End Function

Private Function ParseInterpolatedStringInterpolation() As InterpolationSyntax
Debug.Assert(CurrentToken.Kind = SyntaxKind.OpenBraceToken, "ParseInterpolatedStringEmbeddedExpression called on the wrong token.")
Debug.Assert(CurrentToken.Kind = SyntaxKind.OpenBraceToken, $"{NameOf(ParseInterpolatedStringInterpolation)} called on the wrong token.")

Dim colonToken As PunctuationSyntax = Nothing
Dim excessText As String = Nothing
Expand Down Expand Up @@ -279,7 +279,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
indexOfFirstColon = 0
newTrailingTrivia = Nothing
excessText = Nothing
ElseIf triviaList(0).Kind = SyntaxKind.ColonTrivia
ElseIf triviaList(0).Kind = SyntaxKind.ColonTrivia Then
indexOfFirstColon = 0
newTrailingTrivia = Nothing
excessText = triviaList.GetEndOfTrivia(1).Node.ToFullString()
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/VisualBasic/Portable/Scanner/TokenStream.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax

' peeking current token is strange, but ok. Note that it ignores the state.
If tokenOffset = 0 Then
Debug.Assert(_currentToken.State = state)
Debug.Assert(_currentToken.State = state, $"_CurrentToken.State:={_currentToken.State} state:={state}")

Return GetCurrentToken()
End If
Expand Down
18 changes: 18 additions & 0 deletions src/Compilers/VisualBasic/Test/Syntax/Parser/InterpolatedStringParsingTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,4 +922,22 @@ BC30648: String constants must end with a double quote.

End Sub

<Fact>
Public Sub Test_KeywordExpression()
Dim code =
"Class Example
Public ReadOnly Property [End] As Integer
Public Overrides Function ToString() As String
Return $""{End.ToString()})""
End Function
End Class"
Dim result = Parse(code)
Dim diagnostics = result.GetDiagnostics.ToImmutableListOrEmpty
result.AssertTheseDiagnostics(
<expected>
BC30183: Keyword is not valid as an identifier.
Return $"{End.ToString()})"
Copy link
Member

Choose a reason for hiding this comment

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

can we have a lot more tests than this? we'll need tests of keywords in many different areas followed by any allowable delimiter. We should also have specific tree and diagnostics tests

Copy link
Member

Choose a reason for hiding this comment

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

this also affects far more than interpolated strings. i would expect a lot of statement/expression parsing tests.

~~~
</expected>)
End Sub
End Class