-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[cxx-interop] Add support for subscripts of non-copyable types #86125
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6257,9 +6257,9 @@ makeBaseClassMemberAccessors(DeclContext *declContext, | |
|
|
||
| // Use 'address' or 'mutableAddress' accessors for non-copyable | ||
| // types, unless the base accessor returns it by value. | ||
| bool useAddress = contextTy->isNoncopyable() && | ||
| (baseClassVar->getReadImpl() == ReadImplKind::Stored || | ||
| baseClassVar->getAccessor(AccessorKind::Address)); | ||
| bool useAddress = baseClassVar->getAccessor(AccessorKind::Address) || | ||
| (contextTy->isNoncopyable() && | ||
| baseClassVar->getReadImpl() == ReadImplKind::Stored); | ||
|
|
||
| ParameterList *bodyParams = nullptr; | ||
| if (auto subscript = dyn_cast<SubscriptDecl>(baseClassVar)) { | ||
|
|
@@ -6407,12 +6407,6 @@ static ValueDecl *cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext, | |
| } | ||
|
|
||
| if (auto subscript = dyn_cast<SubscriptDecl>(decl)) { | ||
| auto contextTy = | ||
| newContext->mapTypeIntoEnvironment(subscript->getElementInterfaceType()); | ||
| // Subscripts that return non-copyable types are not yet supported. | ||
| // See: https://github.com/apple/swift/issues/70047. | ||
| if (contextTy->isNoncopyable()) | ||
| return nullptr; | ||
| auto out = SubscriptDecl::create( | ||
| subscript->getASTContext(), subscript->getName(), subscript->getStaticLoc(), | ||
| subscript->getStaticSpelling(), subscript->getSubscriptLoc(), | ||
|
|
@@ -8349,7 +8343,7 @@ const clang::CXXConstructorDecl * | |
| importer::findCopyConstructor(const clang::CXXRecordDecl *decl) { | ||
| for (auto ctor : decl->ctors()) { | ||
| if (ctor->isCopyConstructor() && | ||
| // FIXME: Support default arguments (rdar://142414553) | ||
| // FIXME: Support default arguments | ||
| ctor->getNumParams() == 1 && ctor->getAccess() == clang::AS_public && | ||
| !ctor->isDeleted() && !ctor->isIneligibleOrNotSelected()) | ||
| return ctor; | ||
|
|
@@ -8364,9 +8358,8 @@ static bool hasMoveTypeOperations(const clang::CXXRecordDecl *decl) { | |
| return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) { | ||
| return ctor->isMoveConstructor() && !ctor->isDeleted() && | ||
| !ctor->isIneligibleOrNotSelected() && | ||
| // FIXME: Support default arguments (rdar://142414553) | ||
| ctor->getNumParams() == 1 && | ||
| ctor->getAccess() == clang::AS_public; | ||
| // FIXME: Support default arguments | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accidental change? |
||
| ctor->getNumParams() == 1 && ctor->getAccess() == clang::AS_public; | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -8393,7 +8386,7 @@ static bool | |
| hasConstructorWithUnsupportedDefaultArgs(const clang::CXXRecordDecl *decl) { | ||
| return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) { | ||
| return (ctor->isCopyConstructor() || ctor->isMoveConstructor()) && | ||
| // FIXME: Support default arguments (rdar://142414553) | ||
| // FIXME: Support default arguments | ||
| ctor->getNumParams() != 1; | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1688,8 +1688,13 @@ synthesizeUnwrappingAddressSetterBody(AbstractFunctionDecl *afd, | |
| ASTContext &ctx = setterDecl->getASTContext(); | ||
|
|
||
| auto selfArg = createSelfArg(setterDecl); | ||
| SmallVector<Expr *> arguments; | ||
| for (size_t idx = 0, end = setterDecl->getParameters()->size(); idx < end; | ||
| ++idx) | ||
| arguments.push_back(createParamRefExpr(setterDecl, idx)); | ||
|
|
||
| auto *setterImplCallExpr = | ||
| createAccessorImplCallExpr(setterImpl, selfArg, {}); | ||
| createAccessorImplCallExpr(setterImpl, selfArg, arguments); | ||
|
|
||
| auto *returnStmt = ReturnStmt::createImplicit(ctx, setterImplCallExpr); | ||
|
|
||
|
|
@@ -1708,7 +1713,6 @@ SubscriptDecl *SwiftDeclSynthesizer::makeSubscript(FuncDecl *getter, | |
| FuncDecl *getterImpl = getter ? getter : setter; | ||
| FuncDecl *setterImpl = setter; | ||
|
|
||
| // FIXME: support unsafeAddress accessors. | ||
| // Get the return type wrapped in `Unsafe(Mutable)Pointer<T>`. | ||
| const auto rawElementTy = getterImpl->getResultInterfaceType(); | ||
| // Unwrap `T`. Use rawElementTy for return by value. | ||
|
|
@@ -1737,17 +1741,27 @@ SubscriptDecl *SwiftDeclSynthesizer::makeSubscript(FuncDecl *getter, | |
| getterImpl->getClangNode()); | ||
| subscript->copyFormalAccessFrom(getterImpl); | ||
|
|
||
| AccessorDecl *getterDecl = | ||
| AccessorDecl::create(ctx, getterImpl->getLoc(), getterImpl->getLoc(), | ||
| AccessorKind::Get, subscript, | ||
| /*async*/ false, SourceLoc(), | ||
| /*throws*/ false, SourceLoc(), | ||
| /*ThrownType=*/TypeLoc(), bodyParams, elementTy, dc); | ||
| // Use 'address' or 'mutableAddress' accessors for non-copyable | ||
| // types that are returned indirectly. | ||
| bool useAddress = | ||
| rawElementTy->getAnyPointerElementType() && elementTy->isNoncopyable(); | ||
|
|
||
| AccessorDecl *getterDecl = AccessorDecl::create( | ||
| ctx, getterImpl->getLoc(), getterImpl->getLoc(), | ||
| useAddress ? AccessorKind::Address : AccessorKind::Get, subscript, | ||
| /*async*/ false, SourceLoc(), | ||
| /*throws*/ false, SourceLoc(), | ||
| /*ThrownType=*/TypeLoc(), bodyParams, | ||
| useAddress ? elementTy->wrapInPointer(PTK_UnsafePointer) : elementTy, dc); | ||
| getterDecl->copyFormalAccessFrom(subscript); | ||
| getterDecl->setImplicit(); | ||
| if (!useAddress) | ||
| getterDecl->setImplicit(); | ||
|
Comment on lines
+1757
to
+1758
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we mark this getter as implicit unconditionally?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we mark an |
||
| getterDecl->setIsDynamic(false); | ||
| getterDecl->setIsTransparent(true); | ||
| getterDecl->setBodySynthesizer(synthesizeUnwrappingGetterBody, getterImpl); | ||
| getterDecl->setBodySynthesizer(useAddress | ||
| ? synthesizeUnwrappingAddressGetterBody | ||
| : synthesizeUnwrappingGetterBody, | ||
| getterImpl); | ||
|
|
||
| if (getterImpl->isMutating()) { | ||
| getterDecl->setSelfAccessKind(SelfAccessKind::Mutating); | ||
|
|
@@ -1763,22 +1777,31 @@ SubscriptDecl *SwiftDeclSynthesizer::makeSubscript(FuncDecl *getter, | |
| paramVarDecl->setInterfaceType(elementTy); | ||
|
|
||
| SmallVector<ParamDecl *> setterParams; | ||
| setterParams.push_back(paramVarDecl); | ||
| if (!useAddress) | ||
| setterParams.push_back(paramVarDecl); | ||
| setterParams.append(bodyParams->begin(), bodyParams->end()); | ||
|
|
||
| auto setterParamList = ParameterList::create(ctx, setterParams); | ||
|
|
||
| setterDecl = AccessorDecl::create( | ||
| ctx, setterImpl->getLoc(), setterImpl->getLoc(), AccessorKind::Set, | ||
| ctx, setterImpl->getLoc(), setterImpl->getLoc(), | ||
| useAddress ? AccessorKind::MutableAddress : AccessorKind::Set, | ||
| subscript, | ||
| /*async*/ false, SourceLoc(), | ||
| /*throws*/ false, SourceLoc(), /*ThrownType=*/TypeLoc(), | ||
| setterParamList, TupleType::getEmpty(ctx), dc); | ||
| setterParamList, | ||
| useAddress ? elementTy->wrapInPointer(PTK_UnsafeMutablePointer) | ||
| : TupleType::getEmpty(ctx), | ||
| dc); | ||
| setterDecl->copyFormalAccessFrom(subscript); | ||
| setterDecl->setImplicit(); | ||
| if (!useAddress) | ||
| setterDecl->setImplicit(); | ||
|
Comment on lines
+1797
to
+1798
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
| setterDecl->setIsDynamic(false); | ||
| setterDecl->setIsTransparent(true); | ||
| setterDecl->setBodySynthesizer(synthesizeUnwrappingSetterBody, setterImpl); | ||
| setterDecl->setBodySynthesizer(useAddress | ||
| ? synthesizeUnwrappingAddressSetterBody | ||
| : synthesizeUnwrappingSetterBody, | ||
| setterImpl); | ||
|
|
||
| if (setterImpl->isMutating()) { | ||
| setterDecl->setSelfAccessKind(SelfAccessKind::Mutating); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| // RUN: %empty-directory(%t) | ||
| // RUN: split-file %s %t | ||
| // RUN: %target-build-swift %t%{fs-sep}test.swift -I %t%{fs-sep}Inputs -o %t%{fs-sep}out -cxx-interoperability-mode=default | ||
| // RUN: %target-codesign %t%{fs-sep}out | ||
| // RUN: %target-run %t%{fs-sep}out | ||
| // | ||
| // REQUIRES: executable_test | ||
|
|
||
| //--- Inputs/module.modulemap | ||
| module Test { | ||
| header "noncopyable.h" | ||
| requires cplusplus | ||
| } | ||
|
|
||
| //--- Inputs/noncopyable.h | ||
| #include <string> | ||
|
|
||
| struct NonCopyable { | ||
| NonCopyable() = default; | ||
| NonCopyable(int x) : number(x) {} | ||
| NonCopyable(const NonCopyable& other) = delete; | ||
| NonCopyable(NonCopyable&& other) = default; | ||
|
|
||
| int number = 0; | ||
| }; | ||
|
|
||
| template<typename T> | ||
| struct HasSubscript { | ||
| T &operator[](int idx) { return element; } | ||
| T element; | ||
| }; | ||
|
|
||
| using HasSubscriptInt = HasSubscript<int>; | ||
| using HasSubscriptNonCopyable = HasSubscript<NonCopyable>; | ||
|
|
||
| struct InheritsFromHasSubscript : public HasSubscript<NonCopyable> {}; | ||
|
|
||
| //--- test.swift | ||
| import StdlibUnittest | ||
| import Test | ||
|
|
||
| var NonCopyableTestSuite = TestSuite("NonCopyable") | ||
|
|
||
| func borrow(_ x: borrowing NonCopyable) -> Int32 { return x.number; } | ||
|
|
||
| NonCopyableTestSuite.test("Use subscript") { | ||
| var o1 = HasSubscriptInt(element: 7) | ||
| expectEqual(o1[42], 7) | ||
|
|
||
| var o2 = HasSubscriptNonCopyable(element: NonCopyable(5)) | ||
| expectEqual(borrow(o2[42]), 5) | ||
|
|
||
| var inherited = InheritsFromHasSubscript() | ||
| expectEqual(borrow(inherited[42]), 0) | ||
| } | ||
|
|
||
| NonCopyableTestSuite.test("Mutate subscript") { | ||
| var o1 = HasSubscriptInt(element: 7) | ||
| expectEqual(o1[42], 7) | ||
| o1[42] = 8 | ||
| expectEqual(o1[42], 8) | ||
|
|
||
| var o2 = HasSubscriptNonCopyable(element: NonCopyable(5)) | ||
| expectEqual(borrow(o2[42]), 5) | ||
| o2[42] = NonCopyable(16) | ||
| expectEqual(borrow(o2[42]), 16) | ||
|
|
||
| var inherited = InheritsFromHasSubscript() | ||
| expectEqual(borrow(inherited[42]), 0) | ||
| inherited[42] = NonCopyable(3) | ||
| expectEqual(borrow(inherited[42]), 3) | ||
| } | ||
|
|
||
| runAllTests() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doug noticed these radars numbers and asked for them to be removed. Since this is a small change, in a file that this patch touches, I thought it would be reasonable to do it here instead of creating a separate patch just for this. Would you prefer me to do it in a separate patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep it out of this PR, since you're not modifying this code part here.
If you'd like to remove there rdar links, could you please file GitHub issues and link to them in the comments instead? That way someone encountering this FIXME and actually fixing it would remember to close the corresponding issue. If there isn't an issue link, the issue (or rdar) probably won't get closed.