[Parser] Attributes on MacroExpansionDeclSyntax #1650

rintaro · 2023-05-10T02:10:58Z

Add attributes and modifiers to MacroExpansionDeclSyntax
Diagnose whitespaces between # and the macro name
Attach attributes to MacroExpansionDeclSyntax

rdar://107386648

rintaro · 2023-05-10T02:12:50Z

Sources/SwiftParser/Declarations.swift

+    let (unexpectedBeforePound, poundKeyword) = self.eat(handle)
+    // Don't allow space between '#' and the macro name.
+    if poundKeyword.trailingTriviaByteLength != 0 || self.currentToken.leadingTriviaByteLength != 0 {
+      return RawMacroExpansionDeclSyntax(


The intent here is that parse # followed by whitespaces as #<missing identifier>, and let the caller do the recovery.

I think we should still continue macro attribute parsing if the identifier is at the same line as the #. I would expect that to yield better recovery.

What would you expect as the parsed tree. How do we embed "hasError"?

For example if you have # elif, we should have unexpectedBeforeMacroName = ' elif' and macroName should be a missing identifier with text elif. That’s also what we do when parsing a. b (where whitespace around the . is not balanced).

Thanks. Will do.

rintaro · 2023-05-10T02:13:58Z

Sources/SwiftParser/Expressions.swift

    let poundKeyword = self.consumeAnyToken()
-    let (unexpectedBeforeMacro, macro) = self.expectIdentifier()
+    let (unexpectedBeforeMacro, macro) = self.expectIdentifier(keywordRecovery: true)


e.g. #class is invalid but should be recoverable.

Could you add a test case for #<keyword>?

rintaro · 2023-05-10T13:43:28Z

@swift-ci Please test

rintaro · 2023-05-11T19:05:47Z

@swift-ci Please test

bnbarham · 2023-05-11T22:32:22Z

Tests/SwiftParserTest/translated/IfconfigExprTests.swift

+      ],
+      fixedSource: """
+        #if MY_FLAG
+        #<#identifier#>elif


Lost the space here 🤔?

Yeah.. no idea what happened.. I guess the diagnostic is after the trailing trivia, but the diagnostic system removed the trailing trivia of # because that's preferable. @ahoppen any idea?

To start, this gets parsed as follows

SourceFileSyntax IfConfigDeclSyntax ├─clauses: IfConfigClauseListSyntax │ ╰─[0]: IfConfigClauseSyntax │ ├─poundKeyword: poundIfKeyword │ ├─condition: IdentifierExprSyntax │ │ ╰─identifier: identifier("MY_FLAG") │ ╰─elements: CodeBlockItemListSyntax │ ├─[0]: CodeBlockItemSyntax │ │ ├─item: MacroExpansionExprSyntax │ │ │ ├─poundToken: pound │ │ │ ├─macro: identifier("") MISSING │ │ │ ╰─argumentList: TupleExprElementListSyntax │ │ ╰─semicolon: semicolon MISSING │ ╰─[1]: CodeBlockItemSyntax │ ╰─item: IdentifierExprSyntax │ ╰─identifier: identifier("elif") ╰─poundEndif: poundEndifKeyword

When inserting the missing identifier, we see that # and identifiers don’t need to be separated by a space, so we remove the space. This is really useful for example if you have someCall(1 2) where a comma is missing. In this case you want to insert it after immediate after the 1 and not the space.

So now to the next question: Why don’t we add a space after <#identifier#>. For this, we look at the fixed up view of the tree and discover that the missing identifier is followed by a semicolon in that view. And identifier and semicolons don’t need to be separated by spaces. So no space that needs to be inserted here. We just suppress the missing semicolon diagnostic because the code item already has an error. Because most of the time the missing semicolon diagnostic is actually really misleading.

I think fix here would be to improve how we recover here in general so that elif doesn’t end up in a separate CodeBlockItem anymore.

bnbarham · 2023-05-11T22:33:35Z

Tests/SwiftParserTest/DeclarationTests.swift

@@ -1427,6 +1427,125 @@ final class DeclarationTests: XCTestCase {
    )
  }

+  func testAttributedMacroExpansionDeclaration() {


Could add a test when split by a newline rather than space? And also one for with both attributes and modifiers?

bnbarham · 2023-05-11T22:34:23Z

Sources/SwiftParser/Expressions.swift

    let poundKeyword = self.consumeAnyToken()
-    let (unexpectedBeforeMacro, macro) = self.expectIdentifier()
+    let (unexpectedBeforeMacro, macro) = self.expectIdentifier(keywordRecovery: true)


Could you add a test case for #<keyword>?

bnbarham · 2023-05-11T22:35:58Z

@TiagoMaiaL note that this fixes part of #1395 (ie. whitespace between # and the identifier, but not @).

rintaro · 2023-05-12T04:37:25Z

@swift-ci Please test

rintaro · 2023-05-12T04:54:09Z

@swift-ci Please test Windows

* Add `attributes` and `modifiers` to `MacroExpansionDeclSyntax` * Diagnose whitespaces between # and the macro name * Attach attributes to MacroExpansionDeclSyntax rdar://107386648

rintaro · 2023-05-12T06:36:35Z

@swift-ci Please test

rintaro · 2023-05-12T12:50:11Z

@swift-ci Please test

Sources/SwiftParser/Declarations.swift

ahoppen · 2023-05-12T21:29:24Z

Sources/SwiftParser/Declarations.swift

+    let (unexpectedBeforePound, poundKeyword) = self.eat(handle)
+    // Don't allow space between '#' and the macro name.
+    if poundKeyword.trailingTriviaByteLength != 0 || self.currentToken.leadingTriviaByteLength != 0 {
+      return RawMacroExpansionDeclSyntax(


I think we should still continue macro attribute parsing if the identifier is at the same line as the #. I would expect that to yield better recovery.

Sources/SwiftParser/Expressions.swift

Sources/SwiftParser/TokenSpecSet.swift

Sources/SwiftSyntax/generated/syntaxNodes/SyntaxDeclNodes.swift

Tests/SwiftParserTest/DeclarationTests.swift

ahoppen · 2023-05-12T21:52:34Z

Tests/SwiftParserTest/translated/IfconfigExprTests.swift

+      ],
+      fixedSource: """
+        #if MY_FLAG
+        #<#identifier#>elif


To start, this gets parsed as follows

SourceFileSyntax IfConfigDeclSyntax ├─clauses: IfConfigClauseListSyntax │ ╰─[0]: IfConfigClauseSyntax │ ├─poundKeyword: poundIfKeyword │ ├─condition: IdentifierExprSyntax │ │ ╰─identifier: identifier("MY_FLAG") │ ╰─elements: CodeBlockItemListSyntax │ ├─[0]: CodeBlockItemSyntax │ │ ├─item: MacroExpansionExprSyntax │ │ │ ├─poundToken: pound │ │ │ ├─macro: identifier("") MISSING │ │ │ ╰─argumentList: TupleExprElementListSyntax │ │ ╰─semicolon: semicolon MISSING │ ╰─[1]: CodeBlockItemSyntax │ ╰─item: IdentifierExprSyntax │ ╰─identifier: identifier("elif") ╰─poundEndif: poundEndifKeyword

When inserting the missing identifier, we see that # and identifiers don’t need to be separated by a space, so we remove the space. This is really useful for example if you have someCall(1 2) where a comma is missing. In this case you want to insert it after immediate after the 1 and not the space.

So now to the next question: Why don’t we add a space after <#identifier#>. For this, we look at the fixed up view of the tree and discover that the missing identifier is followed by a semicolon in that view. And identifier and semicolons don’t need to be separated by spaces. So no space that needs to be inserted here. We just suppress the missing semicolon diagnostic because the code item already has an error. Because most of the time the missing semicolon diagnostic is actually really misleading.

I think fix here would be to improve how we recover here in general so that elif doesn’t end up in a separate CodeBlockItem anymore.

ahoppen

@rintaro and I chatted offline that he will address the comments in a follow-up PR

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

ahoppen · 2023-05-15T21:32:33Z

Addressing the review comments in #1665.

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

Address review comments to #1650

rintaro commented May 10, 2023

View reviewed changes

rintaro mentioned this pull request May 10, 2023

[Macros/Parse] Attributes on MacroExpansionDecl swiftlang/swift#65815

Merged

rintaro force-pushed the macros-attributes-rdar107386648 branch from 0b2562a to 72bc06a Compare May 11, 2023 19:02

rintaro marked this pull request as ready for review May 11, 2023 19:08

rintaro requested a review from ahoppen as a code owner May 11, 2023 19:08

rintaro requested a review from bnbarham May 11, 2023 21:30

bnbarham reviewed May 11, 2023

View reviewed changes

rintaro force-pushed the macros-attributes-rdar107386648 branch from 72bc06a to 424d334 Compare May 12, 2023 04:37

[Parser] Attributes on MacroExpansionDeclSyntax

fd89876

* Add `attributes` and `modifiers` to `MacroExpansionDeclSyntax` * Diagnose whitespaces between # and the macro name * Attach attributes to MacroExpansionDeclSyntax rdar://107386648

rintaro force-pushed the macros-attributes-rdar107386648 branch from 424d334 to fd89876 Compare May 12, 2023 06:36

ahoppen reviewed May 12, 2023

View reviewed changes

ahoppen approved these changes May 15, 2023

View reviewed changes

rintaro merged commit 4cd9e58 into swiftlang:main May 15, 2023

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request May 15, 2023

Address review comments to swiftlang#1650

369b18f

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

ahoppen mentioned this pull request May 15, 2023

Address review comments to #1650 #1665

Merged

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request May 15, 2023

Address review comments to swiftlang#1650

fea8c9a

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

rintaro mentioned this pull request May 18, 2023

[5.9][Macro] Attributes for freestanding macro expansion #1675

Merged

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request May 25, 2023

Address review comments to swiftlang#1650

d9f839c

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Jun 1, 2023

Address review comments to swiftlang#1650

fe47a6f

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Jun 2, 2023

Address review comments to swiftlang#1650

07e9c3b

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Jun 2, 2023

Address review comments to swiftlang#1650

236c57f

This addresses the review comments I made on swiftlang#1650. Co-Authored-By: Rintaro Ishizaki <[email protected]>

ahoppen added a commit that referenced this pull request Jun 3, 2023

Merge pull request #1665 from ahoppen/ahoppen/rintaro-review-comments

13f688e

Address review comments to #1650

[Parser] Attributes on MacroExpansionDeclSyntax #1650

[Parser] Attributes on MacroExpansionDeclSyntax #1650

Uh oh!

Conversation

rintaro commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro commented May 10, 2023

Uh oh!

rintaro commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham commented May 11, 2023

Uh oh!

rintaro commented May 12, 2023

Uh oh!

rintaro commented May 12, 2023

Uh oh!

rintaro commented May 12, 2023

Uh oh!

rintaro commented May 12, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented May 15, 2023

Uh oh!

Uh oh!

rintaro commented May 10, 2023 •

edited

Loading

rintaro May 11, 2023 •

edited

Loading