[cxx-interop] Fix circular reference, remove feature flag check

It turns out the query to check the reference semantics of a type had
the side effect of importing some functions/types. This could introduce
circular reference errors with our queries. This PR removes this side
effect from the query and updates the test files. Since we do the same
work later on in another query, the main change is just the wording of
the diagnostics (and we now can also infer immortal references based on
the base types). This PR also reorders some operations, specifically now
we mark base classes as imported before we attempt to import template
arguments.

After these changes it is possible to remove the last feature check for
strict memory safe mode. We want to mark types as @unsafe in both
language modes so we can diagnose redundant unsafe markers even when the
feature is off.

Author: Gabor Horvath

lib/ClangImporter/ClangImporter.cpp

@@ -7792,7 +7792,7 @@ getRefParentOrDiag(const clang::RecordDecl *decl, ASTContext &ctx,
   if (refParentDecls.empty())
     return nullptr;
 
-  std::unordered_set<ValueDecl *> uniqueRetainDecls{}, uniqueReleaseDecls{};
+  std::set<StringRef> uniqueRetainDecls{}, uniqueReleaseDecls{};
   constexpr StringRef retainPrefix = "retain:";
   constexpr StringRef releasePrefix = "release:";
 
@@ -7801,16 +7801,10 @@ getRefParentOrDiag(const clang::RecordDecl *decl, ASTContext &ctx,
     for (const auto *attr : refParentDecl->getAttrs()) {
       if (const auto swiftAttr = llvm::dyn_cast<clang::SwiftAttrAttr>(attr)) {
         const auto &attribute = swiftAttr->getAttribute();
-        llvm::SmallVector<ValueDecl *, 1> valueDecls;
-        if (attribute.starts_with(retainPrefix)) {
-          auto name = attribute.drop_front(retainPrefix.size()).str();
-          valueDecls = getValueDeclsForName(decl, ctx, name);
-          uniqueRetainDecls.insert(valueDecls.begin(), valueDecls.end());
-        } else if (attribute.starts_with(releasePrefix)) {
-          auto name = attribute.drop_front(releasePrefix.size()).str();
-          valueDecls = getValueDeclsForName(decl, ctx, name);
-          uniqueReleaseDecls.insert(valueDecls.begin(), valueDecls.end());
-        }
+        if (attribute.starts_with(retainPrefix))
+          uniqueRetainDecls.insert(attribute.drop_front(retainPrefix.size()));
+        else if (attribute.starts_with(releasePrefix))
+          uniqueReleaseDecls.insert(attribute.drop_front(releasePrefix.size()));
       }
     }
   }
@@ -8175,7 +8169,7 @@ CxxRecordAsSwiftType::evaluate(Evaluator &evaluator,
   return nullptr;
 }
 
-bool anySubobjectsSelfContained(const clang::CXXRecordDecl *decl) {
+static bool anySubobjectsSelfContained(const clang::CXXRecordDecl *decl) {
   // std::pair and std::tuple might have copy and move constructors, or base
   // classes with copy and move constructors, but they are not self-contained
   // types, e.g. `std::pair<UnsafeType, T>`.
@@ -8339,11 +8333,11 @@ CustomRefCountingOperationResult CustomRefCountingOperation::evaluate(
     }
   }
 
-  if (retainReleaseAttrs.empty()) {
+  if (retainReleaseAttrs.empty())
     return {CustomRefCountingOperationResult::noAttribute, nullptr, ""};
-  } else if (retainReleaseAttrs.size() > 1) {
+
+  if (retainReleaseAttrs.size() > 1)
     return {CustomRefCountingOperationResult::tooManyAttributes, nullptr, ""};
-  }
 
   auto name = retainReleaseAttrs.front()
                   ->getAttribute()

lib/ClangImporter/ImportDecl.cpp

@@ -2197,38 +2197,6 @@ namespace {
             loc, std::nullopt, nullptr, dc);
       Impl.ImportedDecls[{decl->getCanonicalDecl(), getVersion()}] = result;
 
-      // We have to do this after populating ImportedDecls to avoid importing
-      // the same multiple times.
-      if (Impl.SwiftContext.LangOpts.hasFeature(
-              Feature::StrictMemorySafety)) {
-        if (const auto *ctsd =
-                dyn_cast<clang::ClassTemplateSpecializationDecl>(decl)) {
-          for (auto arg : ctsd->getTemplateArgs().asArray()) {
-            llvm::SmallVector<clang::TemplateArgument, 1> nonPackArgs;
-            if (arg.getKind() == clang::TemplateArgument::Pack) {
-              auto pack = arg.getPackAsArray();
-              nonPackArgs.assign(pack.begin(), pack.end());
-            } else {
-              nonPackArgs.push_back(arg);
-            }
-            for (auto realArg : nonPackArgs) {
-              if (realArg.getKind() != clang::TemplateArgument::Type)
-                continue;
-              auto SwiftType = Impl.importTypeIgnoreIUO(
-                  realArg.getAsType(), ImportTypeKind::Abstract,
-                  [](Diagnostic &&diag) {}, false, Bridgeability::None,
-                  ImportTypeAttrs());
-              if (SwiftType && SwiftType->isUnsafe()) {
-                auto attr =
-                    new (Impl.SwiftContext) UnsafeAttr(/*implicit=*/true);
-                result->getAttrs().add(attr);
-                break;
-              }
-            }
-          }
-        }
-      }
-
       if (recordHasMoveOnlySemantics(decl)) {
         if (decl->isInStdNamespace() && decl->getName() == "promise") {
           // Do not import std::promise.
@@ -2238,16 +2206,6 @@ namespace {
                                    MoveOnlyAttr(/*Implicit=*/true));
       }
 
-      bool isNonEscapable = false;
-      if (evaluateOrDefault(
-              Impl.SwiftContext.evaluator,
-              ClangTypeEscapability({decl->getTypeForDecl(), &Impl}),
-              CxxEscapability::Unknown) == CxxEscapability::NonEscapable) {
-        result->getAttrs().add(new (Impl.SwiftContext)
-                                   NonEscapableAttr(/*Implicit=*/true));
-        isNonEscapable = true;
-      }
-
       // FIXME: Figure out what to do with superclasses in C++. One possible
       // solution would be to turn them into members and add conversion
       // functions.
@@ -2259,6 +2217,44 @@ namespace {
         }
       }
 
+      // We have to do this after populating ImportedDecls to avoid importing
+      // the same decl multiple times. Also after we imported the bases.
+      if (const auto *ctsd =
+              dyn_cast<clang::ClassTemplateSpecializationDecl>(decl)) {
+        for (auto arg : ctsd->getTemplateArgs().asArray()) {
+          llvm::SmallVector<clang::TemplateArgument, 1> nonPackArgs;
+          if (arg.getKind() == clang::TemplateArgument::Pack) {
+            auto pack = arg.getPackAsArray();
+            nonPackArgs.assign(pack.begin(), pack.end());
+          } else {
+            nonPackArgs.push_back(arg);
+          }
+          for (auto realArg : nonPackArgs) {
+            if (realArg.getKind() != clang::TemplateArgument::Type)
+              continue;
+            auto SwiftType = Impl.importTypeIgnoreIUO(
+                realArg.getAsType(), ImportTypeKind::Abstract,
+                [](Diagnostic &&diag) {}, false, Bridgeability::None,
+                ImportTypeAttrs());
+            if (SwiftType && SwiftType->isUnsafe()) {
+              auto attr = new (Impl.SwiftContext) UnsafeAttr(/*implicit=*/true);
+              result->getAttrs().add(attr);
+              break;
+            }
+          }
+        }
+      }
+
+      bool isNonEscapable = false;
+      if (evaluateOrDefault(
+              Impl.SwiftContext.evaluator,
+              ClangTypeEscapability({decl->getTypeForDecl(), &Impl}),
+              CxxEscapability::Unknown) == CxxEscapability::NonEscapable) {
+        result->getAttrs().add(new (Impl.SwiftContext)
+                                   NonEscapableAttr(/*Implicit=*/true));
+        isNonEscapable = true;
+      }
+
       // Import each of the members.
       SmallVector<VarDecl *, 4> members;
       SmallVector<FuncDecl *, 4> methods;
@@ -2965,7 +2961,7 @@ namespace {
         }
       }
 
-      // It is import that we bail on an unimportable record *before* we import
+      // It is important that we bail on an unimportable record *before* we import
       // any of its members or cache the decl.
       auto semanticsKind = evaluateOrDefault(
           Impl.SwiftContext.evaluator,

test/Interop/Cxx/foreign-reference/Inputs/inheritance.h

@@ -80,9 +80,7 @@ __attribute__((swift_attr("release:immortal"))) ImmortalRefType {};
 
 ImmortalRefType *returnImmortalRefType() { return new ImmortalRefType(); };
 
-// Doubt: Is it fine to not infer in the case of SWIFT_IMMORTAL_REFERENCE
-// rdar://145193396
-struct DerivedFromImmortalRefType : ImmortalRefType {}; // expected-warning {{unable to infer SWIFT_SHARED_REFERENCE for 'DerivedFromImmortalRefType', although one of its transitive base types is marked as SWIFT_SHARED_REFERENCE}}
+struct DerivedFromImmortalRefType : ImmortalRefType {};
 DerivedFromImmortalRefType *returnDerivedFromImmortalRefType() {
     return new DerivedFromImmortalRefType();
 };
@@ -284,7 +282,8 @@ __attribute__((swift_attr("retain:sameretain")))
 __attribute__((swift_attr("release:samerelease"))) B2 {};  // expected-error {{multiple functions 'sameretain' found; there must be exactly one retain function for reference type 'B2'}}
                                                             // expected-error@-1 {{multiple functions 'samerelease' found; there must be exactly one release function for reference type 'B2'}}
 
-struct D : B1, B2 {}; // expected-warning {{unable to infer SWIFT_SHARED_REFERENCE for 'D', although one of its transitive base types is marked as SWIFT_SHARED_REFERENCE}}
+struct D : B1, B2 {}; // expected-error {{multiple functions 'sameretain' found; there must be exactly one retain function for reference type 'D'}}
+                      // expected-error@-1 {{multiple functions 'samerelease' found; there must be exactly one release function for reference type 'D'}}
 D *returnD() { return new D(); };
 } // namespace OverloadedRetainRelease
 

test/Interop/Cxx/foreign-reference/inheritance.swift

@@ -50,7 +50,7 @@ FrtInheritanceTestSuite.test("ParentChild") {
 
   let derivedFromImmortalRefType = ImmortalRefereceExample.returnDerivedFromImmortalRefType()
   expectTrue(
-    !(type(of: derivedFromImmortalRefType) is AnyObject.Type),
+    type(of: derivedFromImmortalRefType) is AnyObject.Type,
     "Expected derivedFromImmortalRefType to be a value type, but it’s a reference type")
 
   let valType = ExplicitAnnotationHasPrecedence1.returnValueType()