[Macros] Track plugin dependencies #65322
Conversation
rintaro
requested review from
ahoppen,
bnbarham,
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci Please smoke test |
rintaro
requested review from
DougGregor
and removed request for
xedin,
slavapestov and
hborla
rintaro
force-pushed
the
plugin-loader-swiftdeps
branch
from
a3ad6cb to
2a4f6db
Compare
There was a problem hiding this comment.
This looks great! Am I understanding correctly that this means we will rebuild every source file in the module when one of the macro implementations that the module uses will change, vs. only rebuilding those source files that use one of the macros?
Does there need to be a corresponding driver change to deal with these new dependencies, or is this all we need?
|
|
||
| // Track the dependency. | ||
| if (DepTracker) | ||
| DepTracker->addDependency(resolvedPath, /*IsSystem=*/false); |
There was a problem hiding this comment.
Should we ever care to pass IsSystem=true here? I could imagine it being true for plugins loaded from lib/swift/host/plugins, but (1) I don't know if that will have any practical effect, and (2) that likely means we'd need to have both -system-plugin-path and -plugin-path, and that just feels annoying.
There was a problem hiding this comment.
This looks great! Am I understanding correctly that this means we will rebuild every source file in the module when one of the macro implementations that the module uses will change, vs. only rebuilding those source files that use one of the macros?
Does there need to be a corresponding driver change to deal with these new dependencies, or is this all we need?
@rintaro could we please add a test to examine the .swiftdeps produced when we have a non-primary source input use a plugin? Once this lands in a snapshot toolchain, would be great to also add a test in the driver incremental build test suite to validate the answer to Doug's question.
On the surface, it looks like .swiftdeps may contain dependencies on plugins from non-primary sources so this may cause coarse-grained incremental re-compilation, but I think that's an okay starting point. And I am pretty sure the driver should be able to handle this without any changes for now.
include/swift/AST/PluginLoader.h
Outdated
| PluginRegistry *Registry = nullptr; | ||
| std::unique_ptr<PluginRegistry> OwnedRegistry = nullptr; |
There was a problem hiding this comment.
Could you add a short comment about the difference between Registry and OwnedRegistry?
rintaro
force-pushed
the
plugin-loader-swiftdeps
branch
from
2a4f6db to
332bae4
Compare
| @@ -34,3 +35,5 @@ | |||
| // CHECK-CONFIRM-ONELINE: {"name":{{.*}}]} | |||
|
|
|||
| import Module2 | |||
|
|
|||
| @freestanding(expression) macro echo<T>(_: T) -> T = #externalMacro(module: "Plugin", type: "EchoMacro") | |||
There was a problem hiding this comment.
This is required to actually load the plugin, otherwise plugin dependencies are not recorded.
There was a problem hiding this comment.
Would it be better if you put this declaration in Inputs/loaded_module_trace_compiler_plugin.swift, so that this is imported from a module like an actual macro adopter library would end up?
rintaro
force-pushed
the
plugin-loader-swiftdeps
branch
from
332bae4 to
0a42292
Compare
|
@swift-ci Please smoke test |
rintaro
force-pushed
the
plugin-loader-swiftdeps
branch
from
0a42292 to
4181f7a
Compare
|
@swift-ci Please smoke test |
|
swiftlang/llvm-project#6695 |
* Factor out ASTContext plugin loading to newly introduced 'PluginLoader' * Insert 'DependencyTracker' to 'PluginLoader' * Add dependencies right before loading the plugins rdar://104938481
rintaro
force-pushed
the
plugin-loader-swiftdeps
branch
from
4181f7a to
a551c01
Compare
|
@swift-ci Please smoke test |
|
swiftlang/llvm-project#6695 |
rdar://104938481