Skip to content

🍒[5.7][Distributed] Force the order of properties in AST #58747

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 10, 2022
Merged

🍒[5.7][Distributed] Force the order of properties in AST #58747

merged 7 commits into from
May 10, 2022

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented May 8, 2022

Issue Summary:

  • Using the LocalTestingDistributedActorSystem crashes without this fix.
  • In general, using distributed actor systems declared in library evolution mode compiled modules would fail

Original PR: #58745
Radar:

  • rdar://92712849 - the root cause of all those issues
  • rdar://92910719 - which disabled the still failing test since the workaround was not good enough

Risk: Low, no impact on adopters

Details:

This is the actual solution for all our mystical offset issues -- the order of the synthesized AST fields MUST match the order IRGen enforces (and the DA needs in any case); where the id and system are the FIRST fields, and must be specifically: id, system because that's what IRGen emits and many places seem to expect the order matches between the original AST and whatever gets derived form it and IR.

This probably also resolves a few other crashers we had with similar crash reasons of weird mismatching offsets, I'll be verifying those.

@ktoso ktoso requested a review from a team as a code owner May 8, 2022 10:51
@ktoso ktoso requested review from DougGregor and xedin May 8, 2022 10:52
@ktoso ktoso changed the title [Distributed] Force the order of properties in AST 🍒[5.7][Distributed] Force the order of properties in AST May 8, 2022
@ktoso
Copy link
Contributor Author

ktoso commented May 8, 2022

@swift-ci please test

@ktoso ktoso added the distributed Feature → concurrency: distributed actor label May 8, 2022
@ktoso ktoso added this to the Swift 5.7 milestone May 8, 2022
@ktoso ktoso added the r5.7 label May 8, 2022
@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

CI having a bad day again

Running Swift tests for: check-swift-only_early_swiftdriver-macosx-x86_64 check-swift-validation-macosx-x86_64 check-swift-only_early_swiftdriver-iphonesimulator-x86_64 check-swift-validation-iphonesimulator-x86_64 check-swift-only_early_swiftdriver-watchsimulator-i386 check-swift-validation-watchsimulator-i386 check-swift-only_early_swiftdriver-watchsimulator-x86_64 check-swift-validation-watchsimulator-x86_64
ERROR: command terminated with a non-zero exit status 1, aborting

@ktoso ktoso force-pushed the pick-wip-init-crash-fix branch from 0ada0f0 to 095f4e2 Compare May 9, 2022 03:55
@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test

@ktoso ktoso force-pushed the pick-wip-init-crash-fix branch from 095f4e2 to 292bb7a Compare May 9, 2022 04:34
@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

Sigh... this patch fixed it properly on main but seems something else needs to be cherry picked to 5.7 for the fix to work on watchsimulator-i386 - this is the only failure right now. Detective time...

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

Checking if #42564 helps...

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test macOS platform

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test Linux

@ktoso ktoso mentioned this pull request May 9, 2022
Merged
@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

See also rdar://92952551 for the watchos issue -- 5.7 seems to be missing some feature that makes this work on main... Will investigate separately

@ktoso ktoso force-pushed the pick-wip-init-crash-fix branch from b057898 to f097d24 Compare May 9, 2022 12:12
@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test

xedin
xedin reviewed May 9, 2022
View reviewed changes
lib/Sema/DerivedConformanceDistributedActor.cpp Outdated
if (auto id = derived.Nominal->getDistributedActorIDProperty()) {
derived.addMemberToConformanceContext(pbDecl, /*hint=*/id);
derived.addMemberToConformanceContext(propDecl, /*hint=*/id);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this else ever be executed? Isn't getDistributedActorIDProperty supposed to always return a valid id field either by returning pre-existing or synthesizing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah you’re right tbh.. I’ll update that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm actually the nullptr might be returned when a cycle is detected in the request evaluator... Perhaps better safe and sorry here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checked -- you're right :) id does not request system so we're good

@ktoso
[Distributed] review feedback: avoid un-necessary handling null id; i…
1d7f2ee 
…t will always be synthesized or present already
@ktoso
Copy link
Contributor Author

ktoso commented May 10, 2022

Applied @xedin review feedback; thanks for approving Doug!

@swift-ci please test and merge

@swift-ci swift-ci merged commit 3322c96 into swiftlang:release/5.7 May 10, 2022
@ktoso ktoso deleted the pick-wip-init-crash-fix branch May 10, 2022 13:12
@AnthonyLatsis AnthonyLatsis added 🍒 release cherry pick Flag: Release branch cherry picks swift 5.7 labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xedin xedin xedin left review comments

@DougGregor DougGregor DougGregor approved these changes

Assignees
No one assigned
Labels
distributed Feature → concurrency: distributed actor 🍒 release cherry pick Flag: Release branch cherry picks swift 5.7
Projects
None yet
Milestone
Swift 5.7
Development

Successfully merging this pull request may close these issues.
6 participants
@ktoso @xedin @DougGregor @swift-ci @AnthonyLatsis @kavon