Skip to content

[Distributed] Don't crash in thunk generation when missing SR conformance #69501

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 6 commits into from
Nov 14, 2023

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Oct 30, 2023

Problem: We would properly diagnose the missing conformance; but the compiler would still try to emit a thunk for the errored func. Tests which only -typecheck -verify would not catch this issue because it is after type checking.

The fix: This prevents distributed thunks from being generated when the func they are based on already has errors -- otherwise we can run into missing types and null pointers as we try to generate the thunk.
a
Radar: rdar://116261701

And found a follow up we should do: rdar://117677422

@ktoso
Copy link
Contributor Author

ktoso commented Oct 30, 2023

@swift-ci please smoke test

@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Oct 30, 2023
@ktoso
Copy link
Contributor Author

ktoso commented Oct 31, 2023

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Oct 31, 2023

@swift-ci please smoke test macOS

Failure was unrelated:

fatal error: module map file '/home/build-user/build/buildbot_linux/swiftpm-linux-x86_64/x86_64-unknown-linux-gnu/release/llbuildBasic.build/module.modulemap' not found ci.swift.org/job/swift-PR-macos-smoke-test/9867/console

@ktoso
Copy link
Contributor Author

ktoso commented Oct 31, 2023

@swift-ci please smoke test macOS

@ktoso
Copy link
Contributor Author

ktoso commented Oct 31, 2023

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Oct 31, 2023

This is looking better now, thanks @slavapestov

The only case we've regressed on now is:


extension NoSerializationRequirementYet
  where SerializationRequirement: Codable {
  // expected-error@+1{{result type 'NotCodable' of distributed instance method 'test4' does not conform to serialization requirement 'Codable'}}
  distributed func test4() -> NotCodable {
    .init()
  }
}

since the getConcreteReplacementForMemberSerializationRequirement will give a null type for this... I'm not sure how to handle this, can you advise?

@ktoso ktoso force-pushed the wip-dont-crash-missing-conformance-param branch 2 times, most recently from de8ed4d to 5595dd3 Compare November 14, 2023 10:17
@ktoso
Copy link
Contributor Author

ktoso commented Nov 14, 2023

@swift-ci please smoke test

@ktoso ktoso force-pushed the wip-dont-crash-missing-conformance-param branch from 5595dd3 to 75e2eba Compare November 14, 2023 12:03
@ktoso
Copy link
Contributor Author

ktoso commented Nov 14, 2023

@swift-ci please smoke test

@ktoso ktoso force-pushed the wip-dont-crash-missing-conformance-param branch from 75e2eba to 0f5e564 Compare November 14, 2023 13:15
@ktoso
Copy link
Contributor Author

ktoso commented Nov 14, 2023

@swift-ci please smoke test

@ktoso ktoso enabled auto-merge November 14, 2023 14:21
@ktoso ktoso merged commit 26a04fe into swiftlang:main Nov 14, 2023
for (auto requirement: signature.getRequirements()) {
if (requirement.getFirstType()->isEqual(SerReqAssocType) &&
requirement.getKind() == RequirementKind::Conformance) {
if (auto nominal = requirement.getSecondType()->getAnyNominal()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The RHS of a conformance requirement is always a protocol. Call requirement.getProtocolDecl() to get it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in #69874

@ktoso ktoso deleted the wip-dont-crash-missing-conformance-param branch November 14, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants