C++ Interop: import const methods as non-mutating

include/swift/AST/ClangModuleLoader.h

@@ -236,6 +236,8 @@ class ClangModuleLoader : public ModuleLoader {
   instantiateCXXFunctionTemplate(ASTContext &ctx,
                                  clang::FunctionTemplateDecl *func,
                                  SubstitutionMap subst) = 0;
+
+  virtual bool isCXXMethodMutating(const clang::CXXMethodDecl *method) = 0;
 };
 
 /// Describes a C++ template instantiation error.

include/swift/ClangImporter/ClangImporter.h

@@ -486,6 +486,8 @@ class ClangImporter final : public ClangModuleLoader {
   instantiateCXXFunctionTemplate(ASTContext &ctx,
                                  clang::FunctionTemplateDecl *func,
                                  SubstitutionMap subst) override;
+
+  bool isCXXMethodMutating(const clang::CXXMethodDecl *method) override;
 };
 
 ImportDecl *createImportDecl(ASTContext &Ctx, DeclContext *DC, ClangNode ClangN,

lib/ClangImporter/ClangImporter.cpp

@@ -4322,3 +4322,8 @@ ClangImporter::instantiateCXXClassTemplate(
   return dyn_cast_or_null<StructDecl>(
       Impl.importDecl(ctsd, Impl.CurrentVersion));
 }
+
+bool ClangImporter::isCXXMethodMutating(const clang::CXXMethodDecl *method) {
+  return isa<clang::CXXConstructorDecl>(method) || !method->isConst() ||
+         method->getParent()->hasMutableFields();
+}

lib/ClangImporter/ImportDecl.cpp

@@ -4177,10 +4177,10 @@ namespace {
             selfIdx = None;
           } else {
             selfIdx = 0;
-            // Workaround until proper const support is handled: Force
-            // everything to be mutating. This implicitly makes the parameter
-            // indirect.
-            selfIsInOut = true;
+            // If the method is imported as mutating, this implicitly makes the
+            // parameter indirect.
+            selfIsInOut = Impl.SwiftContext.getClangModuleLoader()
+                              ->isCXXMethodMutating(mdecl);
           }
         }
       }
@@ -7589,23 +7589,24 @@ SwiftDeclConverter::importAccessor(const clang::ObjCMethodDecl *clangAccessor,
   return accessor;
 }
 
-static InOutExpr *
-createInOutSelfExpr(AccessorDecl *accessorDecl) {
+static Expr *createSelfExpr(AccessorDecl *accessorDecl) {
   ASTContext &ctx = accessorDecl->getASTContext();
 
-  auto inoutSelfDecl = accessorDecl->getImplicitSelfDecl();
-  auto inoutSelfRefExpr =
-      new (ctx) DeclRefExpr(inoutSelfDecl, DeclNameLoc(),
-                            /*implicit=*/ true);
-  inoutSelfRefExpr->setType(LValueType::get(inoutSelfDecl->getInterfaceType()));
-
-  auto inoutSelfExpr =
-      new (ctx) InOutExpr(SourceLoc(),
-                          inoutSelfRefExpr,
-                          accessorDecl->mapTypeIntoContext(
-                              inoutSelfDecl->getValueInterfaceType()),
-                          /*isImplicit=*/ true);
-  inoutSelfExpr->setType(InOutType::get(inoutSelfDecl->getInterfaceType()));
+  auto selfDecl = accessorDecl->getImplicitSelfDecl();
+  auto selfRefExpr = new (ctx) DeclRefExpr(selfDecl, DeclNameLoc(),
+                                           /*implicit*/ true);
+
+  if (!accessorDecl->isMutating()) {
+    selfRefExpr->setType(selfDecl->getInterfaceType());
+    return selfRefExpr;
+  }
+  selfRefExpr->setType(LValueType::get(selfDecl->getInterfaceType()));
+
+  auto inoutSelfExpr = new (ctx) InOutExpr(
+      SourceLoc(), selfRefExpr,
+      accessorDecl->mapTypeIntoContext(selfDecl->getValueInterfaceType()),
+      /*isImplicit*/ true);
+  inoutSelfExpr->setType(InOutType::get(selfDecl->getInterfaceType()));
   return inoutSelfExpr;
 }
 
@@ -7623,7 +7624,7 @@ createParamRefExpr(AccessorDecl *accessorDecl, unsigned index) {
 
 static CallExpr *
 createAccessorImplCallExpr(FuncDecl *accessorImpl,
-                           InOutExpr *inoutSelfExpr,
+                           Expr *selfExpr,
                            DeclRefExpr *keyRefExpr) {
   ASTContext &ctx = accessorImpl->getASTContext();
 
@@ -7634,9 +7635,7 @@ createAccessorImplCallExpr(FuncDecl *accessorImpl,
   accessorImplExpr->setType(accessorImpl->getInterfaceType());
 
   auto accessorImplDotCallExpr =
-      new (ctx) DotSyntaxCallExpr(accessorImplExpr,
-                                  SourceLoc(),
-                                  inoutSelfExpr);
+      new (ctx) DotSyntaxCallExpr(accessorImplExpr, SourceLoc(), selfExpr);
   accessorImplDotCallExpr->setType(accessorImpl->getMethodInterfaceType());
   accessorImplDotCallExpr->setThrows(false);
 
@@ -7656,14 +7655,13 @@ synthesizeSubscriptGetterBody(AbstractFunctionDecl *afd, void *context) {
 
   ASTContext &ctx = getterDecl->getASTContext();
 
-  InOutExpr *inoutSelfExpr = createInOutSelfExpr(getterDecl);
+  Expr *selfExpr = createSelfExpr(getterDecl);
   DeclRefExpr *keyRefExpr = createParamRefExpr(getterDecl, 0);
 
   Type elementTy = getterDecl->getResultInterfaceType();
 
-  auto *getterImplCallExpr = createAccessorImplCallExpr(getterImpl,
-                                                        inoutSelfExpr,
-                                                        keyRefExpr);
+  auto *getterImplCallExpr =
+      createAccessorImplCallExpr(getterImpl, selfExpr, keyRefExpr);
 
   // This default handles C++'s operator[] that returns a value type.
   Expr *propertyExpr = getterImplCallExpr;
@@ -7709,14 +7707,13 @@ synthesizeSubscriptSetterBody(AbstractFunctionDecl *afd, void *context) {
 
   ASTContext &ctx = setterDecl->getASTContext();
 
-  InOutExpr *inoutSelfExpr = createInOutSelfExpr(setterDecl);
+  Expr *selfExpr = createSelfExpr(setterDecl);
   DeclRefExpr *valueParamRefExpr = createParamRefExpr(setterDecl, 0);
   DeclRefExpr *keyParamRefExpr = createParamRefExpr(setterDecl, 1);
 
   Type elementTy = valueParamRefExpr->getDecl()->getInterfaceType();
 
-  auto *setterImplCallExpr = createAccessorImplCallExpr(setterImpl,
-                                                        inoutSelfExpr,
+  auto *setterImplCallExpr = createAccessorImplCallExpr(setterImpl, selfExpr,
                                                         keyParamRefExpr);
 
   VarDecl *pointeePropertyDecl = ctx.getPointerPointeePropertyDecl(PTK_UnsafeMutablePointer);

lib/ClangImporter/ImportType.cpp

@@ -1880,9 +1880,12 @@ ParameterList *ClangImporter::Implementation::importFunctionParameterList(
 
       param->setInterfaceType(parentType.getType());
 
-      // Workaround until proper const support is handled: Force everything to
-      // be mutating. This implicitly makes the parameter indirect.
-      param->setSpecifier(ParamSpecifier::InOut);
+      if (SwiftContext.getClangModuleLoader()->isCXXMethodMutating(CMD)) {
+        // This implicitly makes the parameter indirect.
+        param->setSpecifier(ParamSpecifier::InOut);
+      } else {
+        param->setSpecifier(ParamSpecifier::Default);
+      }
 
       parameters.push_back(param);
     }

lib/SIL/IR/SILFunctionType.cpp

@@ -2846,18 +2846,31 @@ class CFunctionConventions : public CFunctionTypeConventions {
 class CXXMethodConventions : public CFunctionTypeConventions {
   using super = CFunctionTypeConventions;
   const clang::CXXMethodDecl *TheDecl;
+  bool isMutating;
 
 public:
-  CXXMethodConventions(const clang::CXXMethodDecl *decl)
+  CXXMethodConventions(const clang::CXXMethodDecl *decl, bool isMutating)
       : CFunctionTypeConventions(
             ConventionsKind::CXXMethod,
             decl->getType()->castAs<clang::FunctionType>()),
-        TheDecl(decl) {}
+        TheDecl(decl), isMutating(isMutating) {}
   ParameterConvention
   getIndirectSelfParameter(const AbstractionPattern &type) const override {
-    if (TheDecl->isConst())
+    llvm_unreachable(
+        "cxx functions do not have a Swift self parameter; "
+        "foreign self parameter is handled in getIndirectParameter");
+  }
+
+  ParameterConvention
+  getIndirectParameter(unsigned int index, const AbstractionPattern &type,
+                       const TypeLowering &substTL) const override {
+    // `self` is the last parameter.
+    if (index == TheDecl->getNumParams()) {
+      if (isMutating)
+        return ParameterConvention::Indirect_Inout;
       return ParameterConvention::Indirect_In_Guaranteed;
-    return ParameterConvention::Indirect_Inout;
+    }
+    return super::getIndirectParameter(index, type, substTL);
   }
   ResultConvention getResult(const TypeLowering &resultTL) const override {
     if (isa<clang::CXXConstructorDecl>(TheDecl)) {
@@ -2909,7 +2922,9 @@ static CanSILFunctionType getSILFunctionTypeForClangDecl(
     AbstractionPattern origPattern = method->isOverloadedOperator() ?
         AbstractionPattern::getCXXOperatorMethod(origType, method, foreignInfo.Self):
         AbstractionPattern::getCXXMethod(origType, method, foreignInfo.Self);
-    auto conventions = CXXMethodConventions(method);
+    bool isMutating =
+        TC.Context.getClangModuleLoader()->isCXXMethodMutating(method);
+    auto conventions = CXXMethodConventions(method, isMutating);
     return getSILFunctionType(TC, TypeExpansionContext::minimal(), origPattern,
                               substInterfaceType, extInfoBuilder, conventions,
                               foreignInfo, constant, constant, None,

test/ClangImporter/cxx_interop_ir.swift

@@ -1,4 +1,7 @@
 // RUN: %target-swiftxx-frontend -module-name cxx_ir -I %S/Inputs/custom-modules -emit-ir -o - -primary-file %s | %FileCheck %s
+//
+// We can't yet call member functions correctly on Windows (SR-13129).
+// XFAIL: OS=windows-msvc
 
 import CXXInterop
 
@@ -40,9 +43,10 @@ func basicMethods(a: UnsafeMutablePointer<Methods>) -> Int32 {
 }
 
 // CHECK-LABEL: define hidden swiftcc i32 @"$s6cxx_ir17basicMethodsConst1as5Int32VSpySo0D0VG_tF"(i8* %0)
-// CHECK: [[THIS_PTR1:%.*]] = bitcast i8* %0 to %TSo7MethodsV*
+// CHECK: [[THIS_PTR1:%.*]] = alloca %TSo7MethodsV, align 8
 // CHECK: [[THIS_PTR2:%.*]] = bitcast %TSo7MethodsV* [[THIS_PTR1]] to %class.Methods*
-// CHECK: [[RESULT:%.*]] = call {{(signext )?}}i32 @{{_ZNK7Methods17SimpleConstMethodEi|"\?SimpleConstMethod@Methods@@QEBAHH@Z"}}(%class.Methods* [[THIS_PTR2]], i32{{( signext)?}} 3)
+// CHECK: [[THIS_PTR3:%.*]] = bitcast %TSo7MethodsV* [[THIS_PTR1]] to %class.Methods*
+// CHECK: [[RESULT:%.*]] = call {{(signext )?}}i32 @{{_ZNK7Methods17SimpleConstMethodEi|"\?SimpleConstMethod@Methods@@QEBAHH@Z"}}(%class.Methods* [[THIS_PTR3]], i32{{( signext)?}} 3)
 // CHECK: ret i32 [[RESULT]]
 func basicMethodsConst(a: UnsafeMutablePointer<Methods>) -> Int32 {
   return a.pointee.SimpleConstMethod(3)

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

@@ -48,6 +48,11 @@ module MemberVariables {
   requires cplusplus
 }
 
+module MutableMembers {
+  header "mutable-members.h"
+  requires cplusplus
+}
+
 module ProtocolConformance {
   header "protocol-conformance.h"
   requires cplusplus

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

@@ -0,0 +1,21 @@
+#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_MUTABLE_MEMBERS_H
+#define TEST_INTEROP_CXX_CLASS_INPUTS_MUTABLE_MEMBERS_H
+
+struct HasPublicMutableMember {
+  mutable int a = 0;
+
+  int foo() const {
+    a++;
+    return a;
+  }
+};
+
+struct HasPrivateMutableMember {
+private:
+  mutable int a = 0;
+
+public:
+  void bar() const { a++; }
+};
+
+#endif // TEST_INTEROP_CXX_CLASS_INPUTS_MUTABLE_MEMBERS_H

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

@@ -0,0 +1,10 @@
+// RUN: %target-swift-ide-test -print-module -module-to-print=MutableMembers -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s
+
+// CHECK: struct HasPublicMutableMember {
+// CHECK:   var a: Int32
+// CHECK:   mutating func foo() -> Int32
+// CHECK: }
+
+// CHECK: struct HasPrivateMutableMember {
+// CHECK:   mutating func bar()
+// CHECK: }

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

@@ -0,0 +1,6 @@
+// RUN: %target-typecheck-verify-swift -I %S/Inputs -enable-cxx-interop
+
+import MutableMembers
+
+let obj = HasPublicMutableMember(a: 42) // expected-note {{change 'let' to 'var' to make it mutable}}
+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

@@ -0,0 +1,16 @@
+// RUN: %target-run-simple-swift(-I %S/Inputs -Xfrontend -enable-cxx-interop)
+//
+// REQUIRES: executable_test
+
+import StdlibUnittest
+import MutableMembers
+
+var MembersTestSuite = TestSuite("MembersTestSuite")
+
+MembersTestSuite.test("MutableMembers") {
+  var obj = HasPublicMutableMember(a: 1)
+  expectEqual(obj.foo(), 2)
+  expectEqual(obj.foo(), 3)
+}
+
+runAllTests()