[Basic] Add a DiagnosticError llvm::ErrorInfo subclass

[arphaman]

Summary

Clang's DiagnosticError is an llvm::Error payload that stores a partial diagnostic and its location. I'll be using it in D36075.

[vsk]

Would it be more convenient to extend ErrorInfo instead of creating a new diagnostic wrapper? E.g one benefit of passing around Expected<T>'s is that there's some assurance the diagnostics will be reported.

[arphaman]

In D36969#847803, @vsk wrote:

Would it be more convenient to extend ErrorInfo instead of creating a new diagnostic wrapper? E.g one benefit of passing around Expected<T>'s is that there's some assurance the diagnostics will be reported.

Possibly, but one issue I found with ErrorInfo is that it gets harder to report diagnostics. E.g. with DiagnosticOr you will be able to write:

DiagnosticOr<...> function(RefactoringOperation &Op) {
  return Op.Diag(Loc, err::something) << "invalid";
}

But with Expected you'll have to use something like:

Expected<...> function(RefactoringOperation &Op) {
  return llvm::make_error<DiagnosticError>(Op.Diag(Loc, err::something) << "invalid");
}

I don't think that the assurance about reporting these diagnostics is that important for my case. They will be consumed at most in 3 places (clang-refactor, clangd, and libclang) and clangd and it will obvious to the user if errors aren't getting reported.

[arphaman]

In D36969#848518, @arphaman wrote:

In D36969#847803, @vsk wrote:

Would it be more convenient to extend ErrorInfo instead of creating a new diagnostic wrapper? E.g one benefit of passing around Expected<T>'s is that there's some assurance the diagnostics will be reported.

Possibly, but one issue I found with ErrorInfo is that it gets harder to report diagnostics. E.g. with DiagnosticOr you will be able to write:

DiagnosticOr<...> function(RefactoringOperation &Op) {
  return Op.Diag(Loc, err::something) << "invalid";
}

But with Expected you'll have to use something like:

Expected<...> function(RefactoringOperation &Op) {
  return llvm::make_error<DiagnosticError>(Op.Diag(Loc, err::something) << "invalid");
}

I don't think that the assurance about reporting these diagnostics is that important for my case. They will be consumed at most in 3 places (clang-refactor, clangd, and libclang) and clangd and it will obvious to the user if errors aren't getting reported.

Although I guess I could always change the approach completely and use variadic templates instead of <<, e.g.:

template<typename... T>
Error RefactoringOperation::error(SourceLocation Loc, unsigned DiagId, const T &... Arguments) {
  return llvm::make_error<DiagnosticError>(...);
}

DiagnosticOr<...> function(RefactoringOperation &Op) {
   return Op.error(Loc, diag::err_something, "invalid");
}

I think I'll try that instead.

[arphaman]

Use ErrorInfo subclass as suggested by Vedant.

[vsk]

Nice! I'd like to get your thoughts on two candidate ergonomic changes:

unittests/Basic/DiagnosticTest.cpp

81

It might be useful to add an Error-returning wrapper around PDA, e.g:
static Error PartialDiagnosticAt::get(...)

90

Creating a null diagnostic before taking an error may become cumbersome. An alternative is to add a static helper in DiagnosticError:

static ParitalDiagnosticAt &DiagnosticError::take(Error E) {
  PartialDiagnosticAt *PDA = nullptr;
  handleAllErrors(std::move(E), [&](DE) { PDA = &DE.getDiagnostic(); });
  assert(PDA && "Expected a DiagnosticError");
  return *PDA;
}

This is more ergonomic, but the downside is that you lose some flexibility (what happens if there are two kinds of errors?, etc).

Another alternative is to switch to a callback-driven style (though that might require a lot of refactoring).

[arphaman]

In D36969#848906, @vsk wrote:

Nice! I'd like to get your thoughts on two candidate ergonomic changes:

Makes sense, I've added two similar helper functions in the updated patch.

[arphaman]

Add helper functions as suggested by Vedant.

[vsk]

LGTM!

include/clang/Basic/DiagnosticError.h

+//===--- DiagnosticError.h - Diagnostic payload for llvm::Error -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_BASIC_DIAGNOSTIC_ERROR_H
+#define LLVM_CLANG_BASIC_DIAGNOSTIC_ERROR_H
+
+#include "clang/Basic/PartialDiagnostic.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+
+/// \brief Carries a Clang diagnostic in an llvm::Error.
+///
+/// Users should emit the stored diagnostic using the DiagnosticsEngine.
+class DiagnosticError : public llvm::ErrorInfo<DiagnosticError> {
+public:
+  DiagnosticError(PartialDiagnosticAt Diag) : Diag(std::move(Diag)) {}
+
+  void log(raw_ostream &OS) const override { OS << "clang diagnostic"; }
+
+  PartialDiagnosticAt &getDiagnostic() { return Diag; }
+  const PartialDiagnosticAt &getDiagnostic() const { return Diag; }
+
+  /// Creates a new \c DiagnosticError that contains the given diagnostic at
+  /// the given location.
+  static llvm::Error create(SourceLocation Loc, PartialDiagnostic Diag) {
+    return llvm::make_error<DiagnosticError>(
+        PartialDiagnosticAt(Loc, std::move(Diag)));
+  }
+
+  /// Extracts and returns the diagnostic payload from the given \c Error if
+  /// the error is a \c DiagnosticError. Returns none if the given error is not
+  /// a \c DiagnosticError.
+  static Optional<PartialDiagnosticAt> take(llvm::Error &Err) {
+    Optional<PartialDiagnosticAt> Result;
+    Err = llvm::handleErrors(std::move(Err), [&](DiagnosticError &E) {
+      Result = std::move(E.getDiagnostic());
+    });
+    return Result;
+  }
+
+  static char ID;
+
+private:
+  // Users are not expected to use error_code.
+  std::error_code convertToErrorCode() const override {
+    return llvm::inconvertibleErrorCode();
+  }
+
+  PartialDiagnosticAt Diag;
+};
+
+} // end namespace clang
+
+#endif // LLVM_CLANG_BASIC_DIAGNOSTIC_ERROR_H

lib/Basic/Diagnostic.cpp

 //===--- Diagnostic.cpp - C Language Family Diagnostic Handling -----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the Diagnostic-related interfaces.
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/CharInfo.h"
+#include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Locale.h"

 
 PartialDiagnostic::StorageAllocator::~StorageAllocator() {
   // Don't assert if we are in a CrashRecovery context, as this invariant may
   // be invalidated during a crash.
   assert((NumFreeListEntries == NumCached ||
           llvm::CrashRecoveryContext::isRecoveringFromCrash()) &&
          "A partial is on the lam");
 }
+
+char DiagnosticError::ID;

unittests/Basic/DiagnosticTest.cpp

 //===- unittests/Basic/DiagnosticTest.cpp -- Diagnostic engine tests ------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/DiagnosticIDs.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace clang;
 
 namespace {
 
for (unsigned Suppress = 0; Suppress != 2; ++Suppress) {
     EXPECT_TRUE(Diags.hasUnrecoverableErrorOccurred());
 
     // The warning should be emitted and counted only if we're not suppressing
     // after fatal errors.
     EXPECT_EQ(Diags.getNumWarnings(), Suppress ? 0u : 1u);
   }
 }
 
+TEST(DiagnosticTest, diagnosticError) {
+  DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
+                          new IgnoringDiagConsumer());
+  PartialDiagnostic::StorageAllocator Alloc;
+  llvm::Expected<std::pair<int, int>> Value = DiagnosticError::create(
+      SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
+                            << "file"
+                            << "error");
+  ASSERT_TRUE(!Value);
+  llvm::Error Err = Value.takeError();
+  Optional<PartialDiagnosticAt> ErrDiag = DiagnosticError::take(Err);
+  llvm::cantFail(std::move(Err));
+  ASSERT_FALSE(!ErrDiag);
+  EXPECT_EQ(ErrDiag->first, SourceLocation());
+  EXPECT_EQ(ErrDiag->second.getDiagID(), diag::err_cannot_open_file);
+
+  Value = std::make_pair(20, 1);
+  ASSERT_FALSE(!Value);
+  EXPECT_EQ(*Value, std::make_pair(20, 1));
+  EXPECT_EQ(Value->first, 20);
+}
 }