Merge pull request #19991 from jrose-apple/oh-node-you-didnt (#19997)

[ClangImporter] Account for synthesized types when filtering by module

(cherry picked from commit 1b847f3)

Author: jrose-apple

lib/ClangImporter/ClangImporter.cpp

@@ -2013,72 +2013,89 @@ getClangOwningModule(ClangNode Node, const clang::ASTContext &ClangCtx) {
   return nullptr;
 }
 
+static const clang::Module *
+getClangTopLevelOwningModule(ClangNode Node,
+                             const clang::ASTContext &ClangCtx) {
+  const clang::Module *OwningModule = getClangOwningModule(Node, ClangCtx);
+  if (!OwningModule)
+    return nullptr;
+  return OwningModule->getTopLevelModule();
+}
+
 static bool isVisibleFromModule(const ClangModuleUnit *ModuleFilter,
                                 const ValueDecl *VD) {
-  // Include a value from module X if:
-  // * no particular module was requested, or
-  // * module X was specifically requested.
-  if (!ModuleFilter)
-    return true;
+  assert(ModuleFilter);
 
   auto ContainingUnit = VD->getDeclContext()->getModuleScopeContext();
   if (ModuleFilter == ContainingUnit)
     return true;
 
+  // The rest of this function is looking to see if the Clang entity that
+  // caused VD to be imported has redeclarations in the filter module.
   auto Wrapper = dyn_cast<ClangModuleUnit>(ContainingUnit);
   if (!Wrapper)
     return false;
 
   auto ClangNode = VD->getClangNode();
-  assert(ClangNode);
+  if (!ClangNode) {
+    // If we synthesized a ValueDecl, it won't have a Clang node. But so far
+    // all the situations where we synthesize top-level declarations are
+    // situations where we don't have to worry about C redeclarations.
+    // We should only consider the declaration visible from its owning module.
+    auto *SynthesizedTypeAttr =
+        VD->getAttrs().getAttribute<ClangImporterSynthesizedTypeAttr>();
+    assert(SynthesizedTypeAttr);
+
+    // When adding new ClangImporterSynthesizedTypeAttr::Kinds, make sure that
+    // the above statement still holds: "we don't want to allow these
+    // declarations to be treated as present in multiple modules".
+    switch (SynthesizedTypeAttr->getKind()) {
+    case ClangImporterSynthesizedTypeAttr::Kind::NSErrorWrapper:
+    case ClangImporterSynthesizedTypeAttr::Kind::NSErrorWrapperAnon:
+      break;
+    }
 
+    return false;
+  }
+
+  // Macros can be "redeclared" by putting an equivalent definition in two
+  // different modules. (We don't actually check the equivalence.)
+  // FIXME: We're also not checking if the redeclaration is in /this/ module.
+  if (ClangNode.getAsMacro())
+    return true;
+
+  const clang::Decl *D = ClangNode.castAsDecl();
   auto &ClangASTContext = ModuleFilter->getClangASTContext();
-  auto OwningClangModule = getClangOwningModule(ClangNode, ClangASTContext);
 
   // We don't handle Clang submodules; pop everything up to the top-level
   // module.
-  if (OwningClangModule)
-    OwningClangModule = OwningClangModule->getTopLevelModule();
-
+  auto OwningClangModule = getClangTopLevelOwningModule(ClangNode,
+                                                        ClangASTContext);
   if (OwningClangModule == ModuleFilter->getClangModule())
     return true;
 
-  if (auto D = ClangNode.getAsDecl()) {
-    // Handle redeclared decls.
-    if (isa<clang::FunctionDecl>(D) || isa<clang::VarDecl>(D) ||
-        isa<clang::TypedefNameDecl>(D)) {
-      for (auto Redeclaration : D->redecls()) {
-        if (Redeclaration == D)
-          continue;
-        auto OwningClangModule = getClangOwningModule(Redeclaration,
-                                                      ClangASTContext);
-        if (OwningClangModule)
-          OwningClangModule = OwningClangModule->getTopLevelModule();
+  // Handle redeclarable Clang decls by checking each redeclaration.
+  bool IsTagDecl = isa<clang::TagDecl>(D);
+  if (!(IsTagDecl || isa<clang::FunctionDecl>(D) || isa<clang::VarDecl>(D) ||
+        isa<clang::TypedefNameDecl>(D))) {
+    return false;
+  }
 
-        if (OwningClangModule == ModuleFilter->getClangModule())
-          return true;
-      }
-    } else if (isa<clang::TagDecl>(D)) {
-      for (auto Redeclaration : D->redecls()) {
-        if (Redeclaration == D)
-          continue;
-        if (!cast<clang::TagDecl>(Redeclaration)->isCompleteDefinition())
-          continue;
-        auto OwningClangModule = getClangOwningModule(Redeclaration,
-                                                      ClangASTContext);
-        if (OwningClangModule)
-          OwningClangModule = OwningClangModule->getTopLevelModule();
+  for (auto Redeclaration : D->redecls()) {
+    if (Redeclaration == D)
+      continue;
 
-        if (OwningClangModule == ModuleFilter->getClangModule())
-          return true;
-      }
-    }
-  }
+    // For enums, structs, and unions, only count definitions when looking to
+    // see what other modules they appear in.
+    if (IsTagDecl)
+      if (!cast<clang::TagDecl>(Redeclaration)->isCompleteDefinition())
+        continue;
 
-  // Macros can be "redeclared" too, by putting an equivalent definition in two
-  // different modules.
-  if (ClangNode.getAsMacro())
-    return true;
+    auto OwningClangModule = getClangTopLevelOwningModule(Redeclaration,
+                                                          ClangASTContext);
+    if (OwningClangModule == ModuleFilter->getClangModule())
+      return true;
+  }
 
   return false;
 }
@@ -2108,12 +2125,14 @@ class ClangVectorDeclConsumer : public clang::VisibleDeclConsumer {
 
 class FilteringVisibleDeclConsumer : public swift::VisibleDeclConsumer {
   swift::VisibleDeclConsumer &NextConsumer;
-  const ClangModuleUnit *ModuleFilter = nullptr;
+  const ClangModuleUnit *ModuleFilter;
 
 public:
   FilteringVisibleDeclConsumer(swift::VisibleDeclConsumer &consumer,
                                const ClangModuleUnit *CMU)
-      : NextConsumer(consumer), ModuleFilter(CMU) {}
+      : NextConsumer(consumer), ModuleFilter(CMU) {
+    assert(CMU);
+  }
 
   void foundDecl(ValueDecl *VD, DeclVisibilityKind Reason) override {
     if (isVisibleFromModule(ModuleFilter, VD))
@@ -2123,13 +2142,14 @@ class FilteringVisibleDeclConsumer : public swift::VisibleDeclConsumer {
 
 class FilteringDeclaredDeclConsumer : public swift::VisibleDeclConsumer {
   swift::VisibleDeclConsumer &NextConsumer;
-  const ClangModuleUnit *ModuleFilter = nullptr;
+  const ClangModuleUnit *ModuleFilter;
 
 public:
   FilteringDeclaredDeclConsumer(swift::VisibleDeclConsumer &consumer,
                                 const ClangModuleUnit *CMU)
-      : NextConsumer(consumer),
-        ModuleFilter(CMU) {}
+      : NextConsumer(consumer), ModuleFilter(CMU) {
+    assert(CMU);
+  }
 
   void foundDecl(ValueDecl *VD, DeclVisibilityKind Reason) override {
     if (isDeclaredInModule(ModuleFilter, VD))
@@ -2870,10 +2890,7 @@ void ClangModuleUnit::lookupObjCMethods(
   auto &clangCtx = clangSema.getASTContext();
   for (auto objcMethod : objcMethods) {
     // Verify that this method came from this module.
-    auto owningClangModule = getClangOwningModule(objcMethod, clangCtx);
-    if (owningClangModule)
-      owningClangModule = owningClangModule->getTopLevelModule();
-
+    auto owningClangModule = getClangTopLevelOwningModule(objcMethod, clangCtx);
     if (owningClangModule != clangModule) continue;
 
     // If we found a property accessor, import the property.

test/ClangImporter/Inputs/custom-modules/RedeclaredErrorEnum/Base.h

@@ -0,0 +1,9 @@
+@import Foundation;
+
+extern NSString * const SomeErrorDomain;
+// typedef NS_ERROR_ENUM(SomeErrorDomain, SomeErrorCode) { ... }
+typedef enum SomeErrorCode : long SomeErrorCode;
+enum __attribute__((ns_error_domain(SomeErrorDomain))) SomeErrorCode : long {
+  SomeErrorX,
+  SomeErrorY
+};

test/ClangImporter/Inputs/custom-modules/RedeclaredErrorEnum/Redeclared.h

@@ -0,0 +1,7 @@
+@import Foundation;
+@import Base;
+
+extern NSString * const SomeErrorDomain;
+// typedef NS_ERROR_ENUM(SomeErrorDomain, SomeErrorCode);
+typedef enum SomeErrorCode : long SomeErrorCode;
+enum __attribute__((ns_error_domain(SomeErrorDomain))) SomeErrorCode : long;

test/ClangImporter/Inputs/custom-modules/RedeclaredErrorEnum/module.modulemap

@@ -0,0 +1,8 @@
+module Base {
+  header "Base.h"
+}
+
+module Redeclared {
+  header "Redeclared.h"
+  export *
+}

test/ClangImporter/enum-error-redeclared.swift

@@ -0,0 +1,11 @@
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %s -verify -enable-objc-interop -I %S/Inputs/custom-modules/RedeclaredErrorEnum
+
+import Redeclared
+
+// Referencing this error type (defined in Base, redeclared in Redeclared)
+// used to cause a compiler crash (rdar://problem/45414271).
+_ = SomeError.self
+_ = SomeError.Code.self
+
+_ = Redeclared.SomeError.self
+_ = Base.SomeError.self