Merge pull request #2919 from adrian-prantl/better-diagnostics

Improve diagnostics for import failures

Author: adrian-prantl

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

@@ -1479,11 +1479,7 @@ unsigned SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
                         retry = true;
                     },
                     [&](const SwiftASTContextError &SACE) {
-                      if (swift_ast_ctx->GetClangImporter())
-                        DiagnoseSwiftASTContextError();
-                      else
-                        // Discard the shared scratch context and retry.
-                        retry = true;
+                      DiagnoseSwiftASTContextError();
                     },
                     [&](const StringError &SE) {
                       diagnostic_manager.PutString(eDiagnosticSeverityError,

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp

@@ -2869,52 +2869,51 @@ class StoringDiagnosticConsumer : public swift::DiagnosticConsumer {
       const DiagnosticSeverity severity = SeverityForKind(diagnostic.kind);
       const DiagnosticOrigin origin = eDiagnosticOriginSwift;
 
-      if (first_line > 0 && bufferID != UINT32_MAX &&
-          diagnostic.bufferID == bufferID && !diagnostic.bufferName.empty()) {
-        // Make sure the error line is in range.
-        if (diagnostic.line >= first_line && diagnostic.line <= last_line) {
-          // Need to remap the error/warning to a different line.
-          StreamString match;
-          match.Printf("%s:%u:", diagnostic.bufferName.str().c_str(),
-                       diagnostic.line);
-          const size_t match_len = match.GetString().size();
-          size_t match_pos =
-              diagnostic.description.find(match.GetString().str());
-          if (match_pos != std::string::npos) {
-            // We have some <file>:<line>:" instances that need to be updated.
-            StreamString fixed_description;
-            size_t start_pos = 0;
-            do {
-              if (match_pos > start_pos)
-                fixed_description.Printf(
-                    "%s", diagnostic.description.substr(start_pos, match_pos)
-                              .c_str());
-              fixed_description.Printf(
-                  "%s:%u:", diagnostic.bufferName.str().c_str(),
-                  diagnostic.line - first_line + 1);
-              start_pos = match_pos + match_len;
-              match_pos = diagnostic.description.find(match.GetString().str(),
-                                                      start_pos);
-            } while (match_pos != std::string::npos);
-
-            // Append any last remaining text.
-            if (start_pos < diagnostic.description.size())
+      if (first_line > 0 && bufferID != UINT32_MAX) {
+        // Make sure the error line is in range or in another file.
+        if (diagnostic.bufferID == bufferID && !diagnostic.bufferName.empty() &&
+            (diagnostic.line < first_line || diagnostic.line > last_line))
+          continue;
+        // Need to remap the error/warning to a different line.
+        StreamString match;
+        match.Printf("%s:%u:", diagnostic.bufferName.str().c_str(),
+                     diagnostic.line);
+        const size_t match_len = match.GetString().size();
+        size_t match_pos = diagnostic.description.find(match.GetString().str());
+        if (match_pos != std::string::npos) {
+          // We have some <file>:<line>:" instances that need to be updated.
+          StreamString fixed_description;
+          size_t start_pos = 0;
+          do {
+            if (match_pos > start_pos)
               fixed_description.Printf(
-                  "%s", diagnostic.description
-                            .substr(start_pos,
-                                    diagnostic.description.size() - start_pos)
-                            .c_str());
-
-            auto new_diagnostic = std::make_unique<SwiftDiagnostic>(
-                fixed_description.GetData(), severity, origin, bufferID);
-            for (auto fixit : diagnostic.fixits)
-              new_diagnostic->AddFixIt(fixit);
-
-            diagnostic_manager.AddDiagnostic(std::move(new_diagnostic));
+                  "%s",
+                  diagnostic.description.substr(start_pos, match_pos).c_str());
+            fixed_description.Printf(
+                "%s:%u:", diagnostic.bufferName.str().c_str(),
+                diagnostic.line - first_line + 1);
+            start_pos = match_pos + match_len;
+            match_pos =
+                diagnostic.description.find(match.GetString().str(), start_pos);
+          } while (match_pos != std::string::npos);
+
+          // Append any last remaining text.
+          if (start_pos < diagnostic.description.size())
+            fixed_description.Printf(
+                "%s", diagnostic.description
+                          .substr(start_pos,
+                                  diagnostic.description.size() - start_pos)
+                          .c_str());
+
+          auto new_diagnostic = std::make_unique<SwiftDiagnostic>(
+              fixed_description.GetData(), severity, origin, bufferID);
+          for (auto fixit : diagnostic.fixits)
+            new_diagnostic->AddFixIt(fixit);
+
+          diagnostic_manager.AddDiagnostic(std::move(new_diagnostic));
+          if (diagnostic.kind == swift::DiagnosticKind::Error)
             added_one_diagnostic = true;
 
-            continue;
-          }
         }
       }
     }

lldb/test/API/lang/swift/clangimporter/expr_import/Makefile

@@ -0,0 +1,5 @@
+SWIFT_SOURCES := main.swift
+SWIFTFLAGS_EXTRAS = -Xcc -I$(SRCDIR)
+
+include Makefile.rules
+

lldb/test/API/lang/swift/clangimporter/expr_import/TestSwiftExprImport.py

@@ -0,0 +1,27 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+import unittest2
+
+class TestSwiftExprImport(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    
+    def setUp(self):
+        TestBase.setUp(self)
+
+    # Don't run ClangImporter tests if Clangimporter is disabled.
+    @skipIf(setting=('symbols.use-swift-clangimporter', 'false'))
+    @swiftTest
+    def test(self):
+        """Test error handling if the expression evaluator
+           encounters a Clang import failure.
+        """
+        self.build()
+
+        lldbutil.run_to_source_breakpoint(self, "break here",
+                                          lldb.SBFileSpec('main.swift'))
+        self.expect("expr -- import A", error=True,
+                    substrs=['SYNTAX_ERROR',
+                             'could', 'not', 'build', 'module'])

lldb/test/API/lang/swift/clangimporter/expr_import/a.h

@@ -0,0 +1 @@
+int i = SYNTAX_ERROR;

lldb/test/API/lang/swift/clangimporter/expr_import/main.swift

@@ -0,0 +1,4 @@
+func f() {}
+
+print("break here")
+f()

lldb/test/API/lang/swift/clangimporter/expr_import/module.modulemap

@@ -0,0 +1,3 @@
+module A {
+header "a.h"
+}