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.

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
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
42 changes: 29 additions & 13 deletions src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb
Original file line number Diff line number Diff line change
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
) 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 @@ -174,8 +175,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
End Function

Private Function ParseTerm(
Optional BailIfFirstTokenRejected As Boolean = False,
Optional RedimOrNewParent As Boolean = False
Optional bailIfFirstTokenRejected 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 @@ -296,7 +298,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
Dim tokenHasFullWidthChars As Boolean = TokenContainsFullWidthChars(start)

If tokenHasFullWidthChars Then
If BailIfFirstTokenRejected Then
If bailIfFirstTokenRejected Then
Return Nothing
End If

Expand Down Expand Up @@ -410,19 +412,33 @@ 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 ?!

Dim qToken = DirectCast(start, PunctuationSyntax)
qToken = CheckFeatureAvailability(Feature.NullPropagatingOperator, qToken)

GetNextToken()
term = SyntaxFactory.ConditionalAccessExpression(term, qToken, ParsePostFixExpression(RedimOrNewParent, term:=Nothing))
term = SyntaxFactory.ConditionalAccessExpression(term, qToken, ParsePostFixExpression(redimOrNewParent, term:=Nothing))
Else
If BailIfFirstTokenRejected Then
If 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)
Copy link
Member

Choose a reason for hiding this comment

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

Foo is not our naming pattern for locals.

If _InvocationExpression_.Kind = SyntaxKind.InvocationExpression Then
Copy link
Member

Choose a reason for hiding this comment

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

why are you only allowing invocation expressions. this won't help if you did something like $"{End.Prop}"

Return _InvocationExpression_
End If
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.


If bailIfFirstTokenRejected Then
Return Nothing
End If

Expand All @@ -439,7 +455,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
' Valid suffixes are ".", "!", and "(". Everything else is considered
' to end the term.

term = ParsePostFixExpression(RedimOrNewParent, term)
term = ParsePostFixExpression(redimOrNewParent, term)
End If

If CurrentToken IsNot Nothing AndAlso CurrentToken.Kind = SyntaxKind.QuestionToken Then
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,4 +332,4 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax

End Class

End Namespace
End Namespace
Copy link
Member

Choose a reason for hiding this comment

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

please undo this.

2 changes: 1 addition & 1 deletion src/Compilers/VisualBasic/Portable/Scanner/TokenStream.vb
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
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