[iOS][macOS] Handle Swift extensions in framework exports check#186120
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the symbol verification logic in verify_exported.dart by adding a regular expression to identify internal Swift extensions on Objective-C classes. Feedback suggests improving the robustness of this regular expression by allowing digits in class names and including support for Objective-C protocols.
| // * 08Internal: 8 chars long, "Internal" | ||
| // * A: Substitution for "Flutter" from earlier part of the name-mangled symbol. | ||
| // * 11SwiftCommon: 11 chars long, "SwiftCommon" | ||
| final swiftInternalExtensionRegExp = RegExp(r'^_\$sSo\d+[A-Za-z]+C\d+Internal.*Swift.*E'); |
There was a problem hiding this comment.
The regular expression for matching the extended Objective-C type has a couple of potential issues:
- Digits in identifiers:
[A-Za-z]+will fail if the class name contains digits (e.g.,Flutter2). Using[A-Za-z0-9]+is more robust as Swift mangled names frequently include digits. - Protocols: The
Cmarker specifically matches classes. If extensions on Objective-C protocols (which use thePmarker) should also be handled, consider using[CP]instead.
| final swiftInternalExtensionRegExp = RegExp(r'^_\$sSo\d+[A-Za-z]+C\d+Internal.*Swift.*E'); | |
| final swiftInternalExtensionRegExp = RegExp(r'^_\$sSo\d+[A-Za-z0-9]+[CP]\d+Internal.*Swift.*E'); |
There was a problem hiding this comment.
Good bot. I'm impressed. Verified locally and this is true. Updated.
There was a problem hiding this comment.
this can be a good candidate for test case if/when you decide to add some tests in the future.
3b9beaa to
eded773
Compare
eded773 to
e5292d5
Compare
e5292d5 to
55a4233
Compare
Allows Swift extension symbols defined in internal modules on Objective-C classes to pass the symbol verification. Swift extensions on Objective-C classes (which reside in Swift's 'So' namespace) mangle differently than standard Swift symbols. Instead of starting with the module name (e.g., `_$s25InternalFlutterSwift...`), they start with the extended type (e.g., `_$sSo14FlutterTracingC...`) and have the module name embedded inside. Furthermore, the Swift compiler may compress repeated tokens in the module name (e.g., substituting 'Flutter' with 'A' in `InternalFlutterSwiftCommon` if 'Flutter' was already used in the extended type name), resulting in `InternalA11Swift` in the mangled symbol. This is pretty common in extensions since we prefix all our Obj-C types with "Flutter". Issue: flutter#186119
55a4233 to
4468d25
Compare
| // ("Flutter" from earlier in the symbol)] + [11 bytes long, "SwiftCommon"]. | ||
| // * E: extension. | ||
| // | ||
| // Details: https://github.com/swiftlang/swift/blob/main/docs/ABI/Mangling.rst |
There was a problem hiding this comment.
This is pretty long but name mangling is also awful and I have to re-learn how it works every time I come back to this code, so maybe useful :/
hellohuanlin
left a comment
There was a problem hiding this comment.
should we write some tests?
| return !(cSymbol || cInternalSymbol || objcSymbol || swiftInternalSymbol); | ||
| }); | ||
| if (unexpectedEntries.isNotEmpty) { | ||
| print('ERROR: $libFlutter exports unexpected symbols:'); |
There was a problem hiding this comment.
I wonder what's the purpose of checking the symbols in the first place? When will we get unexpected symbols?
There was a problem hiding this comment.
What's the purpose of checking the symbols?
This was added as part of an effort to shave every last byte we could off framework size back right after Flutter 1.0. Symbol tables take up space, and exporting symbols that end users don't require uses space in the .symtab section(s) of binaries.
@goderbauer did most of the work on this, but it was part of a broader effort, which involved stripping unused ICU tables (CJK workbreak tables and hyphenation among others if I remember right).
The original patch was flutter-team-archive/engine#6363.
When will we get unexpected symbols?
This is the second useful part of the check.
Generally speaking, in Obj-C code, we mark all intentionally exported symbols in our public API as FLUTTER_EXPORT. Those symbols should always start with Flutter in order to reduce the change of symbol collisions with other libraries. Obj-C convention is to pick some prefix for your library Flutter for us, NS for Apple (from NeXTStep), etc.
Swift has modules and so this really isn't an issue. We should, however, verify that the Obj-C symbols associated with Swift API/ABI start with Flutter, which is what these checks do -- though we also check for Internal because we do not allow public Swift API in Flutter.
One day we'll have a plan for that, but for the moment, we keep it 100% Obj-C to ensure compatibility for all our users.
Why are we getting them here?
While Swift has a stable ABI, and these won't change over time, we're still in the early phases of Swift usage in the embedder, so the first time we use some feature of Swift we hadn't before (in this case a type extension), we're discovering new corners of Swift name mangling we hadn't before.
Could we do something else instead?
There's still value to these checks, but I suspect there might be a better way to check this stuff (maybe by looking at otool or nm output?). It's probably worth looking into that as a followup, but that makes these checks quite platform-specific.
There was a problem hiding this comment.
Actually I should check; my guess is we are getting the symbols from nm or similar already; maybe we could try to recognise Swift symbols and demangle them and check them that way.
In a sense, this effectively is a test, but I think for the purposes of documentation, I agree it's probably worth writing tests that check the regexes against expected categories of symbols. That would involve a bit of refactoring, but happy to do it, but maybe in a followup since we've always treated this as a check akin to a test in the CI pipeline. This is something like the CI check for code-signing or the check for entitlements. I recall that sometime last year I actually did go and add tests for that stuff because I got fed up with it breaking. As I say, probably makes sense to refactor this to test the regexes at least though, but I'd be pretty tempted to do that as a followup. If people feel strongly, happy to send a refactoring patch then stack this on top. |
|
To clarify, does this change expose anything new? Or only the Swift extensions for any ObjC symbols that were already exposed? |
Allows Swift extension symbols defined in internal modules on Objective-C classes to pass the symbol verification.
Swift extensions on Objective-C classes (which reside in Swift's 'So' namespace) mangle differently than standard Swift symbols. Instead of starting with the module name (e.g.,
_$s25InternalFlutterSwift...), they start with the extended type (e.g.,_$sSo14FlutterTracingC...) and have the module name embedded inside.Furthermore, the Swift compiler may compress repeated tokens in the module name (e.g., substituting 'Flutter' with 'A' in
InternalFlutterSwiftCommonif 'Flutter' was already used in the extended type name), resulting inInternalA11Swiftin the mangled symbol. This is pretty common in extensions since we prefix all our Obj-C types with "Flutter".This updates this code to allow the Swift extension added in this patch to pass:
#185918
From that failure:
xcrun swift-demangle -expand '_$sSo14FlutterTracingC08InternalA11SwiftCommonE10beginScopeyAC05TraceG0CSSFZ'We see:
Swift has a stable ABI so these aren't at risk of changing, so this updates the regexes themselves to handle Swift extensions.
No tests because this script is itself a test.
Fixes: #186119
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.