[cxx-interop] allow shared ref retain function to return self #77837
Note to self: need separate error message for invalid retain return value.
Tests are now failing, investigating.
Tests passing again.
LGTM
Just double checking we don't need to do anything extra at the retain call site to ignore the return value.
| 2566 | auto resultInterfaceType = operationFn->getResultInterfaceType(); | ||
| 2567 | if (!resultInterfaceType->isVoid()) { | ||
| 2568 | if (operationKind == CustomRefCountingOperationKind::release || | ||
| 2569 | resultInterfaceType->getAnyNominal() != paramDecl) |
DougGregor219 days agoThis check isn't correct: it's comparing the declaration result's nominal type (if there is one) against the parameter declaration itself (which has, e.g., the name of the parameter in it). Instead, you need to compare the result type against the parameter type. I also expect that we need to look through an optional type on the result type, in case the retain function handles null pointers in some manner. Specifically, I think it should be:
| resultInterfaceType->getAnyNominal() != paramDecl) | |
| !resultInterfaceType->lookThroughSingleOptionalType()->isEqual(paramType)) |
DougGregor219 days agoI'm confused about why this PR worked without this change suggested above. Running tests in CI without the change because I would expect it to fail.
lhoward219 days agoFix applied (shouldn't disrupt running builds to check previous commit).
DougGregor219 days agoAh, thank you for the typo fix here!
@swift-ci please smoke test macOS
[cxx-interop] fix typo in RetainReleaseOperationKind enum
[cxx-interop] allow shared ref retain function to return self
@swift-ci please smoke test
What's the next release one might expect to see this in?
Once it's merged to main, we can cherry-pick to the release/6.1 branch for the 6.1 release (Q1 2025)
@swift-ci please smoke test
This seems very reasonable, LGTM, thank you!
@swift-ci please smoke test Linux
@lhoward I'm going to merge now. Would you cherry-pick this to the release/6.1 branch? There are instructions at https://forums.swift.org/t/swift-6-1-release-process/75442#p-344524-pull-requests-for-release-branch-8
Retain functions that return self need to be manually annotated with SWIFT_RETURNS_RETAINED. Perhaps this could be inferred in a future patch.
Login to write a write a comment.
Many existing C APIs for retaining references, including Apple's own, return the reference. Support this pattern, along with the existing
voidreturn signature, with when importing reference types from C++.More: https://forums.swift.org/t/swift-shared-reference-signature-error/76194/7