Merge pull request swiftlang#29418 from slavapestov/lazy-member-loading-inconsistency

ClangImporter: Reconcile Clang declaration hidden-ness between loadAllMembers() and lazy loading

Author: slavapestov

lib/ClangImporter/ClangImporter.cpp

@@ -2867,22 +2867,23 @@ void ClangModuleUnit::lookupValue(DeclName name, NLKind lookupKind,
   }
 }
 
-/// Determine whether the given Clang entry is visible.
-///
-/// FIXME: this is an elaborate hack to badly reflect Clang's
-/// submodule visibility into Swift.
-static bool isVisibleClangEntry(clang::ASTContext &ctx,
-                                SwiftLookupTable::SingleEntry entry) {
-  if (auto clangDecl = entry.dyn_cast<clang::NamedDecl *>()) {
-    // For a declaration, check whether the declaration is hidden.
-    if (!clangDecl->isHidden()) return true;
+bool ClangImporter::Implementation::isVisibleClangEntry(
+    const clang::NamedDecl *clangDecl) {
+  // For a declaration, check whether the declaration is hidden.
+  if (!clangDecl->isHidden()) return true;
 
-    // Is any redeclaration visible?
-    for (auto redecl : clangDecl->redecls()) {
-      if (!cast<clang::NamedDecl>(redecl)->isHidden()) return true;
-    }
+  // Is any redeclaration visible?
+  for (auto redecl : clangDecl->redecls()) {
+    if (!cast<clang::NamedDecl>(redecl)->isHidden()) return true;
+  }
 
-    return false;
+  return false;
+}
+
+bool ClangImporter::Implementation::isVisibleClangEntry(
+  SwiftLookupTable::SingleEntry entry) {
+  if (auto clangDecl = entry.dyn_cast<clang::NamedDecl *>()) {
+    return isVisibleClangEntry(clangDecl);
   }
 
   // If it's a macro from a module, check whether the module has been imported.
@@ -2917,15 +2918,13 @@ ClangModuleUnit::lookupNestedType(Identifier name,
   if (!baseTypeContext)
     return nullptr;
 
-  auto &clangCtx = owner.getClangASTContext();
-
   // FIXME: This is very similar to what's in Implementation::lookupValue and
   // Implementation::loadAllMembers.
   SmallVector<TypeDecl *, 2> results;
   for (auto entry : lookupTable->lookup(SerializedSwiftName(name.str()),
                                         baseTypeContext)) {
     // If the entry is not visible, skip it.
-    if (!isVisibleClangEntry(clangCtx, entry)) continue;
+    if (!owner.isVisibleClangEntry(entry)) continue;
 
     auto *clangDecl = entry.dyn_cast<clang::NamedDecl *>();
     if (!clangDecl)
@@ -2993,14 +2992,13 @@ void ClangImporter::loadExtensions(NominalTypeDecl *nominal,
 
   // Dig through each of the Swift lookup tables, creating extensions
   // where needed.
-  auto &clangCtx = Impl.getClangASTContext();
   (void)Impl.forEachLookupTable([&](SwiftLookupTable &table) -> bool {
       // FIXME: If we already looked at this for this generation,
       // skip.
 
       for (auto entry : table.allGlobalsAsMembersInContext(effectiveClangContext)) {
         // If the entry is not visible, skip it.
-        if (!isVisibleClangEntry(clangCtx, entry)) continue;
+        if (!Impl.isVisibleClangEntry(entry)) continue;
 
         if (auto decl = entry.dyn_cast<clang::NamedDecl *>()) {
           // Import the context of this declaration, which has the
@@ -3581,7 +3579,7 @@ void ClangImporter::Implementation::lookupValue(
 
   for (auto entry : table.lookup(name.getBaseName(), clangTU)) {
     // If the entry is not visible, skip it.
-    if (!isVisibleClangEntry(clangCtx, entry)) continue;
+    if (!isVisibleClangEntry(entry)) continue;
 
     ValueDecl *decl;
     // If it's a Clang declaration, try to import it.
@@ -3685,11 +3683,9 @@ void ClangImporter::Implementation::lookupObjCMembers(
        SwiftLookupTable &table,
        DeclName name,
        VisibleDeclConsumer &consumer) {
-  auto &clangCtx = getClangASTContext();
-
   for (auto clangDecl : table.lookupObjCMembers(name.getBaseName())) {
     // If the entry is not visible, skip it.
-    if (!isVisibleClangEntry(clangCtx, clangDecl)) continue;
+    if (!isVisibleClangEntry(clangDecl)) continue;
 
     forEachDistinctName(clangDecl,
                         [&](ImportedName importedName,
@@ -3801,8 +3797,6 @@ ClangImporter::Implementation::loadNamedMembers(
   auto table = findLookupTable(*CMO);
   assert(table && "clang module without lookup table");
 
-  clang::ASTContext &clangCtx = getClangASTContext();
-
   assert(isa<clang::ObjCContainerDecl>(CD) || isa<clang::NamespaceDecl>(CD));
 
   ensureSuperclassMembersAreLoaded(dyn_cast<ClassDecl>(D));
@@ -3812,7 +3806,7 @@ ClangImporter::Implementation::loadNamedMembers(
                                   effectiveClangContext)) {
     if (!entry.is<clang::NamedDecl *>()) continue;
     auto member = entry.get<clang::NamedDecl *>();
-    if (!isVisibleClangEntry(clangCtx, member)) continue;
+    if (!isVisibleClangEntry(member)) continue;
 
     // Skip Decls from different clang::DeclContexts
     if (member->getDeclContext() != CDC) continue;
@@ -3833,7 +3827,7 @@ ClangImporter::Implementation::loadNamedMembers(
                                                   effectiveClangContext)) {
     if (!entry.is<clang::NamedDecl *>()) continue;
     auto member = entry.get<clang::NamedDecl *>();
-    if (!isVisibleClangEntry(clangCtx, member)) continue;
+    if (!isVisibleClangEntry(member)) continue;
 
      // Skip Decls from different clang::DeclContexts
     if (member->getDeclContext() != CDC) continue;

lib/ClangImporter/ImportDecl.cpp

@@ -8670,7 +8670,8 @@ void ClangImporter::Implementation::collectMembersToAdd(
   for (const clang::Decl *m : objcContainer->decls()) {
     auto nd = dyn_cast<clang::NamedDecl>(m);
     if (nd && nd == nd->getCanonicalDecl() &&
-        nd->getDeclContext() == objcContainer)
+        nd->getDeclContext() == objcContainer &&
+        isVisibleClangEntry(nd))
       insertMembersAndAlternates(nd, members);
   }
 

lib/ClangImporter/ImporterImpl.h

@@ -1299,6 +1299,13 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   /// false otherwise.
   bool forEachLookupTable(llvm::function_ref<bool(SwiftLookupTable &table)> fn);
 
+  /// Determine whether the given Clang entry is visible.
+  ///
+  /// FIXME: this is an elaborate hack to badly reflect Clang's
+  /// submodule visibility into Swift.
+  bool isVisibleClangEntry(const clang::NamedDecl *clangDecl);
+  bool isVisibleClangEntry(SwiftLookupTable::SingleEntry entry);
+
   /// Look for namespace-scope values with the given name in the given
   /// Swift lookup table.
   void lookupValue(SwiftLookupTable &table, DeclName name,

test/ClangImporter/Inputs/SwiftPrivateAttr.txt

@@ -1,4 +1,4 @@
-import Foundation
+@_exported import Foundation
 
 protocol __PrivProto {
 }

test/ClangImporter/Inputs/custom-modules/module.map

@@ -165,6 +165,7 @@ module SubclassExistentialsExtra {
 
 module SwiftPrivateAttr {
   header "SwiftPrivateAttr.h"
+  export *
 }
 
 module TestProtocols { header "Protocols.h" }

test/IDE/Inputs/custom-modules/ImportAsMemberC.h

@@ -1,3 +1,5 @@
+@import ObjectiveC;
+
 typedef const void *CFTypeRef __attribute__((objc_bridge(id)));
 
 typedef const struct __attribute__((objc_bridge(id))) CCPowerSupply *CCPowerSupplyRef;

test/IDE/Inputs/custom-modules/module.map

@@ -35,6 +35,7 @@ module ImportAsMember {
   module C {
     requires objc
     header "ImportAsMemberC.h"
+    export *
   }
 
   module APINotes {

test/IDE/import_as_member.swift

@@ -106,6 +106,9 @@
 // PRINT-APINOTES-4:      @available(swift, obsoleted: 3, renamed: "Struct1.NewApiNoteType")
 // PRINT-APINOTES-4-NEXT: typealias IAMStruct1APINoteType = Struct1.NewApiNoteType
 
+#if canImport(Foundation)
+import Foundation
+#endif
 import ImportAsMember.A
 import ImportAsMember.B
 import ImportAsMember.APINotes

test/IRGen/Inputs/usr/include/module.map

@@ -1,32 +1,40 @@
 
 module gizmo {
   header "Gizmo.h"
+  export *
 }
 
 module SRoA {
   header "SRoA.h"
+  export *
 }
 
 module ObjectiveC {
   header "BridgeTestObjectiveC.h"
+  export *
 }
 
 module CoreCooling {
   header "CoreCooling.h"
+  export *
 }
 
 module Foundation {
   header "BridgeTestFoundation.h"
+  export *
 }
 
 module CoreFoundation {
   header "BridgeTestCoreFoundation.h"
+  export *
 }
 
 module ObjCInterop {
   header "ObjCInterop.h"
+  export *
 }
 
 module error_domains {
   header "error_domains.h"
+  export *
 }

test/SILGen/import_as_member.swift

@@ -2,6 +2,7 @@
 // REQUIRES: objc_interop
 import ImportAsMember.A
 import ImportAsMember.Class
+import Foundation
 
 public func returnGlobalVar() -> Double {
 	return Struct1.globalVar
@@ -65,7 +66,6 @@ extension SomeClass {
 }
 
 public func useSpecialInit() -> Struct1 {
-  // FIXME: the below triggers an assert, due to number or params mismatch
-  // return Struct1(specialLabel:())
+  return Struct1(specialLabel:())
 }
 

test/api-digester/Inputs/Foo-new-version/foo.h

@@ -1,4 +1,4 @@
-#import <Foundation.h>
+@import ObjectiveC;
 
 @protocol ObjcProt
   -(void) someFunctionFromProt;

test/api-digester/Inputs/Foo-new-version/module.modulemap

@@ -1,3 +1,4 @@
 module Foo {
   header "foo.h"
+  export *
 }

test/api-digester/Inputs/Foo/foo.h

@@ -1,4 +1,4 @@
-#import <Foundation.h>
+@import ObjectiveC;
 
 @protocol ObjcProt
   -(void) someFunctionFromProt;

test/api-digester/Inputs/Foo/module.modulemap

@@ -1,3 +1,4 @@
 module Foo {
   header "foo.h"
+  export *
 }

validation-test/Reflection/Inputs/ObjCClasses/module.map

@@ -1,3 +1,4 @@
 module ObjCClasses {
   header "ObjCClasses.h"
+  export *
 }