Skip to content

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

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

jmschonfeld
Copy link
Contributor

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.

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

}
#endif

private struct UIDNAHookICU: UIDNAHook {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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? {
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

@jmschonfeld
Copy link
Contributor Author

@swift-ci please test

@jmschonfeld jmschonfeld merged commit 196376b into swiftlang:main Jul 23, 2024
3 checks passed
@jmschonfeld jmschonfeld deleted the reliable-upcalls branch July 23, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants