Add the ability to attach notes to Diagnostic/InFlightDiagnostic.
authorRiver Riddle <riverriddle@google.com>
Fri, 3 May 2019 18:40:22 +0000 (11:40 -0700)
committerMehdi Amini <joker.eph@gmail.com>
Mon, 6 May 2019 15:26:54 +0000 (08:26 -0700)
    Notes are a way to add additional context to a diagnostic, but don't really make sense as standalone diagnostics. Moving forward, notes will no longer be able to be constructed directly and must be attached to a parent Diagnostic.

    Notes can be attached via `attachNote`:

      auto diag = ...;
      diag.attachNote() << "This is a note";

--

PiperOrigin-RevId: 246545971

mlir/include/mlir/IR/Block.h
mlir/include/mlir/IR/Diagnostics.h
mlir/include/mlir/IR/OpDefinition.h
mlir/include/mlir/IR/Operation.h
mlir/lib/AffineOps/AffineOps.cpp
mlir/lib/Analysis/Verifier.cpp
mlir/lib/IR/Block.cpp
mlir/lib/IR/Diagnostics.cpp
mlir/lib/IR/Operation.cpp

index 4f33f9e..646e546 100644 (file)
@@ -396,11 +396,10 @@ public:
   }
 
   /// Check that this does not use any value defined outside it.
-  /// Emit errors if `noteEmitter` is provided; this callback is used to point
+  /// Emit errors if `noteLoc` is provided; this location is used to point
   /// to the operation containing the region, the actual error is reported at
   /// the operation with an offending use.
-  bool
-  isIsolatedAbove(llvm::function_ref<void(const Twine &)> noteEmitter = {});
+  bool isIsolatedAbove(llvm::Optional<Location> noteLoc = llvm::None);
 
 private:
   RegionType blocks;
index ebb70ba..608f2fe 100644 (file)
@@ -129,6 +129,22 @@ inline raw_ostream &operator<<(raw_ostream &os, const DiagnosticArgument &arg) {
 /// to the DiagnosticEngine. It should generally not be constructed directly,
 /// and instead used transitively via InFlightDiagnostic.
 class Diagnostic {
+  using NoteVector = std::vector<std::unique_ptr<Diagnostic>>;
+
+  /// This class implements a wrapper iterator around NoteVector::iterator to
+  /// implicitly dereference the unique_ptr.
+  template <typename IteratorTy, typename NotePtrTy = decltype(*IteratorTy()),
+            typename ResultTy = decltype(**IteratorTy())>
+  class NoteIteratorImpl
+      : public llvm::mapped_iterator<IteratorTy, ResultTy (*)(NotePtrTy)> {
+    static ResultTy &unwrap(NotePtrTy note) { return *note; }
+
+  public:
+    NoteIteratorImpl(IteratorTy it)
+        : llvm::mapped_iterator<IteratorTy, ResultTy (*)(NotePtrTy)>(it,
+                                                                     &unwrap) {}
+  };
+
 public:
   Diagnostic(Location loc, DiagnosticSeverity severity)
       : loc(loc), severity(severity) {}
@@ -170,6 +186,22 @@ public:
   /// Converts the diagnostic to a string.
   std::string str() const;
 
+  /// Attaches a note to this diagnostic. A new location may be optionally
+  /// provided, if not, then the location defaults to the one specified for this
+  /// diagnostic. Notes may not be attached to other notes.
+  Diagnostic &attachNote(llvm::Optional<Location> noteLoc = llvm::None);
+
+  using note_iterator = NoteIteratorImpl<NoteVector::iterator>;
+  using const_note_iterator = NoteIteratorImpl<NoteVector::const_iterator>;
+
+  /// Returns the notes held by this diagnostic.
+  llvm::iterator_range<note_iterator> getNotes() {
+    return {notes.begin(), notes.end()};
+  }
+  llvm::iterator_range<const_note_iterator> getNotes() const {
+    return {notes.begin(), notes.end()};
+  }
+
 private:
   Diagnostic(const Diagnostic &rhs) = delete;
   Diagnostic &operator=(const Diagnostic &rhs) = delete;
@@ -187,6 +219,9 @@ private:
   /// those arguments. This is used to guarantee the liveness of non-constant
   /// strings used in diagnostics.
   std::vector<std::pair<llvm::SmallString<0>, unsigned>> stringArguments;
+
+  /// A list of attached notes.
+  NoteVector notes;
 };
 
 inline raw_ostream &operator<<(raw_ostream &os, const Diagnostic &diag) {
@@ -224,6 +259,12 @@ public:
     return *this;
   }
 
+  /// Attaches a note to this diagnostic.
+  Diagnostic &attachNote(llvm::Optional<Location> noteLoc = llvm::None) {
+    assert(isInFlight() && "diagnostic not inflight");
+    return impl->attachNote(noteLoc);
+  }
+
   /// Reports the diagnostic to the engine.
   void report();
 
@@ -271,7 +312,7 @@ public:
   // Diagnostic handler registration and use.  MLIR supports the ability for the
   // IR to carry arbitrary metadata about operation location information.  If a
   // problem is detected by the compiler, it can invoke the emitError /
-  // emitWarning / emitNote method on an Operation and have it get reported
+  // emitWarning / emitRemark method on an Operation and have it get reported
   // through this interface.
   //
   // Tools using MLIR are encouraged to register error handlers and define a
@@ -292,6 +333,8 @@ public:
 
   /// Create a new inflight diagnostic with the given location and severity.
   InFlightDiagnostic emit(Location loc, DiagnosticSeverity severity) {
+    assert(severity != DiagnosticSeverity::Note &&
+           "notes should not be emitted directly");
     return InFlightDiagnostic(this, Diagnostic(loc, severity));
   }
 
index 636122a..08fd4ef 100644 (file)
@@ -115,10 +115,6 @@ public:
   /// handlers that may be listening.
   InFlightDiagnostic emitWarning(const Twine &message);
 
-  /// Emit a note about this operation, reporting up to any diagnostic
-  /// handlers that may be listening.
-  InFlightDiagnostic emitNote(const Twine &message);
-
   /// Emit a remark about this operation, reporting up to any diagnostic
   /// handlers that may be listening.
   InFlightDiagnostic emitRemark(const Twine &message);
@@ -708,9 +704,8 @@ public:
                                 NthRegionIsIsolatedAbove<RegionIdx>::Impl> {
   public:
     static LogicalResult verifyTrait(Operation *op) {
-      auto noteEmitter = [op](const Twine &message) { op->emitNote(message); };
-      return op->getRegion(RegionIdx).isIsolatedAbove(noteEmitter) ? success()
-                                                                   : failure();
+      return op->getRegion(RegionIdx).isIsolatedAbove(op->getLoc()) ? success()
+                                                                    : failure();
     }
   };
 };
index 23e147a..908f001 100644 (file)
@@ -441,10 +441,6 @@ public:
   /// handlers that may be listening.
   InFlightDiagnostic emitWarning(const Twine &message);
 
-  /// Emit a note about this operation, reporting up to any diagnostic
-  /// handlers that may be listening.
-  InFlightDiagnostic emitNote(const Twine &message);
-
   /// Emit a remark about this operation, reporting up to any diagnostic
   /// handlers that may be listening.
   InFlightDiagnostic emitRemark(const Twine &message);
index 7428345..1e26d0e 100644 (file)
@@ -692,9 +692,10 @@ static LogicalResult checkHasAffineTerminator(OpState &op, Block &block) {
     return success();
 
   op.emitOpError("expects regions to end with '" +
-                 AffineTerminatorOp::getOperationName() + "'");
-  op.emitNote("in custom textual format, the absence of terminator implies '" +
-              AffineTerminatorOp::getOperationName() + "'");
+                 AffineTerminatorOp::getOperationName() + "'")
+          .attachNote()
+      << "in custom textual format, the absence of terminator implies '"
+      << AffineTerminatorOp::getOperationName() << "'";
   return failure();
 }
 
index ee4fd73..6d0b91d 100644 (file)
@@ -349,10 +349,10 @@ LogicalResult FuncVerifier::verifyOpDominance(Operation &op) {
     if (domInfo->properlyDominates(operand, &op))
       continue;
 
-    op.emitError("operand #" + Twine(operandNo) +
-                 " does not dominate this use");
+    auto diag = op.emitError("operand #" + Twine(operandNo) +
+                             " does not dominate this use");
     if (auto *useOp = operand->getDefiningOp())
-      useOp->emitNote("operand defined here");
+      diag.attachNote(useOp->getLoc()) << "operand defined here";
     return failure();
   }
 
index a06a905..a440ca9 100644 (file)
@@ -353,15 +353,14 @@ void Region::cloneInto(Region *dest, BlockAndValueMapping &mapper,
     it->walk(remapOperands);
 }
 
-// Check that the given `region` does not use any value defined outside its
-// ancestor region `limit`.  That is, given `A{B{C{}}}` with limit `B`, `C` is
-// allowed to use values defined in `B` but not those defined in `A`.
-// Emit errors if `emitOpNote` is provided; this callback is used to point to
-// the operation containing the region, the actual error is reported at the
-// operation with an offending use.
-static bool
-isRegionIsolatedAbove(Region &region, Region &limit,
-                      llvm::function_ref<void(const Twine &)> emitOpNote = {}) {
+/// Check that the given `region` does not use any value defined outside its
+/// ancestor region `limit`.  That is, given `A{B{C{}}}` with limit `B`, `C` is
+/// allowed to use values defined in `B` but not those defined in `A`.
+/// Emit errors if `noteLoc` is provided; this location is used to point to
+/// the operation containing the region, the actual error is reported at the
+/// operation with an offending use.
+static bool isRegionIsolatedAbove(Region &region, Region &limit,
+                                  llvm::Optional<Location> noteLoc) {
   assert(limit.isAncestor(&region) &&
          "expected isolation limit to be an ancestor of the given region");
 
@@ -379,9 +378,10 @@ isRegionIsolatedAbove(Region &region, Region &limit,
           // Check that any value that is used by an operation is defined in the
           // same region as either an operation result or a block argument.
           if (operand->getContainingRegion()->isProperAncestor(&limit)) {
-            if (emitOpNote) {
-              op.emitOpError("using value defined outside the region");
-              emitOpNote("required by region isolation constraints");
+            if (noteLoc) {
+              op.emitOpError("using value defined outside the region")
+                      .attachNote(noteLoc)
+                  << "required by region isolation constraints";
             }
             return false;
           }
@@ -397,9 +397,8 @@ isRegionIsolatedAbove(Region &region, Region &limit,
   return true;
 }
 
-bool Region::isIsolatedAbove(
-    llvm::function_ref<void(const Twine &)> noteEmitter) {
-  return isRegionIsolatedAbove(*this, *this, noteEmitter);
+bool Region::isIsolatedAbove(llvm::Optional<Location> noteLoc) {
+  return isRegionIsolatedAbove(*this, *this, noteLoc);
 }
 
 Region *llvm::ilist_traits<::mlir::Block>::getContainingRegion() {
index b266183..ba82040 100644 (file)
@@ -76,6 +76,24 @@ std::string Diagnostic::str() const {
   return os.str();
 }
 
+/// Attaches a note to this diagnostic. A new location may be optionally
+/// provided, if not, then the location defaults to the one specified for this
+/// diagnostic. Notes may not be attached to other notes.
+Diagnostic &Diagnostic::attachNote(llvm::Optional<Location> noteLoc) {
+  // We don't allow attaching notes to notes.
+  assert(severity != DiagnosticSeverity::Note &&
+         "cannot attach a note to a note");
+
+  // If a location wasn't provided then reuse our location.
+  if (!noteLoc)
+    noteLoc = loc;
+
+  /// Append and return a new note.
+  notes.push_back(
+      llvm::make_unique<Diagnostic>(*noteLoc, DiagnosticSeverity::Note));
+  return *notes.back();
+}
+
 //===----------------------------------------------------------------------===//
 // InFlightDiagnostic
 //===----------------------------------------------------------------------===//
@@ -119,9 +137,6 @@ struct DiagnosticEngineImpl {
 /// the default behavior if not.
 void DiagnosticEngineImpl::emit(Location loc, StringRef msg,
                                 DiagnosticSeverity severity) {
-  // Lock access to the handler.
-  llvm::sys::SmartScopedLock<true> lock(mutex);
-
   // If we had a handler registered, emit the diagnostic using it.
   if (handler) {
     // TODO(b/131756158) FusedLoc should be handled by the diagnostic handler
@@ -181,5 +196,12 @@ auto DiagnosticEngine::getHandler() -> HandlerTy {
 /// Emit a diagnostic using the registered issue handler if present, or with
 /// the default behavior if not.
 void DiagnosticEngine::emit(const Diagnostic &diag) {
+  assert(diag.getSeverity() != DiagnosticSeverity::Note &&
+         "notes should not be emitted directly");
+  llvm::sys::SmartScopedLock<true> lock(impl->mutex);
   impl->emit(diag.getLocation(), diag.str(), diag.getSeverity());
+
+  // Emit any notes that were attached to this diagnostic.
+  for (auto &note : diag.getNotes())
+    impl->emit(note.getLocation(), note.str(), note.getSeverity());
 }
index da02c58..2ed3ea3 100644 (file)
@@ -311,13 +311,6 @@ InFlightDiagnostic Operation::emitRemark(const Twine &message) {
   return getContext()->emitRemark(getLoc(), message);
 }
 
-/// Emit a note about this operation, reporting up to any diagnostic
-/// handlers that may be listening.
-InFlightDiagnostic Operation::emitNote(const Twine &message) {
-  return getContext()->getDiagEngine().emit(getLoc(), DiagnosticSeverity::Note)
-         << message;
-}
-
 /// Emit a warning about this operation, reporting up to any diagnostic
 /// handlers that may be listening.
 InFlightDiagnostic Operation::emitWarning(const Twine &message) {
@@ -668,12 +661,6 @@ InFlightDiagnostic OpState::emitWarning(const Twine &message) {
   return getOperation()->emitWarning(message);
 }
 
-/// Emit a note about this operation, reporting up to any diagnostic
-/// handlers that may be listening.
-InFlightDiagnostic OpState::emitNote(const Twine &message) {
-  return getOperation()->emitNote(message);
-}
-
 /// Emit a remark about this operation, reporting up to any diagnostic
 /// handlers that may be listening.
 InFlightDiagnostic OpState::emitRemark(const Twine &message) {