[Bridging PCH] Warn on non-redundant implicit bridging-header imports.

We're trying to get rid of implicit bridging-header imports, as a feature.
These are IMPORTED_HEADER blocks left in modules built with bridging
headers, that trigger re-importing the bridging header into any client
that imports the module.

As a half-way measure to deprecating them, we add a warning here that
triggers when an implicit bridging-header import occurs that is _not_
suppressed as redundant by clang.

Author: graydon

include/swift/AST/DiagnosticsClangImporter.def

@@ -79,6 +79,11 @@ WARNING(unresolvable_clang_decl,none,
         "imported declaration '%0' could not be mapped to '%1'",
         (StringRef, StringRef))
 
+WARNING(implicit_bridging_header_imported_from_module,none,
+        "implicit import of bridging header '%0' via module %1 "
+        "is deprecated and will be removed in a later version of Swift",
+        (StringRef, Identifier))
+
 #ifndef DIAG_NO_UNDEF
 # if defined(DIAG)
 #  undef DIAG

include/swift/ClangImporter/ClangImporter.h

@@ -201,13 +201,16 @@ class ClangImporter final : public ClangModuleLoader {
   /// \param diagLoc A location to attach any diagnostics to if import fails.
   /// \param trackParsedSymbols If true, tracks decls and macros that were
   ///        parsed from the bridging header.
+  /// \param implicitImport If true, indicates that this import was implicit
+  ///        from a reference in a module file (deprecated behaviour).
   ///
   /// \returns true if there was an error importing the header.
   ///
   /// \sa getImportedHeaderModule
   bool importBridgingHeader(StringRef header, ModuleDecl *adapter,
                             SourceLoc diagLoc = {},
-                            bool trackParsedSymbols = false);
+                            bool trackParsedSymbols = false,
+                            bool implicitImport = false);
 
   /// Imports an Objective-C PCH file into the shared imported header module.
   ///

lib/ClangImporter/ClangImporter.cpp

@@ -827,7 +827,8 @@ bool ClangImporter::addSearchPath(StringRef newSearchPath, bool isFramework) {
 bool ClangImporter::Implementation::importHeader(
     Module *adapter, StringRef headerName, SourceLoc diagLoc,
     bool trackParsedSymbols,
-    std::unique_ptr<llvm::MemoryBuffer> sourceBuffer) {
+    std::unique_ptr<llvm::MemoryBuffer> sourceBuffer,
+    bool implicitImport) {
   // Don't even try to load the bridging header if the Clang AST is in a bad
   // state. It could cause a crash.
   auto &clangDiags = getClangASTContext().getDiagnostics();
@@ -879,6 +880,27 @@ bool ClangImporter::Implementation::importHeader(
     consumer.reset();
   }
 
+  // We're trying to discourage (and eventually deprecate) the use of implicit
+  // bridging-header imports triggered by IMPORTED_HEADER blocks in
+  // modules. There are two sub-cases to consider:
+  //
+  //   #1 The implicit import actually occurred.
+  //
+  //   #2 The user explicitly -import-objc-header'ed some header or PCH that
+  //      makes the implicit import redundant.
+  //
+  // It's not obvious how to exactly differentiate these cases given the
+  // interface clang gives us, but we only want to warn on case #1, and the
+  // non-emptiness of allParsedDecls is a _definite_ sign that we're in case
+  // #1. So we treat that as an approximation of the condition we're after, and
+  // accept that we might fail to warn in the odd case where "the import
+  // occurred" but didn't introduce any new decls.
+  if (implicitImport && !allParsedDecls.empty()) {
+    SwiftContext.Diags.diagnose(
+      diagLoc, diag::implicit_bridging_header_imported_from_module,
+      llvm::sys::path::filename(headerName), adapter->getName());
+  }
+
   // We can't do this as we're parsing because we may want to resolve naming
   // conflicts between the things we've parsed.
   for (auto group : allParsedDecls)
@@ -921,7 +943,7 @@ bool ClangImporter::importHeader(StringRef header, Module *adapter,
                                                            /*OpenFile=*/true);
   if (headerFile && headerFile->getSize() == expectedSize &&
       headerFile->getModificationTime() == expectedModTime) {
-    return importBridgingHeader(header, adapter, diagLoc);
+    return importBridgingHeader(header, adapter, diagLoc, false, true);
   }
 
   if (!cachedContents.empty() && cachedContents.back() == '\0')
@@ -930,12 +952,13 @@ bool ClangImporter::importHeader(StringRef header, Module *adapter,
     llvm::MemoryBuffer::getMemBuffer(cachedContents, header)
   };
   return Impl.importHeader(adapter, header, diagLoc, /*trackParsedSymbols=*/false,
-                           std::move(sourceBuffer));
+                           std::move(sourceBuffer), true);
 }
 
 bool ClangImporter::importBridgingHeader(StringRef header, Module *adapter,
                                          SourceLoc diagLoc,
-                                         bool trackParsedSymbols) {
+                                         bool trackParsedSymbols,
+                                         bool implicitImport) {
   if (llvm::sys::path::extension(header).endswith(PCH_EXTENSION)) {
     return importBridgingPCH(header, adapter);
   }
@@ -958,7 +981,7 @@ bool ClangImporter::importBridgingHeader(StringRef header, Module *adapter,
   };
 
   return Impl.importHeader(adapter, header, diagLoc, trackParsedSymbols,
-                           std::move(sourceBuffer));
+                           std::move(sourceBuffer), implicitImport);
 }
 
 bool ClangImporter::importBridgingPCH(StringRef pchFile, ModuleDecl *adapter,

lib/ClangImporter/ImporterImpl.h

@@ -596,7 +596,8 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   /// Imports the given header contents into the Clang context.
   bool importHeader(Module *adapter, StringRef headerName, SourceLoc diagLoc,
                     bool trackParsedSymbols,
-                    std::unique_ptr<llvm::MemoryBuffer> contents);
+                    std::unique_ptr<llvm::MemoryBuffer> contents,
+                    bool implicitImport);
 
   /// \brief Retrieve the imported module that should contain the given
   /// Clang decl.

test/ClangImporter/MixedSource/broken-bridging-header.swift

@@ -19,7 +19,7 @@
 
 // REQUIRES: objc_interop
 
-import HasBridgingHeader // expected-error {{failed to import bridging header}} expected-error {{failed to load module 'HasBridgingHeader'}}
+import HasBridgingHeader // expected-error {{failed to import bridging header}} expected-error {{failed to load module 'HasBridgingHeader'}} expected-warning {{implicit import of bridging header}}
 
 // MISSING-HEADER: error: bridging header '{{.*}}/fake.h' does not exist
 // MISSING-HEADER-NOT: error:

test/ClangImporter/MixedSource/import-mixed-with-header-twice.swift

@@ -13,7 +13,7 @@
 // USE-SERIALIZED-HEADER: redefinition of 'Point2D'
 // USE-SERIALIZED-HEADER: previous definition is here
 
-import MixedWithHeaderAgain
+import MixedWithHeaderAgain // expected-warning {{implicit import of bridging header 'header-again.h' via module 'MixedWithHeaderAgain' is deprecated and will be removed in a later version of Swift}}
 
 func testLine(line: Line) {
   testLineImpl(line)

test/ClangImporter/MixedSource/import-mixed-with-header.swift

@@ -14,7 +14,7 @@
 
 // XFAIL: linux
 
-import MixedWithHeader
+import MixedWithHeader // expected-warning {{implicit import of bridging header 'header.h' via module 'MixedWithHeader' is deprecated and will be removed in a later version of Swift}}
 
 func testReexportedClangModules(_ foo : FooProto) {
   _ = foo.bar as CInt