Merge pull request #67296 from zoecarver/only-methods-on-self-contained-types-are-unsafe

[cxx-interop] Only mark projections of self-contained types as unsafe.

Author: zoecarver

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

@@ -27,7 +27,11 @@ public struct SourceLoc {
     guard bridged.isValid() else {
       return nil
     }
+#if $NewCxxMethodSafetyHeuristics
+    self.locationInFile = bridged.getOpaquePointerValue().assumingMemoryBound(to: UInt8.self)
+#else
     self.locationInFile = bridged.__getOpaquePointerValueUnsafe().assumingMemoryBound(to: UInt8.self)
+#endif
   }
 
   public var bridged: swift.SourceLoc {
@@ -57,9 +61,15 @@ public struct CharSourceRange {
   }
 
   public init?(bridged: swift.CharSourceRange) {
+#if $NewCxxMethodSafetyHeuristics
+    guard let start = SourceLoc(bridged: bridged.getStart()) else {
+      return nil
+    }
+#else
     guard let start = SourceLoc(bridged: bridged.__getStartUnsafe()) else {
       return nil
     }
+#endif
     self.init(start: start, byteLength: bridged.getByteLength())
   }
 

SwiftCompilerSources/Sources/Basic/Utils.swift

@@ -66,19 +66,34 @@ public struct StringRef : CustomStringConvertible, NoReflectionChildren {
   public var description: String { string }
 
   public var count: Int {
+#if $NewCxxMethodSafetyHeuristics
+    Int(_bridged.bytes_end() - _bridged.bytes_begin())
+#else
     Int(_bridged.__bytes_endUnsafe() - _bridged.__bytes_beginUnsafe())
+#endif
   }
 
   public subscript(index: Int) -> UInt8 {
-    let buffer = UnsafeBufferPointer<UInt8>(start: _bridged.__bytes_beginUnsafe(),
+#if $NewCxxMethodSafetyHeuristics
+    let buffer = UnsafeBufferPointer<UInt8>(start: _bridged.bytes_begin(),
                                             count: count)
+#else
+    let buffer = UnsafeBufferPointer<UInt8>(start: _bridged.bytes_begin(),
+                                            count: count)
+#endif
     return buffer[index]
   }
 
   public static func ==(lhs: StringRef, rhs: StaticString) -> Bool {
+#if $NewCxxMethodSafetyHeuristics
+    let lhsBuffer = UnsafeBufferPointer<UInt8>(
+      start: lhs._bridged.bytes_begin(),
+      count: lhs.count)
+#else
     let lhsBuffer = UnsafeBufferPointer<UInt8>(
       start: lhs._bridged.__bytes_beginUnsafe(),
       count: lhs.count)
+#endif
     return rhs.withUTF8Buffer { (rhsBuffer: UnsafeBufferPointer<UInt8>) in
       if lhsBuffer.count != rhsBuffer.count { return false }
       return lhsBuffer.elementsEqual(rhsBuffer, by: ==)

include/swift/Basic/Features.def

@@ -91,6 +91,7 @@ LANGUAGE_FEATURE(BuiltinStackAlloc, 0, "Builtin.stackAlloc", true)
 LANGUAGE_FEATURE(BuiltinUnprotectedStackAlloc, 0, "Builtin.unprotectedStackAlloc", true)
 LANGUAGE_FEATURE(BuiltinTaskRunInline, 0, "Builtin.taskRunInline", true)
 LANGUAGE_FEATURE(BuiltinUnprotectedAddressOf, 0, "Builtin.unprotectedAddressOf", true)
+LANGUAGE_FEATURE(NewCxxMethodSafetyHeuristics, 0, "Only import C++ methods that return pointers (projections) on owned types as unsafe", true)
 SUPPRESSIBLE_LANGUAGE_FEATURE(SpecializeAttributeWithAvailability, 0, "@_specialize attribute with availability", true)
 LANGUAGE_FEATURE(BuiltinAssumeAlignment, 0, "Builtin.assumeAlignment", true)
 LANGUAGE_FEATURE(BuiltinCreateTaskGroupWithFlags, 0, "Builtin.createTaskGroupWithFlags", true)

lib/AST/ASTPrinter.cpp

@@ -3484,6 +3484,10 @@ static bool usesFeaturePlaygroundExtendedCallbacks(Decl *decl) {
   return false;
 }
 
+static bool usesFeatureNewCxxMethodSafetyHeuristics(Decl *decl) {
+  return decl->hasClangNode();
+}
+
 /// Suppress the printing of a particular feature.
 static void suppressingFeature(PrintOptions &options, Feature feature,
                                llvm::function_ref<void()> action) {

lib/ClangImporter/ClangImporter.cpp

@@ -6789,6 +6789,37 @@ CxxRecordAsSwiftType::evaluate(Evaluator &evaluator,
   return nullptr;
 }
 
+bool anySubobjectsSelfContained(const clang::CXXRecordDecl *decl) {
+  if (!decl->getDefinition())
+    return false;
+
+  if (hasCustomCopyOrMoveConstructor(decl) || hasOwnedValueAttr(decl))
+    return true;
+  
+  auto checkType = [](clang::QualType t) {
+    if (auto recordType = dyn_cast<clang::RecordType>(t.getCanonicalType())) {
+      if (auto cxxRecord =
+              dyn_cast<clang::CXXRecordDecl>(recordType->getDecl())) {
+        return anySubobjectsSelfContained(cxxRecord);
+      }
+    }
+
+    return false;
+  };
+
+  for (auto field : decl->fields()) {
+    if (checkType(field->getType()))
+      return true;
+  }
+
+  for (auto base : decl->bases()) {
+    if (checkType(base.getType()))
+      return true;
+  }
+  
+  return false;
+}
+
 bool IsSafeUseOfCxxDecl::evaluate(Evaluator &evaluator,
                                   SafeUseOfCxxDeclDescriptor desc) const {
   const clang::Decl *decl = desc.decl;
@@ -6806,10 +6837,24 @@ bool IsSafeUseOfCxxDecl::evaluate(Evaluator &evaluator,
     if (isForeignReferenceType(method->getReturnType()))
       return true;
 
-    // If it returns a pointer or reference, that's a projection.
+    // begin and end methods likely return an interator, so they're unsafe. This
+    // is required so that automatic the conformance to RAC works properly.
+    if (method->getNameAsString() == "begin" ||
+        method->getNameAsString() == "end")
+      return false;
+
+    auto parentQualType = method
+      ->getParent()->getTypeForDecl()->getCanonicalTypeUnqualified();
+
+    bool parentIsSelfContained =
+      !isForeignReferenceType(parentQualType) &&
+      anySubobjectsSelfContained(method->getParent());
+
+    // If it returns a pointer or reference from an owned parent, that's a
+    // projection (unsafe).
     if (method->getReturnType()->isPointerType() ||
         method->getReturnType()->isReferenceType())
-      return false;
+      return !parentIsSelfContained;
 
     // Check if it's one of the known unsafe methods we currently
     // mark as safe by default.
@@ -6824,20 +6869,22 @@ bool IsSafeUseOfCxxDecl::evaluate(Evaluator &evaluator,
               dyn_cast<clang::CXXRecordDecl>(returnType->getDecl())) {
         if (isSwiftClassType(cxxRecordReturnType))
           return true;
+
         if (hasIteratorAPIAttr(cxxRecordReturnType) ||
-            isIterator(cxxRecordReturnType)) {
+            isIterator(cxxRecordReturnType))
           return false;
-        }
 
         // Mark this as safe to help our diganostics down the road.
         if (!cxxRecordReturnType->getDefinition()) {
           return true;
         }
 
-        if (!hasCustomCopyOrMoveConstructor(cxxRecordReturnType) &&
+        // A projection of a view type (such as a string_view) from a self
+        // contained parent is a proejction (unsafe).
+        if (!anySubobjectsSelfContained(cxxRecordReturnType) &&
             !hasOwnedValueAttr(cxxRecordReturnType) &&
             hasPointerInSubobjects(cxxRecordReturnType)) {
-          return false;
+          return !parentIsSelfContained;
         }
       }
     }

test/Interop/Cxx/class/Inputs/type-classification.h

@@ -232,6 +232,7 @@ struct __attribute__((swift_attr("import_iterator"))) Iterator {
 };
 
 struct HasMethodThatReturnsIterator {
+  HasMethodThatReturnsIterator(const HasMethodThatReturnsIterator&);
   Iterator getIterator() const;
 };
 
@@ -240,6 +241,7 @@ struct IteratorBox {
 };
 
 struct HasMethodThatReturnsIteratorBox {
+  HasMethodThatReturnsIteratorBox(const HasMethodThatReturnsIteratorBox&);
   IteratorBox getIteratorBox() const;
 };
 

test/Interop/Cxx/class/fixit-add-safe-to-import-self-contained.swift

@@ -12,6 +12,8 @@ module Test {
 struct Ptr { int *p; };
 
 struct X {
+  X(const X&);
+  
   int *test() { }
   Ptr other() { }
 };

test/Interop/Cxx/class/invalid-unsafe-projection-errors.swift

@@ -13,13 +13,14 @@ struct Ptr { int *p; };
 struct __attribute__((swift_attr("import_owned"))) StirngLiteral { const char *name; };
 
 struct M {
-        int *_Nonnull test1() const;
-        int &test2() const;
-        Ptr test3() const;
+  M(const M&);
+  int *_Nonnull test1() const;
+  int &test2() const;
+  Ptr test3() const;
 
-        int *begin() const;
+  int *begin() const;
 
-        StirngLiteral stringLiteral() const { return StirngLiteral{"M"}; }
+  StirngLiteral stringLiteral() const { return StirngLiteral{"M"}; }
 };
 
 //--- test.swift

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

@@ -52,6 +52,7 @@ struct Unsafe {
 };
 
 struct HasAmbiguousUnsafeMethods {
+  HasAmbiguousUnsafeMethods(const HasAmbiguousUnsafeMethods&);
   Unsafe getUnsafe() const { return Unsafe(); }
   Unsafe getUnsafe() { return Unsafe(); }
 };

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

@@ -6,4 +6,11 @@ module Methods {
 module AmbiguousMethods {
   header "ambiguous_methods.h"
   requires cplusplus
-}
+}
+
+module UnsafeProjections {
+  header "unsafe-projections.h"
+  requires cplusplus
+  
+  export *
+}

test/Interop/Cxx/class/method/Inputs/unsafe-projections.h

@@ -0,0 +1,94 @@
+#ifndef TEST_INTEROP_CXX_CLASS_METHOD_UNSAFE_PROJECTIONS_H
+#define TEST_INTEROP_CXX_CLASS_METHOD_UNSAFE_PROJECTIONS_H
+
+#include <string>
+
+struct NestedSelfContained;
+struct Empty;
+struct SelfContained;
+struct ExplicitSelfContained;
+struct NestedExplicitSelfContained;
+
+struct View {
+  void *ptr;
+  
+  void *data() const;
+  void *empty() const;
+  std::string name() const;
+  NestedSelfContained nested() const;
+  ExplicitSelfContained explicitSelfContained() const;
+  NestedExplicitSelfContained explicitNested() const;
+};
+
+struct SelfContained {
+  void *ptr;
+  SelfContained(const SelfContained&);
+  
+  std::string name() const;
+  SelfContained selfContained() const;
+  NestedSelfContained nested() const;
+  Empty empty() const;
+  int value() const;
+  View view() const;
+  int *pointer() const;
+  ExplicitSelfContained explicitSelfContained() const;
+  NestedExplicitSelfContained explicitNested() const;
+};
+
+struct NestedSelfContained {
+  SelfContained member;
+  
+  std::string name() const;
+  SelfContained selfContained() const;
+  NestedSelfContained nested() const;
+  Empty empty() const;
+  int value() const;
+  View view() const;
+  int *pointer() const;
+  ExplicitSelfContained explicitSelfContained() const;
+  NestedExplicitSelfContained explicitNested() const;
+};
+
+struct InheritSelfContained: SelfContained {
+  std::string name() const;
+  SelfContained selfContained() const;
+  NestedSelfContained nested() const;
+  Empty empty() const;
+  int value() const;
+  View view() const;
+  int *pointer() const;
+};
+
+struct __attribute__((swift_attr("import_owned"))) ExplicitSelfContained {
+  void *ptr;
+  
+  void *pointer() const;
+  View view() const;
+  NestedSelfContained nested() const;
+};
+
+struct NestedExplicitSelfContained {
+  ExplicitSelfContained m;
+
+  SelfContained selfContained() const;
+  NestedSelfContained nested() const;
+  int value() const;
+  View view() const;
+  int *pointer() const;
+};
+
+struct Empty {
+  Empty empty() const;
+  void *pointer() const;
+  SelfContained selfContained() const;
+};
+
+struct IntPair {
+  int a; int b;
+
+  int first() const;
+  void *pointer() const;
+  SelfContained selfContained() const;
+};
+
+#endif // TEST_INTEROP_CXX_CLASS_METHOD_UNSAFE_PROJECTIONS_H