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

Lazy loading checked if the ClangDecl was hidden, but loading all
members did not. Let's make loadAllMembers() behave like the lazy
path, and fix some of the mock SDKs in the test suite.

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

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 *
 }