[Distributed] correct take semantics for synthesized ID assignments #63779
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
cleanup: no need to dump input always
|
@swift-ci please smoke test |
ktoso
force-pushed
the
wip-distributed-inits-sil-issue
branch
from
700bc18 to
7ecfc82
Compare
ktoso
commented
Feb 20, 2023
| } | ||
| someField = try getSomeClass() | ||
| } | ||
|
|
There was a problem hiding this comment.
This adds a test for a class based actor system which was a thing we didn't cover well, but in the real world most systems will be; This is the same as the other sil_n tests but for a class.
ktoso
commented
Feb 20, 2023
| @@ -209,7 +209,7 @@ void SILGenFunction::emitDistActorIdentityInit(ConstructorDecl *ctor, | |||
| { temp, selfMetatypeValue }); | |||
|
|
|||
| // --- initialize the property. | |||
| initializeProperty(*this, loc, borrowedSelfArg, var, temp); | |||
There was a problem hiding this comment.
The issue was here, so changing to take here is correct, but the other sites want to remain not-take.
Thanks @eeckstein and @drexin for the pointers !
xedin
approved these changes
Feb 20, 2023
|
@swift-ci please smoke test |
ktoso
commented
Feb 21, 2023
| /// | ||
| /// By adding the `Any` we don't know the full size of the struct making the type in SIL `$*ActorAddress` | ||
| /// when we try to store or pass it around, which triggers `isAddressOnly` guarded paths which we need to test. | ||
| public struct NotLoadableActorAddress: Hashable, Sendable, Codable { |
There was a problem hiding this comment.
The important bit to reproduce the issue
ktoso
force-pushed
the
wip-distributed-inits-sil-issue
branch
from
b92995a to
629ac65
Compare
|
Added another test that exercises the AddressOnly case as well. |
|
@swift-ci please smoke test and merge |
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.
Resolves #62898
Resolves rdar://104583893
This is a surprising regression because it always was wrong, but only started failing in recent toolchains where this started getting verified.
I'll also wok on getting distributed cluster into the source compat build, which we didn't complete for some reason before: swiftlang/swift-source-compat-suite#730 which would have likely caught this when the toolchain changed to detect this wrong lifetime.
I also confirmed this works on the reproducer steps, targeting swift-distributed actors
mainand indeed this started failing but used to somehow sneakily still pass on older toolchains:git clone https://github.com/apple/swift-distributed-actors.gitexport SWIFT_EXEC=$HOME/code/swift-project/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/bin/swiftcswift run --package-path Samples/Sources/SampleDiningPhilosophersecho $SWIFT_EXEC; echo; echo; /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2023-02-18-a.xctoolchain/usr/bin/swift run --package-path Samples/Sources/SampleDiningPhilosophers