Skip to content

[Macros] Type check user-defined macro plugins #61861

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

Jump to bottom
Merged
merged 1 commit into from
Nov 3, 2022
Merged

[Macros] Type check user-defined macro plugins #61861

merged 1 commit into from
Nov 3, 2022

Conversation

rxwei
Copy link
Contributor

@rxwei rxwei commented Nov 2, 2022 •
edited
Loading

Type check user-defined macros plugins with user-provided type signatures.

Also, load plugin libraries with RTLD_LOCAL instead of RTLD_GLOBAL to prevent symbol collision between plugins. llvm::sys::DynamicLibrary only supports RTLD_GLOBAL so we use the plain dlopen instead. This does not work on Windows and needs to be fixed.

Friend PR: swiftlang/swift-syntax#1042

@rxwei rxwei marked this pull request as ready for review November 2, 2022 01:14
@rxwei rxwei mentioned this pull request Nov 2, 2022
Merged
@rxwei
Copy link
Contributor Author

rxwei commented Nov 2, 2022

swiftlang/swift-syntax#1042
@swift-ci please test

@rxwei rxwei force-pushed the compiler-plugin-type-checking branch from 3fbb42d to dc35c4b Compare November 2, 2022 01:16
DougGregor
DougGregor approved these changes Nov 2, 2022
View reviewed changes
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.

LGTM, I think there's some follow-up I'd like to do, but this is ready to land.

lib/Sema/TypeCheckMacros.cpp
if (builtinMacro) {
const char *evaluatedSourcePtr;
ptrdiff_t evaluatedSourceLength;
swift_ASTGen_getMacroTypeSignature(builtinMacro, &evaluatedSourcePtr,
Copy link
Member

Choose a reason for hiding this comment

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

It's interesting how we have similar code on both sides of the C++ divide. I want to think about this more.

lib/Sema/TypeCheckMacros.cpp Outdated
nullptr);
parser.consumeTokenWithoutFeedingReceiver();
SmallVector<Decl *, 1> decls;
parser.parseTopLevel(decls);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I'd like to move toward using the new parser here, because we can.

test/Macros/macro_plugin_exec.swift
@@ -7,5 +7,8 @@
// FIXME: Swift parser is not enabled on Linux CI yet.
// REQUIRES: OS=macosx

let (result, code) = #customStringify(3 + 2 - 1)
print(result, code)
print(#customStringify(3 + 2 - 1))
Copy link
Member

Choose a reason for hiding this comment

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

Should we FileCheck the output here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@DougGregor
Copy link
Member

This does not work on Windows and needs to be fixed.

Do we know what this entails? Is it because RTLD_LOCAL isn't possible on Windows?

@DougGregor
Copy link
Member

swiftlang/swift-syntax#1042
@swift-ci please smoke test

@rxwei
Copy link
Contributor Author

rxwei commented Nov 2, 2022 •
edited
Loading

This does not work on Windows and needs to be fixed.

Do we know what this entails? Is it because RTLD_LOCAL isn't possible on Windows?

On Windows we should call LoadLibraryW. llvm::sys::DynamicLibrary::HandleSet is based on one of these two platform-specific implementations:

... but it's not in public headers. I think the right thing to do is to copy llvm::sys::DynamicLibrary::HandleSet over and add an argument to specify local loading. @compnerd, any thoughts?

@rxwei
Copy link
Contributor Author

rxwei commented Nov 2, 2022

swiftlang/swift-syntax#1042
@swift-ci please smoke test

@CodaFi
Copy link
Contributor

CodaFi commented Nov 2, 2022

swiftlang/swift-syntax#1042
@swift-ci please smoke test

@compnerd
Copy link
Member

compnerd commented Nov 2, 2022

Do we know what this entails? Is it because RTLD_LOCAL isn't possible on Windows?

On Windows we should call LoadLibraryW. llvm::sys::DynamicLibrary::HandleSet is based on one of these two platform-specific implementations:

... but it's not in public headers. I think the right thing to do is to copy llvm::sys::DynamicLibrary::HandleSet over and add an argument to specify local loading. @compnerd, any thoughts?

I think that we should just directly use LoadLibraryW by including Windows.h since we are directly calling dlopen here. We already have a good example of this in the Concurrency runtime where we dynamically load and resolve symbols from dispatch.

https://github.com/apple/swift/blob/d93c02212518b08f870b33bea3b9457ef09cd94b/stdlib/public/Concurrency/Task.cpp#L1528-L1544

@rxwei
Copy link
Contributor Author

rxwei commented Nov 2, 2022

Ok. We won't be able to test it in a meaningful way though, because SWIFT_SWIFT_PARSER isn't enabled on Windows.

@rxwei
Copy link
Contributor Author

rxwei commented Nov 2, 2022

swiftlang/swift-syntax#1042
@swift-ci please smoke test

@compnerd
Copy link
Member

compnerd commented Nov 2, 2022

Hmm, while we cannot use the swift-parser for official builds, we should be able to try to enable that for the CI tests ...

@rxwei
[Macros] Type check user-defined macro plugins
c41ca94 
Type check user-defined macros plugins with user-provided type signatures.

Also, load plugin libraries with `RTLD_LOCAL` instead of `RTLD_GLOBAL` to prevent symbol collision between plugins. `llvm::sys::DynamicLibrary` only supports `RTLD_GLOBAL` so we use plain the `dlopen` instead. This does not work on Windows and needs to be fixed.
@rxwei rxwei force-pushed the compiler-plugin-type-checking branch from dc35c4b to c41ca94 Compare November 2, 2022 17:56
@rxwei
Copy link
Contributor Author

rxwei commented Nov 2, 2022

swiftlang/swift-syntax#1042
@swift-ci please smoke test

@rxwei
Copy link
Contributor Author

rxwei commented Nov 2, 2022

swiftlang/swift-syntax#1042
@swift-ci please smoke test macOS

@rxwei rxwei merged commit d059735 into swiftlang:main Nov 3, 2022
rxwei added a commit to swiftlang/swift-syntax that referenced this pull request Nov 3, 2022
@rxwei
[Macros] Implement compiler plugin type signature requirements (#1042)
f2a64d9 
Implement `_genericSignature` and `_typeSignature` requirements in `_CompilerPlugin`. This enables type checking for macro plugins.

Friend PR: swiftlang/swift#61861
@rxwei rxwei deleted the compiler-plugin-type-checking branch November 3, 2022 15:40
meg-gupta pushed a commit to meg-gupta/swift that referenced this pull request Nov 9, 2022
@rxwei @meg-gupta
[Macros] Type check user-defined macro plugins (swiftlang#61861)
1f305ab 
Type check user-defined macros plugins with user-provided type signatures.

Also, load plugin libraries with `RTLD_LOCAL` instead of `RTLD_GLOBAL` to prevent symbol collision between plugins. `llvm::sys::DynamicLibrary` only supports `RTLD_GLOBAL` so we use the plain `dlopen` instead. This does not work on Windows and needs to be fixed.

Friend PR: swiftlang/swift-syntax#1042
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DougGregor DougGregor DougGregor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rxwei @DougGregor @CodaFi @compnerd