BridgeJS: Use @JS types from other modules in the same package#730
BridgeJS: Use @JS types from other modules in the same package#730kateinoigakukun merged 5 commits intoswiftwasm:mainfrom
@JS types from other modules in the same package#730Conversation
kateinoigakukun
left a comment
There was a problem hiding this comment.
Thanks for working on this! I still haven't taken a closer look at the diff but a few questions:
- Do we support AOT codegen cases? To codegen AOT, we need to require all dependency modules to AOT codegen as well to get skeletons. Can we produce a clear diagnostic when BridgeJS plugin is configured for a dependency module but not AOTed.
- Did you see any particular blocker for supporting cross-package reference?
Yep, this is supported. I’ve pushed a commit to emit a diagnostic when attempting to AOT codegen a target which depends on a BridgeJS enabled target which has not been AOT codegened. If you don’t explicitly specify the target, it will AOT codegen them all in the correct order.
I tried to keep the scope of this PR smaller so I didn’t look into it too much, but this should be possible as long as we can find the skeleton file. I’m not sure how build plugins work in package dependencies, but I believe the worst case would be requiring the dependency to use AOT codegen for this use case. |
Or I guess we could just re-parse the source assuming we are able to locate that. |
krodak
left a comment
There was a problem hiding this comment.
Hey William,
Really like how this came together. The ExternalModuleIndex is a clean abstraction, the disambiguation via module qualifiers follows Swift conventions naturally, and local-shadows-external is the right default. The dependency ordering in the command plugin and the build plugin skeleton discovery are well thought through.
Thanks for the detailed comment in BridgeJSTool.swift about the skeleton-to-Swift injective mapping invariant and the hash escape hatch if it ever breaks. That kind of forward-looking note saves a lot of head-scratching later.
Test coverage is solid across type categories, structural positions, multi-module scenarios, and error diagnostics. The AOT diagnostic from commit 2 and the resource-bundling fix in commit 3 are both good catches.
LGTM!
kateinoigakukun
left a comment
There was a problem hiding this comment.
Overall looks good to me!
Support using
@JStypes from one internal module in@JSAPI in another.This works by parsing the BridgeJS skeleton of each dependency, and using that to resolve types if we fail to find a match within the target. The skeleton of the dependency is passed to the tool using
--dependency-skeleton <Module>=<path>. For this to work with theBridgeJSbuild plugin, the dependency must contain a BridgeJS config file to declare that it uses BridgeJS.There is no attempt to match Swift’s type resolution exactly: most ambiguity requires the module to be explicitly specified. Local types will shadow external types matching Swift’s behaviour.
This doesn’t support module selectors, since I think this would require bumping the minimum SwiftSyntax version.
This doesn’t support using types from modules in other packages. Extending an
@JStype defined in another module is also unsupported.A required field is introduced into the skeleton format. As far as I am aware this shouldn’t be an issue, since there is no need for the skeleton format to be backwards compatible at this time. If there is in fact such need, it should be trivial to make this optional when decoding.
Addresses issue #720