Merge pull request #31124 from owenv/pretty-printed-diags-source-location-fixes

swift-ci · web-flow · commit 0ab9fd718ab1 · 2020-05-20T13:10:36.000-07:00
diff --git a/include/swift/Basic/SourceManager.h b/include/swift/Basic/SourceManager.h
@@ -268,6 +268,11 @@ class SourceManager {
     else
       return 0;
   }
+
+public:
+  bool isLocInVirtualFile(SourceLoc Loc) const {
+    return getVirtualFile(Loc) != nullptr;
+  }
 };
 
 } // end namespace swift
diff --git a/lib/Frontend/PrintingDiagnosticConsumer.cpp b/lib/Frontend/PrintingDiagnosticConsumer.cpp
@@ -381,6 +381,9 @@ namespace {
     };
 
     unsigned LineNumber;
+    // The line number displayed to the user. This may differ from the actual
+    // line number if #sourceLocation is used.
+    unsigned DisplayLineNumber;
     std::string LineText;
     SmallVector<Message, 1> Messages;
     SmallVector<Highlight, 1> Highlights;
@@ -448,8 +451,10 @@ namespace {
     }
 
   public:
-    AnnotatedLine(unsigned LineNumber, StringRef LineText)
-        : LineNumber(LineNumber), LineText(LineText) {}
+    AnnotatedLine(unsigned LineNumber, unsigned DisplayLineNumber,
+                  StringRef LineText)
+        : LineNumber(LineNumber), DisplayLineNumber(DisplayLineNumber),
+          LineText(LineText) {}
 
     unsigned getLineNumber() { return LineNumber; }
 
@@ -469,7 +474,7 @@ namespace {
     }
 
     void render(unsigned LineNumberIndent, raw_ostream &Out) {
-      printNumberedGutter(LineNumber, LineNumberIndent, Out);
+      printNumberedGutter(DisplayLineNumber, LineNumberIndent, Out);
 
       // Determine if the line is all-ASCII. This will determine a number of
       // later formatting decisions.
@@ -628,21 +633,25 @@ namespace {
     /// diagnostic message. This is printed alongside the file path so it can be
     /// parsed by editors and other tooling.
     SourceLoc PrimaryLoc;
+    /// Whether the excerpt is from a virtual file (e.g. one introduced using
+    /// #sourceLocation).
+    bool FromVirtualFile;
     std::vector<AnnotatedLine> AnnotatedLines;
 
     /// Return the AnnotatedLine for a given SourceLoc, creating it if it
     /// doesn't already exist.
     AnnotatedLine &lineForLoc(SourceLoc Loc) {
-      // FIXME: This call to `getLineAndColumn` is expensive.
-      unsigned lineNo = SM.getLineAndColumn(Loc).first;
-      AnnotatedLine newLine(lineNo, "");
+      // FIXME: This call to `getLineNumber` is expensive.
+      unsigned lineNo = SM.getLineNumber(Loc);
+      AnnotatedLine newLine(lineNo, 0, "");
       auto iter =
           std::lower_bound(AnnotatedLines.begin(), AnnotatedLines.end(),
                            newLine, [](AnnotatedLine l1, AnnotatedLine l2) {
                              return l1.getLineNumber() < l2.getLineNumber();
                            });
       if (iter == AnnotatedLines.end() || iter->getLineNumber() != lineNo) {
         newLine.LineText = SM.getLineString(BufferID, lineNo);
+        newLine.DisplayLineNumber = SM.getLineAndColumn(Loc).first;
         return *AnnotatedLines.insert(iter, newLine);
       } else {
         return *iter;
@@ -658,10 +667,8 @@ namespace {
 
     void lineRangesForRange(CharSourceRange Range,
                             SmallVectorImpl<CharSourceRange> &LineRanges) {
-      // FIXME: The calls to `getLineAndColumn` and `getLocForLineCol` are
-      // expensive.
-      unsigned startLineNo = SM.getLineAndColumn(Range.getStart()).first;
-      unsigned endLineNo = SM.getLineAndColumn(Range.getEnd()).first;
+      unsigned startLineNo = SM.getLineNumber(Range.getStart());
+      unsigned endLineNo = SM.getLineNumber(Range.getEnd());
 
       if (startLineNo == endLineNo) {
         LineRanges.push_back(Range);
@@ -687,10 +694,19 @@ namespace {
       LineRanges.push_back(CharSourceRange(SM, lastLineStart, Range.getEnd()));
     }
 
+    void printLineEllipsis(raw_ostream &Out) {
+      Out.changeColor(ColoredStream::Colors::CYAN, true);
+      Out << llvm::formatv("{0}...\n",
+                           llvm::fmt_repeat(" ", getPreferredLineNumberIndent()));
+      Out.resetColor();
+    }
+
   public:
     AnnotatedFileExcerpt(SourceManager &SM, unsigned BufferID,
                          SourceLoc PrimaryLoc)
-        : SM(SM), BufferID(BufferID), PrimaryLoc(PrimaryLoc) {}
+        : SM(SM), BufferID(BufferID), PrimaryLoc(PrimaryLoc) {
+      FromVirtualFile = SM.isLocInVirtualFile(PrimaryLoc);
+    }
 
     unsigned getPreferredLineNumberIndent() {
       // The lines are already in sorted ascending order, and we render one line
@@ -734,13 +750,14 @@ namespace {
       auto primaryLineAndColumn = SM.getLineAndColumn(PrimaryLoc);
       Out.changeColor(ColoredStream::Colors::CYAN);
       Out << std::string(lineNumberIndent + 1, '=') << " "
-          << SM.getIdentifierForBuffer(BufferID) << ":"
+          << SM.getDisplayNameForLoc(PrimaryLoc) << ":"
           << primaryLineAndColumn.first << ":" << primaryLineAndColumn.second
           << " " << std::string(lineNumberIndent + 1, '=') << "\n";
       Out.resetColor();
 
-      // Print one extra line at the top for context.
-      if (AnnotatedLines.front().getLineNumber() > 1)
+      // Print one extra line at the top for context, so long as this isn't an
+      // excerpt from a virtual file.
+      if (AnnotatedLines.front().getLineNumber() > 1 && !FromVirtualFile)
         printNumberedLine(SM, BufferID,
                           AnnotatedLines.front().getLineNumber() - 1,
                           lineNumberIndent, Out);
@@ -754,14 +771,18 @@ namespace {
       for (auto line = AnnotatedLines.begin() + 1; line != AnnotatedLines.end();
            ++line) {
         unsigned lineNumber = line->getLineNumber();
-        if (lineNumber - lastLineNumber > maxIntermediateLines) {
+        if (FromVirtualFile) {
+          // Don't print intermediate lines in virtual files, as they may not
+          // make sense in context. Instead, just print an ellipsis between them
+          // if they're not consecutive in the actual source file.
+          if (lineNumber - lastLineNumber > 1) {
+            printLineEllipsis(Out);
+          }
+        } else if (lineNumber - lastLineNumber > maxIntermediateLines) {
           // Use an ellipsis to denote an ommitted part of the file.
           printNumberedLine(SM, BufferID, lastLineNumber + 1, lineNumberIndent,
                             Out);
-          Out.changeColor(ColoredStream::Colors::CYAN);
-          Out << llvm::formatv("{0}...\n",
-                               llvm::fmt_repeat(" ", lineNumberIndent));
-          Out.resetColor();
+          printLineEllipsis(Out);
           printNumberedLine(SM, BufferID, lineNumber - 1, lineNumberIndent,
                             Out);
         } else {
@@ -774,11 +795,14 @@ namespace {
         line->render(lineNumberIndent, Out);
         lastLineNumber = lineNumber;
       }
-      // Print one extra line at the bottom for context.
-      printNumberedLine(
-          SM, BufferID,
-          AnnotatedLines[AnnotatedLines.size() - 1].getLineNumber() + 1,
-          lineNumberIndent, Out);
+      // Print one extra line at the bottom for context, so long as the excerpt
+      // isn't from a virtual file.
+      if (!FromVirtualFile) {
+        printNumberedLine(
+            SM, BufferID,
+            AnnotatedLines[AnnotatedLines.size() - 1].getLineNumber() + 1,
+            lineNumberIndent, Out);
+      }
     }
   };
 } // end anonymous namespace
@@ -788,14 +812,17 @@ namespace swift {
 /// complete diagnostic message.
 class AnnotatedSourceSnippet {
   SourceManager &SM;
-  std::map<unsigned, AnnotatedFileExcerpt> FileExcerpts;
+  std::map<StringRef, AnnotatedFileExcerpt> FileExcerpts;
   SmallVector<std::pair<DiagnosticKind, std::string>, 1>
       UnknownLocationMessages;
 
   AnnotatedFileExcerpt &excerptForLoc(SourceLoc Loc) {
+    StringRef bufName = SM.getDisplayNameForLoc(Loc);
     unsigned bufID = SM.findBufferContainingLoc(Loc);
-    FileExcerpts.emplace(bufID, AnnotatedFileExcerpt(SM, bufID, Loc));
-    return FileExcerpts.find(bufID)->second;
+    // Use the buffer display name as the key in the excerpt map instead of the
+    // buffer identifier to respect #sourceLocation directives.
+    FileExcerpts.emplace(bufName, AnnotatedFileExcerpt(SM, bufID, Loc));
+    return FileExcerpts.find(bufName)->second;
   }
 
 public:
diff --git a/test/diagnostics/Inputs/RenamedObjc.h b/test/diagnostics/Inputs/RenamedObjc.h
@@ -0,0 +1,9 @@
+@import Foundation;
+
+#define SWIFT_NAME(X) __attribute__((swift_name(#X)))
+
+#pragma clang assume_nonnull begin
+@interface SwiftNameTest : NSObject
++ (instancetype)g:(id)x outParam:(int *)foo SWIFT_NAME(init(g:));
+@end
+#pragma clang assume_nonnull end
diff --git a/test/diagnostics/Inputs/module.map b/test/diagnostics/Inputs/module.map
@@ -0,0 +1,4 @@
+module RenamedObjc {
+  header "RenamedObjc.h"
+  export *
+}
diff --git a/test/diagnostics/pretty-printed-diags-in-clang-buffer.swift b/test/diagnostics/pretty-printed-diags-in-clang-buffer.swift
@@ -0,0 +1,20 @@
+// RUN: %empty-directory(%t.mcp)
+// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -enable-experimental-diagnostic-formatting -I %S/Inputs/ -typecheck %s -module-cache-path %t.mcp 2>&1 | %FileCheck %s
+
+// REQUIRES: objc_interop
+
+import RenamedObjc
+
+let foo = SwiftNameTest(g: "")
+
+// CHECK: SOURCE_DIR{{[/\]+}}test{{[/\]+}}diagnostics{{[/\]+}}Inputs{{[/\]+}}RenamedObjc.h:7:1
+// CHECK: 7 | + (instancetype)g:(id)x outParam:(int *)foo SWIFT_NAME(init(g:));
+// CHECK:   | ^ warning: too few parameters in swift_name attribute (expected 2; got 1)
+// CHECK:   | ^ note: please report this issue to the owners of 'RenamedObjc'
+
+
+// CHECK: SOURCE_DIR{{[/\]+}}test{{[/\]+}}diagnostics{{[/\]+}}pretty-printed-diags-in-clang-buffer.swift:8:28
+// CHECK: 7 |
+// CHECK: 8 | let foo = SwiftNameTest(g: "")
+// CHECK:   |                            ~~
+// CHECK:   |                            ^ error: argument passed to call that takes no arguments
diff --git a/test/diagnostics/pretty-printed-source-loc-directive-diags.swift b/test/diagnostics/pretty-printed-source-loc-directive-diags.swift
@@ -0,0 +1,77 @@
+// RUN: not %target-swift-frontend -enable-experimental-diagnostic-formatting -typecheck %s 2>&1 | %FileCheck %s
+
+// Error split between the real file and a virtual one.
+#sourceLocation(file: "abc.swift", line: 9)
+let x = 1
+#sourceLocation()
+let x = 2
+
+// Error split between two virtual files.
+#sourceLocation(file: "abc.swift", line: 4)
+let y = 1
+#sourceLocation(file: "xyz.swift", line: 18)
+let y = 2
+#sourceLocation()
+
+// Error within a virtual file on non-consecutive lines.
+#sourceLocation(file: "abc.swift", line: 1)
+let z = 1
+// space
+let z = 2
+#sourceLocation()
+
+// Error with note location placed in the same virtual file via a separate #sourceLocation block.
+#sourceLocation(file: "abc.swift", line: 1)
+let a = 1
+#sourceLocation()
+
+
+#sourceLocation(file: "abc.swift", line: 10)
+let a = 2
+#sourceLocation()
+
+// Error at the beginning of a virtual file.
+#sourceLocation(file: "abc.swift", line: 1)
+let any: Any = ""
+let zz: Int = any
+#sourceLocation()
+
+// CHECK: SOURCE_DIR{{[/\]+}}test{{[/\]+}}diagnostics{{[/\]+}}pretty-printed-source-loc-directive-diags.swift:[[#LINE:]]:5
+// CHECK: [[#LINE-1]] | #sourceLocation()
+// CHECK: [[#LINE]]   | let x = 2
+// CHECK:             |     ^ error: invalid redeclaration of 'x'
+// CHECK: [[#LINE+1]] |
+// CHECK: abc.swift:9:5
+// CHECK: 9 | let x = 1
+// CHECK:   |     ^ note: 'x' previously declared here
+
+
+// CHECK: abc.swift:4:5
+// CHECK:  4 | let y = 1
+// CHECK:    |     ^ note: 'y' previously declared here
+// CHECK: xyz.swift:18:5
+// CHECK: 18 | let y = 2
+// CHECK:    |     ^ error: invalid redeclaration of 'y'
+
+
+// CHECK: abc.swift:3:5
+// CHECK:  1 | let z = 1
+// CHECK:    |     ^ note: 'z' previously declared here
+// CHECK:   ...
+// CHECK:  3 | let z = 2
+// CHECK:    |     ^ error: invalid redeclaration of 'z'
+
+
+// CHECK: abc.swift:10:5
+// CHECK:  1 | let a = 1
+// CHECK:    |     ^ note: 'a' previously declared here
+// CHECK:   ...
+// CHECK: 10 | let a = 2
+// CHECK:    |     ^ error: invalid redeclaration of 'a'
+
+
+// CHECK: abc.swift:2:15
+// CHECK:  2 | let zz: Int = any as! Int
+// CHECK:    |               ~~~++++++++
+// CHECK:    |               ^ error: cannot convert value of type 'Any' to specified type 'Int' [insert ' as! Int']
+
