Provide fallback SourceLoc for swiftinterface build errors

When a swiftinterface fails to build for any of various reasons, we try to diagnose the failure at the site of the `import` declaration. But if the import is implicitly added—which happens for many SDK modules, like the standard library and ClangImporter overlays—there is no source location for the import, so the error ends up being diagnosed at <unknown>:0. This causes a number of issues; most notably, Xcode doesn’t display the diagnostic as prominently as others.

This change falls back to diagnosing the error at line 1, column 1 of the swiftinterface file itself. This is perhaps not an ideal location, and it won’t help with I/O errors where we can’t open the swiftinterface file (and therefore can’t diagnose an error in it), but it should improve the way we display most module interface building errors.

Author: beccadax

lib/Frontend/ModuleInterfaceBuilder.cpp

@@ -137,8 +137,11 @@ void ModuleInterfaceBuilder::configureSubInvocation(
 bool ModuleInterfaceBuilder::extractSwiftInterfaceVersionAndArgs(
     swift::version::Version &Vers, StringRef &CompilerVersion,
     llvm::StringSaver &SubArgSaver, SmallVectorImpl<const char *> &SubArgs) {
+  llvm::vfs::FileSystem &fs = *sourceMgr.getFileSystem();
   auto FileOrError = swift::vfs::getFileOrSTDIN(fs, interfacePath);
   if (!FileOrError) {
+    // Don't use this->diagnose() because it'll just try to re-open
+    // interfacePath.
     diags.diagnose(diagnosticLoc, diag::error_open_input_file,
                    interfacePath, FileOrError.getError().message());
     return true;
@@ -149,17 +152,16 @@ bool ModuleInterfaceBuilder::extractSwiftInterfaceVersionAndArgs(
   auto FlagRe = getSwiftInterfaceModuleFlagsRegex();
   SmallVector<StringRef, 1> VersMatches, FlagMatches, CompMatches;
   if (!VersRe.match(SB, &VersMatches)) {
-    diags.diagnose(diagnosticLoc,
-                   diag::error_extracting_version_from_module_interface);
+    diagnose(diag::error_extracting_version_from_module_interface);
     return true;
   }
   if (!FlagRe.match(SB, &FlagMatches)) {
-    diags.diagnose(diagnosticLoc,
-                   diag::error_extracting_flags_from_module_interface);
+    diagnose(diag::error_extracting_flags_from_module_interface);
     return true;
   }
   assert(VersMatches.size() == 2);
   assert(FlagMatches.size() == 2);
+  // FIXME We should diagnose this at a location that makes sense:
   Vers = swift::version::Version(VersMatches[1], SourceLoc(), &diags);
   llvm::cl::TokenizeGNUCommandLine(FlagMatches[1], SubArgSaver, SubArgs);
 
@@ -178,6 +180,8 @@ bool ModuleInterfaceBuilder::extractSwiftInterfaceVersionAndArgs(
 bool ModuleInterfaceBuilder::collectDepsForSerialization(
     CompilerInstance &SubInstance, SmallVectorImpl<FileDependency> &Deps,
     bool IsHashBased) {
+  llvm::vfs::FileSystem &fs = *sourceMgr.getFileSystem();
+
   auto &Opts = SubInstance.getASTContext().SearchPathOpts;
   SmallString<128> SDKPath(Opts.SDKPath);
   path::native(SDKPath);
@@ -267,6 +271,8 @@ bool ModuleInterfaceBuilder::buildSwiftModuleInternal(
       llvm::RestorePrettyStackState(savedInnerPrettyStackState);
     };
 
+    llvm::vfs::FileSystem &fs = *sourceMgr.getFileSystem();
+    
     // Note that we don't assume cachePath is the same as the Clang
     // module cache path at this point.
     if (!moduleCachePath.empty())
@@ -296,9 +302,8 @@ bool ModuleInterfaceBuilder::buildSwiftModuleInternal(
     // minor versions might be interesting for debugging, or special-casing a
     // compatible field variant.
     if (Vers.asMajorVersion() != InterfaceFormatVersion.asMajorVersion()) {
-      diags.diagnose(diagnosticLoc,
-                     diag::unsupported_version_of_module_interface,
-                     interfacePath, Vers);
+      diagnose(diag::unsupported_version_of_module_interface, interfacePath,
+               Vers);
       SubError = true;
       return;
     }
@@ -313,8 +318,7 @@ bool ModuleInterfaceBuilder::buildSwiftModuleInternal(
       auto DiagKind = diag::serialization_name_mismatch;
       if (subInvocation.getLangOptions().DebuggerSupport)
         DiagKind = diag::serialization_name_mismatch_repl;
-      diags.diagnose(diagnosticLoc, DiagKind, subInvocation.getModuleName(),
-                     ExpectedModuleName);
+      diagnose(DiagKind, subInvocation.getModuleName(), ExpectedModuleName);
       SubError = true;
       return;
     }
@@ -341,9 +345,9 @@ bool ModuleInterfaceBuilder::buildSwiftModuleInternal(
         auto builtByCompiler =
             getSwiftInterfaceCompilerVersionForCurrentCompiler(
                 SubInstance.getASTContext());
-        diags.diagnose(diagnosticLoc, diag::module_interface_build_failed,
-                       moduleName, emittedByCompiler == builtByCompiler,
-                       emittedByCompiler, builtByCompiler);
+        diagnose(diag::module_interface_build_failed, moduleName,
+                 emittedByCompiler == builtByCompiler, emittedByCompiler,
+                 builtByCompiler);
       }
     };
 
@@ -439,8 +443,7 @@ bool ModuleInterfaceBuilder::buildSwiftModule(StringRef OutPath,
     // necessary for performance. Fallback to building the module in case of any lock
     // related errors.
     if (RemarkRebuild) {
-      diags.diagnose(SourceLoc(), diag::interface_file_lock_failure,
-                     interfacePath);
+      diagnose(diag::interface_file_lock_failure, interfacePath);
     }
     // Clear out any potential leftover.
     Locked.unsafeRemoveLockFile();
@@ -472,8 +475,7 @@ bool ModuleInterfaceBuilder::buildSwiftModule(StringRef OutPath,
       // another process to complete the build so swift does not do it done
       // twice. If case of timeout, build it ourselves.
       if (RemarkRebuild) {
-        diags.diagnose(SourceLoc(), diag::interface_file_lock_timed_out,
-                       interfacePath);
+        diagnose(diag::interface_file_lock_timed_out, interfacePath);
       }
       // Clear the lock file so that future invocations can make progress.
       Locked.unsafeRemoveLockFile();

lib/Frontend/ModuleInterfaceBuilder.h

@@ -33,7 +33,7 @@ class SearchPathOptions;
 class DependencyTracker;
 
 class ModuleInterfaceBuilder {
-  llvm::vfs::FileSystem &fs;
+  SourceManager &sourceMgr;
   DiagnosticEngine &diags;
   const StringRef interfacePath;
   const StringRef moduleName;
@@ -48,6 +48,19 @@ class ModuleInterfaceBuilder {
   CompilerInvocation subInvocation;
   SmallVector<StringRef, 3> extraDependencies;
 
+  /// Emit a diagnostic tied to this declaration.
+  template<typename ...ArgTypes>
+  InFlightDiagnostic diagnose(
+      Diag<ArgTypes...> ID,
+      typename detail::PassArgument<ArgTypes>::type... Args) const {
+    SourceLoc loc = diagnosticLoc;
+    if (loc.isInvalid()) {
+      // Diagnose this inside the interface file, if possible.
+      loc = sourceMgr.getLocFromExternalSource(interfacePath, 1, 1);
+    }
+    return diags.diagnose(loc, ID, std::move(Args)...);
+  }
+
   void configureSubInvocationInputsAndOutputs(StringRef OutPath);
 
   void configureSubInvocation(const SearchPathOptions &SearchPathOpts,
@@ -86,7 +99,7 @@ class ModuleInterfaceBuilder {
                             bool disableInterfaceFileLock = false,
                             SourceLoc diagnosticLoc = SourceLoc(),
                             DependencyTracker *tracker = nullptr)
-    : fs(*sourceMgr.getFileSystem()), diags(diags),
+    : sourceMgr(sourceMgr), diags(diags),
       interfacePath(interfacePath), moduleName(moduleName),
       moduleCachePath(moduleCachePath), prebuiltCachePath(prebuiltCachePath),
       serializeDependencyHashes(serializeDependencyHashes),

validation-test/ParseableInterface/failing-overlay.swift

@@ -3,6 +3,4 @@
 
 import ImportsOverlay
 
-// FIXME: It would be better if this had a useful location, like inside the
-// C header that caused the importing of the overlay.
-// CHECK: <unknown>:0: error: failed to build module 'HasOverlay' from its module interface; the compiler that produced it, '(unspecified, file possibly handwritten)', may have used features that aren't supported by this compiler, '{{.*}}Swift version{{.*}}'
+// CHECK: HasOverlay.swiftinterface:1:1: error: failed to build module 'HasOverlay' from its module interface; the compiler that produced it, '(unspecified, file possibly handwritten)', may have used features that aren't supported by this compiler, '{{.*}}Swift version{{.*}}'