Skip to content

Commit bacc58e

Browse files
hyphyp
committed
[cxx-interop] provide correct referential access to non-copyable base fields from a derived value type
1 parent 51d0e84 commit bacc58e

File tree

4 files changed

+179
-40
lines changed

4 files changed

+179
-40
lines changed

‎lib/ClangImporter/ClangImporter.cpp

Lines changed: 124 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -5053,13 +5053,22 @@ synthesizeBaseClassMethodBody(AbstractFunctionDecl *afd, void *context) {
50535053
return {body, /*isTypeChecked=*/true};
50545054
}
50555055

5056+
// How should the synthesized C++ method that returns the field of interest
5057+
// from the base class should return the value - by value, or by reference.
5058+
enum ReferenceReturnTypeBehaviorForBaseAccessorSynthesis {
5059+
ReturnByValue,
5060+
ReturnByReference,
5061+
ReturnByMutableReference
5062+
};
5063+
50565064
// Synthesize a C++ method that returns the field of interest from the base
50575065
// class. This lets Clang take care of the cast from the derived class
50585066
// to the base class while the field is accessed.
50595067
static clang::CXXMethodDecl *synthesizeCxxBaseGetterAccessorMethod(
50605068
ClangImporter &impl, const clang::CXXRecordDecl *derivedClass,
50615069
const clang::CXXRecordDecl *baseClass, const clang::FieldDecl *field,
5062-
ValueDecl *retainOperationFn) {
5070+
ValueDecl *retainOperationFn,
5071+
ReferenceReturnTypeBehaviorForBaseAccessorSynthesis behavior) {
50635072
auto &clangCtx = impl.getClangASTContext();
50645073
auto &clangSema = impl.getClangSema();
50655074

@@ -5068,16 +5077,30 @@ static clang::CXXMethodDecl *synthesizeCxxBaseGetterAccessorMethod(
50685077
if (name.isIdentifier()) {
50695078
std::string newName;
50705079
llvm::raw_string_ostream os(newName);
5071-
os << "__synthesizedBaseGetterAccessor_"
5080+
os << (behavior == ReferenceReturnTypeBehaviorForBaseAccessorSynthesis::
5081+
ReturnByMutableReference
5082+
? "__synthesizedBaseSetterAccessor_"
5083+
: "__synthesizedBaseGetterAccessor_")
50725084
<< name.getAsIdentifierInfo()->getName();
50735085
name = clang::DeclarationName(
50745086
&impl.getClangPreprocessor().getIdentifierTable().get(os.str()));
50755087
}
50765088
auto returnType = field->getType();
50775089
if (returnType->isReferenceType())
50785090
returnType = returnType->getPointeeType();
5091+
auto valueReturnType = returnType;
5092+
if (behavior !=
5093+
ReferenceReturnTypeBehaviorForBaseAccessorSynthesis::ReturnByValue) {
5094+
returnType = clangCtx.getRValueReferenceType(
5095+
behavior == ReferenceReturnTypeBehaviorForBaseAccessorSynthesis::
5096+
ReturnByReference
5097+
? returnType.withConst()
5098+
: returnType);
5099+
}
50795100
clang::FunctionProtoType::ExtProtoInfo info;
5080-
info.TypeQuals.addConst();
5101+
if (behavior != ReferenceReturnTypeBehaviorForBaseAccessorSynthesis::
5102+
ReturnByMutableReference)
5103+
info.TypeQuals.addConst();
50815104
info.ExceptionSpec.Type = clang::EST_NoThrow;
50825105
auto ftype = clangCtx.getFunctionType(returnType, {}, info);
50835106
auto newMethod = clang::CXXMethodDecl::Create(
@@ -5128,9 +5151,10 @@ static clang::CXXMethodDecl *synthesizeCxxBaseGetterAccessorMethod(
51285151
/*HadMultipleCandidates=*/false,
51295152
clang::DeclarationNameInfo(field->getDeclName(),
51305153
clang::SourceLocation()),
5131-
returnType, clang::VK_LValue, clang::OK_Ordinary);
5132-
auto returnCast = clangSema.ImpCastExprToType(
5133-
memberExpr, returnType, clang::CK_LValueToRValue, clang::VK_PRValue);
5154+
valueReturnType, clang::VK_LValue, clang::OK_Ordinary);
5155+
auto returnCast = clangSema.ImpCastExprToType(memberExpr, valueReturnType,
5156+
clang::CK_LValueToRValue,
5157+
clang::VK_PRValue);
51345158
if (!returnCast.isUsable())
51355159
return nullptr;
51365160
return returnCast.get();
@@ -5187,7 +5211,11 @@ static clang::CXXMethodDecl *synthesizeCxxBaseGetterAccessorMethod(
51875211
// The method's body takes the following form:
51885212
// return self.__synthesizedBaseCall_fn(args...)
51895213
static std::pair<BraceStmt *, bool>
5190-
synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
5214+
synthesizeBaseClassFieldGetterOrAddressGetterBody(AbstractFunctionDecl *afd,
5215+
void *context,
5216+
AccessorKind kind) {
5217+
assert(kind == AccessorKind::Get || kind == AccessorKind::Address ||
5218+
kind == AccessorKind::MutableAddress);
51915219
ASTContext &ctx = afd->getASTContext();
51925220

51935221
AccessorDecl *getterDecl = cast<AccessorDecl>(afd);
@@ -5201,8 +5229,7 @@ synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
52015229
if (baseClassVar->getClangDecl())
52025230
baseClangDecl = baseClassVar->getClangDecl();
52035231
else
5204-
baseClangDecl =
5205-
getCalledBaseCxxMethod(baseClassVar->getAccessor(AccessorKind::Get));
5232+
baseClangDecl = getCalledBaseCxxMethod(baseClassVar->getAccessor(kind));
52065233

52075234
clang::CXXMethodDecl *baseGetterCxxMethod = nullptr;
52085235
if (auto *md = dyn_cast_or_null<clang::CXXMethodDecl>(baseClangDecl)) {
@@ -5215,9 +5242,12 @@ synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
52155242
getterDecl->getResultInterfaceType()->isForeignReferenceType()
52165243
? ReferenceReturnTypeBehaviorForBaseMethodSynthesis::
52175244
RemoveReferenceIfPointer
5218-
: ReferenceReturnTypeBehaviorForBaseMethodSynthesis::
5219-
RemoveReference,
5220-
/*forceConstQualifier=*/true);
5245+
: (kind != AccessorKind::Get
5246+
? ReferenceReturnTypeBehaviorForBaseMethodSynthesis::
5247+
KeepReference
5248+
: ReferenceReturnTypeBehaviorForBaseMethodSynthesis::
5249+
RemoveReference),
5250+
/*forceConstQualifier=*/kind != AccessorKind::MutableAddress);
52215251
} else if (auto *fd = dyn_cast_or_null<clang::FieldDecl>(baseClangDecl)) {
52225252
ValueDecl *retainOperationFn = nullptr;
52235253
// Check if this field getter is returning a retainable FRT.
@@ -5240,7 +5270,14 @@ synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
52405270
*static_cast<ClangImporter *>(ctx.getClangModuleLoader()),
52415271
cast<clang::CXXRecordDecl>(derivedStruct->getClangDecl()),
52425272
cast<clang::CXXRecordDecl>(baseStruct->getClangDecl()), fd,
5243-
retainOperationFn);
5273+
retainOperationFn,
5274+
kind == AccessorKind::Get
5275+
? ReferenceReturnTypeBehaviorForBaseAccessorSynthesis::ReturnByValue
5276+
: (kind == AccessorKind::Address
5277+
? ReferenceReturnTypeBehaviorForBaseAccessorSynthesis::
5278+
ReturnByReference
5279+
: ReferenceReturnTypeBehaviorForBaseAccessorSynthesis::
5280+
ReturnByMutableReference));
52445281
}
52455282

52465283
if (!baseGetterCxxMethod) {
@@ -5253,12 +5290,17 @@ synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
52535290
auto *baseGetterMethod = cast<FuncDecl>(
52545291
ctx.getClangModuleLoader()->importDeclDirectly(baseGetterCxxMethod));
52555292

5256-
auto selfDecl = getterDecl->getImplicitSelfDecl();
5257-
auto selfExpr = new (ctx) DeclRefExpr(selfDecl, DeclNameLoc(),
5258-
/*implicit*/ true);
5259-
selfExpr->setType(selfDecl->getTypeInContext());
5260-
5261-
Argument selfArg = Argument::unlabeled(selfExpr);
5293+
Argument selfArg = [&]() {
5294+
auto selfDecl = getterDecl->getImplicitSelfDecl();
5295+
auto selfExpr = new (ctx) DeclRefExpr(selfDecl, DeclNameLoc(),
5296+
/*implicit*/ true);
5297+
if (kind == AccessorKind::MutableAddress) {
5298+
selfExpr->setType(LValueType::get(selfDecl->getInterfaceType()));
5299+
return Argument::implicitInOut(ctx, selfExpr);
5300+
}
5301+
selfExpr->setType(selfDecl->getTypeInContext());
5302+
return Argument::unlabeled(selfExpr);
5303+
}();
52625304

52635305
auto *baseMemberExpr =
52645306
new (ctx) DeclRefExpr(ConcreteDeclRef(baseGetterMethod), DeclNameLoc(),
@@ -5294,6 +5336,19 @@ synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
52945336
return {body, /*isTypeChecked=*/true};
52955337
}
52965338

5339+
static std::pair<BraceStmt *, bool>
5340+
synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
5341+
return synthesizeBaseClassFieldGetterOrAddressGetterBody(afd, context,
5342+
AccessorKind::Get);
5343+
}
5344+
5345+
static std::pair<BraceStmt *, bool>
5346+
synthesizeBaseClassFieldAddressGetterBody(AbstractFunctionDecl *afd,
5347+
void *context) {
5348+
return synthesizeBaseClassFieldGetterOrAddressGetterBody(
5349+
afd, context, AccessorKind::Address);
5350+
}
5351+
52975352
// For setters we have to pass self as a pointer and then emit an assign:
52985353
// %0 = Builtin.addressof(&self)
52995354
// %1 = Builtin.reinterpretCast<UnsafeMutablePointer<Derived>>(%0)
@@ -5355,13 +5410,24 @@ synthesizeBaseClassFieldSetterBody(AbstractFunctionDecl *afd, void *context) {
53555410
return {body, /*isTypeChecked=*/true};
53565411
}
53575412

5413+
static std::pair<BraceStmt *, bool>
5414+
synthesizeBaseClassFieldAddressSetterBody(AbstractFunctionDecl *afd,
5415+
void *context) {
5416+
return synthesizeBaseClassFieldGetterOrAddressGetterBody(
5417+
afd, context, AccessorKind::MutableAddress);
5418+
}
5419+
53585420
static SmallVector<AccessorDecl *, 2>
53595421
makeBaseClassMemberAccessors(DeclContext *declContext,
53605422
AbstractStorageDecl *computedVar,
53615423
AbstractStorageDecl *baseClassVar) {
53625424
auto &ctx = declContext->getASTContext();
53635425
auto computedType = computedVar->getInterfaceType();
53645426

5427+
// Use 'address' or 'mutableAddress' accessors for non-copyable
5428+
// types.
5429+
bool useAddress = computedType->isNoncopyable(declContext);
5430+
53655431
ParameterList *bodyParams = nullptr;
53665432
if (auto subscript = dyn_cast<SubscriptDecl>(baseClassVar)) {
53675433
computedType = computedType->getAs<FunctionType>()->getResult();
@@ -5375,18 +5441,22 @@ makeBaseClassMemberAccessors(DeclContext *declContext,
53755441
auto getterDecl = AccessorDecl::create(
53765442
ctx,
53775443
/*FuncLoc=*/SourceLoc(),
5378-
/*AccessorKeywordLoc=*/SourceLoc(), AccessorKind::Get, computedVar,
5444+
/*AccessorKeywordLoc=*/SourceLoc(),
5445+
useAddress ? AccessorKind::Address : AccessorKind::Get, computedVar,
53795446
/*StaticLoc=*/SourceLoc(),
53805447
StaticSpellingKind::None, // TODO: we should handle static vars.
53815448
/*Async=*/false, /*AsyncLoc=*/SourceLoc(),
53825449
/*Throws=*/false,
5383-
/*ThrowsLoc=*/SourceLoc(), /*ThrownType=*/TypeLoc(),
5384-
bodyParams, computedType, declContext);
5450+
/*ThrowsLoc=*/SourceLoc(), /*ThrownType=*/TypeLoc(), bodyParams,
5451+
useAddress ? computedType->wrapInPointer(PTK_UnsafePointer)
5452+
: computedType,
5453+
declContext);
53855454
getterDecl->setIsTransparent(true);
53865455
getterDecl->setAccess(AccessLevel::Public);
5387-
getterDecl->setBodySynthesizer(synthesizeBaseClassFieldGetterBody,
5456+
getterDecl->setBodySynthesizer(useAddress
5457+
? synthesizeBaseClassFieldAddressGetterBody
5458+
: synthesizeBaseClassFieldGetterBody,
53885459
baseClassVar);
5389-
53905460
if (baseClassVar->getWriteImpl() == WriteImplKind::Immutable)
53915461
return {getterDecl};
53925462

@@ -5396,28 +5466,33 @@ makeBaseClassMemberAccessors(DeclContext *declContext,
53965466
newValueParam->setSpecifier(ParamSpecifier::Default);
53975467
newValueParam->setInterfaceType(computedType);
53985468

5399-
ParameterList *setterBodyParams = nullptr;
5400-
if (auto subscript = dyn_cast<SubscriptDecl>(baseClassVar)) {
5401-
auto idxParam = subscript->getIndices()->get(0);
5402-
bodyParams = ParameterList::create(ctx, { idxParam });
5403-
setterBodyParams = ParameterList::create(ctx, { newValueParam, idxParam });
5404-
} else {
5405-
setterBodyParams = ParameterList::create(ctx, { newValueParam });
5406-
}
5469+
SmallVector<ParamDecl *, 2> setterParamDecls;
5470+
if (!useAddress)
5471+
setterParamDecls.push_back(newValueParam);
5472+
if (auto subscript = dyn_cast<SubscriptDecl>(baseClassVar))
5473+
setterParamDecls.push_back(subscript->getIndices()->get(0));
5474+
ParameterList *setterBodyParams =
5475+
ParameterList::create(ctx, setterParamDecls);
54075476

54085477
auto setterDecl = AccessorDecl::create(
54095478
ctx,
54105479
/*FuncLoc=*/SourceLoc(),
5411-
/*AccessorKeywordLoc=*/SourceLoc(), AccessorKind::Set, computedVar,
5480+
/*AccessorKeywordLoc=*/SourceLoc(),
5481+
useAddress ? AccessorKind::MutableAddress : AccessorKind::Set,
5482+
computedVar,
54125483
/*StaticLoc=*/SourceLoc(),
54135484
StaticSpellingKind::None, // TODO: we should handle static vars.
54145485
/*Async=*/false, /*AsyncLoc=*/SourceLoc(),
54155486
/*Throws=*/false,
5416-
/*ThrowsLoc=*/SourceLoc(), /*ThrownType=*/TypeLoc(),
5417-
setterBodyParams, TupleType::getEmpty(ctx),declContext);
5487+
/*ThrowsLoc=*/SourceLoc(), /*ThrownType=*/TypeLoc(), setterBodyParams,
5488+
useAddress ? computedType->wrapInPointer(PTK_UnsafeMutablePointer)
5489+
: TupleType::getEmpty(ctx),
5490+
declContext);
54185491
setterDecl->setIsTransparent(true);
54195492
setterDecl->setAccess(AccessLevel::Public);
5420-
setterDecl->setBodySynthesizer(synthesizeBaseClassFieldSetterBody,
5493+
setterDecl->setBodySynthesizer(useAddress
5494+
? synthesizeBaseClassFieldAddressSetterBody
5495+
: synthesizeBaseClassFieldSetterBody,
54215496
baseClassVar);
54225497
setterDecl->setSelfAccessKind(SelfAccessKind::Mutating);
54235498

@@ -5504,6 +5579,10 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
55045579
}
55055580

55065581
if (auto subscript = dyn_cast<SubscriptDecl>(decl)) {
5582+
// Subscripts that return non-copyable types are not yet supported.
5583+
// See: https://github.com/apple/swift/issues/70047.
5584+
if (subscript->getElementInterfaceType()->isNoncopyable(newContext))
5585+
return nullptr;
55075586
auto out = SubscriptDecl::create(
55085587
subscript->getASTContext(), subscript->getName(), subscript->getStaticLoc(),
55095588
subscript->getStaticSpelling(), subscript->getSubscriptLoc(),
@@ -5528,12 +5607,17 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
55285607
out->setIsDynamic(var->isDynamic());
55295608
out->copyFormalAccessFrom(var);
55305609
out->getASTContext().evaluator.cacheOutput(HasStorageRequest{out}, false);
5531-
out->setAccessors(SourceLoc(),
5532-
makeBaseClassMemberAccessors(newContext, out, var),
5533-
SourceLoc());
5610+
auto accessors = makeBaseClassMemberAccessors(newContext, out, var);
5611+
out->setAccessors(SourceLoc(), accessors, SourceLoc());
55345612
auto isMutable = var->getWriteImpl() == WriteImplKind::Immutable
55355613
? StorageIsNotMutable : StorageIsMutable;
5536-
out->setImplInfo(StorageImplInfo::getComputed(isMutable));
5614+
out->setImplInfo(
5615+
accessors[0]->getAccessorKind() == AccessorKind::Address
5616+
? (accessors[1] ? StorageImplInfo(ReadImplKind::Address,
5617+
WriteImplKind::MutableAddress,
5618+
ReadWriteImplKind::MutableAddress)
5619+
: StorageImplInfo(ReadImplKind::Address))
5620+
: StorageImplInfo::getComputed(isMutable));
55375621
out->setIsSetterMutating(true);
55385622
return out;
55395623
}

‎test/Interop/Cxx/class/move-only/Inputs/move-only-cxx-value-type.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,4 +41,16 @@ struct NonCopyableHolder {
4141
NonCopyable x;
4242
};
4343

44+
struct NonCopyableHolderDerived: NonCopyableHolder {
45+
inline NonCopyableHolderDerived(int x) : NonCopyableHolder(x) {}
46+
};
47+
48+
struct NonCopyableHolderDerivedDerived: NonCopyableHolderDerived {
49+
inline NonCopyableHolderDerivedDerived(int x) : NonCopyableHolderDerived(x) {}
50+
51+
inline int getActualX() const {
52+
return x.x;
53+
}
54+
};
55+
4456
#endif // TEST_INTEROP_CXX_CLASS_MOVE_ONLY_VT_H

‎test/Interop/Cxx/class/move-only/inherited-field-access-silgen.swift

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// RUN: %target-swift-emit-sil -I %S/Inputs -enable-experimental-cxx-interop %s -validate-tbd-against-ir=none | %FileCheck %s
2+
3+
import MoveOnlyCxxValueType
4+
5+
func testGetX() -> CInt {
6+
let derived = NonCopyableHolderDerivedDerived(42)
7+
return derived.x.x
8+
}
9+
10+
let _ = testGetX()
11+
12+
func testSetX(_ x: CInt) {
13+
var derived = NonCopyableHolderDerivedDerived(42)
14+
derived.x.x = 2
15+
}
16+
17+
testSetX(2)
18+
19+
// CHECK: sil shared [transparent] @$sSo024NonCopyableHolderDerivedD0V1xSo0aB0Vvlu : $@convention(method) (@in_guaranteed NonCopyableHolderDerivedDerived) -> UnsafePointer<NonCopyable>
20+
// CHECK: {{.*}}(%[[SELF_VAL:.*]] : $*NonCopyableHolderDerivedDerived):
21+
// CHECK: function_ref @{{.*}}__synthesizedBaseCall___synthesizedBaseGetterAccessor{{.*}} : $@convention(cxx_method) (@in_guaranteed NonCopyableHolderDerivedDerived) -> UnsafePointer<NonCopyable>
22+
// CHECK-NEXT: apply %{{.*}}(%[[SELF_VAL]])
23+
24+
// CHECK: sil shared [transparent] @$sSo024NonCopyableHolderDerivedD0V1xSo0aB0Vvau : $@convention(method) (@inout NonCopyableHolderDerivedDerived) -> UnsafeMutablePointer<NonCopyable>
25+
// CHECK: function_ref @{{.*}}__synthesizedBaseCall___synthesizedBaseSetterAccessor{{.*}} : $@convention(cxx_method) (@inout NonCopyableHolderDerivedDerived) -> UnsafeMutablePointer<NonCopyable>

‎test/Interop/Cxx/class/move-only/move-only-cxx-value-type.swift

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,24 @@ MoveOnlyCxxValueType.test("Test move only field access in holder") {
4848
k = inoutNC(&c.x, 7)
4949
expectEqual(k, 7)
5050
expectEqual(c.x.x, 7)
51+
c.x.x = 78
52+
expectEqual(c.x.x, 78)
53+
c.x.mutMethod(5)
54+
expectEqual(c.x.x, 5)
55+
}
56+
57+
MoveOnlyCxxValueType.test("Test move only field access in derived holder") {
58+
var c = NonCopyableHolderDerivedDerived(-11)
59+
var k = borrowNC(c.x)
60+
expectEqual(k, -33)
61+
expectEqual(c.getActualX(), -11)
62+
k = inoutNC(&c.x, 7)
63+
expectEqual(k, 7)
64+
expectEqual(c.x.x, 7)
65+
c.x.x = 78
66+
expectEqual(c.x.x, 78)
67+
c.x.mutMethod(5)
68+
expectEqual(c.x.x, 5)
5169
}
5270

5371
runAllTests()

0 commit comments

Comments
 (0)