Merge pull request #85296 from j-hui/revert-template-arg-import

[cxx-interop] Revert changes to template argument import/safety check

Author: kavon

include/swift/ClangImporter/ClangImporterRequests.h

@@ -624,53 +624,6 @@ class CxxValueSemantics
 void simple_display(llvm::raw_ostream &out, CxxValueSemanticsDescriptor desc);
 SourceLoc extractNearestSourceLoc(CxxValueSemanticsDescriptor desc);
 
-struct ClangTypeExplicitSafetyDescriptor final {
-  clang::CanQualType type;
-
-  ClangTypeExplicitSafetyDescriptor(clang::CanQualType type) : type(type) {}
-  ClangTypeExplicitSafetyDescriptor(clang::QualType type)
-      : type(type->getCanonicalTypeUnqualified()) {}
-
-  friend llvm::hash_code
-  hash_value(const ClangTypeExplicitSafetyDescriptor &desc) {
-    return llvm::hash_combine(desc.type.getTypePtrOrNull());
-  }
-
-  friend bool operator==(const ClangTypeExplicitSafetyDescriptor &lhs,
-                         const ClangTypeExplicitSafetyDescriptor &rhs) {
-    return lhs.type == rhs.type;
-  }
-
-  friend bool operator!=(const ClangTypeExplicitSafetyDescriptor &lhs,
-                         const ClangTypeExplicitSafetyDescriptor &rhs) {
-    return !(lhs == rhs);
-  }
-};
-
-void simple_display(llvm::raw_ostream &out,
-                    ClangTypeExplicitSafetyDescriptor desc);
-SourceLoc extractNearestSourceLoc(ClangTypeExplicitSafetyDescriptor desc);
-
-/// Determine the safety of the given Clang declaration.
-class ClangTypeExplicitSafety
-    : public SimpleRequest<ClangTypeExplicitSafety,
-                           ExplicitSafety(ClangTypeExplicitSafetyDescriptor),
-                           RequestFlags::Cached> {
-public:
-  using SimpleRequest::SimpleRequest;
-
-  // Source location
-  SourceLoc getNearestLoc() const { return SourceLoc(); };
-  bool isCached() const { return true; }
-
-private:
-  friend SimpleRequest;
-
-  // Evaluation.
-  ExplicitSafety evaluate(Evaluator &evaluator,
-                          ClangTypeExplicitSafetyDescriptor desc) const;
-};
-
 struct CxxDeclExplicitSafetyDescriptor final {
   const clang::Decl *decl;
   bool isClass;

include/swift/ClangImporter/ClangImporterTypeIDZone.def

@@ -48,9 +48,6 @@ SWIFT_REQUEST(ClangImporter, ClangTypeEscapability,
 SWIFT_REQUEST(ClangImporter, CxxValueSemantics,
               CxxValueSemanticsKind(CxxValueSemanticsDescriptor), Cached,
               NoLocationInfo)
-SWIFT_REQUEST(ClangImporter, ClangTypeExplicitSafety,
-              ExplicitSafety(ClangTypeExplicitSafetyDescriptor), Cached,
-              NoLocationInfo)
 SWIFT_REQUEST(ClangImporter, ClangDeclExplicitSafety,
               ExplicitSafety(CxxDeclExplicitSafetyDescriptor), Cached,
               NoLocationInfo)

lib/ClangImporter/ClangImporter.cpp

@@ -8723,56 +8723,6 @@ SourceLoc swift::extractNearestSourceLoc(SafeUseOfCxxDeclDescriptor desc) {
   return SourceLoc();
 }
 
-void swift::simple_display(llvm::raw_ostream &out,
-                           ClangTypeExplicitSafetyDescriptor desc) {
-  auto qt = static_cast<clang::QualType>(desc.type);
-  out << "Checking if type '" << qt.getAsString() << "' is explicitly safe.\n";
-}
-
-SourceLoc swift::extractNearestSourceLoc(ClangTypeExplicitSafetyDescriptor desc) {
-  return SourceLoc();
-}
-
-ExplicitSafety ClangTypeExplicitSafety::evaluate(
-    Evaluator &evaluator, ClangTypeExplicitSafetyDescriptor desc) const {
-  auto clangType = static_cast<clang::QualType>(desc.type);
-
-  // Handle pointers.
-  auto pointeeType = clangType->getPointeeType();
-  if (!pointeeType.isNull()) {
-    // Function pointers are okay.
-    if (pointeeType->isFunctionType())
-      return ExplicitSafety::Safe;
-
-    // Pointers to record types are okay if they come in as foreign reference
-    // types.
-    if (auto *recordDecl = pointeeType->getAsRecordDecl()) {
-      if (hasImportAsRefAttr(recordDecl))
-        return ExplicitSafety::Safe;
-    }
-
-    // All other pointers are considered unsafe.
-    return ExplicitSafety::Unsafe;
-  }
-
-  // Handle records recursively.
-  if (auto recordDecl = clangType->getAsTagDecl()) {
-    // If we reached this point the types is not imported as a shared reference,
-    // so we don't need to check the bases whether they are shared references.
-    auto req = ClangDeclExplicitSafety({recordDecl, false});
-    if (evaluator.hasActiveRequest(req))
-      // Cycles are allowed in templates, e.g.:
-      //   template <typename>   class Foo { ... }; // throws away template arg
-      //   template <typename T> class Bar : Foo<Bar<T>> { ... };
-      // We need to avoid them here.
-      return ExplicitSafety::Unspecified;
-    return evaluateOrDefault(evaluator, req, ExplicitSafety::Unspecified);
-  }
-
-  // Everything else is safe.
-  return ExplicitSafety::Safe;
-}
-
 void swift::simple_display(llvm::raw_ostream &out,
                            CxxDeclExplicitSafetyDescriptor desc) {
   out << "Checking if '";
@@ -8844,13 +8794,43 @@ CustomRefCountingOperationResult CustomRefCountingOperation::evaluate(
 
 /// Check whether the given Clang type involves an unsafe type.
 static bool hasUnsafeType(Evaluator &evaluator, clang::QualType clangType) {
-  auto req = ClangTypeExplicitSafety({clangType});
-  if (evaluator.hasActiveRequest(req))
-    // If there is a cycle in a type, assume ExplicitSafety is Unspecified,
-    // i.e., not unsafe:
-    return false;
-  return evaluateOrDefault(evaluator, req, ExplicitSafety::Unspecified) ==
-         ExplicitSafety::Unsafe;
+  // Handle pointers.
+  auto pointeeType = clangType->getPointeeType();
+  if (!pointeeType.isNull()) {
+    // Function pointers are okay.
+    if (pointeeType->isFunctionType())
+      return false;
+    
+    // Pointers to record types are okay if they come in as foreign reference
+    // types.
+    if (auto recordDecl = pointeeType->getAsRecordDecl()) {
+      if (hasImportAsRefAttr(recordDecl))
+        return false;
+    }
+    
+    // All other pointers are considered unsafe.
+    return true;
+  }
+
+  // Handle records recursively.
+  if (auto recordDecl = clangType->getAsTagDecl()) {
+    // If we reached this point the types is not imported as a shared reference,
+    // so we don't need to check the bases whether they are shared references.
+    auto safety = evaluateOrDefault(
+        evaluator, ClangDeclExplicitSafety({recordDecl, false}),
+        ExplicitSafety::Unspecified);
+    switch (safety) {
+      case ExplicitSafety::Unsafe:
+        return true;
+        
+      case ExplicitSafety::Safe:
+      case ExplicitSafety::Unspecified:
+        return false;        
+    }
+  }
+    
+  // Everything else is safe.
+  return false;
 }
 
 ExplicitSafety
@@ -8888,29 +8868,6 @@ ClangDeclExplicitSafety::evaluate(Evaluator &evaluator,
           ClangTypeEscapability({recordDecl->getTypeForDecl(), nullptr}),
           CxxEscapability::Unknown) != CxxEscapability::Unknown)
     return ExplicitSafety::Safe;
-
-  // A template class is unsafe if any of its type arguments are unsafe.
-  // Note that this does not rely on the record being defined.
-  if (const auto *ctsd =
-          dyn_cast<clang::ClassTemplateSpecializationDecl>(recordDecl)) {
-    for (auto arg : ctsd->getTemplateArgs().asArray()) {
-      switch (arg.getKind()) {
-      case clang::TemplateArgument::Type:
-        if (hasUnsafeType(evaluator, arg.getAsType()))
-          return ExplicitSafety::Unsafe;
-        break;
-      case clang::TemplateArgument::Pack:
-        for (auto pkArg : arg.getPackAsArray()) {
-          if (pkArg.getKind() == clang::TemplateArgument::Type &&
-              hasUnsafeType(evaluator, pkArg.getAsType()))
-            return ExplicitSafety::Unsafe;
-        }
-        break;
-      default:
-        continue;
-      }
-    }
-  }
 
   // If we don't have a definition, leave it unspecified.
   recordDecl = recordDecl->getDefinition();
@@ -8924,7 +8881,7 @@ ClangDeclExplicitSafety::evaluate(Evaluator &evaluator,
         return ExplicitSafety::Unsafe;
     }
   }
-
+  
   // Check the fields.
   for (auto field : recordDecl->fields()) {
     if (hasUnsafeType(evaluator, field->getType()))

lib/ClangImporter/ImportDecl.cpp

@@ -69,7 +69,6 @@
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
-#include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetInfo.h"
@@ -2302,6 +2301,34 @@ 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,

test/Interop/Cxx/class/safe-interop-mode.swift

@@ -83,27 +83,6 @@ struct OwnedData {
   void takeSharedObject(SharedObject *) const;
 };
 
-// A class template that throws away its type argument.
-//
-// If this template is instantiated with an unsafe type, it should be considered
-// unsafe even if that type is never used.
-template <typename> struct TTake {};
-
-using TTakeInt = TTake<int>;
-using TTakePtr = TTake<int *>;
-using TTakeSafeTuple = TTake<SafeTuple>;
-using TTakeUnsafeTuple = TTake<UnsafeTuple>;
-
-// An escapability or explicit safety annotation means a type is considered safe
-// even if it would otherwise be considered unsafe.
-template <typename> struct SWIFT_ESCAPABLE TEscape {};
-template <typename> struct __attribute__((swift_attr("safe"))) TSafe { void *ptr; };
-
-using TEscapePtr = TEscape<int *>;
-using TEscapeUnsafeTuple = TEscape<UnsafeTuple>;
-using TSafePtr = TSafe<int *>;
-using TSafeTuple = TSafe<UnsafeTuple>;
-
 struct HoldsShared {
   SharedObject* obj;
 
@@ -205,26 +184,3 @@ func useSharedReference(frt: DerivedFromSharedObject, h: HoldsShared) {
   let _ = frt
   let _ = h.getObj()
 }
-
-func useTTakeInt(x: TTakeInt) {
-  _ = x
-}
-
-func useTTakePtr(x: TTakePtr) {
-  // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}
-  _ = x // expected-note{{reference to parameter 'x' involves unsafe type}}
-}
-
-func useTTakeSafeTuple(x: TTakeSafeTuple) {
-  _ = x
-}
-
-func useTTakeUnsafeTuple(x: TTakeUnsafeTuple) {
-  // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}
-  _ = x // expected-note{{reference to parameter 'x' involves unsafe type}}
-}
-
-func useTEscapePtr(x: TEscapePtr) { _ = x }
-func useTEscapeUnsafeTuple(x: TEscapeUnsafeTuple) { _ = x }
-func useTSafePtr(x: TSafePtr) { _ = x }
-func useTSafeTuple(x: TSafeTuple) { _ = x }