Merge pull request swiftlang#12169 from jrose-apple/dance-dance-deduplication

[ClangImporter] Don't add duplicate search paths

Author: jrose-apple

lib/ClangImporter/ClangDiagnosticConsumer.cpp

@@ -17,6 +17,7 @@
 #include "swift/AST/DiagnosticsClangImporter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Lex/LexDiagnostic.h"
 #include "llvm/ADT/STLExtras.h"
 
@@ -36,17 +37,37 @@ namespace {
          callback(fn) {}
 
   private:
+    /// Is this a diagnostic that doesn't do the user any good to show if it
+    /// is located in one of Swift's synthetic buffers? If so, returns true to
+    /// suppress it.
+    static bool shouldSuppressDiagInSwiftBuffers(clang::DiagOrStoredDiag info) {
+      if (info.isNull())
+        return false;
+
+      unsigned ID;
+      if (auto *activeDiag = info.dyn_cast<const clang::Diagnostic *>())
+        ID = activeDiag->getID();
+      else
+        ID = info.get<const clang::StoredDiagnostic *>()->getID();
+      return ID == clang::diag::note_module_import_here ||
+             ID == clang::diag::err_module_not_built;
+    }
+
+    /// Returns true if \p loc is inside one of Swift's synthetic buffers.
+    static bool isInSwiftBuffers(clang::FullSourceLoc loc) {
+      StringRef bufName = StringRef(loc.getManager().getBufferName(loc));
+      return bufName == ClangImporter::Implementation::moduleImportBufferName ||
+             bufName == ClangImporter::Implementation::bridgingHeaderBufferName;
+    }
+
     void emitDiagnosticMessage(clang::FullSourceLoc Loc,
                                clang::PresumedLoc PLoc,
                                clang::DiagnosticsEngine::Level Level,
                                StringRef Message,
                                ArrayRef<clang::CharSourceRange> Ranges,
                                clang::DiagOrStoredDiag Info) override {
-      StringRef bufName = StringRef(Loc.getManager().getBufferName(Loc));
-      if (bufName == ClangImporter::Implementation::moduleImportBufferName ||
-          bufName == ClangImporter::Implementation::bridgingHeaderBufferName) {
+      if (shouldSuppressDiagInSwiftBuffers(Info) && isInSwiftBuffers(Loc))
         return;
-      }
       callback(Loc, Level, Message);
     }
 
@@ -61,8 +82,9 @@ namespace {
 
     void emitNote(clang::FullSourceLoc Loc, StringRef Message) override {
       // We get invalid note locations when trying to describe where a module
-      // is imported and the actual location is in Swift.
-      if (Loc.isInvalid())
+      // is imported and the actual location is in Swift. We also want to ignore
+      // things like "in module X imported from <swift-imported-modules>".
+      if (Loc.isInvalid() || isInSwiftBuffers(Loc))
         return;
       emitDiagnosticMessage(Loc, {}, clang::DiagnosticsEngine::Note, Message,
                             {}, {});
@@ -227,6 +249,7 @@ void ClangDiagnosticConsumer::HandleDiagnostic(
     clang::FullSourceLoc clangDiagLoc(clangDiag.getLocation(),
                                       clangDiag.getSourceManager());
     renderer.emitDiagnostic(clangDiagLoc, clangDiagLevel, message,
-                            clangDiag.getRanges(), clangDiag.getFixItHints());
+                            clangDiag.getRanges(), clangDiag.getFixItHints(),
+                            &clangDiag);
   }
 }

lib/ClangImporter/ClangImporter.cpp

@@ -1064,9 +1064,21 @@ bool ClangImporter::addSearchPath(StringRef newSearchPath, bool isFramework,
     return true;
 
   auto &headerSearchInfo = Impl.getClangPreprocessor().getHeaderSearchInfo();
-  headerSearchInfo.AddSearchPath({entry, isSystem ?
-                                           clang::SrcMgr::C_System :
-                                           clang::SrcMgr::C_User, isFramework},
+  auto exists = std::any_of(headerSearchInfo.search_dir_begin(),
+                            headerSearchInfo.search_dir_end(),
+                            [&](const clang::DirectoryLookup &lookup) -> bool {
+    if (isFramework)
+      return lookup.getFrameworkDir() == entry;
+    return lookup.getDir() == entry;
+  });
+  if (exists) {
+    // Don't bother adding a search path that's already there. Clang would have
+    // removed it via deduplication at the time the search path info gets built.
+    return false;
+  }
+
+  auto kind = isSystem ? clang::SrcMgr::C_System : clang::SrcMgr::C_User;
+  headerSearchInfo.AddSearchPath({entry, kind, isFramework},
                                  /*isAngled=*/true);
 
   // In addition to changing the current preprocessor directly, we still need

validation-test/ClangImporter/Inputs/explicit-system-framework-path/override/Test.framework/Headers/Test.h

@@ -0,0 +1,3 @@
+#warning "Test.h found in override"
+
+extern int TestFrameworkFromOverride;

validation-test/ClangImporter/Inputs/explicit-system-framework-path/override/Test.framework/Modules/module.modulemap

@@ -0,0 +1,3 @@
+framework module Test {
+  umbrella header "Test.h"
+}

validation-test/ClangImporter/Inputs/explicit-system-framework-path/override/TestUser.framework/Headers/TestUser.h

@@ -0,0 +1,3 @@
+#warning "TestUser found in override"
+
+@import Test;

validation-test/ClangImporter/Inputs/explicit-system-framework-path/override/TestUser.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+framework module TestUser {
+  umbrella header "TestUser.h"
+  export *
+}

validation-test/ClangImporter/Inputs/explicit-system-framework-path/sdkroot/Library/Frameworks/Test.framework/Headers/Test.h

@@ -0,0 +1,3 @@
+#warning "Test.h found in SDK"
+
+extern int TestFrameworkFromSDK;

validation-test/ClangImporter/Inputs/explicit-system-framework-path/sdkroot/Library/Frameworks/Test.framework/Modules/module.modulemap

@@ -0,0 +1,3 @@
+framework module Test {
+  umbrella header "Test.h"
+}

validation-test/ClangImporter/explicit-system-framework-path.swift

@@ -0,0 +1,14 @@
+// RUN: %target-swift-frontend -F %S/Inputs/explicit-system-framework-path/sdkroot/Library/Frameworks -F %S/Inputs/explicit-system-framework-path/override -typecheck -sdk %S/Inputs/explicit-system-framework-path/sdkroot %s 2>&1 | %FileCheck %s
+
+// Make sure we're matching Clang's behavior on this.
+// RUN: echo '@import TestUser;' | %clang -F %S/Inputs/explicit-system-framework-path/sdkroot/Library/Frameworks -F %S/Inputs/explicit-system-framework-path/override -isysroot %S/Inputs/explicit-system-framework-path/sdkroot -x objective-c -fmodules -fsyntax-only - 2>&1 | %FileCheck %s
+
+// This only works with framework search paths because normal include paths
+// will pop up a "redefinition of module 'X'" error. Only Apple platforms have
+// default framework search paths, so just limit this to macOS.
+// REQUIRES: OS=macosx
+
+import TestUser
+
+// CHECK: TestUser found in override
+// CHECK: Test.h found in override