[6.1][Distributed] Protocol mangling and primary associated type fixes #77895
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tocols Previously we would not propagate those into the generated distributed actor, making a lot of generic distributed actor protocols impossible to express. We indeed cannot handle protocols WITHOUT primary associated types, but we certainly can handle them with! This resolves rdar://139332556
This is a crucial fix without which we can crash on some distributed protocol declarations with @resolvable. We cannot "just" use a String to represent the "fake base" of the thunks, and must instead find the $Target macro generated type and use it as the base of the thunk's mangling. Calls are made in such way that record for the protocol requirement: `$s4main28GreeterDefinedSystemProtocolP5greetSSyYaKFTEHF` points at `$$s4main29$GreeterDefinedSystemProtocolC5greetSSyYaKFTE` which makes a dispatch through the _apropriate_ witness table. And the record for the $witness named e.g. `$s4main29$GreeterDefinedSystemProtocolC5greetSSyYaKFTEHF` points to `$s4main28GreeterDefinedSystemProtocolPAA11Distributed01_F9ActorStubRzrlE5greetSSyYaKFTE` which is an extension method: `distributed thunk (extension in main):main.GreeterDefinedSystemProtocol< where A: Distributed._DistributedActorStub>.greet() async throws -> Swift.String`, this very specific design allows us to call the "right method" on the recieving end of a remote call where we do not know the recipient type.
ktoso
changed the title
ktoso
added
distributed
|
@swift-ci please test |
ktoso
changed the title
|
@swift-ci please build toolchain |
ktoso
changed the title
ktoso
changed the title
|
Added some review followup |
|
@swift-ci please test |
DougGregor
approved these changes
Dec 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
6.1 pick for #77584
Description: Distributed accessible function record mangling was too naively just using "$" + "Name" + "C" when recording protocol calls in order to point at the stub type ($NameC for Name). This breaks roundtrip mangling when more complex types are involved, like arrays or more generics as the mangling must be made properly with the class ($Name) decl context. This change makes the mangling proper and also fixes how we deal with primary associated types in these protocols (which would also crash without this change).
Scope/Impact: Low, only distributed method accessors of previously crashing shape are addressed.
Risk: Low, only distributed accessors are affected. No impact to other modules.
Testing: CI testing; Added tests which previously blew up and are now working correctly
Reviewed by: @xedin
Original PR: #77584
Radar: rdar://139332556&139781083
Previously we would not propagate those into the generated distributed
actor, making a lot of generic distributed actor protocols impossible to
express.
We indeed cannot handle protocols WITHOUT primary associated types, but
we certainly can handle them with!
This resolves rdar://139332556