Merge pull request #76892 from swiftlang/egorzhdan/cxx-private-fields

[cxx-interop] Import private fields of C++ structs

Author: egorzhdan

lib/ClangImporter/ClangImporter.cpp

@@ -5777,6 +5777,9 @@ makeBaseClassMemberAccessors(DeclContext *declContext,
     bodyParams = ParameterList::createEmpty(ctx);
   }
 
+  assert(baseClassVar->getFormalAccess() == AccessLevel::Public &&
+         "base class member must be public");
+
   auto getterDecl = AccessorDecl::create(
       ctx,
       /*FuncLoc=*/SourceLoc(),
@@ -7325,6 +7328,10 @@ Decl *ClangImporter::importDeclDirectly(const clang::NamedDecl *decl) {
 
 ValueDecl *ClangImporter::Implementation::importBaseMemberDecl(
     ValueDecl *decl, DeclContext *newContext) {
+  // Do not clone private C++ decls.
+  if (decl->getFormalAccess() < AccessLevel::Public)
+    return nullptr;
+
   // Make sure we don't clone the decl again for this class, as that would
   // result in multiple definitions of the same symbol.
   std::pair<ValueDecl *, DeclContext *> key = {decl, newContext};

lib/ClangImporter/ImportDecl.cpp

@@ -4117,8 +4117,17 @@ namespace {
 
       auto type = importedType.getType();
 
+      // Private C++ fields should also be private in Swift. Since Swift does
+      // not have a notion of protected field, map protected C++ fields to
+      // private Swift fields.
+      AccessLevel accessLevel =
+          (decl->getAccess() == clang::AccessSpecifier::AS_private ||
+           decl->getAccess() == clang::AccessSpecifier::AS_protected)
+              ? AccessLevel::Private
+              : AccessLevel::Public;
+
       auto result =
-        Impl.createDeclWithClangNode<VarDecl>(decl, AccessLevel::Public,
+        Impl.createDeclWithClangNode<VarDecl>(decl, accessLevel,
                               /*IsStatic*/ false,
                               VarDecl::Introducer::Var,
                               Impl.importSourceLoc(decl->getLocation()),
@@ -8825,15 +8834,15 @@ ClangImporter::Implementation::importDeclImpl(const clang::NamedDecl *ClangDecl,
   if (ClangDecl->isInvalidDecl())
     return nullptr;
 
-  // Private and protected C++ class members should never be used, so we skip
-  // them entirely (instead of importing them with a corresponding Swift access
-  // level) to remove clutter from the module interface.
-  //
-  // We omit protected members in addition to private members because Swift
-  // structs can't inherit from C++ classes, so there's effectively no way to
-  // access them.
+  // Private and protected C++ class members should never be used from Swift,
+  // however, parts of the Swift typechecker rely on being able to iterate over
+  // all of the stored fields of a particular struct. This means we still need
+  // to add private fields to the Swift AST.
+  // 
+  // Other kinds of private and protected C++ decls are not relevant for Swift.
   clang::AccessSpecifier access = ClangDecl->getAccess();
-  if (access == clang::AS_protected || access == clang::AS_private)
+  if ((access == clang::AS_protected || access == clang::AS_private) &&
+      !isa<clang::FieldDecl>(ClangDecl))
     return nullptr;
 
   bool SkippedOverTypedef = false;
@@ -9530,15 +9539,18 @@ void ClangImporter::Implementation::loadAllMembersOfRecordDecl(
   for (auto member: members) {
     // This means we found a member in a C++ record's base class.
     if (swiftDecl->getClangDecl() != clangRecord) {
+      auto namedMember = cast<ValueDecl>(member);
+      if (namedMember->getFormalAccess() < AccessLevel::Public)
+        continue;
       // Do not clone the base member into the derived class
       // when the derived class already has a member of such
       // name and arity.
       auto memberArity =
-          getImportedBaseMemberDeclArity(cast<ValueDecl>(member));
+          getImportedBaseMemberDeclArity(namedMember);
       bool shouldAddBaseMember = true;
       for (const auto *currentMember : swiftDecl->getMembers()) {
         auto vd = dyn_cast<ValueDecl>(currentMember);
-        if (vd->getName() == cast<ValueDecl>(member)->getName()) {
+        if (vd->getName() == namedMember->getName()) {
           if (memberArity == getImportedBaseMemberDeclArity(vd)) {
             shouldAddBaseMember = false;
             break;
@@ -9548,7 +9560,7 @@ void ClangImporter::Implementation::loadAllMembersOfRecordDecl(
       if (!shouldAddBaseMember)
         continue;
       // So we need to clone the member into the derived class.
-      if (auto newDecl = importBaseMemberDecl(cast<ValueDecl>(member), swiftDecl)) {
+      if (auto newDecl = importBaseMemberDecl(namedMember, swiftDecl)) {
         swiftDecl->addMember(newDecl);
       }
       continue;

lib/ClangImporter/SwiftDeclSynthesizer.cpp

@@ -96,7 +96,7 @@ static AccessorDecl *makeFieldGetterDecl(ClangImporter::Implementation &Impl,
       /*Throws=*/false,
       /*ThrowsLoc=*/SourceLoc(), /*ThrownType=*/TypeLoc(),
       params, getterType, importedDecl, clangNode);
-  getterDecl->setAccess(AccessLevel::Public);
+  getterDecl->setAccess(importedFieldDecl->getFormalAccess());
   getterDecl->setIsObjC(false);
   getterDecl->setIsDynamic(false);
 
@@ -128,7 +128,7 @@ static AccessorDecl *makeFieldSetterDecl(ClangImporter::Implementation &Impl,
   setterDecl->setIsObjC(false);
   setterDecl->setIsDynamic(false);
   setterDecl->setSelfAccessKind(SelfAccessKind::Mutating);
-  setterDecl->setAccess(AccessLevel::Public);
+  setterDecl->setAccess(importedFieldDecl->getFormalAccess());
 
   return setterDecl;
 }

lib/Sema/TypeCheckInvertible.cpp

@@ -18,6 +18,7 @@
 
 #include "TypeCheckInvertible.h"
 #include "swift/AST/ASTContext.h"
+#include "swift/AST/ClangModuleLoader.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/Basic/Assertions.h"
 #include "TypeChecker.h"
@@ -309,6 +310,23 @@ bool StorageVisitor::visit(NominalTypeDecl *nominal, DeclContext *dc) {
         return true;
     }
 
+    // If this is a C++ struct, walk the members of its base types.
+    if (auto cxxRecordDecl =
+            dyn_cast_or_null<clang::CXXRecordDecl>(nominal->getClangDecl())) {
+      for (auto cxxBase : cxxRecordDecl->bases()) {
+        if (auto cxxBaseDecl = cxxBase.getType()->getAsCXXRecordDecl()) {
+          auto clangModuleLoader = dc->getASTContext().getClangModuleLoader();
+          auto importedDecl =
+              clangModuleLoader->importDeclDirectly(cxxBaseDecl);
+          if (auto nominalBaseDecl =
+                  dyn_cast_or_null<NominalTypeDecl>(importedDecl)) {
+            if (visit(nominalBaseDecl, dc))
+              return true;
+          }
+        }
+      }
+    }
+
     return false;
   }
 

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

@@ -78,6 +78,11 @@ module ProtocolConformance {
   requires cplusplus
 }
 
+module Sendable {
+  header "sendable.h"
+  requires cplusplus
+}
+
 module StructuredBindingsGetMethod {
   header "structured-bindings-get-method.h"
   requires cplusplus

test/Interop/Cxx/class/Inputs/sendable.h

@@ -0,0 +1,39 @@
+struct HasPrivatePointerField {
+private:
+  const int *ptr;
+};
+
+struct HasProtectedPointerField {
+protected:
+  const int *ptr;
+};
+
+struct HasPublicPointerField {
+  const int *ptr;
+};
+
+struct HasPrivateNonSendableField {
+private:
+  HasPrivatePointerField f;
+};
+
+struct HasProtectedNonSendableField {
+protected:
+  HasProtectedPointerField f;
+};
+
+struct HasPublicNonSendableField {
+  HasPublicPointerField f;
+};
+
+struct DerivedFromHasPublicPointerField : HasPublicPointerField {};
+struct DerivedFromHasPublicNonSendableField : HasPublicNonSendableField {};
+struct DerivedFromHasPrivatePointerField : HasPrivatePointerField {};
+
+struct DerivedPrivatelyFromHasPublicPointerField : private HasPublicPointerField {};
+struct DerivedPrivatelyFromHasPublicNonSendableField : private HasPublicNonSendableField {};
+struct DerivedPrivatelyFromHasPrivatePointerField : private HasPrivatePointerField {};
+
+struct DerivedProtectedFromHasPublicPointerField : protected HasPublicPointerField {};
+struct DerivedProtectedFromHasPublicNonSendableField : protected HasPublicNonSendableField {};
+struct DerivedProtectedFromHasPrivatePointerField : protected HasPrivatePointerField {};

test/Interop/Cxx/class/access-specifiers-module-interface.swift

@@ -1,7 +1,7 @@
 // Test module interface produced for C++ access specifiers test.
 // In particular, we don't want any of the private members showing up here.
 
-// RUN: %target-swift-ide-test -print-module -module-to-print=AccessSpecifiers -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s
+// RUN: %target-swift-ide-test -print-module -module-to-print=AccessSpecifiers -access-filter-public -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s
 
 // CHECK:      struct PublicPrivate {
 // CHECK-NEXT:   init()

test/Interop/Cxx/class/access-specifiers-typechecker.swift

@@ -26,7 +26,7 @@ var publicFlagEnumVar: PublicPrivate.PublicFlagEnum
 
 // Cannot access any private members and types.
 
-v.PrivateMemberVar = 1 // expected-error {{value of type 'PublicPrivate' has no member 'PrivateMemberVar'}}
+v.PrivateMemberVar = 1 // expected-error {{'PrivateMemberVar' is inaccessible due to 'private' protection level}}
 PublicPrivate.PrivateStaticMemberVar = 1 // expected-error {{'PublicPrivate' has no member 'PrivateStaticMemberVar'}}
 v.privateMemberFunc() // expected-error {{value of type 'PublicPrivate' has no member 'privateMemberFunc'}}
 

test/Interop/Cxx/class/inheritance/using-base-members-module-interface.swift

@@ -1,5 +1,5 @@
-// RUN: %target-swift-ide-test -print-module -module-to-print=UsingBaseMembers -I %S/Inputs -source-filename=x -cxx-interoperability-mode=swift-6 | %FileCheck %s
-// RUN: %target-swift-ide-test -print-module -module-to-print=UsingBaseMembers -I %S/Inputs -source-filename=x -cxx-interoperability-mode=upcoming-swift | %FileCheck %s
+// RUN: %target-swift-ide-test -print-module -module-to-print=UsingBaseMembers -access-filter-public -I %S/Inputs -source-filename=x -cxx-interoperability-mode=swift-6 | %FileCheck %s
+// RUN: %target-swift-ide-test -print-module -module-to-print=UsingBaseMembers -access-filter-public -I %S/Inputs -source-filename=x -cxx-interoperability-mode=upcoming-swift | %FileCheck %s
 
 // CHECK: struct PublicBase {
 // CHECK-NEXT:   init()

test/Interop/Cxx/class/memberwise-initializer-module-interface.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-ide-test -print-module -module-to-print=MemberwiseInitializer -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s
+// RUN: %target-swift-ide-test -print-module -module-to-print=MemberwiseInitializer -access-filter-public -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s
 
 // CHECK:      struct StructPrivateOnly {
 // CHECK-NEXT:   init()

test/Interop/Cxx/class/sendable-typechecker.swift

@@ -0,0 +1,25 @@
+// RUN: %target-typecheck-verify-swift -I %S/Inputs -swift-version 6 -cxx-interoperability-mode=upcoming-swift
+
+import Sendable // expected-warning {{add '@preconcurrency' to treat 'Sendable'-related errors from module 'Sendable' as warnings}}
+
+func takesSendable<T: Sendable>(_ x: T.Type) {}
+
+takesSendable(HasPrivatePointerField.self) // expected-error {{type 'HasPrivatePointerField' does not conform to the 'Sendable' protocol}}
+takesSendable(HasProtectedPointerField.self) // expected-error {{type 'HasProtectedPointerField' does not conform to the 'Sendable' protocol}}
+takesSendable(HasPublicPointerField.self) // expected-error {{type 'HasPublicPointerField' does not conform to the 'Sendable' protocol}}
+
+takesSendable(HasPrivateNonSendableField.self) // expected-error {{type 'HasPrivateNonSendableField' does not conform to the 'Sendable' protocol}}
+takesSendable(HasProtectedNonSendableField.self) // expected-error {{type 'HasProtectedNonSendableField' does not conform to the 'Sendable' protocol}}
+takesSendable(HasPublicNonSendableField.self) // expected-error {{type 'HasPublicNonSendableField' does not conform to the 'Sendable' protocol}}
+
+takesSendable(DerivedFromHasPublicPointerField.self) // expected-error {{type 'DerivedFromHasPublicPointerField' does not conform to the 'Sendable' protocol}}
+takesSendable(DerivedFromHasPublicNonSendableField.self) // expected-error {{type 'DerivedFromHasPublicNonSendableField' does not conform to the 'Sendable' protocol}}
+takesSendable(DerivedFromHasPrivatePointerField.self) // expected-error {{type 'DerivedFromHasPrivatePointerField' does not conform to the 'Sendable' protocol}}
+
+takesSendable(DerivedPrivatelyFromHasPublicPointerField.self) // expected-error {{type 'DerivedPrivatelyFromHasPublicPointerField' does not conform to the 'Sendable' protocol}}
+takesSendable(DerivedPrivatelyFromHasPublicNonSendableField.self) // expected-error {{type 'DerivedPrivatelyFromHasPublicNonSendableField' does not conform to the 'Sendable' protocol}}
+takesSendable(DerivedPrivatelyFromHasPrivatePointerField.self) // expected-error {{type 'DerivedPrivatelyFromHasPrivatePointerField' does not conform to the 'Sendable' protocol}}
+
+takesSendable(DerivedProtectedFromHasPublicPointerField.self) // expected-error {{type 'DerivedProtectedFromHasPublicPointerField' does not conform to the 'Sendable' protocol}}
+takesSendable(DerivedProtectedFromHasPublicNonSendableField.self) // expected-error {{type 'DerivedProtectedFromHasPublicNonSendableField' does not conform to the 'Sendable' protocol}}
+takesSendable(DerivedProtectedFromHasPrivatePointerField.self) // expected-error {{type 'DerivedProtectedFromHasPrivatePointerField' does not conform to the 'Sendable' protocol}}

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift

@@ -263,9 +263,9 @@
 
 // CHECK:      struct PrivatePropertyWithSameName {
 // CHECK-NEXT:    init()
-// CHECK-NEXT:    var value: Int32
 // CHECK-NEXT:    func getValue() -> Int32
 // CHECK-NEXT:    mutating func setValue(_ i: Int32)
+// CHECK-NEXT:    var value: Int32
 // CHECK-NEXT: }
 
 // CHECK:      struct SnakeCaseGetterSetter {

test/Interop/Cxx/operators/member-inline-module-interface.swift

@@ -243,6 +243,7 @@
 // CHECK-NEXT:   mutating func __operatorStar() -> UnsafeMutablePointer<Int32>
 // CHECK-NEXT:   @available(*, unavailable, message: "use .pointee property")
 // CHECK-NEXT:   func __operatorStar() -> UnsafePointer<Int32>
+// CHECK-NEXT:   var value: Int32
 // CHECK-NEXT: }
 
 // CHECK: struct AmbiguousOperatorStar2 {
@@ -254,6 +255,7 @@
 // CHECK-NEXT:   func __operatorStar() -> UnsafePointer<Int32>
 // CHECK-NEXT:   @available(*, unavailable, message: "use .pointee property")
 // CHECK-NEXT:   func __operatorStar() -> UnsafePointer<Int32>
+// CHECK-NEXT:   var value: Int32
 // CHECK-NEXT: }
 
 // CHECK: struct DerivedFromConstIterator {