Merge pull request #81035 from swiftlang/susmonteiro/metadata-private-fields

[cxx-interop] Fix metadata mismatch regarding fields of structs

Author: susmonteiro

lib/IRGen/Field.h

@@ -96,12 +96,20 @@ struct Field {
   bool operator!=(Field other) const { return declOrKind != other.declOrKind; }
 };
 
+// Don't export private C++ fields that were imported as private Swift fields.
+// The type of a private field might not have all the type witness operations
+// that Swift requires, for instance, `std::unique_ptr<IncompleteType>` would
+// not have a destructor.
+bool isExportableField(Field field);
+
 /// Iterate all the fields of the given struct or class type, including
 /// any implicit fields that might be accounted for in
 /// getFieldVectorLength.
 void forEachField(IRGenModule &IGM, const NominalTypeDecl *typeDecl,
                   llvm::function_ref<void(Field field)> fn);
 
+unsigned countExportableFields(IRGenModule &IGM, const NominalTypeDecl *type);
+
 } // end namespace irgen
 } // end namespace swift
 

lib/IRGen/GenMeta.cpp

@@ -1840,7 +1840,7 @@ namespace {
 
     void addLayoutInfo() {
       // uint32_t NumFields;
-      B.addInt32(getNumFields(getType()));
+      B.addInt32(countExportableFields(IGM, getType()));
 
       // uint32_t FieldOffsetVectorOffset;
       B.addInt32(FieldVectorOffset / IGM.getPointerSize());

lib/IRGen/GenReflection.cpp

@@ -946,35 +946,13 @@ class FieldTypeMetadataBuilder : public ReflectionMetadataBuilder {
     B.addInt16(uint16_t(kind));
     B.addInt16(FieldRecordSize);
 
-    // Filter to select which fields we'll export FieldDescriptors for.
-    auto exportable_field =
-      [](Field field) {
-        // Don't export private C++ fields that were imported as private Swift fields.
-        // The type of a private field might not have all the type witness
-        // operations that Swift requires, for instance,
-        // `std::unique_ptr<IncompleteType>` would not have a destructor.
-        if (field.getKind() == Field::Kind::Var &&
-            field.getVarDecl()->getClangDecl() &&
-            field.getVarDecl()->getFormalAccess() == AccessLevel::Private)
-          return false;
-        // All other fields are exportable
-        return true;
-      };
-
-    // Count exportable fields
-    int exportableFieldCount = 0;
-    forEachField(IGM, NTD, [&](Field field) {
-      if (exportable_field(field)) {
-        ++exportableFieldCount;
-      }
-    });
-
     // Emit exportable fields, prefixed with a count
-    B.addInt32(exportableFieldCount);
+    B.addInt32(countExportableFields(IGM, NTD));
+
+    // Filter to select which fields we'll export FieldDescriptor for.
     forEachField(IGM, NTD, [&](Field field) {
-      if (exportable_field(field)) {
+      if (isExportableField(field))
         addField(field);
-      }
     });
   }
 

lib/IRGen/StructLayout.cpp

@@ -589,6 +589,15 @@ unsigned irgen::getNumFields(const NominalTypeDecl *target) {
   return numFields;
 }
 
+bool irgen::isExportableField(Field field) {
+  if (field.getKind() == Field::Kind::Var &&
+      field.getVarDecl()->getClangDecl() &&
+      field.getVarDecl()->getFormalAccess() == AccessLevel::Private)
+    return false;
+  // All other fields are exportable
+  return true;
+}
+
 void irgen::forEachField(IRGenModule &IGM, const NominalTypeDecl *typeDecl,
                          llvm::function_ref<void(Field field)> fn) {
   auto classDecl = dyn_cast<ClassDecl>(typeDecl);
@@ -610,6 +619,17 @@ void irgen::forEachField(IRGenModule &IGM, const NominalTypeDecl *typeDecl,
   }
 }
 
+unsigned irgen::countExportableFields(IRGenModule &IGM,
+                                      const NominalTypeDecl *type) {
+  // Don't count private C++ fields that were imported as private Swift fields
+  unsigned exportableFieldCount = 0;
+  forEachField(IGM, type, [&](Field field) {
+    if (isExportableField(field))
+      ++exportableFieldCount;
+  });
+  return exportableFieldCount;
+}
+
 SILType Field::getType(IRGenModule &IGM, SILType baseType) const {
   switch (getKind()) {
   case Field::Var:

lib/IRGen/StructMetadataVisitor.h

@@ -63,8 +63,11 @@ template <class Impl> class StructMetadataVisitor
 
     // Struct field offsets.
     asImpl().noteStartOfFieldOffsets();
-    for (VarDecl *prop : Target->getStoredProperties())
-      asImpl().addFieldOffset(prop);
+    for (VarDecl *prop : Target->getStoredProperties()) {
+      if (!(prop->getClangDecl() &&
+            prop->getFormalAccess() == AccessLevel::Private))
+        asImpl().addFieldOffset(prop);
+    }
 
     asImpl().noteEndOfFieldOffsets();
 

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

@@ -152,3 +152,9 @@ module PIMPL {
   requires cplusplus
   export *
 }
+
+module SimpleStructs {
+  header "simple-structs.h"
+  requires cplusplus
+  export *
+}

test/Interop/Cxx/class/Inputs/simple-structs.h

@@ -0,0 +1,55 @@
+#ifndef TEST_INTEROP_CXX_CLASS_INPUTS_SIMPLE_STRUCTS_H
+#define TEST_INTEROP_CXX_CLASS_INPUTS_SIMPLE_STRUCTS_H
+
+struct HasPrivateFieldsOnly {
+private:
+  int priv1;
+  int priv2;
+
+public:
+  HasPrivateFieldsOnly(int i1, int i2) : priv1(i1), priv2(i2) {}
+};
+
+struct HasPublicFieldsOnly {
+  int publ1;
+  int publ2;
+
+  HasPublicFieldsOnly(int i1, int i2) : publ1(i1), publ2(i2) {}
+};
+
+struct HasPrivatePublicProtectedFields {
+private:
+  int priv1;
+
+public:
+  int publ1;
+
+protected:
+  int prot1;
+
+protected:
+  int prot2;
+
+private:
+  int priv2;
+
+public:
+  int publ2;
+
+  HasPrivatePublicProtectedFields(int i1, int i2, int i3, int i4, int i5,
+                                  int i6)
+      : priv1(i1), publ1(i2), prot1(i3), prot2(i4), priv2(i5),
+        publ2(i6) {}
+};
+
+struct Outer {
+private:
+  HasPrivatePublicProtectedFields privStruct;
+
+public:
+  HasPrivatePublicProtectedFields publStruct;
+
+  Outer() : privStruct(1, 2, 3, 4, 5, 6), publStruct(7, 8, 9, 10, 11, 12) {}
+};
+
+#endif

test/Interop/Cxx/class/for-each-field.swift

@@ -0,0 +1,76 @@
+// RUN: %target-run-simple-swift(-cxx-interoperability-mode=default -Xfrontend -disable-availability-checking -I %S/Inputs)
+
+// REQUIRES: executable_test
+// REQUIRES: reflection
+
+@_spi(Reflection) import Swift
+import SimpleStructs
+import StdlibUnittest
+
+func checkFieldsWithKeyPath<T>(
+  of type: T.Type,
+  options: _EachFieldOptions = [],
+  fields: [String: PartialKeyPath<T>]
+) {
+  var count = 0
+
+  _forEachFieldWithKeyPath(of: T.self, options: options) {
+    charPtr, keyPath in
+    count += 1
+
+    let fieldName = String(cString: charPtr)
+    if fieldName == "" {
+      expectTrue(false, "Empty field name")
+      return true
+    }
+    guard let checkKeyPath = fields[fieldName] else {
+      expectTrue(false, "Unexpected field '\(fieldName)'")
+      return true
+    }
+
+    expectTrue(checkKeyPath == keyPath)
+    return true
+  }
+
+  expectEqual(fields.count, count)
+}
+
+var ForEachFieldTestSuite = TestSuite("ForEachField")
+
+ForEachFieldTestSuite.test("HasPrivateFieldsOnly") {
+  checkFieldsWithKeyPath(
+    of: HasPrivateFieldsOnly.self,
+    fields: [:]
+  )
+}
+
+ForEachFieldTestSuite.test("HasPublicFieldsOnly") {
+  checkFieldsWithKeyPath(
+    of: HasPublicFieldsOnly.self,
+    fields: [
+      "publ1": \HasPublicFieldsOnly.publ1,
+      "publ2": \HasPublicFieldsOnly.publ2
+    ]
+  )
+}
+
+ForEachFieldTestSuite.test("HasPrivatePublicProtectedFields") {
+  checkFieldsWithKeyPath(
+    of: HasPrivatePublicProtectedFields.self,
+    fields: [
+      "publ1": \HasPrivatePublicProtectedFields.publ1,
+      "publ2": \HasPrivatePublicProtectedFields.publ2
+    ]
+  )
+}
+
+ForEachFieldTestSuite.test("Outer") {
+  checkFieldsWithKeyPath(
+    of: Outer.self,
+    fields: [
+      "publStruct": \Outer.publStruct
+    ]
+  )
+}
+
+runAllTests()

test/Interop/Cxx/class/print-simple-structs.swift

@@ -0,0 +1,37 @@
+// RUN: %target-run-simple-swift(-cxx-interoperability-mode=default -Xfrontend -disable-availability-checking -I %S/Inputs) | %FileCheck %s
+
+// REQUIRES: executable_test
+
+import SimpleStructs
+
+func printCxxStructPrivateFields() {
+    let s = HasPrivateFieldsOnly(1, 2)
+    print(s)
+}
+
+func printCxxStructPublicFields() {
+    let s = HasPublicFieldsOnly(1, 2)
+    print(s)
+}
+
+func printCxxStructPrivatePublicProtectedFields() {
+    let s = HasPrivatePublicProtectedFields(1, 2, 3, 4, 5, 6)
+    print(s)
+}
+
+func printCxxStructNested() {
+    let s = Outer()
+    print(s)
+}
+
+printCxxStructPrivateFields() 
+// CHECK: HasPrivateFieldsOnly()
+
+printCxxStructPublicFields()
+// CHECK: HasPublicFieldsOnly(publ1: 1, publ2: 2)
+
+printCxxStructPrivatePublicProtectedFields()
+// CHECK: HasPrivatePublicProtectedFields(publ1: 2, publ2: 6)
+
+printCxxStructNested()
+// CHECK: Outer(publStruct: {{.*}}.HasPrivatePublicProtectedFields(publ1: 8, publ2: 12))

test/Interop/Cxx/stdlib/Inputs/std-string-and-vector.h

@@ -9,3 +9,28 @@ struct Item {
 inline Item get_item() {
   return {};
 }
+
+std::vector<int> makeVecOfInt() { return {1, 2, 3}; }
+std::vector<std::string> makeVecOfString() { return {"Hello", "World"}; }
+
+struct S {
+private:
+  std::string privStr;
+  std::vector<std::string> privVec;
+
+public:
+  std::string pubStr;
+  std::vector<std::string> pubVec;
+
+protected:
+  std::string protStr;
+  std::vector<std::string> protVec;
+
+public:
+  S() : privStr("private"), privVec({"private", "vector"}), 
+        pubStr("public"), pubVec({"a", "public", "vector"}), 
+        protStr("protected"), protVec({"protected", "vector"}) {}
+
+  std::vector<std::string> getPrivVec() const { return privVec; }
+  std::string getProtStr() const { return protStr; }
+};

test/Interop/Cxx/stdlib/Inputs/std-unique-ptr.h

@@ -2,6 +2,7 @@
 #define TEST_INTEROP_CXX_STDLIB_INPUTS_STD_UNIQUE_PTR_H
 
 #include <memory>
+#include <string>
 
 struct NonCopyable {
     NonCopyable(int x) : x(x) {}
@@ -29,6 +30,10 @@ std::unique_ptr<int> makeInt() {
   return std::make_unique<int>(42);
 }
 
+std::unique_ptr<std::string> makeString() {
+  return std::make_unique<std::string>("Unique string");
+}
+
 std::unique_ptr<int[]> makeArray() {
   int *array = new int[3];
   array[0] = 1;
@@ -55,4 +60,10 @@ std::unique_ptr<HasDtor> makeDtor() {
   return std::make_unique<HasDtor>();
 }
 
+std::shared_ptr<int> makeIntShared() { return std::make_unique<int>(42); }
+
+std::shared_ptr<std::string> makeStringShared() {
+  return std::make_unique<std::string>("Shared string");
+}
+
 #endif // TEST_INTEROP_CXX_STDLIB_INPUTS_STD_UNIQUE_PTR_H