[Cxx iterop] Disambiguate between methods with the same name but different constness

Author: Huddie

lib/ClangImporter/ImportDecl.cpp

@@ -3438,9 +3438,29 @@ namespace {
       SmallVector<FuncDecl *, 4> methods;
       SmallVector<ConstructorDecl *, 4> ctors;
 
+      // Cxx methods may have the same name but differ in "constness".
+      // In such a case we must differentiate in swift (See VisitFunction).
+      // Before importing the different CXXMethodDecl's we track functions
+      // that differ this way so we can disambiguate later
+      for (auto m : decl->decls()) {
+        if (auto method = dyn_cast<clang::CXXMethodDecl>(m)) {
+          if(method->getDeclName().isIdentifier()) {
+            if(Impl.cxxMethods.find(method->getName()) == Impl.cxxMethods.end()) {
+              Impl.cxxMethods[method->getName()] = {};
+            }
+            if(method->isConst()) {
+              // Add to const set
+              Impl.cxxMethods[method->getName()].first.insert(method);
+            } else {
+              // Add to mutable set
+              Impl.cxxMethods[method->getName()].second.insert(method);
+            }
+          }
+        }
+      }
+
       // FIXME: Import anonymous union fields and support field access when
       // it is nested in a struct.
-
       for (auto m : decl->decls()) {
         if (isa<clang::AccessSpecDecl>(m)) {
           // The presence of AccessSpecDecls themselves does not influence
@@ -3983,6 +4003,25 @@ namespace {
       if (!dc)
         return nullptr;
 
+      // Handle cases where 2 CXX methods differ strictly in "constness"
+      // In such a case append a suffix ("Mutating") to the mutable version
+      // of the method when importing to swift
+      if(decl->getDeclName().isIdentifier()) {
+        const auto &cxxMethodPair = Impl.cxxMethods[decl->getName()];
+        const auto &constFuncPtrs = cxxMethodPair.first;
+        const auto &mutFuncPtrs = cxxMethodPair.second;
+
+        // Check to see if this function has both const & mut versions and
+        // that this decl refers to the mutable version.
+        if (!constFuncPtrs.empty() && mutFuncPtrs.contains(decl)) {
+          auto newName = decl->getName().str() + "Mutating";
+          auto newId = dc->getASTContext().getIdentifier(newName);
+          auto oldArgNames = importedName.getDeclName().getArgumentNames();
+          auto newDeclName = DeclName(Impl.SwiftContext, newId, oldArgNames);
+          importedName.setDeclName(newDeclName);
+        }
+      }
+
       DeclName name = accessorInfo ? DeclName() : importedName.getDeclName();
       auto selfIdx = importedName.getSelfIndex();
 

lib/ClangImporter/ImporterImpl.h

@@ -569,6 +569,10 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   llvm::MapVector<std::pair<NominalTypeDecl *, Type>,
                   std::pair<FuncDecl *, FuncDecl *>> cxxSubscripts;
 
+  /// Keep track of cxx function names, params etc in order to
+  /// allow for de-duping functions that differ strictly on "constness".
+  llvm::DenseMap<llvm::StringRef, std::pair<llvm::DenseSet<clang::FunctionDecl *>, llvm::DenseSet<clang::FunctionDecl *>>> cxxMethods;
+
   /// Keeps track of the Clang functions that have been turned into
   /// properties.
   llvm::DenseMap<const clang::FunctionDecl *, VarDecl *> FunctionsAsProperties;

test/Interop/Cxx/class/method/Inputs/ambiguous_methods.h

@@ -0,0 +1,48 @@
+#ifndef TEST_INTEROP_CXX_CLASS_AMBIGUOUS_METHOD_METHODS_H
+#define TEST_INTEROP_CXX_CLASS_AMBIGUOUS_METHOD_METHODS_H
+
+struct HasAmbiguousMethods {
+
+  // No input
+  void ping() { ++mutableMethodsCalledCount; }
+  void ping() const {}
+
+  // One input
+  int increment(int a) {
+    ++mutableMethodsCalledCount;
+    return a + 1;
+  }
+
+  int increment(int a) const {
+    return a + 1;
+  }
+
+  // Multiple input with out param
+  void increment(int a, int b, int &c) {
+    ++mutableMethodsCalledCount;
+    c = a + b;
+  }
+
+  void increment(int a, int b, int &c) const {
+    c = a + b;
+  }
+
+  // Multiple input with inout param
+  void increment(int &a, int b) {
+    ++mutableMethodsCalledCount;
+    a += b;
+  }
+
+  void increment(int &a, int b) const {
+    a += b;
+  }
+
+  // No input with output
+  int numberOfMutableMethodsCalled() { return ++mutableMethodsCalledCount; }
+  int numberOfMutableMethodsCalled() const { return mutableMethodsCalledCount; }
+
+private:
+  int mutableMethodsCalledCount = 0;
+};
+
+#endif // TEST_INTEROP_CXX_CLASS_AMBIGUOUS_METHOD_METHODS_H

test/Interop/Cxx/class/method/Inputs/methods.h

@@ -25,9 +25,6 @@ struct HasMethods {
 
   NonTrivialInWrapper nonConstPassThroughAsWrapper(int a) { return {a}; }
   NonTrivialInWrapper constPassThroughAsWrapper(int a) const { return {a}; }
-
-  int increment(int a) { return a + 1; }
-  int increment(int a) const { return a + 1; }
 };
 
 #endif // TEST_INTEROP_CXX_CLASS_METHOD_METHODS_H

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

@@ -1,4 +1,9 @@
 module Methods {
   header "methods.h"
   requires cplusplus
+}
+
+module AmbiguousMethods {
+  header "ambiguous_methods.h"
+  requires cplusplus
 }

test/Interop/Cxx/class/method/ambiguous-methods-module-interface.swift

@@ -0,0 +1,16 @@
+// RUN: %target-swift-ide-test -print-module -module-to-print=AmbiguousMethods -I %S/Inputs -source-filename=x -enable-cxx-interop | %FileCheck %s
+
+// CHECK: mutating func pingMutating()
+// CHECK: func ping()
+
+// CHECK: mutating func incrementMutating(_ a: Int32) -> Int32
+// CHECK: func increment(_ a: Int32) -> Int32
+
+// CHECK: mutating func incrementMutating(_ a: Int32, _ b: Int32, _ c: inout Int32)
+// CHECK: func increment(_ a: Int32, _ b: Int32, _ c: inout Int32)
+
+// CHECK: mutating func incrementMutating(_ a: inout Int32, _ b: Int32)
+// CHECK: func increment(_ a: inout Int32, _ b: Int32)
+
+// CHECK: mutating func numberOfMutableMethodsCalledMutating() -> Int32
+// CHECK: func numberOfMutableMethodsCalled() -> Int32

test/Interop/Cxx/class/method/ambiguous-methods.swift

@@ -0,0 +1,80 @@
+// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -enable-cxx-interop)
+//
+// REQUIRES: executable_test
+//
+// Crash when running on windows: rdar://88391102
+// XFAIL: OS=windows-msvc
+
+import StdlibUnittest
+import AmbiguousMethods
+
+var CxxAmbiguousMethodTestSuite = TestSuite("CxxAmbiguousMethods")
+
+CxxAmbiguousMethodTestSuite.test("numberOfMutableMethodsCalled: () -> Int") {
+  var instance = HasAmbiguousMethods()
+
+  // Sanity check. Make sure we start at 0
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  // Make sure calling numberOfMutableMethodsCalled above didn't
+  // change the count
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  // Check that mutable version _does_ change the mutable call count
+  expectEqual(1, instance.numberOfMutableMethodsCalledMutating())
+
+  expectEqual(1, instance.numberOfMutableMethodsCalled())
+}
+
+CxxAmbiguousMethodTestSuite.test("Basic Increment: (Int) -> Int") {
+  var instance = HasAmbiguousMethods()
+  var a: Int32 = 0
+
+  // Sanity check. Make sure we start at 0
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  // Non mutable version should NOT change count
+  a = instance.increment(a);
+  expectEqual(1, a)
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  a = instance.incrementMutating(a);
+  expectEqual(2, a)
+  expectEqual(1, instance.numberOfMutableMethodsCalled())
+}
+
+CxxAmbiguousMethodTestSuite.test("Out Param Increment: (Int, Int, inout Int) -> Void") {
+  var instance = HasAmbiguousMethods()
+  var out: Int32 = 0
+
+  // Sanity check. Make sure we start at 0
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  // Non mutable version should NOT change count
+  instance.increment(0, 1, &out);
+  expectEqual(1, out)
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  instance.incrementMutating(5, 2, &out);
+  expectEqual(7, out)
+  expectEqual(1, instance.numberOfMutableMethodsCalled())
+}
+
+CxxAmbiguousMethodTestSuite.test("Inout Param Increment: (inout Int, Int) -> Void") {
+  var instance = HasAmbiguousMethods()
+  var inoutVal: Int32 = 0
+
+  // Sanity check. Make sure we start at 0
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  // Non mutable version should NOT change count
+  instance.increment(&inoutVal, 1);
+  expectEqual(1, inoutVal)
+  expectEqual(0, instance.numberOfMutableMethodsCalled())
+
+  instance.incrementMutating(&inoutVal, 2);
+  expectEqual(3, inoutVal)
+  expectEqual(1, instance.numberOfMutableMethodsCalled())
+}
+
+runAllTests()

test/Interop/Cxx/class/method/methods-module-interface.swift

@@ -16,10 +16,4 @@
 // CHECK: func constSumAsWrapper(_ a: NonTrivialInWrapper, _ b: NonTrivialInWrapper) -> NonTrivialInWrapper
 
 // CHECK: mutating func nonConstPassThroughAsWrapper(_ a: Int32) -> NonTrivialInWrapper
-// CHECK: func constPassThroughAsWrapper(_ a: Int32) -> NonTrivialInWrapper
-
-// CHECK: mutating func incrementMutable(_ a: Int32) -> Int32
-// CHECK: func incrementMutable(_ a: Int32) -> Int32
-
-// CHECK: const func increment(_ a: Int32) -> Int32
-// CHECK: func increment(_ a: Int32) -> Int32
+// CHECK: func constPassThroughAsWrapper(_ a: Int32) -> NonTrivialInWrapper