Skip to content

Commit a8f126f

Browse files
egorzhdanegorzhdan
committed
C++ Interop: import const methods as non-mutating
This change makes ClangImporter import some C++ member functions as non-mutating, given that they satisfy two requirements: * the function itself is marked as `const` * the parent struct doesn't contain any `mutable` members `get` accessors of subscript operators are now also imported as non-mutating if the C++ `operator[]` satisfies the requirements above. Fixes SR-12795.
1 parent 03ff464 commit a8f126f

23 files changed

+200
-128
lines changed

‎include/swift/AST/ClangModuleLoader.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,8 @@ class ClangModuleLoader : public ModuleLoader {
236236
instantiateCXXFunctionTemplate(ASTContext &ctx,
237237
clang::FunctionTemplateDecl *func,
238238
SubstitutionMap subst) = 0;
239+
240+
virtual bool isCXXMethodMutating(const clang::CXXMethodDecl *method) = 0;
239241
};
240242

241243
/// Describes a C++ template instantiation error.

‎include/swift/ClangImporter/ClangImporter.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,8 @@ class ClangImporter final : public ClangModuleLoader {
486486
instantiateCXXFunctionTemplate(ASTContext &ctx,
487487
clang::FunctionTemplateDecl *func,
488488
SubstitutionMap subst) override;
489+
490+
bool isCXXMethodMutating(const clang::CXXMethodDecl *method) override;
489491
};
490492

491493
ImportDecl *createImportDecl(ASTContext &Ctx, DeclContext *DC, ClangNode ClangN,

‎lib/ClangImporter/ClangImporter.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4322,3 +4322,8 @@ ClangImporter::instantiateCXXClassTemplate(
43224322
return dyn_cast_or_null<StructDecl>(
43234323
Impl.importDecl(ctsd, Impl.CurrentVersion));
43244324
}
4325+
4326+
bool ClangImporter::isCXXMethodMutating(const clang::CXXMethodDecl *method) {
4327+
return isa<clang::CXXConstructorDecl>(method) || !method->isConst() ||
4328+
method->getParent()->hasMutableFields();
4329+
}

‎lib/ClangImporter/ImportDecl.cpp

Lines changed: 27 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4177,10 +4177,10 @@ namespace {
41774177
selfIdx = None;
41784178
} else {
41794179
selfIdx = 0;
4180-
// Workaround until proper const support is handled: Force
4181-
// everything to be mutating. This implicitly makes the parameter
4182-
// indirect.
4183-
selfIsInOut = true;
4180+
// If the method is imported as mutating, this implicitly makes the
4181+
// parameter indirect.
4182+
selfIsInOut = Impl.SwiftContext.getClangModuleLoader()
4183+
->isCXXMethodMutating(mdecl);
41844184
}
41854185
}
41864186
}
@@ -7589,23 +7589,24 @@ SwiftDeclConverter::importAccessor(const clang::ObjCMethodDecl *clangAccessor,
75897589
return accessor;
75907590
}
75917591

7592-
static InOutExpr *
7593-
createInOutSelfExpr(AccessorDecl *accessorDecl) {
7592+
static Expr *createSelfExpr(AccessorDecl *accessorDecl) {
75947593
ASTContext &ctx = accessorDecl->getASTContext();
75957594

7596-
auto inoutSelfDecl = accessorDecl->getImplicitSelfDecl();
7597-
auto inoutSelfRefExpr =
7598-
new (ctx) DeclRefExpr(inoutSelfDecl, DeclNameLoc(),
7599-
/*implicit=*/ true);
7600-
inoutSelfRefExpr->setType(LValueType::get(inoutSelfDecl->getInterfaceType()));
7601-
7602-
auto inoutSelfExpr =
7603-
new (ctx) InOutExpr(SourceLoc(),
7604-
inoutSelfRefExpr,
7605-
accessorDecl->mapTypeIntoContext(
7606-
inoutSelfDecl->getValueInterfaceType()),
7607-
/*isImplicit=*/ true);
7608-
inoutSelfExpr->setType(InOutType::get(inoutSelfDecl->getInterfaceType()));
7595+
auto selfDecl = accessorDecl->getImplicitSelfDecl();
7596+
auto selfRefExpr = new (ctx) DeclRefExpr(selfDecl, DeclNameLoc(),
7597+
/*implicit*/ true);
7598+
7599+
if (!accessorDecl->isMutating()) {
7600+
selfRefExpr->setType(selfDecl->getInterfaceType());
7601+
return selfRefExpr;
7602+
}
7603+
selfRefExpr->setType(LValueType::get(selfDecl->getInterfaceType()));
7604+
7605+
auto inoutSelfExpr = new (ctx) InOutExpr(
7606+
SourceLoc(), selfRefExpr,
7607+
accessorDecl->mapTypeIntoContext(selfDecl->getValueInterfaceType()),
7608+
/*isImplicit*/ true);
7609+
inoutSelfExpr->setType(InOutType::get(selfDecl->getInterfaceType()));
76097610
return inoutSelfExpr;
76107611
}
76117612

@@ -7623,7 +7624,7 @@ createParamRefExpr(AccessorDecl *accessorDecl, unsigned index) {
76237624

76247625
static CallExpr *
76257626
createAccessorImplCallExpr(FuncDecl *accessorImpl,
7626-
InOutExpr *inoutSelfExpr,
7627+
Expr *selfExpr,
76277628
DeclRefExpr *keyRefExpr) {
76287629
ASTContext &ctx = accessorImpl->getASTContext();
76297630

@@ -7634,9 +7635,7 @@ createAccessorImplCallExpr(FuncDecl *accessorImpl,
76347635
accessorImplExpr->setType(accessorImpl->getInterfaceType());
76357636

76367637
auto accessorImplDotCallExpr =
7637-
new (ctx) DotSyntaxCallExpr(accessorImplExpr,
7638-
SourceLoc(),
7639-
inoutSelfExpr);
7638+
new (ctx) DotSyntaxCallExpr(accessorImplExpr, SourceLoc(), selfExpr);
76407639
accessorImplDotCallExpr->setType(accessorImpl->getMethodInterfaceType());
76417640
accessorImplDotCallExpr->setThrows(false);
76427641

@@ -7656,14 +7655,13 @@ synthesizeSubscriptGetterBody(AbstractFunctionDecl *afd, void *context) {
76567655

76577656
ASTContext &ctx = getterDecl->getASTContext();
76587657

7659-
InOutExpr *inoutSelfExpr = createInOutSelfExpr(getterDecl);
7658+
Expr *selfExpr = createSelfExpr(getterDecl);
76607659
DeclRefExpr *keyRefExpr = createParamRefExpr(getterDecl, 0);
76617660

76627661
Type elementTy = getterDecl->getResultInterfaceType();
76637662

7664-
auto *getterImplCallExpr = createAccessorImplCallExpr(getterImpl,
7665-
inoutSelfExpr,
7666-
keyRefExpr);
7663+
auto *getterImplCallExpr =
7664+
createAccessorImplCallExpr(getterImpl, selfExpr, keyRefExpr);
76677665

76687666
// This default handles C++'s operator[] that returns a value type.
76697667
Expr *propertyExpr = getterImplCallExpr;
@@ -7709,14 +7707,13 @@ synthesizeSubscriptSetterBody(AbstractFunctionDecl *afd, void *context) {
77097707

77107708
ASTContext &ctx = setterDecl->getASTContext();
77117709

7712-
InOutExpr *inoutSelfExpr = createInOutSelfExpr(setterDecl);
7710+
Expr *selfExpr = createSelfExpr(setterDecl);
77137711
DeclRefExpr *valueParamRefExpr = createParamRefExpr(setterDecl, 0);
77147712
DeclRefExpr *keyParamRefExpr = createParamRefExpr(setterDecl, 1);
77157713

77167714
Type elementTy = valueParamRefExpr->getDecl()->getInterfaceType();
77177715

7718-
auto *setterImplCallExpr = createAccessorImplCallExpr(setterImpl,
7719-
inoutSelfExpr,
7716+
auto *setterImplCallExpr = createAccessorImplCallExpr(setterImpl, selfExpr,
77207717
keyParamRefExpr);
77217718

77227719
VarDecl *pointeePropertyDecl = ctx.getPointerPointeePropertyDecl(PTK_UnsafeMutablePointer);

‎lib/ClangImporter/ImportType.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1880,9 +1880,12 @@ ParameterList *ClangImporter::Implementation::importFunctionParameterList(
18801880

18811881
param->setInterfaceType(parentType.getType());
18821882

1883-
// Workaround until proper const support is handled: Force everything to
1884-
// be mutating. This implicitly makes the parameter indirect.
1885-
param->setSpecifier(ParamSpecifier::InOut);
1883+
if (SwiftContext.getClangModuleLoader()->isCXXMethodMutating(CMD)) {
1884+
// This implicitly makes the parameter indirect.
1885+
param->setSpecifier(ParamSpecifier::InOut);
1886+
} else {
1887+
param->setSpecifier(ParamSpecifier::Default);
1888+
}
18861889

18871890
parameters.push_back(param);
18881891
}

‎lib/SIL/IR/SILFunctionType.cpp

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2846,18 +2846,31 @@ class CFunctionConventions : public CFunctionTypeConventions {
28462846
class CXXMethodConventions : public CFunctionTypeConventions {
28472847
using super = CFunctionTypeConventions;
28482848
const clang::CXXMethodDecl *TheDecl;
2849+
bool isMutating;
28492850

28502851
public:
2851-
CXXMethodConventions(const clang::CXXMethodDecl *decl)
2852+
CXXMethodConventions(const clang::CXXMethodDecl *decl, bool isMutating)
28522853
: CFunctionTypeConventions(
28532854
ConventionsKind::CXXMethod,
28542855
decl->getType()->castAs<clang::FunctionType>()),
2855-
TheDecl(decl) {}
2856+
TheDecl(decl), isMutating(isMutating) {}
28562857
ParameterConvention
28572858
getIndirectSelfParameter(const AbstractionPattern &type) const override {
2858-
if (TheDecl->isConst())
2859+
llvm_unreachable(
2860+
"cxx functions do not have a Swift self parameter; "
2861+
"foreign self parameter is handled in getIndirectParameter");
2862+
}
2863+
2864+
ParameterConvention
2865+
getIndirectParameter(unsigned int index, const AbstractionPattern &type,
2866+
const TypeLowering &substTL) const override {
2867+
// `self` is the last parameter.
2868+
if (index == TheDecl->getNumParams()) {
2869+
if (isMutating)
2870+
return ParameterConvention::Indirect_Inout;
28592871
return ParameterConvention::Indirect_In_Guaranteed;
2860-
return ParameterConvention::Indirect_Inout;
2872+
}
2873+
return super::getIndirectParameter(index, type, substTL);
28612874
}
28622875
ResultConvention getResult(const TypeLowering &resultTL) const override {
28632876
if (isa<clang::CXXConstructorDecl>(TheDecl)) {
@@ -2909,7 +2922,9 @@ static CanSILFunctionType getSILFunctionTypeForClangDecl(
29092922
AbstractionPattern origPattern = method->isOverloadedOperator() ?
29102923
AbstractionPattern::getCXXOperatorMethod(origType, method, foreignInfo.Self):
29112924
AbstractionPattern::getCXXMethod(origType, method, foreignInfo.Self);
2912-
auto conventions = CXXMethodConventions(method);
2925+
bool isMutating =
2926+
TC.Context.getClangModuleLoader()->isCXXMethodMutating(method);
2927+
auto conventions = CXXMethodConventions(method, isMutating);
29132928
return getSILFunctionType(TC, TypeExpansionContext::minimal(), origPattern,
29142929
substInterfaceType, extInfoBuilder, conventions,
29152930
foreignInfo, constant, constant, None,

‎test/ClangImporter/cxx_interop_ir.swift

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
// RUN: %target-swiftxx-frontend -module-name cxx_ir -I %S/Inputs/custom-modules -emit-ir -o - -primary-file %s | %FileCheck %s
2+
//
3+
// We can't yet call member functions correctly on Windows (SR-13129).
4+
// XFAIL: OS=windows-msvc
25

36
import CXXInterop
47

@@ -40,9 +43,10 @@ func basicMethods(a: UnsafeMutablePointer<Methods>) -> Int32 {
4043
}
4144

4245
// CHECK-LABEL: define hidden swiftcc i32 @"$s6cxx_ir17basicMethodsConst1as5Int32VSpySo0D0VG_tF"(i8* %0)
43-
// CHECK: [[THIS_PTR1:%.*]] = bitcast i8* %0 to %TSo7MethodsV*
46+
// CHECK: [[THIS_PTR1:%.*]] = alloca %TSo7MethodsV, align 8
4447
// CHECK: [[THIS_PTR2:%.*]] = bitcast %TSo7MethodsV* [[THIS_PTR1]] to %class.Methods*
45-
// CHECK: [[RESULT:%.*]] = call {{(signext )?}}i32 @{{_ZNK7Methods17SimpleConstMethodEi|"\?SimpleConstMethod@Methods@@QEBAHH@Z"}}(%class.Methods* [[THIS_PTR2]], i32{{( signext)?}} 3)
48+
// CHECK: [[THIS_PTR3:%.*]] = bitcast %TSo7MethodsV* [[THIS_PTR1]] to %class.Methods*
49+
// CHECK: [[RESULT:%.*]] = call {{(signext )?}}i32 @{{_ZNK7Methods17SimpleConstMethodEi|"\?SimpleConstMethod@Methods@@QEBAHH@Z"}}(%class.Methods* [[THIS_PTR3]], i32{{( signext)?}} 3)
4650
// CHECK: ret i32 [[RESULT]]
4751
func basicMethodsConst(a: UnsafeMutablePointer<Methods>) -> Int32 {
4852
return a.pointee.SimpleConstMethod(3)

‎test/Interop/Cxx/class/Inputs/module.modulemap

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,11 @@ module MemberVariables {
4848
requires cplusplus
4949
}
5050

51+
module MutableMembers {
52+
header "mutable-members.h"
53+
requires cplusplus
54+
}
55+
5156
module ProtocolConformance {
5257
header "protocol-conformance.h"
5358
requires cplusplus

‎test/Interop/Cxx/class/Inputs/mutable-members.h

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_MUTABLE_MEMBERS_H
2+
#define TEST_INTEROP_CXX_CLASS_INPUTS_MUTABLE_MEMBERS_H
3+
4+
struct HasPublicMutableMember {
5+
mutable int a = 0;
6+
7+
int foo() const {
8+
a++;
9+
return a;
10+
}
11+
};
12+
13+
struct HasPrivateMutableMember {
14+
private:
15+
mutable int a = 0;
16+
17+
public:
18+
void bar() const { a++; }
19+
};
20+
21+
#endif // TEST_INTEROP_CXX_CLASS_INPUTS_MUTABLE_MEMBERS_H

‎test/Interop/Cxx/class/mutable-members-module-interface.swift

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// RUN: %target-swift-ide-test -print-module -module-to-print=MutableMembers -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s
2+
3+
// CHECK: struct HasPublicMutableMember {
4+
// CHECK: var a: Int32
5+
// CHECK: mutating func foo() -> Int32
6+
// CHECK: }
7+
8+
// CHECK: struct HasPrivateMutableMember {
9+
// CHECK: mutating func bar()
10+
// CHECK: }

‎test/Interop/Cxx/class/mutable-members-typechecker.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// RUN: %target-typecheck-verify-swift -I %S/Inputs -enable-cxx-interop
2+
3+
import MutableMembers
4+
5+
let obj = HasPublicMutableMember(a: 42) // expected-note {{change 'let' to 'var' to make it mutable}}
6+
let i = obj.foo() // expected-error {{cannot use mutating member on immutable value: 'obj' is a 'let' constant}}

‎test/Interop/Cxx/class/mutable-members.swift

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// RUN: %target-run-simple-swift(-I %S/Inputs -Xfrontend -enable-cxx-interop)
2+
//
3+
// REQUIRES: executable_test
4+
5+
import StdlibUnittest
6+
import MutableMembers
7+
8+
var MembersTestSuite = TestSuite("MembersTestSuite")
9+
10+
MembersTestSuite.test("MutableMembers") {
11+
var obj = HasPublicMutableMember(a: 1)
12+
expectEqual(obj.foo(), 2)
13+
expectEqual(obj.foo(), 3)
14+
}
15+
16+
runAllTests()

0 commit comments

Comments
 (0)