[Macros/Parse] Attributes on MacroExpansionDecl #65815

rintaro · 2023-05-10T02:03:54Z

Parse #<identifier> after an attribute list as a MacroExpansionDecl regardless of the position
Diagnose whitespaces between # and the macro name
Correctly attach attributes to MacroExpansionDecl
Fix OrigDeclAttributes to handle modifiers (use getLocation() instead of AtLoc.)

Type checking and macro expansion are TODO

rdar://107386648

rintaro · 2023-05-10T02:15:26Z

swiftlang/swift-syntax#1650
@swift-ci Please smoke test

rintaro · 2023-05-10T02:16:25Z

swiftlang/swift-syntax#1650
@swift-ci Please Test Source Compatibility

rintaro · 2023-05-10T02:32:28Z

test/Parse/line-directive.swift

-// expected-error@+2 2 {{consecutive declarations on a line must be separated by ';}}
-// expected-error@+1 2 {{expected a macro identifier}}
+// expected-error@+2 {{consecutive declarations on a line must be separated by ';}}
+// expected-error@+1 {{expected a macro identifier}}
 / ###line 25 "line-directive.swift"


This is improved because previously # not followed by an identifier was true for isStartOfSwiftDecl(). Now it's false so the expression parsing recovery skips the second #.

Makes sense, thank you!

rintaro · 2023-05-10T04:40:02Z

swiftlang/swift-syntax#1650
@swift-ci Please smoke test

DougGregor · 2023-05-10T21:34:37Z

lib/Sema/TypeCheckMacros.cpp

@@ -1049,8 +1049,10 @@ swift::expandFreestandingMacro(MacroExpansionDecl *med) {

  auto topLevelItems = macroSourceFile->getTopLevelItems();
  for (auto item : topLevelItems) {
-    if (auto *decl = item.dyn_cast<Decl *>())
+    if (auto *decl = item.dyn_cast<Decl *>()) {
+      decl->getAttrs().add(med->getAttrs());
      decl->setDeclContext(dc);


This will cause problems when the same attribute is added to multiple places. Per our offline discussion, I think it's best for us to syntactically add the attributes (e.g., by splicing them into the DeclSyntax nodes we get back from the macro) rather than do it semantically. That way, we'll see the attributes in the expansion.

I scoped down this PR to Parse support only. Removed this bit.

DougGregor · 2023-05-10T21:34:54Z

lib/Sema/TypeCheckMacros.cpp

@@ -1549,6 +1551,9 @@ ConcreteDeclRef ResolveMacroRequest::evaluate(Evaluator &evaluator,
      macroRef.getArgs(), roles);
  }

+  // FIXME: Type checking should reflect the attributes on the expansion.
+  // e.g. '@available(macOS 999) #Future { newAPIFrom999() }'.


The actual fix for this will be in TypeRefinementContextBuilder.

DougGregor · 2023-05-10T21:35:06Z

test/Parse/line-directive.swift

-// expected-error@+2 2 {{consecutive declarations on a line must be separated by ';}}
-// expected-error@+1 2 {{expected a macro identifier}}
+// expected-error@+2 {{consecutive declarations on a line must be separated by ';}}
+// expected-error@+1 {{expected a macro identifier}}
 / ###line 25 "line-directive.swift"


Makes sense, thank you!

* Parse `#<identifier>` attribute list as a `MacroExpansionDecl` regardless of the position * Diagnose whitespaces between `#` and the macro name. * Correctly attach attributes to `MacroExpansionDecl` * Fix `OrigDeclAttributes` to handle modifiers (use `getLocation()` instead of `AtLoc`.) Type checking is a TODO rdar://107386648

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

swiftlang/swift-syntax#1650
@swift-ci Please smoke test

rintaro · 2023-05-12T04:56:29Z

swiftlang/swift-syntax#1650
@swift-ci Please smoke test

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

swiftlang/swift-syntax#1650
@swift-ci Please smoke test

ahoppen · 2023-05-12T21:10:37Z

lib/Parse/ParseDecl.cpp

@@ -4917,7 +4939,8 @@ bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes) {
    // it's an actor declaration, otherwise, it isn't.
    do {
      consumeToken();
-    } while (isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false));
+    } while (isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false,
+                                /*hadAttrsOrModifiers=*/true));


Can actor be a modifier here?

actor is still declared as a (deprecated) modifier, and it's diagnosed as a error in parseDeclModifierList(). The diagnostic message is not great, though.

Ah, OK. Makes sense then. Just wanted to double-check

ahoppen · 2023-05-12T21:11:19Z

lib/Parse/ParseDecl.cpp

@@ -4976,7 +5001,8 @@ bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes) {
  // Otherwise, do a recursive parse.
  Parser::BacktrackingScope Backtrack(*this);
  consumeToken(tok::identifier);
-  return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false);
+  return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false,
+                            /*hadAttrsOrModifiers=*/true);


What’s the attribute or modifier that has been consumed here?

At this point Tok is a contextual keyword, and all decl introducers with contextual keyword (e.g. macro) are handled above. The all remaining cases are simple decl modifiers.

At this point Tok is a contextual keyword

We don’t know that it’s a contextual keyword, right? We just continue pretending that it might be, but really, it could be any identifier, right? Or am I missing something?

There's

// If this can't possibly be a contextual keyword, then this identifier is // not interesting. Bail out. if (!Tok.isContextualDeclKeyword()) return false;

above.

Ah, OK. I just didn’t read far enough back. It’s what you get from large functions that implicitly retain state from other early exits…

Thanks for the explanation. Good with me 👍🏽

ahoppen · 2023-05-12T21:12:38Z

lib/Parse/ParseExpr.cpp

+    if (!isStartOfFreestandingMacroExpansion()) {
+      consumeToken(tok::pound);
+      diagnose(Tok.getLoc(),
+               diag::macro_expansion_expr_expected_macro_identifier);
+      return makeParserError();
+    }


Weren’t we diagnosing this before? Out of curiosity: Is there a test case that triggers this?

rintaro commented May 10, 2023

View reviewed changes

rintaro force-pushed the macros-attributes-rdar107386648 branch from 9b92a14 to 9017dea Compare May 10, 2023 04:39

DougGregor reviewed May 10, 2023

View reviewed changes

rintaro force-pushed the macros-attributes-rdar107386648 branch from 9017dea to 74ddec5 Compare May 11, 2023 18:59

rintaro force-pushed the macros-attributes-rdar107386648 branch from 74ddec5 to 9fc1521 Compare May 11, 2023 19:04

rintaro changed the title ~~[Macros] Attributes on MacroExpansionDecl~~ [Macros/Parse] Attributes on MacroExpansionDecl May 11, 2023

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

rintaro requested review from ahoppen, bnbarham, CodaFi, hborla, slavapestov and xedin as code owners May 11, 2023 19:08

xedin removed their request for review May 12, 2023 16:23

ahoppen reviewed May 12, 2023

View reviewed changes

ahoppen approved these changes May 15, 2023

View reviewed changes

rintaro merged commit ee5b3b1 into swiftlang:main May 15, 2023

rintaro mentioned this pull request May 18, 2023

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

Merged

[Macros/Parse] Attributes on MacroExpansionDecl #65815

[Macros/Parse] Attributes on MacroExpansionDecl #65815

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!

rintaro commented May 10, 2023

Uh oh!

rintaro commented May 10, 2023

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!

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 11, 2023

Uh oh!

rintaro commented May 12, 2023

Uh oh!

rintaro commented May 12, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro commented May 10, 2023 •

edited

Loading