Extended Keyword Parsing to cover member lookup expressions. #29233

AdamSpeight2008 · 2018-08-12T13:15:04Z

Fix for #29231
Extend the parsing for keywords, to look ahead at the next token to ee if it is a qualifier token.

If it is then treat the keyword as an identifier (with an attached diagnostic) thus considering it member lookup expression.
This allows Interpolation String to "correctly" parse this form as an expression, thus the rest is parsed as part of the string interpolation and not treat the expression as text.

…ee if it is a qualifier token. If it is then treat the keyword as an identifier (with an attached diagnostic) thus considering it member lookup expression. This allows Interpolation String to "correctly" parse this form as an expression, thus the rest is parsed as part of the string interpolation and not treat the expression as text.

jaredpar · 2018-08-13T15:37:21Z

src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb

@@ -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


Please revert

@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

jaredpar · 2018-08-13T15:37:37Z

src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb

@@ -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


Please use camelCased names.

jaredpar · 2018-08-13T15:38:17Z

src/Compilers/VisualBasic/Portable/Parser/ParseInterpolatedString.vb

@@ -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


Please revert.

See Issue #27893

jaredpar · 2018-08-13T15:38:51Z

This PR does not follow our coding style in pretty much every change. Please correct this.

CyrusNajmabadi · 2018-08-13T16:48:35Z

src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb

                    Else
+MissingExpression:


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

CyrusNajmabadi · 2018-08-13T16:49:22Z

src/Compilers/VisualBasic/Test/Syntax/Parser/InterpolatedStringParsingTests.vb

+        result.AssertTheseDiagnostics(
+        <expected>
+BC30183: Keyword is not valid as an identifier.
+    Return $"{End.ToString()})"


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

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

+ Revert inadvertent changes + Use camelCase + Remove GoTo by using nested `If Then` rather that `Else If`

CyrusNajmabadi · 2018-08-13T19:29:39Z

src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb

+                                End If
+                            End If
+
+                        End If


extra newline unnecessary. though you can potentially put a comment there saying it is intentional that you are falling through.

CyrusNajmabadi · 2018-08-13T19:29:54Z

src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb

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


Foo is not our naming pattern for locals.

CyrusNajmabadi · 2018-08-13T19:30:43Z

src/Compilers/VisualBasic/Portable/Parser/ParseExpression.vb

+                                Dim keywordAsIdentifier = ParseSimpleNameExpressionAllowingKeywordAndTypeArguments()
+                                keywordAsIdentifier = ReportSyntaxError(keywordAsIdentifier, ERRID.ERR_InvalidUseOfKeyword)
+                                Dim _InvocationExpression_ = ParseExpressionCore(prevTerm:=keywordAsIdentifier)
+                                If _InvocationExpression_.Kind = SyntaxKind.InvocationExpression Then


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

CyrusNajmabadi · 2018-08-13T19:30:53Z

src/Compilers/VisualBasic/Portable/Parser/ParseInterpolatedString.vb

@@ -332,4 +332,4 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax

    End Class

-End Namespace
+End Namespace


please undo this.

+ Revert inadvertent changes + Additional unit tests + Use `prevTerm` when constructing `ConditionalAccessExpression`

CyrusNajmabadi · 2018-08-13T20:33:30Z

@AdamSpeight2008 Please do not 'format document' in the compiler codebase. If/when the compiler wants to format things, they would do it as an entirely independent PR.

AdamSpeight2008 · 2018-08-13T20:43:34Z

@CyrusNajmabadi Formatting changes are partially due to Issue #27893. (WIP Fix #27943)
Are there any additional unit tests that I could add?

CyrusNajmabadi · 2018-08-13T20:52:40Z

@AdamSpeight2008 Your change is unrelated to interpolated string parsing. I would keep around the tests you have. But you need more general tests showing a variety of keywords followed by delimiters in a variety of different circumstances.

I would check with other compiler devs to see waht else they would want. If this was C# i know it would be good to have tests that validate the tree shape. a-la:

roslyn/src/Compilers/CSharp/Test/Syntax/Parsing/AsyncParsingTests.cs

Lines 42 to 49 in 4cd7d5e

    
           N(SyntaxKind.CompilationUnit); 
        
           { 
        
               N(SyntaxKind.ClassDeclaration); 
        
               { 
        
                   N(SyntaxKind.ClassKeyword); 
        
                   N(SyntaxKind.IdentifierToken); 
        
                   N(SyntaxKind.OpenBraceToken); 
        
                   N(SyntaxKind.MethodDeclaration);

But i don't know if VB has anything equivalent.

We'd likely also want some semantic and IOperation tests to make sure this doesn't cause anything downstream to crash.

jaredpar · 2018-10-01T17:14:56Z

Closing as this PR has gotten stale at this point.

AdamSpeight2008 requested a review from a team as a code owner August 12, 2018 13:15

AdamSpeight2008 force-pushed the master_keywordexpr branch from 225815c to 577c6f0 Compare August 12, 2018 15:18

jaredpar reviewed Aug 13, 2018

View reviewed changes

jaredpar self-assigned this Aug 13, 2018

jaredpar added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 13, 2018

CyrusNajmabadi reviewed Aug 13, 2018

View reviewed changes

Initial feedback work.

0071d44

+ Revert inadvertent changes + Use camelCase + Remove GoTo by using nested `If Then` rather that `Else If`

CyrusNajmabadi reviewed Aug 13, 2018

View reviewed changes

Further Feedback changes.

0710328

+ Revert inadvertent changes + Additional unit tests + Use `prevTerm` when constructing `ConditionalAccessExpression`

Revert Changes

5e675c5

Additional Unit Tests

6b8dc2e

jaredpar closed this Oct 1, 2018

AdamSpeight2008 mentioned this pull request Apr 26, 2019

VB.NET: using framework type name as field name in value tuple crashes 'Extract Method' and 'Introduce Variable' refatorings #21662

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended Keyword Parsing to cover member lookup expressions. #29233

Extended Keyword Parsing to cover member lookup expressions. #29233

AdamSpeight2008 commented Aug 12, 2018 •

edited

Loading

jaredpar Aug 13, 2018

AdamSpeight2008 Aug 13, 2018 •

edited

Loading

jaredpar Aug 13, 2018

jaredpar Aug 13, 2018

AdamSpeight2008 Aug 13, 2018

jaredpar commented Aug 13, 2018

CyrusNajmabadi Aug 13, 2018

CyrusNajmabadi Aug 13, 2018

CyrusNajmabadi Aug 13, 2018

CyrusNajmabadi Aug 13, 2018

CyrusNajmabadi Aug 13, 2018

CyrusNajmabadi Aug 13, 2018

CyrusNajmabadi Aug 13, 2018

CyrusNajmabadi commented Aug 13, 2018

AdamSpeight2008 commented Aug 13, 2018 •

edited

Loading

CyrusNajmabadi commented Aug 13, 2018

jaredpar commented Oct 1, 2018

+                                              End If
+                                          End If
+                                      End If

		Else
		MissingExpression:

Extended Keyword Parsing to cover member lookup expressions. #29233

Extended Keyword Parsing to cover member lookup expressions. #29233

Conversation

AdamSpeight2008 commented Aug 12, 2018 • edited Loading

Choose a reason for hiding this comment

AdamSpeight2008 Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 13, 2018

AdamSpeight2008 commented Aug 13, 2018 • edited Loading

CyrusNajmabadi commented Aug 13, 2018

jaredpar commented Oct 1, 2018

AdamSpeight2008 commented Aug 12, 2018 •

edited

Loading

AdamSpeight2008 Aug 13, 2018 •

edited

Loading

AdamSpeight2008 commented Aug 13, 2018 •

edited

Loading