Harmonize the various diagnostics.

This patch tries to follow the clang diagnostic style. Lexically we do not start
with capital letter and do not end with punctuation. We provide some guarantees
with asserts for that.

In addition, we try to position the diagnostics in a better place by adding the
source location range so that we get the carret diagnostic pointing to the right
code.

Author: vgvassilev

include/clad/Differentiator/BaseForwardModeVisitor.h

@@ -95,14 +95,6 @@ class BaseForwardModeVisitor
   DifferentiateVarDecl(const clang::VarDecl* VD);
   virtual DeclDiff<clang::VarDecl>
   DifferentiateVarDecl(const clang::VarDecl* VD, bool ignoreInit);
-  /// Shorthand for warning on differentiation of unsupported operators
-  void unsupportedOpWarn(clang::SourceLocation loc,
-                         llvm::ArrayRef<llvm::StringRef> args = {}) {
-    diag(clang::DiagnosticsEngine::Warning, loc,
-         "attempt to differentiate unsupported operator,  derivative \
-                         set to 0",
-         args);
-  }
   StmtDiff VisitCXXForRangeStmt(const clang::CXXForRangeStmt* FRS);
   StmtDiff VisitWhileStmt(const clang::WhileStmt* WS);
   StmtDiff VisitDoStmt(const clang::DoStmt* DS);

include/clad/Differentiator/CladUtils.h

@@ -13,10 +13,12 @@
 #include "clang/AST/Type.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Sema/Ownership.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/StringRef.h"
 
+#include <cassert>
 #include <string>
 
 namespace clang {
@@ -49,16 +51,18 @@ namespace clad {
                                                      clang::Stmt* initial,
                                                      clang::Stmt* S);
 
-    /// Shorthand to issues a warning or error.
     template <std::size_t N>
-    void EmitDiag(clang::Sema& semaRef,
-              clang::DiagnosticsEngine::Level level, // Warning or Error
-              clang::SourceLocation loc, const char (&format)[N],
-              llvm::ArrayRef<llvm::StringRef> args = {}) {
-      unsigned diagID = semaRef.Diags.getCustomDiagID(level, format);
-      clang::Sema::SemaDiagnosticBuilder stream = semaRef.Diag(loc, diagID);
-      for (auto arg : args)
-        stream << arg;
+    clang::Sema::SemaDiagnosticBuilder
+    diag(clang::Sema& S, clang::DiagnosticsEngine::Level Level,
+         clang::SourceLocation Loc, const char (&Format)[N]) {
+      static_assert(N > 1, "Diagnostic format string must not be empty");
+      assert(!std::isupper(Format[0]) && "Diagnostics start with lower case!");
+      assert((std::isalpha(Format[N - 2]) || Format[N - 2] == ')' ||
+              Format[N - 2] == '\'' || std::isdigit(Format[N - 2])) &&
+             "Diagnostics end with no punctuation!");
+      unsigned DiagID = S.Diags.getCustomDiagID(Level, Format);
+      clang::Sema::SemaDiagnosticBuilder B = S.Diag(Loc, DiagID);
+      return B;
     }
 
     /// Creates nested name specifier associated with declaration context

include/clad/Differentiator/DerivativeBuilder.h

@@ -9,11 +9,13 @@
 
 #include "Compatibility.h"
 
+#include "clad/Differentiator/CladUtils.h"
 #include "clad/Differentiator/DerivedFnCollector.h"
 #include "clad/Differentiator/DiffPlanner.h"
 
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Sema/Sema.h"
 
 #include <array>
@@ -126,14 +128,10 @@ namespace clad {
                           llvm::MutableArrayRef<clang::Expr*> ARargs);
     /// Shorthand to issues a warning or error.
     template <std::size_t N>
-    void diag(clang::DiagnosticsEngine::Level level, // Warning or Error
-              clang::SourceLocation loc,
-              const char (&format)[N],
-              llvm::ArrayRef<llvm::StringRef> args = {}) {
-      unsigned diagID = m_Sema.Diags.getCustomDiagID(level, format);
-      clang::Sema::SemaDiagnosticBuilder stream = m_Sema.Diag(loc, diagID);
-      for (auto arg : args)
-        stream << arg;
+    clang::Sema::SemaDiagnosticBuilder
+    diag(clang::DiagnosticsEngine::Level Level, clang::SourceLocation Loc,
+         const char (&Format)[N]) {
+      return utils::diag(m_Sema, Level, Loc, Format);
     }
 
     /// Lookup the result of finding a custom derivative or numerical

include/clad/Differentiator/ReverseModeVisitor.h

@@ -19,6 +19,8 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Sema/Sema.h"
 
 #include <llvm/ADT/ArrayRef.h>
@@ -504,14 +506,6 @@ namespace clad {
                          const clang::ParmVarDecl* param,
                          llvm::SmallVectorImpl<clang::Stmt*>& PreCallStmts,
                          bool isNonDiff, bool isCUDAKernel = false);
-    /// Shorthand for warning on differentiation of unsupported operators
-    void unsupportedOpWarn(clang::SourceLocation loc,
-                           llvm::ArrayRef<llvm::StringRef> args = {}) {
-      diag(clang::DiagnosticsEngine::Warning,
-           loc,
-           "attempt to differentiate unsupported operator, ignored.",
-           args);
-    }
 
     /// Allows to easily create and manage a counter for counting the number of
     /// executed iterations of a loop.

include/clad/Differentiator/VisitorBase.h

@@ -11,9 +11,11 @@
 #include "DerivativeBuilder.h"
 #include "clad/Differentiator/CladUtils.h"
 
+#include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/AST/Type.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Ownership.h"
@@ -25,6 +27,7 @@
 #include "llvm/Support/PrettyStackTrace.h"
 
 #include <array>
+#include <cassert>
 #include <stack>
 #include <unordered_map>
 
@@ -411,10 +414,39 @@ namespace clad {
     /// A flag for silencing warnings/errors output by diag function.
     /// Shorthand to issues a warning or error.
     template <std::size_t N>
-    void diag(clang::DiagnosticsEngine::Level level, // Warning or Error
-              clang::SourceLocation loc, const char (&format)[N],
-              llvm::ArrayRef<llvm::StringRef> args = {}) {
-      m_Builder.diag(level, loc, format, args);
+    clang::Sema::SemaDiagnosticBuilder
+    diag(clang::DiagnosticsEngine::Level level, clang::SourceLocation loc,
+         const char (&format)[N]) {
+      return m_Builder.diag(level, loc, format);
+    }
+
+    void diagUnsupported(const clang::Decl* D) {
+      clang::SourceLocation L = D->getBeginLoc();
+      diag(clang::DiagnosticsEngine::Warning, L,
+           "declaration kind '%0' is not supported")
+          << D->getDeclKindName() << L;
+    }
+
+    void diagUnsupported(const clang::Stmt* S) {
+      clang::SourceLocation L = S->getBeginLoc();
+      diag(clang::DiagnosticsEngine::Warning, L,
+           "statement kind '%0' is not supported")
+          << S->getStmtClassName() << L;
+    }
+
+    void diagUnsupportedIndirectCalls(const clang::CallExpr* CE) {
+      assert(!CE->getDirectCallee() && "This is a direct callee");
+      clang::SourceLocation L = CE->getBeginLoc();
+      diag(clang::DiagnosticsEngine::Warning, L,
+           "differentiation of indirect calls is not supported")
+          << L;
+    }
+
+    /// Shorthand for warning on differentiation of unsupported operators
+    void unsupportedOpWarn(clang::SourceLocation loc) {
+      diag(clang::DiagnosticsEngine::Warning, loc,
+           "attempted to differentiate unsupported operator; treated as "
+           "non-differentiable");
     }
 
     /// Creates unique identifier of the form "_nameBase<number>" that is

lib/Differentiator/BaseForwardModeVisitor.cpp

@@ -9,6 +9,7 @@
 #include "ConstantFolder.h"
 
 #include "clad/Differentiator/CladUtils.h"
+#include "clad/Differentiator/DerivativeBuilder.h"
 #include "clad/Differentiator/DiffMode.h"
 #include "clad/Differentiator/DiffPlanner.h"
 #include "clad/Differentiator/ErrorEstimator.h"
@@ -86,12 +87,11 @@ DerivativeAndOverload BaseForwardModeVisitor::Derive() {
     // or pointer type, only one of the indices have been requested
     if (DVI.size() > 1 || (isArrayOrPointerType(diffVarInfo.param->getType()) &&
                            (diffVarInfo.paramIndexInterval.size() != 1))) {
-      diag(DiagnosticsEngine::Error,
-           m_DiffReq.Args ? m_DiffReq.Args->getEndLoc() : noLoc,
-           "Forward mode differentiation w.r.t. several parameters at once is "
-           "not "
-           "supported, call 'clad::differentiate' for each parameter "
-           "separately");
+      SourceLocation L = m_DiffReq.Args ? m_DiffReq.Args->getBeginLoc() : noLoc;
+      diag(DiagnosticsEngine::Error, L,
+           "forward mode differentiation w.r.t. several parameters at once is "
+           "not supported; call 'clad::differentiate' for each parameter")
+          << L;
       return {};
     }
 
@@ -113,10 +113,11 @@ DerivativeAndOverload BaseForwardModeVisitor::Derive() {
       if (!m_IndependentVar->getType()
                ->getPointeeOrArrayElementType()
                ->isRealType()) {
-        diag(DiagnosticsEngine::Error, m_IndependentVar->getEndLoc(),
-             "attempted differentiation w.r.t. a parameter ('%0') which is not"
-             " an array or pointer of a real type",
-             {m_IndependentVar->getNameAsString()});
+        SourceLocation L = m_IndependentVar->getBeginLoc();
+        diag(DiagnosticsEngine::Error, L,
+             "attempted differentiation w.r.t. parameter %0 which is not"
+             " array or pointer of real type")
+            << m_IndependentVar << L;
         return {};
       }
       m_IndependentVarIndex = diffVarInfo.paramIndexInterval.Start;
@@ -130,10 +131,11 @@ DerivativeAndOverload BaseForwardModeVisitor::Derive() {
         isField = true;
       }
       if (!IsRealNonReferenceType(T)) {
-        diag(DiagnosticsEngine::Error, m_DiffReq.Args->getEndLoc(),
-             "Attempted differentiation w.r.t. %0 '%1' which is not "
-             "of real type.",
-             {(isField ? "member" : "parameter"), diffVarInfo.source});
+        SourceLocation L = m_DiffReq.Args->getBeginLoc();
+        diag(DiagnosticsEngine::Error, L,
+             "attempted differentiation w.r.t. %select{member|parameter}0 '%1' "
+             "which is not of real type")
+            << isField << diffVarInfo.source << L;
         return {};
       }
     }
@@ -338,11 +340,11 @@ void BaseForwardModeVisitor::GenerateSeeds(const clang::FunctionDecl* dFD) {
       // Produce an error.
       if (param != m_IndependentVar &&
           !utils::GetValueType(dParamType).isConstQualified()) {
-        // FIXME: Use diagnostics style as in #1596
-        diag(DiagnosticsEngine::Error, param->getLocation(),
+        SourceLocation L = param->getLocation();
+        diag(DiagnosticsEngine::Error, L,
              "dependent non-const pointer and array parameters "
-             "are not supported; differentiate w.r.t. '%0' or mark it const",
-             {param->getNameAsString()});
+             "are not supported; differentiate w.r.t. %0 or mark it const")
+            << param << L;
       }
       continue;
     }
@@ -454,8 +456,7 @@ void BaseForwardModeVisitor::GenerateSeeds(const clang::FunctionDecl* dFD) {
 }
 
 StmtDiff BaseForwardModeVisitor::VisitStmt(const Stmt* S) {
-  diag(DiagnosticsEngine::Warning, S->getBeginLoc(),
-       "attempted to differentiate unsupported statement, no changes applied");
+  diagUnsupported(S);
   // Unknown stmt, just clone it.
   return StmtDiff(Clone(S));
 }
@@ -1012,8 +1013,7 @@ DiffMode BaseForwardModeVisitor::GetPushForwardMode() {
 StmtDiff BaseForwardModeVisitor::VisitCallExpr(const CallExpr* CE) {
   const FunctionDecl* FD = CE->getDirectCallee();
   if (!FD) {
-    diag(DiagnosticsEngine::Warning, CE->getBeginLoc(),
-         "Differentiation of only direct calls is supported. Ignored");
+    diagUnsupportedIndirectCalls(CE);
     return StmtDiff(Clone(CE));
   }
 
@@ -1350,9 +1350,11 @@ BaseForwardModeVisitor::VisitBinaryOperator(const BinaryOperator* BinOp) {
         (Ldiff.getExpr_dx()->isModifiableLvalue(m_Context) !=
          Expr::MLV_Valid) &&
         !isCladArrayType(Ldiff.getExpr_dx()->getType())) {
-      diag(DiagnosticsEngine::Warning, BinOp->getEndLoc(),
+      SourceLocation L = BinOp->getBeginLoc();
+      diag(DiagnosticsEngine::Warning, L,
            "derivative of an assignment attempts to assign to unassignable "
-           "expr, assignment ignored");
+           "expr, assignment ignored")
+          << L;
       opDiff = ConstantFolder::synthesizeLiteral(m_Context.IntTy, m_Context, 0);
     } else if (opCode == BO_Assign || opCode == BO_AddAssign ||
                opCode == BO_SubAssign) {
@@ -1507,11 +1509,9 @@ StmtDiff BaseForwardModeVisitor::VisitDeclStmt(const DeclStmt* DS) {
     if (typeDecl && (clad::utils::hasNonDifferentiableAttribute(typeDecl) ||
                      typeDecl->isLambda())) {
       for (auto* D : DS->decls()) {
-        if (auto* VD = dyn_cast<VarDecl>(D))
-          decls.push_back(VD);
-        else
-          diag(DiagnosticsEngine::Warning, D->getEndLoc(),
-               "Unsupported declaration");
+        assert(isa<VarDecl>(D) && "Mixed decl types in a single decl stmt is "
+                                  "not standard c++ syntax");
+        decls.push_back(cast<VarDecl>(D));
       }
       Stmt* DSClone = BuildDeclStmt(decls);
       return StmtDiff(DSClone, nullptr);
@@ -1556,8 +1556,7 @@ StmtDiff BaseForwardModeVisitor::VisitDeclStmt(const DeclStmt* DS) {
       if (SADDiff.getDecl_dx())
         declsDiff.push_back(SADDiff.getDecl_dx());
     } else {
-      diag(DiagnosticsEngine::Warning, D->getEndLoc(),
-           "Unsupported declaration");
+      diagUnsupported(D);
     }
   }
 
@@ -1984,10 +1983,12 @@ BaseForwardModeVisitor::DeriveSwitchStmtBodyHelper(const Stmt* stmt,
         // We can also solve this issue by creating new scope and compound
         // statement block wherever they are required instead of enclosing all
         // the statements of a case label in a single compound statement.
-        diag(DiagnosticsEngine::Error, containedSC->getBeginLoc(),
-             "Differentiating switch case label contained in a compound "
+        SourceLocation L = containedSC->getBeginLoc();
+        diag(DiagnosticsEngine::Error, L,
+             "differentiating switch case label contained in a compound "
              "statement, other than the switch statement compound "
-             "statement, is not supported.");
+             "statement, is not supported")
+            << L;
         return activeSC;
       }
     }

lib/Differentiator/DerivativeBuilder.cpp

@@ -526,14 +526,12 @@ static void registerDerivative(Decl* D, Sema& S, const DiffRequest& R) {
         // functions or custom derivatives.
         if (!request.DeclarationOnly ||
             !(m_DFC.IsCladDerivative(FD) || m_DFC.IsCustomDerivative(FD))) {
-          if (request.VerboseDiags)
-            diag(DiagnosticsEngine::Error,
-                 request.CallContext ? request.CallContext->getBeginLoc()
-                                     : noLoc,
-                 "attempted differentiation of function '%0', which does not "
-                 "have a "
-                 "definition",
-                 {FD->getNameAsString()});
+          if (request.VerboseDiags) {
+            SourceLocation L = request.CallContext->getBeginLoc();
+            diag(DiagnosticsEngine::Error, L,
+                 "attempted differentiation of function %0, without definition")
+                << FD << L;
+          }
           return {};
         }
       }
@@ -543,11 +541,11 @@ static void registerDerivative(Decl* D, Sema& S, const DiffRequest& R) {
 
       // check if the function is non-differentiable.
       if (clad::utils::hasNonDifferentiableAttribute(FD)) {
-        diag(DiagnosticsEngine::Error,
-             request.CallContext ? request.CallContext->getBeginLoc() : noLoc,
-             "attempted differentiation of function '%0', which is marked as "
-             "non-differentiable",
-             {FD->getNameAsString()});
+        SourceLocation L = request.CallContext->getBeginLoc();
+        diag(DiagnosticsEngine::Error, L,
+             "attempted differentiation of function %0, which is marked as "
+             "non-differentiable")
+            << FD;
         return {};
       }
 
@@ -556,23 +554,21 @@ static void registerDerivative(Decl* D, Sema& S, const DiffRequest& R) {
       if (const CXXMethodDecl* MD = dyn_cast<CXXMethodDecl>(FD)) {
         const CXXRecordDecl* CD = MD->getParent();
         if (clad::utils::hasNonDifferentiableAttribute(CD)) {
-          diag(DiagnosticsEngine::Error, MD->getLocation(),
-               "attempted differentiation of method '%0' in class '%1', which "
-               "is "
-               "marked as "
-               "non-differentiable",
-               {MD->getNameAsString(), CD->getNameAsString()});
+          SourceLocation L = MD->getLocation();
+          diag(DiagnosticsEngine::Error, L,
+               "attempted differentiation of method %0 in class %1, which "
+               "is marked as non-differentiable")
+              << MD << CD << L;
           return {};
         }
       }
     } else if (const VarDecl* VD = request.Global) {
       // Warn the user about the usage of global variables.
-      auto diagId = m_Sema.Diags.getCustomDiagID(
-          DiagnosticsEngine::Warning,
-          "The gradient utilizes a global variable '%0'"
-          ". Please make sure to properly reset '%0' before re-running "
-          "the gradient.");
-      m_Sema.Diag(VD->getLocation(), diagId) << VD->getName();
+      SourceLocation L = VD->getLocation();
+      diag(DiagnosticsEngine::Warning, L,
+           "gradient uses a global variable %0; "
+           "rerunning the gradient requires %0 to be reset")
+          << VD << L;
     }
 
     DerivativeAndOverload result{};