Skip to content

[Macros/Parse] Attributes on MacroExpansionDecl #65815

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.

Sign up for GitHub

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 15, 2023
Merged

[Macros/Parse] Attributes on MacroExpansionDecl #65815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion include/swift/Parse/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,11 +900,15 @@ class Parser {
// Decl Parsing

/// Returns true if parser is at the start of a Swift decl or decl-import.
bool isStartOfSwiftDecl(bool allowPoundIfAttributes = true);
bool isStartOfSwiftDecl(bool allowPoundIfAttributes = true,
bool hadAttrsOrModifiers = false);

/// Returns true if the parser is at the start of a SIL decl.
bool isStartOfSILDecl();

/// Returns true if the parser is at a freestanding macro expansion.
bool isStartOfFreestandingMacroExpansion();

/// Parse the top-level Swift items into the provided vector.
///
/// Each item will be a declaration, statement, or expression.
Expand Down
2 changes: 1 addition & 1 deletion lib/AST/Attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ OrigDeclAttrFilter::operator()(const DeclAttribute *Attr) const {
auto declLoc = decl->getStartLoc();
auto *mod = decl->getModuleContext();
auto *declFile = mod->getSourceFileContainingLocation(declLoc);
auto *attrFile = mod->getSourceFileContainingLocation(Attr->AtLoc);
auto *attrFile = mod->getSourceFileContainingLocation(Attr->getLocation());
if (!declFile || !attrFile)
return Attr;

Expand Down
94 changes: 70 additions & 24 deletions lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4786,7 +4786,8 @@ static void skipAttribute(Parser &P) {
}
}

bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes) {
bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes,
bool hadAttrsOrModifiers) {
if (Tok.is(tok::at_sign) && peekToken().is(tok::kw_rethrows)) {
// @rethrows does not follow the general rule of @<identifier> so
// it is needed to short circuit this else there will be an infinite
Expand Down Expand Up @@ -4835,22 +4836,40 @@ bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes) {
(Tok.is(tok::pound_endif) && !allowPoundIfAttributes))
return true;

return isStartOfSwiftDecl(allowPoundIfAttributes);
return isStartOfSwiftDecl(allowPoundIfAttributes,
/*hadAttrsOrModifiers=*/true);
}

if (Tok.is(tok::pound) && peekToken().is(tok::identifier)) {
// Macro expansions at the top level are declarations.
return !isInSILMode() && SF.Kind != SourceFileKind::Interface &&
CurDeclContext->isModuleScopeContext() && !allowTopLevelCode();
if (Tok.is(tok::pound)) {
if (isStartOfFreestandingMacroExpansion()) {
if (isInSILMode() || SF.Kind == SourceFileKind::Interface)
return false;

// Parse '#<identifier>' after attrs/modifiers as a macro expansion decl.
if (hadAttrsOrModifiers)
return true;

// Macro expansions at the top level of non-script file are declarations.
return CurDeclContext->isModuleScopeContext() && !allowTopLevelCode();
}

// Otherwise, prefer parsing it as an expression.
return false;
}

// Skip a #if that contains only attributes in all branches. These will be
// parsed as attributes of a declaration, not as separate declarations.
if (Tok.is(tok::pound_if) && allowPoundIfAttributes) {
BacktrackingScope backtrack(*this);
bool sawAnyAttributes = false;
return skipIfConfigOfAttributes(sawAnyAttributes) &&
(Tok.is(tok::eof) || (sawAnyAttributes && isStartOfSwiftDecl()));
if (!skipIfConfigOfAttributes(sawAnyAttributes))
return false;
if (Tok.is(tok::eof))
return true;
if (!sawAnyAttributes)
return false;
return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/true,
/*hadAttrsOrModifiers=*/true);
}

// If we have a decl modifying keyword, check if the next token is a valid
Expand All @@ -4872,13 +4891,15 @@ bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes) {
// If we found the start of a decl while trying to skip over the
// paren, then we have something incomplete like 'private('. Return
// true for better recovery.
if (isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false))
if (isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false,
/*hadAttrsOrModifiers=*/true))
return true;

skipSingle();
}
}
return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false);
return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false,
/*hadAttrsOrModifiers=*/true);
}
}

Expand All @@ -4905,7 +4926,8 @@ bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes) {
consumeToken(tok::l_paren);
consumeToken(tok::identifier);
consumeToken(tok::r_paren);
return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false);
return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false,
/*hadAttrsOrModifiers=*/true);
}

if (Tok.isContextualKeyword("actor")) {
Expand All @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

Can actor be a modifier here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

return Tok.is(tok::identifier);
}

Expand Down Expand Up @@ -4960,12 +4983,14 @@ bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes) {
// If we found the start of a decl while trying to skip over the
// paren, then we have something incomplete like 'package('. Return
// true for better recovery.
if (isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false))
if (isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false,
/*hadAttrsOrModifiers=*/true))
return true;
skipSingle();
}
}
return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false);
return isStartOfSwiftDecl(/*allowPoundIfAttributes=*/false,
/*hadAttrsOrModifiers=*/true);
}
}

Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 👍🏽

}

bool Parser::isStartOfSILDecl() {
Expand Down Expand Up @@ -5004,6 +5030,19 @@ bool Parser::isStartOfSILDecl() {
llvm_unreachable("Unhandled case in switch");
}

bool Parser::isStartOfFreestandingMacroExpansion() {
// Check if "'#' <identifier>" without any whitespace between them.
if (!Tok.is(tok::pound))
return false;
if (Tok.getRange().getEnd() != peekToken().getLoc())
return false;
if (!peekToken().isAny(tok::identifier, tok::code_complete) &&
// allow keywords right after '#' so we can diagnose it when parsing.
!peekToken().isKeyword())
return false;
return true;
}

void Parser::consumeDecl(ParserPosition BeginParserPosition,
ParseDeclOptions Flags,
bool IsTopLevel) {
Expand Down Expand Up @@ -5256,9 +5295,15 @@ Parser::parseDecl(ParseDeclOptions Flags,
// Handled below.
break;
case tok::pound:
if (Tok.isAtStartOfLine() &&
peekToken().is(tok::code_complete) &&
Tok.getLoc().getAdvancedLoc(1) == peekToken().getLoc()) {
if (!isStartOfFreestandingMacroExpansion()) {
consumeToken(tok::pound);
diagnose(Tok.getLoc(),
diag::macro_expansion_decl_expected_macro_identifier);
DeclResult = makeParserError();
break;
}

if (peekToken().is(tok::code_complete)) {
consumeToken();
if (CodeCompletionCallbacks) {
CodeCompletionCallbacks->completeAfterPoundDirective();
Expand All @@ -5270,6 +5315,7 @@ Parser::parseDecl(ParseDeclOptions Flags,

// Parse as a macro expansion.
DeclResult = parseDeclMacroExpansion(Flags, Attributes);
StaticLoc = SourceLoc(); // Ignore 'static' on macro expansion
break;

case tok::pound_if:
Expand Down Expand Up @@ -9803,10 +9849,10 @@ Parser::parseDeclMacroExpansion(ParseDeclOptions flags,
}
}

return makeParserResult(
status,
new (Context) MacroExpansionDecl(
CurDeclContext, poundLoc, macroNameRef, macroNameLoc,
leftAngleLoc, Context.AllocateCopy(genericArgs), rightAngleLoc,
argList));
auto *med = new (Context) MacroExpansionDecl(
CurDeclContext, poundLoc, macroNameRef, macroNameLoc, leftAngleLoc,
Context.AllocateCopy(genericArgs), rightAngleLoc, argList);
med->getAttrs() = attributes;

return makeParserResult(status, med);
}
6 changes: 6 additions & 0 deletions lib/Parse/ParseExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1841,6 +1841,12 @@ ParserResult<Expr> Parser::parseExprPrimary(Diag<> ID, bool isExprBasic) {
}

case tok::pound:
if (!isStartOfFreestandingMacroExpansion()) {
consumeToken(tok::pound);
diagnose(Tok.getLoc(),
diag::macro_expansion_expr_expected_macro_identifier);
return makeParserError();
}
Comment on lines +1844 to +1849
Copy link
Member

Choose a reason for hiding this comment

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

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

if (peekToken().is(tok::code_complete) &&
Tok.getLoc().getAdvancedLoc(1) == peekToken().getLoc()) {
return parseExprPoundCodeCompletion(/*ParentKind*/None);
Expand Down
4 changes: 4 additions & 0 deletions lib/Sema/TypeCheckDeclPrimary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2058,6 +2058,10 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
}

void visitMacroExpansionDecl(MacroExpansionDecl *MED) {
// TODO: Type check attributes.
// Type checking arguments should reflect the attributes.
// e.g. '@available(macOS 999) #Future { newAPIFrom999() }'.

// Assign a discriminator.
(void)MED->getDiscriminator();
// Decls in expansion already visited as auxiliary decls.
Expand Down
4 changes: 2 additions & 2 deletions test/Parse/line-directive.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ public struct S {
// expected-error@+5{{operators must have one or two arguments}}
// expected-error@+4{{member operator '/()' must have at least one argument of type 'S'}}
// expected-error@+3{{expected '{' in body of function declaration}}
// 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"
Copy link
Member Author

Choose a reason for hiding this comment

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

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 #.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thank you!

}
// expected-error@+1{{#line directive was renamed to #sourceLocation}}
Expand Down
20 changes: 20 additions & 0 deletions test/Parse/macro_decl.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ extension String {

#memberwiseInit(flavor: .chocolate, haha: true) { "abc" }

@available(macOS 999, *)
public #memberwiseInit()

static internal #memberwiseInit

struct Foo {
#memberwiseInit

Expand All @@ -26,3 +31,18 @@ extension String {
// expected-error @+1 {{expected a macro identifier for a pound literal declaration}}
#()
}

@RandomAttr #someFunc

public #someFunc

#someFunc

func test() {
@discardableResult #someFunc

dynamic #someFunc

@CustomAttr
isolated #someFunc
}
3 changes: 1 addition & 2 deletions test/Parse/object_literals.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,4 @@ let _ = #notAPound // expected-error {{no macro named 'notAPound'}}
let _ = #notAPound(1, 2) // expected-error {{no macro named 'notAPound'}}
let _ = #Color // expected-error {{no macro named 'Color'}}

let _ = [##] // expected-error 2 {{expected a macro identifier}} {{none}}
// expected-error @-1 {{consecutive statements on a line must be separated by ';'}}
let _ = [##] // expected-error {{expected a macro identifier}} {{none}}
2 changes: 0 additions & 2 deletions test/Parse/operator_decl.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ infix operator aa--: A // expected-error {{'aa' is considered an identifier and
infix operator <<$$@< // expected-error {{'$$' is considered an identifier and must not appear within an operator name}}
infix operator !!@aa // expected-error {{'@' is not allowed in operator names}}
infix operator #++= // expected-error {{'#' is not allowed in operator names}}
// expected-error@-1 {{expected a macro identifier}}
infix operator ++=# // expected-error {{'#' is not allowed in operator names}}
infix operator -># // expected-error {{'#' is not allowed in operator names}}

Expand All @@ -61,7 +60,6 @@ infix operator -># // expected-error {{'#' is not allowed in operator names}}
infix operator =#=
// expected-error@-1 {{'#' is not allowed in operator names}}
// expected-error@-2 {{'=' must have consistent whitespace on both sides}}
// expected-error@-3 {{expected a macro identifier}}

infix operator +++=
infix operator *** : A
Expand Down
4 changes: 2 additions & 2 deletions test/Parse/raw_string_errors.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ let _ = #"\##("invalid")"#
let _ = ###"""invalid"######
// expected-error@-1{{too many '#' characters in closing delimiter}}{{26-29=}}
// expected-error@-2{{consecutive statements on a line must be separated by ';'}}
// expected-error@-3 3 {{expected a macro identifier}}
// expected-error@-3{{expected a macro identifier}}

let _ = ####"invalid"###
// expected-error@-1{{unterminated string literal}}

let _ = ###"invalid"######
// expected-error@-1{{too many '#' characters in closing delimiter}}{{24-27=}}
// expected-error@-2{{consecutive statements on a line must be separated by ';'}}
// expected-error@-3 3 {{expected a macro identifier}}
// expected-error@-3{{expected a macro identifier}}

let _ = ##"""aa
foobar
Expand Down
2 changes: 1 addition & 1 deletion test/StringProcessing/Frontend/disable-flag.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ _ = /x/
// expected-error@-2 {{cannot find 'x' in scope}}
// expected-error@-3 {{'/' is not a postfix unary operator}}

_ = #/x/# // expected-error 2 {{expected a macro identifier}}
_ = #/x/# // expected-error {{expected a macro identifier}}

func foo(_ x: Regex<Substring>) {} // expected-error {{cannot find type 'Regex' in scope}}