Skip to content

[Macros] Only freestanding expression macros can have a non-Void result type #66504

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
Jun 9, 2023
Merged

[Macros] Only freestanding expression macros can have a non-Void result type #66504

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
8 changes: 8 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7101,6 +7101,14 @@ ERROR(macro_in_nested,none,
ERROR(macro_without_role,none,
"macro %0 must declare its applicable roles via '@freestanding' or @attached'",
(DeclName))
ERROR(macro_result_type_cannot_be_used,none,
"only a freestanding expression macro can produce a result of type %0",
(Type))
NOTE(macro_remove_result_type,none,
"remove the result type if the macro does not produce a value",
())
NOTE(macro_make_freestanding_expression,none,
"make this macro a freestanding expression macro", ())
ERROR(macro_expansion_missing_pound,none,
"expansion of macro %0 requires leading '#'", (DeclName))
ERROR(macro_expansion_missing_arguments,none,
Expand Down
6 changes: 6 additions & 0 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3357,12 +3357,18 @@ TypeRepr *ValueDecl::getResultTypeRepr() const {
returnRepr = FD->getResultTypeRepr();
} else if (auto *SD = dyn_cast<SubscriptDecl>(this)) {
returnRepr = SD->getElementTypeRepr();
} else if (auto *MD = dyn_cast<MacroDecl>(this)) {
returnRepr = MD->resultType.getTypeRepr();
}

return returnRepr;
}

TypeRepr *ValueDecl::getOpaqueResultTypeRepr() const {
// FIXME: Macros don't allow opaque result types yet.
if (isa<MacroDecl>(this))
return nullptr;

auto *returnRepr = this->getResultTypeRepr();

auto *dc = getDeclContext();
Expand Down
17 changes: 17 additions & 0 deletions lib/Sema/TypeCheckDeclPrimary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2060,6 +2060,23 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
break;
}
}

// If the macro has a (non-Void) result type, it must have the freestanding
// expression role. Other roles cannot have result types.
if (auto resultTypeRepr = MD->getResultTypeRepr()) {
if (!MD->getMacroRoles().contains(MacroRole::Expression) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

If a macro had multiple roles (including freestanding(expression)), should we reject non-void types in that case too?

@attached(accessor) @freestanding(expression)
macro Foo() -> Int

Because in this case the return type -> Int still doesn't make sense for the accessor role.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think we should. The result type only applies to the expression role, and it's fine for it to be ignored for any attached roles.

Note that we don't currently diagnose when a macro has multiple freestanding roles. I'm handling that (and related cleanup) via a separate pull request

!MD->getResultInterfaceType()->isEqual(Ctx.getVoidType())) {
auto resultType = MD->getResultInterfaceType();
Ctx.Diags.diagnose(
MD->arrowLoc, diag::macro_result_type_cannot_be_used, resultType)
.highlight(resultTypeRepr->getSourceRange());
Ctx.Diags.diagnose(MD->arrowLoc, diag::macro_make_freestanding_expression)
.fixItInsert(MD->getAttributeInsertionLoc(false),
"@freestanding(expression)\n");
Ctx.Diags.diagnose(MD->arrowLoc, diag::macro_remove_result_type)
.fixItRemove(SourceRange(MD->arrowLoc, resultTypeRepr->getEndLoc()));
}
}
}

void visitMacroExpansionDecl(MacroExpansionDecl *MED) {
Expand Down
13 changes: 12 additions & 1 deletion test/Macros/macros_diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ internal struct X { } // expected-note{{type declared here}}
// expected-warning@-1{{external macro implementation type}}

struct ZZZ {
macro m5() -> Int = #externalMacro(module: "BuiltinMacros", type: "Blah")
macro m5() = #externalMacro(module: "BuiltinMacros", type: "Blah")
// expected-error@-1{{macro 'm5()' can only be declared at file scope}}
// expected-error@-2{{macro 'm5()' must declare its applicable roles}}
// expected-warning@-3{{external macro implementation type}}
Expand Down Expand Up @@ -200,3 +200,14 @@ struct SomeType {
// expected-error@-2{{use of protocol 'Hashable' as a type must be written 'any Hashable'}}
// expected-error@-3{{external macro implementation type}}
}



@freestanding(declaration) macro nonExpressionReturnsInt<T>(_: T) -> Int = #externalMacro(module: "A", type: "B")
// expected-warning@-1{{external macro implementation type}}
// expected-error@-2{{only a freestanding expression macro can produce a result of type 'Int'}}
// expected-note@-3{{make this macro a freestanding expression macro}}{{1-1=@freestanding(expression)\n}}
// expected-note@-4{{remove the result type if the macro does not produce a value}}{{67-74=}}

@freestanding(declaration) macro nonExpressionReturnsVoid<T>(_: T) -> Void = #externalMacro(module: "A", type: "B")
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we should be allowing the declaration of any return type for non-freestanding-expression macros. Specifying Void here just doesn’t make any sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I agree with that. #66526

// expected-warning@-1{{external macro implementation type}}