-
Notifications
You must be signed in to change notification settings - Fork 191
Use dynamic replacement instead of _typeByName for internationalization upcalls #756
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
Conversation
@swift-ci please test |
} | ||
#endif | ||
|
||
private struct UIDNAHookICU: UIDNAHook { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I misread it or was UIDNAHookICU
changed from final class
to a struct
👀? Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the only reason it was a final class
was so that _typeByName
would work (it can only lookup classes). I changed it to a struct
to be more in-line with how we typically write swift types now that a class isn't required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that makes total sense.
@@ -158,17 +158,16 @@ package protocol UIDNAHook { | |||
static func decode(_ host: some StringProtocol) -> String? | |||
} | |||
|
|||
dynamic package func _uidnaHook() -> UIDNAHook.Type? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
From the implementation PR it looks like dynamic replacement would work with properties too. So if we want to keep this as a read-only var
I suppose we can keep doing that. I'm fine with this though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I could do that too, I'm fine with either - happy to change it if you have a preference for the variable spelling
} | ||
#endif | ||
|
||
private struct UIDNAHookICU: UIDNAHook { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that makes total sense.
@swift-ci please test |
We currently use
_typeByName
to "up call" from types in FoundationEssentials to types in FoundationInternationalization (mainly for finding ICU-based versions of types defined in the essentials module). Now that swift-foundation exists in the toolchain,_typeByName
is conflating the two versions of FoundationInternationalization when building a copy locally resulting in test crashes and failures. Instead, this transitions to the use of@_dynamicReplacement
in order to swap out the up-call function at runtime for an implementation that returns the resulting type. Since the replacement implementation is statically type checked at compile time, it statically references the correct version of the type avoiding the failures.