[cxx-interop] Disambiguate const and non-const methods consistently

When importing a C++ struct that contains two methods that only differ in const-ness, we append `Mutating` to the name of the non-const method to make it possible to call from Swift unambiguously.

Unfortunately that logic was dependent on the order in which we import methods of a class: the `Mutating` suffix was added when another method with the same name was already imported.

This caused lookup failures, and the behavior was incorrect when the pair of methods return instances of an unsafe type: the const overload was renamed as `Unsafe` properly, but the non-const overload was not renamed.

Author: egorzhdan

lib/ClangImporter/ImportDecl.cpp

@@ -2105,30 +2105,6 @@ namespace {
       // The name of every member.
       llvm::DenseSet<StringRef> allMemberNames;
 
-      // 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()) {
-            auto contextMap = Impl.cxxMethods.find(method->getDeclContext());
-            if (contextMap == Impl.cxxMethods.end() ||
-                contextMap->second.find(method->getName()) ==
-                    contextMap->second.end()) {
-              Impl.cxxMethods[method->getDeclContext()][method->getName()] = {};
-            }
-            if (method->isConst()) {
-              // Add to const set
-              Impl.cxxMethods[method->getDeclContext()][method->getName()].first.insert(method);
-            } else {
-              // Add to mutable set
-              Impl.cxxMethods[method->getDeclContext()][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()) {
@@ -2214,10 +2190,6 @@ namespace {
         }
 
         if (auto MD = dyn_cast<FuncDecl>(member)) {
-
-          // When 2 CXXMethods diff by "constness" alone we differentiate them
-          // by changing the name of one. That changed method needs to be added
-          // to the lookup table since it cannot be found lazily.
           if (auto cxxMethod = dyn_cast<clang::CXXMethodDecl>(m)) {
             auto cxxOperatorKind = cxxMethod->getOverloadedOperator();
 
@@ -2272,16 +2244,6 @@ namespace {
               // Make sure the synthesized decl can be found by lookupDirect.
               result->addMemberToLookupTable(opFuncDecl);
             }
-
-            if (cxxMethod->getDeclName().isIdentifier()) {
-              auto &mutableFuncPtrs =
-                  Impl.cxxMethods[cxxMethod->getDeclContext()]
-                                 [cxxMethod->getName()]
-                                     .second;
-              if (mutableFuncPtrs.contains(cxxMethod)) {
-                result->addMemberToLookupTable(member);
-              }
-            }
           }
           methods.push_back(MD);
           continue;
@@ -3031,25 +2993,6 @@ namespace {
         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->getDeclContext()][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/ImportName.cpp

@@ -2200,6 +2200,34 @@ ImportedName NameImporter::importNameImpl(const clang::NamedDecl *D,
     }
   }
 
+  SmallString<16> newName;
+  // Check if we need to rename the C++ method to disambiguate it.
+  if (auto method = dyn_cast<clang::CXXMethodDecl>(D)) {
+    if (!method->isConst() && !method->isOverloadedOperator()) {
+      // See if any other methods within the same struct have the same name, but
+      // differ in constness.
+      auto otherDecls = dc->lookup(method->getDeclName());
+      bool shouldRename = false;
+      for (auto otherDecl : otherDecls) {
+        if (otherDecl == D)
+          continue;
+        if (auto otherMethod = dyn_cast<clang::CXXMethodDecl>(otherDecl)) {
+          // TODO: what if the other method is also non-const?
+          if (otherMethod->isConst()) {
+            shouldRename = true;
+            break;
+          }
+        }
+      }
+
+      if (shouldRename) {
+        newName = baseName;
+        newName += "Mutating";
+        baseName = newName;
+      }
+    }
+  }
+
   // swift_newtype-ed declarations may have common words with the type name
   // stripped.
   if (auto newtypeDecl = findSwiftNewtype(D, clangSema, version)) {

lib/ClangImporter/ImporterImpl.h

@@ -614,14 +614,6 @@ 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<const clang::DeclContext *, llvm::DenseMap<llvm::StringRef,
-                 std::pair<
-                     llvm::DenseSet<clang::FunctionDecl *>,
-                     llvm::DenseSet<clang::FunctionDecl *>>>>
-      cxxMethods;
-
   // Keep track of the decls that were already cloned for this specific class.
   llvm::DenseMap<std::pair<ValueDecl *, DeclContext *>, ValueDecl *>
       clonedBaseMembers;

lib/PrintAsClang/experimental-interoperability-version.json

@@ -1 +1 @@
-10
+11

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

@@ -47,4 +47,13 @@ struct HasAmbiguousMethods2 {
   }
 };
 
+struct Unsafe {
+  int *ptr;
+};
+
+struct HasAmbiguousUnsafeMethods {
+  Unsafe getUnsafe() const { return Unsafe(); }
+  Unsafe getUnsafe() { return Unsafe(); }
+};
+
 #endif // TEST_INTEROP_CXX_CLASS_AMBIGUOUS_METHOD_METHODS_H

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

@@ -14,4 +14,9 @@
 
 // CHECK: struct HasAmbiguousMethods2
 // CHECK: func increment(_ a: Int32) -> Int32
-// CHECK-NOT: mutating func incrementMutating(_ a: Int32) -> Int32
+// CHECK-NOT: mutating func incrementMutating(_ a: Int32) -> Int32
+
+// CHECK: struct HasAmbiguousUnsafeMethods {
+// CHECK:   func __getUnsafeUnsafe() -> Unsafe
+// CHECK:   mutating func __getUnsafeMutatingUnsafe() -> Unsafe
+// CHECK: }

test/Interop/Cxx/stdlib/msvcprt-module-interface.swift

@@ -12,10 +12,10 @@
 // CHECK-STRING:   typealias size_t = size_t
 // CHECK-STRING:   static func to_string(_ _Val: Int32) -> std.string
 // CHECK-STRING:   static func to_wstring(_ _Val: Int32) -> std.wstring
-// CHECK-STRING:   struct __CxxTemplateInstSs {
+// CHECK-STRING:   struct __CxxTemplateInstSs : CxxRandomAccessCollection {
 // CHECK-STRING:     typealias value_type = CChar
 // CHECK-STRING:   }
-// CHECK-STRING:   struct __CxxTemplateInstSbIwSt11char_traitsIwESaIwEE {
+// CHECK-STRING:   struct __CxxTemplateInstSbIwSt11char_traitsIwESaIwEE : CxxRandomAccessCollection {
 // CHECK-STRING:     typealias value_type = CWideChar
 // CHECK-STRING:   }
 // CHECK-STRING:   typealias string = std.__CxxTemplateInstSs