[cxx-interop] Remove annotationOnly flag from Escapability request

Author: susmonteiro

include/swift/ClangImporter/ClangImporterRequests.h

@@ -531,18 +531,14 @@ enum class CxxEscapability { Escapable, NonEscapable, Unknown };
 struct EscapabilityLookupDescriptor final {
   const clang::Type *type;
   ClangImporter::Implementation *impl;
-  // Only explicitly ~Escapable annotated types are considered ~Escapable.
-  // This is for backward compatibility, so we continue to import aggregates
-  // containing pointers as Escapable types.
-  bool annotationOnly = true;
 
   friend llvm::hash_code hash_value(const EscapabilityLookupDescriptor &desc) {
     return llvm::hash_combine(desc.type);
   }
 
   friend bool operator==(const EscapabilityLookupDescriptor &lhs,
                          const EscapabilityLookupDescriptor &rhs) {
-    return lhs.type == rhs.type && lhs.annotationOnly == rhs.annotationOnly;
+    return lhs.type == rhs.type;
   }
 
   friend bool operator!=(const EscapabilityLookupDescriptor &lhs,

lib/ClangImporter/ClangImporter.cpp

@@ -5493,12 +5493,18 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
 
   // Escapability inference rules:
   // - array and vector types have the same escapability as their element type
-  // - pointer and reference types are currently imported as escapable
+  // - pointer and reference types are currently imported as unknown
   // (importing them as non-escapable broke backward compatibility)
   // - a record type is escapable or non-escapable if it is explicitly annotated
   // as such
   // - a record type is escapable if it is annotated with SWIFT_ESCAPABLE_IF()
   // and none of the annotation arguments are non-escapable
+  // - an aggregate or non-cxx record is escapable if none of their fields or
+  // bases are non-escapable (as long as they have a definition)
+  //   * we only infer escapability for simple types, with no user-declared
+  //   constructors, virtual bases or virtual member functions
+  //   * for more complex CxxRecordDecls, we rely solely on escapability
+  //   annotations
   // - in all other cases, the record has unknown escapability (e.g. no
   // escapability annotations, malformed escapability annotations)
 
@@ -5554,14 +5560,8 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
 
         continue;
       }
-      // The `annotationOnly` flag used to control which types we infered
-      // escapability for. Currently, this flag is always set to true, meaning
-      // that any type without an annotation (CxxRecordDecls, aggregates, decls
-      // lacking definition, etc.) will raise `hasUnknown`.
-      if (desc.annotationOnly) {
-        hasUnknown = true;
-        continue;
-      }
+      // Only try to infer escapability if the record doesn't have any
+      // escapability annotations
       auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl);
       if (recordDecl->getDefinition() &&
           (!cxxRecordDecl || cxxRecordDecl->isAggregate())) {
@@ -5571,7 +5571,11 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
         }
         for (auto field : recordDecl->fields())
           maybePushToStack(field->getType()->getUnqualifiedDesugaredType());
-        continue;
+      } else {
+        // We only infer escapability for simple types, such as aggregates and
+        // RecordDecls that are not CxxRecordDecls. For more complex
+        // CxxRecordDecls, we rely solely on escapability annotations.
+        hasUnknown = true;
       }
     } else if (type->isArrayType()) {
       auto elemTy = cast<clang::ArrayType>(type)
@@ -5582,10 +5586,9 @@ ClangTypeEscapability::evaluate(Evaluator &evaluator,
       maybePushToStack(vecTy->getElementType()->getUnqualifiedDesugaredType());
     } else if (type->isAnyPointerType() || type->isBlockPointerType() ||
                type->isMemberPointerType() || type->isReferenceType()) {
-      if (desc.annotationOnly)
-        hasUnknown = true;
-      else
-        return CxxEscapability::NonEscapable;
+      // pointer and reference types are currently imported as unknown
+      // (importing them as non-escapable broke backward compatibility)
+      hasUnknown = true;
     }
   }
   return hasUnknown ? CxxEscapability::Unknown : CxxEscapability::Escapable;

test/Interop/Cxx/class/nonescapable-errors.swift

@@ -118,34 +118,31 @@ struct SWIFT_ESCAPABLE Invalid {
 
 struct SWIFT_NONESCAPABLE NonEscapable {};
 
-template<typename T>
-struct HasAnonUnion {
-    union {
-        int known;
-        T unknown;
-    };
-};
+using NonEscapableOptional = std::optional<NonEscapable>;
 
-template<typename T>
-struct HasAnonStruct {
-    struct {
-        int known;
-        T unknown;
-    };
-};
+// Infered as non-escapable 
+struct Aggregate {
+  int a;
+  View b;
+  bool c;
 
-template<typename T>
-struct SWIFT_NONESCAPABLE NonEscapableHasAnonUnion {
-    union {
-        int known;
-        T unknown;
-    };
-};
+  void someMethod() {}
+}; 
 
-using HasAnonUnionNonEscapable = HasAnonUnion<NonEscapable>;
-using HasAnonStructNonEscapable = HasAnonStruct<NonEscapable>;
-using NonEscapableHasAnonUnionNonEscapable = NonEscapableHasAnonUnion<NonEscapable>;
-using NonEscapableOptional = std::optional<NonEscapable>;
+// This is a complex record (has user-declared constructors), so we don't infer escapability.
+// By default, it's imported as escapable, which generates an error 
+// because of the non-escapable field 'View'
+struct ComplexRecord {
+  int a;
+  View b;
+  bool c;
+
+  ComplexRecord() : a(1), b(), c(false) {}
+  ComplexRecord(const ComplexRecord &other) = default;
+}; 
+
+Aggregate m1();
+ComplexRecord m2();
 
 //--- test.swift
 import Test
@@ -233,20 +230,24 @@ public func test3(_ x: inout View) {
     // CHECK-NO-LIFETIMES: pointer to non-escapable type 'View' cannot be imported
 }
 
-public func anonymousUnions() {
-    _ = HasAnonUnionNonEscapable()
-    // CHECK: error: cannot find 'HasAnonUnionNonEscapable' in scope
-    // CHECK-NO-LIFETIMES: error: cannot find 'HasAnonUnionNonEscapable' in scope
-    _ = HasAnonStructNonEscapable()
-    // CHECK: error: cannot find 'HasAnonStructNonEscapable' in scope
-    // CHECK-NO-LIFETIMES: error: cannot find 'HasAnonStructNonEscapable' in scope
-    _ = NonEscapableHasAnonUnionNonEscapable()
+public func optional() {
     _ = NonEscapableOptional()
     // CHECK: error: cannot infer the lifetime dependence scope on an initializer with a ~Escapable parameter, specify '@_lifetime(borrow {{.*}})' or '@_lifetime(copy {{.*}})'
     // CHECK-NO-LIFETIMES: error: an initializer cannot return a ~Escapable result
     // CHECK-NO-LIFETIMES: error: an initializer cannot return a ~Escapable result
 }
 
+public func inferedEscapability() {
+    m1()
+    // CHECK: nonescapable.h:130:11: error: a function with a ~Escapable result needs a parameter to depend on
+    // CHECK-NO-LIFETIMES: nonescapable.h:130:11: error: a function cannot return a ~Escapable result
+    m2()
+    // CHECK: error: 'm2()' is unavailable: return type is unavailable in Swift
+    // CHECK: note: 'm2()' has been explicitly marked unavailable here
+    // CHECK-NO-LIFETIMES: error: 'm2()' is unavailable: return type is unavailable in Swift
+    // CHECK-NO-LIFETIMES: note: 'm2()' has been explicitly marked unavailable here
+}
+
     // CHECK-NOT: error
     // CHECK-NOT: warning
     // CHECK-NO-LIFETIMES-NOT: error

test/Interop/Cxx/class/nonescapable-lifetimebound.swift

@@ -166,6 +166,56 @@ using ReadonlyBytes = ReadonlySpan<unsigned char>;
 using Bytes = Span<unsigned char>;
 } // namespace rdar153081347
 
+struct SWIFT_NONESCAPABLE NonEscapable {
+    const int *p;
+};
+
+template<typename T>
+struct HasAnonUnion {
+    union {
+        int known;
+        T unknown;
+    };
+};
+
+template<typename T>
+struct HasAnonStruct {
+    struct {
+        int known;
+        T unknown;
+    };
+};
+
+template<typename T>
+struct SWIFT_NONESCAPABLE NonEscapableHasAnonUnion {
+    union {
+        int known;
+        T unknown;
+    };
+};
+
+using HasAnonUnionNonEscapable = HasAnonUnion<NonEscapable>;
+using HasAnonStructNonEscapable = HasAnonStruct<NonEscapable>;
+using NonEscapableHasAnonUnionNonEscapable = NonEscapableHasAnonUnion<NonEscapable>;
+
+HasAnonUnionNonEscapable makeAnonUnionNonEscapable(const Owner &owner [[clang::lifetimebound]]) {
+    HasAnonUnionNonEscapable result;
+    result.unknown = {&owner.data};
+    return result;
+}
+
+HasAnonStructNonEscapable makeAnonStructNonEscapable(const Owner &owner [[clang::lifetimebound]]) {
+    return {1, &owner.data};
+}
+
+NonEscapableHasAnonUnionNonEscapable makeNonEscapableHasAnonUnionNonEscapable(
+                                        const Owner &owner [[clang::lifetimebound]]) {
+    NonEscapableHasAnonUnionNonEscapable result;
+    result.unknown = {&owner.data};
+    return result;
+}
+
+
 // CHECK: sil {{.*}}[clang makeOwner] {{.*}}: $@convention(c) () -> Owner
 // CHECK: sil {{.*}}[clang getView] {{.*}} : $@convention(c) (@in_guaranteed Owner) -> @lifetime(borrow address 0) @owned View
 // CHECK: sil {{.*}}[clang getViewFromFirst] {{.*}} : $@convention(c) (@in_guaranteed Owner, @in_guaranteed Owner) -> @lifetime(borrow address 0) @owned View
@@ -182,6 +232,9 @@ using Bytes = Span<unsigned char>;
 // CHECK: sil {{.*}}[clang CaptureView.handOut] {{.*}} : $@convention(cxx_method) (@lifetime(copy 1) @inout View, @in_guaranteed CaptureView) -> ()
 // CHECK: sil {{.*}}[clang NS.getView] {{.*}} : $@convention(c) (@in_guaranteed Owner) -> @lifetime(borrow address 0) @owned View
 // CHECK: sil {{.*}}[clang moveOnlyId] {{.*}} : $@convention(c) (@in_guaranteed MoveOnly) -> @lifetime(borrow {{.*}}0) @out MoveOnly
+// CHECK: sil {{.*}}[clang makeAnonUnionNonEscapable] {{.*}} : $@convention(c) (@in_guaranteed Owner) -> @lifetime(borrow address 0) @owned HasAnonUnion<NonEscapable>
+// CHECK: sil {{.*}}[clang makeAnonStructNonEscapable] {{.*}} : $@convention(c) (@in_guaranteed Owner) -> @lifetime(borrow address 0) @owned HasAnonStruct<NonEscapable>
+// CHECK: sil {{.*}}[clang makeNonEscapableHasAnonUnionNonEscapable] {{.*}} : $@convention(c) (@in_guaranteed Owner) -> @lifetime(borrow address 0) @owned NonEscapableHasAnonUnion<NonEscapable>
 
 //--- test.swift
 
@@ -215,3 +268,10 @@ func canImportMoveOnlyNonEscapable(_ x: borrowing MoveOnly) {
 }
 
 func testInheritedCtors(_ s: rdar153081347.Bytes) {}
+
+func anonymousUnionsAndStructs(_ v: borrowing View) {
+    let o = makeOwner()
+    let _ = makeAnonUnionNonEscapable(o)
+    let _ = makeAnonStructNonEscapable(o)
+    let _ = makeNonEscapableHasAnonUnionNonEscapable(o)
+}

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

@@ -101,8 +101,10 @@ struct HoldsShared {
                                SWIFT_RETURNS_UNRETAINED;
 };
 
-template <typename, typename> struct TTake2 {};
-template <typename T> struct PassThru {};
+template <typename F, typename S> struct SWIFT_ESCAPABLE_IF(F, S) TTake2 {};
+template <typename T> struct PassThru {
+  T field;
+};
 struct IsUnsafe { int *p; };
 struct HasUnsafe : TTake2<PassThru<HasUnsafe>, IsUnsafe> {};
 using AlsoUnsafe = PassThru<HasUnsafe>;
@@ -207,17 +209,15 @@ func useTTakeInt(x: TTakeInt) {
 }
 
 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}}
+  _ = x
 }
 
 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}}
+  _ = x
 }
 
 func useTTakeUnsafeTuple(x: HasUnsafe) {