[ClangImporter] Preserve macros from all implicit submodules. (#3983)

...instead of picking one definition arbitrarily. This comes from the
new "lookup table" design in Swift 3---we no longer just look for any
"visible" (imported) macro definition, but instead need to know them
up front. This works fine when there's only one definition per module,
but for modules like 'OpenGL' on macOS, with mutually-exclusive
submodules 'GL' and 'GL3', the compiler was arbitrarily deciding that
all of the macros the submodules had in common belonged to 'GL'.

The new model tries to decide if it's possible for two modules to be
imported separately, and keeps both macro entries if possible, only
deduplicating equivalent definitions at the last minute (when
importing into Swift). This /still/ doesn't perfectly match the
behavior you'd get in C, where a submodule and its parent module could
theoretically have conflicting definitions and you'd be fine as long
as you only imported one of them, but hopefully (a) it's close enough,
and (b) nobody is doing that. (The Swift compiler will prefer the
definition in the parent module even if the submodule is the only one
imported.)

rdar://problem/26731529

Author: jrose-apple

lib/ClangImporter/ClangImporter.cpp

@@ -856,22 +856,28 @@ void ClangImporter::Implementation::addMacrosToLookupTable(
   for (const auto &macro : pp.macros(false)) {
     // Find the local history of this macro directive.
     clang::MacroDirective *MD = pp.getLocalMacroDirectiveHistory(macro.first);
-    if (!MD) continue;
 
     // Walk the history.
     for (; MD; MD = MD->getPrevious()) {
-      // Check whether we have a macro defined in this module.
-      auto info = pp.getMacroInfo(macro.first);
-      if (!info || info->isFromASTFile() || info->isBuiltinMacro()) continue;
+      // Don't look at any definitions that are followed by undefs.
+      // FIXME: This isn't quite correct across explicit submodules -- one
+      // submodule might define a macro, while another defines and then
+      // undefines the same macro. If they are processed in that order, the
+      // history will have the undef at the end, and we'll miss the first
+      // definition.
+      if (isa<clang::UndefMacroDirective>(MD))
+        break;
 
       // Only interested in macro definitions.
       auto *defMD = dyn_cast<clang::DefMacroDirective>(MD);
       if (!defMD) continue;
 
+      // Is this definition from this module?
+      auto info = defMD->getInfo();
+      if (!info || info->isFromASTFile()) continue;
+
       // If we hit a builtin macro, we're done.
-      if (auto info = defMD->getInfo()) {
-        if (info->isBuiltinMacro()) break;
-      }
+      if (info->isBuiltinMacro()) break;
 
       // If we hit a macro with invalid or predefined location, we're done.
       auto loc = defMD->getLocation();
@@ -882,7 +888,7 @@ void ClangImporter::Implementation::addMacrosToLookupTable(
       // Add this entry.
       auto name = importMacroName(macro.first, info, clangCtx);
       if (name.empty()) continue;
-      table.addEntry(name, info, clangCtx.getTranslationUnitDecl());
+      table.addEntry(name, info, clangCtx.getTranslationUnitDecl(), &pp);
     }
   }
 }

lib/ClangImporter/SwiftLookupTable.cpp

@@ -19,6 +19,7 @@
 #include "swift/Basic/Version.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/Lex/MacroInfo.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ASTWriter.h"
@@ -43,6 +44,76 @@ static bool matchesExistingDecl(clang::Decl *decl, clang::Decl *existingDecl) {
   return false;
 }
 
+namespace {
+enum class MacroConflictAction {
+  Discard,
+  Replace,
+  AddAsAlternative
+};
+}
+
+/// Based on the Clang module structure, decides what to do when a new
+/// definition of an existing macro is seen: discard it, have it replace the
+/// old one, or add it as an alternative.
+///
+/// Specifically, if the innermost explicit submodule containing \p newMacro
+/// contains the innermost explicit submodule containing \p existingMacro,
+/// \p newMacro should replace \p existingMacro; if they're the same module,
+/// \p existingMacro should stay in place. Otherwise, they don't share an
+/// explicit module, and should be considered alternatives.
+///
+/// Note that the above assumes that macro definitions are processed in reverse
+/// order, i.e. the first definition seen is the last in a translation unit.
+///
+/// If we're not currently building a module, then the "latest" macro wins,
+/// which (by the same assumption) should be the existing macro.
+static MacroConflictAction
+considerReplacingExistingMacro(const clang::MacroInfo *newMacro,
+                               const clang::MacroInfo *existingMacro,
+                               const clang::Preprocessor *PP) {
+  assert(PP);
+  assert(newMacro);
+  assert(existingMacro);
+  assert(newMacro->getOwningModuleID() == 0);
+  assert(existingMacro->getOwningModuleID() == 0);
+
+  if (PP->getLangOpts().CurrentModule.empty())
+    return MacroConflictAction::Discard;
+
+  clang::ModuleMap &moduleInfo = PP->getHeaderSearchInfo().getModuleMap();
+  const clang::SourceManager &sourceMgr = PP->getSourceManager();
+
+  auto findContainingExplicitModule =
+      [&moduleInfo, &sourceMgr](const clang::MacroInfo *macro)
+        -> const clang::Module * {
+
+    clang::SourceLocation definitionLoc = macro->getDefinitionLoc();
+    assert(definitionLoc.isValid() &&
+           "implicitly-defined macros shouldn't show up in a module's lookup");
+    clang::FullSourceLoc fullLoc(definitionLoc, sourceMgr);
+
+    const clang::Module *module = moduleInfo.inferModuleFromLocation(fullLoc);
+    assert(module && "we are building a module; everything should be modular");
+
+    while (module->isSubModule()) {
+      if (module->IsExplicit)
+        break;
+      module = module->Parent;
+    }
+    return module;
+  };
+
+  const clang::Module *newModule = findContainingExplicitModule(newMacro);
+  const clang::Module *existingModule =
+      findContainingExplicitModule(existingMacro);
+
+  if (existingModule == newModule)
+    return MacroConflictAction::Discard;
+  if (existingModule->isSubModuleOf(newModule))
+    return MacroConflictAction::Replace;
+  return MacroConflictAction::AddAsAlternative;
+}
+
 bool SwiftLookupTable::contextRequiresName(ContextKind kind) {
   switch (kind) {
   case ContextKind::ObjCClass:
@@ -262,7 +333,8 @@ static bool isGlobalAsMember(SwiftLookupTable::SingleEntry entry,
 }
 
 bool SwiftLookupTable::addLocalEntry(SingleEntry newEntry,
-                                     SmallVectorImpl<uintptr_t> &entries) {
+                                     SmallVectorImpl<uintptr_t> &entries,
+                                     const clang::Preprocessor *PP) {
   // Check whether this entry matches any existing entry.
   auto decl = newEntry.dyn_cast<clang::NamedDecl *>();
   auto macro = newEntry.dyn_cast<clang::MacroInfo *>();
@@ -272,10 +344,21 @@ bool SwiftLookupTable::addLocalEntry(SingleEntry newEntry,
         matchesExistingDecl(decl, mapStoredDecl(existingEntry)))
       return false;
 
-    // If it matches an existing macro, overwrite the existing entry.
+    // If it matches an existing macro, decide on the best course of action.
     if (macro && isMacroEntry(existingEntry)) {
-      existingEntry = encodeEntry(macro);
-      return false;
+      MacroConflictAction action =
+         considerReplacingExistingMacro(macro,
+                                        mapStoredMacro(existingEntry),
+                                        PP);
+      switch (action) {
+      case MacroConflictAction::Discard:
+        return false;
+      case MacroConflictAction::Replace:
+        existingEntry = encodeEntry(macro);
+        return false;
+      case MacroConflictAction::AddAsAlternative:
+        break;
+      }
     }
   }
 
@@ -288,7 +371,8 @@ bool SwiftLookupTable::addLocalEntry(SingleEntry newEntry,
 }
 
 void SwiftLookupTable::addEntry(DeclName name, SingleEntry newEntry,
-                                EffectiveClangContext effectiveContext) {
+                                EffectiveClangContext effectiveContext,
+                                const clang::Preprocessor *PP) {
   assert(!Reader && "Cannot modify a lookup table stored on disk");
 
   // Translate the context.
@@ -311,7 +395,7 @@ void SwiftLookupTable::addEntry(DeclName name, SingleEntry newEntry,
   // If this is a global imported as a member, record is as such.
   if (isGlobalAsMember(newEntry, context)) {
     auto &entries = GlobalsAsMembers[context];
-    (void)addLocalEntry(newEntry, entries);
+    (void)addLocalEntry(newEntry, entries, PP);
   }
 
   // Find the list of entries for this base name.
@@ -321,7 +405,7 @@ void SwiftLookupTable::addEntry(DeclName name, SingleEntry newEntry,
   for (auto &entry : entries) {
     if (entry.Context == context) {
       // We have entries for this context.
-      (void)addLocalEntry(newEntry, entry.DeclsOrMacros);
+      (void)addLocalEntry(newEntry, entry.DeclsOrMacros, PP);
       return;
     }
   }

lib/ClangImporter/SwiftLookupTable.h

@@ -297,7 +297,8 @@ class SwiftLookupTable {
   /// present.
   ///
   /// \returns true if the entry was added, false otherwise.
-  bool addLocalEntry(SingleEntry newEntry, SmallVectorImpl<uintptr_t> &entries);
+  bool addLocalEntry(SingleEntry newEntry, SmallVectorImpl<uintptr_t> &entries,
+                     const clang::Preprocessor *PP);
 
 public:
   explicit SwiftLookupTable(SwiftLookupTableReader *reader) : Reader(reader) { }
@@ -324,7 +325,8 @@ class SwiftLookupTable {
   /// \param newEntry The Clang declaration or macro.
   /// \param effectiveContext The effective context in which name lookup occurs.
   void addEntry(DeclName name, SingleEntry newEntry,
-                EffectiveClangContext effectiveContext);
+                EffectiveClangContext effectiveContext,
+                const clang::Preprocessor *PP = nullptr);
 
   /// Add an Objective-C category or extension to the table.
   void addCategory(clang::ObjCCategoryDecl *category);

test/ClangModules/Inputs/custom-modules/MacrosRedefWithParallelSubmodules/A.h

@@ -0,0 +1,2 @@
+#define MRWPS_REDEF_1 "hello"
+#define MRWPS_REDEF_2 "swift"

test/ClangModules/Inputs/custom-modules/MacrosRedefWithParallelSubmodules/B.h

@@ -0,0 +1,2 @@
+#define MRWPS_REDEF_1 "hello"
+#define MRWPS_REDEF_2 "world"

test/ClangModules/Inputs/custom-modules/MacrosRedefWithParallelSubmodules/module.modulemap

@@ -0,0 +1,8 @@
+module MacrosRedefWithParallelSubmodules {
+  explicit module A {
+    header "A.h"
+  }
+  explicit module B {
+    header "B.h"
+  }
+}

test/ClangModules/Inputs/custom-modules/MacrosRedefWithSubmodules/Inner.h

@@ -0,0 +1,2 @@
+#define MRWS_REDEF_1 "hello"
+#define MRWS_REDEF_2 "swift"

test/ClangModules/Inputs/custom-modules/MacrosRedefWithSubmodules/Outer.h

@@ -0,0 +1,2 @@
+#define MRWS_REDEF_1 "hello"
+#define MRWS_REDEF_2 "world"

test/ClangModules/Inputs/custom-modules/MacrosRedefWithSubmodules/module.modulemap

@@ -0,0 +1,6 @@
+module MacrosRedefWithSubmodules {
+  header "Outer.h"
+  explicit module TheSubmodule {
+    header "Inner.h"
+  }
+}

test/ClangModules/Inputs/macros_redef.h

@@ -0,0 +1,7 @@
+#define BRIDGING_HEADER_1 1
+#undef BRIDGING_HEADER_1
+#define BRIDGING_HEADER_1 "1"
+
+#define BRIDGING_HEADER_2 2
+#define BRIDGING_HEADER_2 "2"
+

test/ClangModules/macros_redef.swift

@@ -1,11 +1,46 @@
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -I %S/Inputs/custom-modules -parse -verify %s
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -I %S/Inputs/custom-modules -import-objc-header %S/Inputs/macros_redef.h -parse %s
+
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -I %S/Inputs/custom-modules -import-objc-header %S/Inputs/macros_redef.h -DCONFLICT -parse -verify %s
 
 import MacrosRedefA
 import MacrosRedefB
 
-func testMacroRedef() {
+#if CONFLICT
+import MacrosRedefWithSubmodules
+import MacrosRedefWithSubmodules.TheSubmodule
+import MacrosRedefWithParallelSubmodules.A
+import MacrosRedefWithParallelSubmodules.B
+#else
+import MacrosRedefWithSubmodules.TheSubmodule
+import MacrosRedefWithParallelSubmodules.A
+#endif
+
+func testFrameworkRedef() {
   var s: String
   s = REDEF_1
+#if CONFLICT
   s = REDEF_2 // expected-error{{ambiguous use of 'REDEF_2'}}
+#endif
+}
+
+func testBridgingHeaderRedef() {
+  var s: String
+  s = BRIDGING_HEADER_1
+  s = BRIDGING_HEADER_2
+  _ = s
+}
+
+func testSubmodules() {
+  var s: String
+  s = MRWS_REDEF_1
+  s = MRWS_REDEF_2
+  _ = s
+}
+
+func testParallelSubmodules() {
+  var s: String
+  s = MRWPS_REDEF_1
+  s = MRWPS_REDEF_2 // expected-error{{ambiguous use of 'MRWPS_REDEF_2'}}
+  _ = s
 }
 

test/Interpreter/SDK/submodules_smoke_test.swift

@@ -8,6 +8,8 @@
 import OpenGL.GL3
 _ = glGetString
 _ = OpenGL.glGetString
+_ = GL_COLOR_BUFFER_BIT
+_ = OpenGL.GL_COLOR_BUFFER_BIT
 
 import AppKit.NSPanGestureRecognizer