Skip to content

[Parse] Improve freestanding macro expansion parsing #65967

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

Merged
merged 1 commit into from
May 19, 2023

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented May 17, 2023

  • Unify macro expansion parsing logic between MacroExpansionExpr and MacroExpansionDecl (introduce parseFreestandingMacroExpansion )
  • Diagnose whitespace between '#' and the macro name
  • Diagnose keyword as a macro name

@rintaro
Copy link
Member Author

rintaro commented May 17, 2023

@swift-ci Please smoke test

@rintaro rintaro changed the title [Parse] Improve macro expansion parsing [Parse] Improve freestanding macro expansion parsing May 17, 2023
@@ -1731,6 +1731,9 @@ class Parser {
/// cases this doesn't actually make sense but we need to accept them for
/// backwards compatibility.
AllowLowercaseAndUppercaseSelf = 1 << 6,

/// Parse keyword as identifier but emit fix-it to escape it.
AllowKeywordWithBacktickEscape = AllowKeywords | 1 << 7
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated. removing

@@ -1731,6 +1731,9 @@ class Parser {
/// cases this doesn't actually make sense but we need to accept them for
/// backwards compatibility.
AllowLowercaseAndUppercaseSelf = 1 << 6,

/// Parse keyword as identifier but emit fix-it to escape it.
AllowKeywordWithBacktickEscape = AllowKeywords | 1 << 7
Copy link
Member

Choose a reason for hiding this comment

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

Are you using this anywhere?

Comment on lines 2323 to 2324
auto closuresStatus = parseTrailingClosures(
/*isExprBasic*/ false, macroNameLoc.getSourceRange(), trailingClosures);
Copy link
Member

Choose a reason for hiding this comment

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

Reading this, I now realize that we aren’t emitting a warning for trailing closure inside an if condition. If it’s easy to fix that in this PR, I think that would be good, otherwise I can file an issue for it.

@freestanding(expression)
macro myMacro(x: () -> Void) -> Bool


func myFunc(x: () -> Void) -> Bool { true }

func test() {
  if myFunc {} {} // warning: trailing closure in this context is confusable with the body of the statement; pass as a parenthesized argument to silence this warning
  if #myMacro {} {} // no warning
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I just forgot to pass isExprBasic here? But that's wasn't the case before this PR. I will check.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to update checkStmtConditionTrailingClosure() in Sema, but it turned out that the current type checked MacroExpansionExpr doesn't have coerced ArgumentList. That is required for getting closure argument label for fix-it. Let's just file an issue for now.

Copy link
Member

@ahoppen ahoppen May 17, 2023

Choose a reason for hiding this comment

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

OK, thanks. It’s “only” a warning as well, so it doesn’t break source.

* Unify macro expansion parsing logic between MacroExpansionExpr and
  MacroExpansionDecl
* Diagnose whitespace between '#' and the macro name
* Diagnose keyword as a macro name
@rintaro rintaro force-pushed the macroexpanson-name-sameline branch from 3a4811e to e3a0f24 Compare May 17, 2023 18:46
@rintaro
Copy link
Member Author

rintaro commented May 17, 2023

@swift-ci Please smoke test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants