Merge pull request #62575 from apple/egorzhdan/cxx-disambiguate

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

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