[mlir][Diagnostics] Don't print note source line if it is the same as the previous diagnostic

[rriddle]

Summary

This revision updates the SourceMgrDiagnosticHandler to not print the source location of a note if it is the same location as the previously printed diagnostic. This helps avoid redundancy, and potential confusion, when looking at the diagnostic output.

[mehdi_amini]

We don't have tests for the note emission?

mlir/include/mlir/IR/Diagnostics.h

551

Worth documenting it here?

[rriddle]

In D76787#1941737, @mehdi_amini wrote:

We don't have tests for the note emission?

We don't have tests that check that a specific line of source code is printed. We have many tests for note emission.

mlir/include/mlir/IR/Diagnostics.h

551

When I originally wrote a comment it came out as redundant with the parameter name.

[rriddle]

Rebase

mlir/include/mlir/IR/Diagnostics.h

 class SourceMgrDiagnosticHandler : public ScopedDiagnosticHandler {
 public:
   SourceMgrDiagnosticHandler(llvm::SourceMgr &mgr, MLIRContext *ctx,
                              raw_ostream &os);
   SourceMgrDiagnosticHandler(llvm::SourceMgr &mgr, MLIRContext *ctx);
   ~SourceMgrDiagnosticHandler();
 
   /// Emit the given diagnostic information with the held source manager.
-  void emitDiagnostic(Location loc, Twine message, DiagnosticSeverity kind);
+  void emitDiagnostic(Location loc, Twine message, DiagnosticSeverity kind,
+                      bool displaySourceLine = true);
 
 protected:
   /// Emit the given diagnostic with the held source manager.
   void emitDiagnostic(Diagnostic &diag);
 
   /// Get a memory buffer for the given file, or nullptr if no file is
   /// available.
   const llvm::MemoryBuffer *getBufferForFile(StringRef filename);

mlir/lib/IR/Diagnostics.cpp

 
 SourceMgrDiagnosticHandler::SourceMgrDiagnosticHandler(llvm::SourceMgr &mgr,
                                                        MLIRContext *ctx)
     : SourceMgrDiagnosticHandler(mgr, ctx, llvm::errs()) {}
 
 SourceMgrDiagnosticHandler::~SourceMgrDiagnosticHandler() {}
 
 void SourceMgrDiagnosticHandler::emitDiagnostic(Location loc, Twine message,
-                                                DiagnosticSeverity kind) {
+                                                DiagnosticSeverity kind,
+                                                bool displaySourceLine) {
   // Extract a file location from this loc.
   auto fileLoc = getFileLineColLoc(loc);
 
   // If one doesn't exist, then print the raw message without a source location.
   if (!fileLoc) {
     std::string str;
     llvm::raw_string_ostream strOS(str);
     if (!loc.isa<UnknownLoc>())
       strOS << loc << ": ";
     strOS << message;
     return mgr.PrintMessage(os, llvm::SMLoc(), getDiagKind(kind), strOS.str());
   }
 
-  // Otherwise, try to convert the file location to an SMLoc.
+  // Otherwise if we are displaying the source line, try to convert the file
+  // location to an SMLoc.
+  if (displaySourceLine) {
     auto smloc = convertLocToSMLoc(*fileLoc);
     if (smloc.isValid())
       return mgr.PrintMessage(os, smloc, getDiagKind(kind), message);
+  }
 
   // If the conversion was unsuccessful, create a diagnostic with the file
-  // information.
-  llvm::SMDiagnostic diag(fileLoc->getFilename(), getDiagKind(kind),
-                          message.str());
+  // information. We manually combine the line and column to avoid asserts in
+  // the constructor of SMDiagnostic that takes a location.
+  std::string locStr;
+  llvm::raw_string_ostream locOS(locStr);
+  locOS << fileLoc->getFilename() << ":" << fileLoc->getLine() << ":"
+        << fileLoc->getColumn();
+  llvm::SMDiagnostic diag(locOS.str(), getDiagKind(kind), message.str());
   diag.print(nullptr, os);
 }
 
 /// Emit the given diagnostic with the held source manager.
 void SourceMgrDiagnosticHandler::emitDiagnostic(Diagnostic &diag) {
   // Emit the diagnostic.
-  auto loc = diag.getLocation();
+  Location loc = diag.getLocation();
   emitDiagnostic(loc, diag.str(), diag.getSeverity());
 
   // If the diagnostic location was a call site location, then print the call
   // stack as well.
   if (auto callLoc = getCallSiteLoc(loc)) {
     // Print the call stack while valid, or until the limit is reached.
-    Location callerLoc = callLoc->getCaller();
+    loc = callLoc->getCaller();
     for (unsigned curDepth = 0; curDepth < callStackLimit; ++curDepth) {
-      emitDiagnostic(callerLoc, "called from", DiagnosticSeverity::Note);
-      callLoc = getCallSiteLoc(callerLoc);
-      if (!callLoc)
+      emitDiagnostic(loc, "called from", DiagnosticSeverity::Note);
+      if ((callLoc = getCallSiteLoc(loc)))
+        loc = callLoc->getCaller();
+      else
         break;
-      callerLoc = callLoc->getCaller();
     }
   }
 
-  // Emit each of the notes.
-  for (auto &note : diag.getNotes())
-    emitDiagnostic(note.getLocation(), note.str(), note.getSeverity());
+  // Emit each of the notes. Only display the source code if the location is
+  // different from the previous location.
+  for (auto &note : diag.getNotes()) {
+    emitDiagnostic(note.getLocation(), note.str(), note.getSeverity(),
+                   /*displaySourceLine=*/loc != note.getLocation());
+    loc = note.getLocation();
+  }
 }
 
 /// Get a memory buffer for the given file, or nullptr if one is not found.
 const llvm::MemoryBuffer *
 SourceMgrDiagnosticHandler::getBufferForFile(StringRef filename) {
   return impl->getBufferForFile(mgr, filename);
 }
 

mlir/test/IR/diagnostic-handler.mlir

 // RUN: not mlir-opt %s -o - 2>&1 | FileCheck %s
 // This test verifies that diagnostic handler can emit the call stack successfully.
 
 // -----
 
 // Emit the first available call stack in the fused location.
 func @constant_out_of_range() {
-  // CHECK: mysource1: error: 'std.constant' op requires attribute's type ('i64') to match op's return type ('i1')
-  // CHECK-NEXT: mysource2: note: called from
-  // CHECK-NEXT: mysource3: note: called from
+  // CHECK: mysource1:0:0: error: 'std.constant' op requires attribute's type ('i64') to match op's return type ('i1')
+  // CHECK-NEXT: mysource2:1:0: note: called from
+  // CHECK-NEXT: mysource3:2:0: note: called from
   %x = "std.constant"() {value = 100} : () -> i1 loc(fused["bar", callsite("foo"("mysource1":0:0) at callsite("mysource2":1:0 at "mysource3":2:0))])
   return
 }