Skip to content

[Macros] Implement unqualified lookup for global macro-expanded peer declarations. #64065

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 2 commits into from
Mar 4, 2023

Conversation

hborla
Copy link
Member

@hborla hborla commented Mar 3, 2023

This change adds MacroDecl::getIntroducedNames, which populates the given vector with a list of names introduced by the macro decl. This list of macro-covered names is used in unqualified lookup to lazily expand declarations with attached macros covering a requested name. This can also be used later for name validation of attached macros.

Resolves: rdar://104345950

@hborla hborla requested review from slavapestov and xedin as code owners March 3, 2023 08:46
@hborla hborla force-pushed the global-peer-macro-lookup branch from 4909ee5 to 6e6ec02 Compare March 3, 2023 08:50
@hborla
Copy link
Member Author

hborla commented Mar 3, 2023

@swift-ci please smoke test

1 similar comment
@hborla
Copy link
Member Author

hborla commented Mar 3, 2023

@swift-ci please smoke test

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is looking great!

lib/AST/Decl.cpp Outdated

case MacroIntroducedDeclNameKind::Arbitrary:
// Only freestanding and member macros can have arbitrary names.
assert(role == MacroRole::Member || role == MacroRole::Declaration);
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not have this assert; ill-formed code could trigger this unless we're blocking this function from getting called on ill-formed code, and that feels dicey.

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 replaced the assertion with a FIXME for now, and I'm working on adding support for arbitrary through this mechanism locally.

return nullptr;
}

void MacroDecl::getIntroducedNames(MacroRole role, ValueDecl *attachedTo,
Copy link
Member

Choose a reason for hiding this comment

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

Should this somehow report to its caller when arbitrary is there?

Comment on lines +338 to +344
// We do not need a fully resolved macro until expansion. Instead, we
// conservatively consider peer names for all macro declarations with a
// custom attribute name. Unqualified lookup for that name will later
// invoke expansion of the macro, and will yield no results if the resolved
// macro does not produce the requested name, so the only impact is possibly
// expanding earlier than needed / unnecessarily looking in the top-level
// auxiliary decl cache.
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach a lot!

@hborla hborla force-pushed the global-peer-macro-lookup branch from d554d13 to 57cca85 Compare March 3, 2023 22:02
@hborla
Copy link
Member Author

hborla commented Mar 3, 2023

@swift-ci please smoke test

@xedin xedin removed their request for review March 3, 2023 23:16
@hborla hborla merged commit b02e458 into swiftlang:main Mar 4, 2023
@hborla hborla deleted the global-peer-macro-lookup branch March 4, 2023 00:52
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