Forbid ambiguous casts to and from ResultT (#8147)

* Forbid ambiguous casts to and from ResultT

* Reformat

* Changed enabled_if checks to check for implicit casts to Result

* Added comments

arangod/Cluster/ResultT.h

@@ -23,6 +23,8 @@
 #ifndef ARANGODB_BASICS_RESULT_T_H
 #define ARANGODB_BASICS_RESULT_T_H
 
+#include <type_traits>
+
 #include <boost/optional.hpp>
 
 #include "Basics/Common.h"
@@ -55,6 +57,7 @@ namespace arangodb {
 // or, having a `Result result` with result.fail() being true,
 //   return result;
 // .
+// Some implicit conversions are disabled when they could cause ambiguity.
 template <typename T>
 class ResultT {
  public:
@@ -74,22 +77,54 @@ class ResultT {
     return ResultT(boost::none, errorNumber, errorMessage);
   }
 
-  // These are not explicit on purpose
+  ResultT static error(Result const& other) {
+    TRI_ASSERT(other.fail());
+    return ResultT(boost::none, other);
+  }
+
+  ResultT static error(Result&& other) {
+    TRI_ASSERT(other.fail());
+    return ResultT(boost::none, std::move(other));
+  }
+
+  // This is disabled if U is implicitly convertible to Result
+  // (e.g., if U = int) to avoid ambiguous construction.
+  // Use ::success() or ::error() instead in that case.
+  template <typename U = T, typename = std::enable_if_t<!std::is_convertible<U, Result>::value>>
+  // This is not explicit on purpose
+  // NOLINTNEXTLINE(google-explicit-constructor)
   ResultT(Result const& other) : _result(other) {
     // .ok() is not allowed here, as _val should be expected to be initialized
     // iff .ok() is true.
     TRI_ASSERT(other.fail());
   }
 
+  // This is disabled if U is implicitly convertible to Result
+  // (e.g., if U = int) to avoid ambiguous construction.
+  // Use ::success() or ::error() instead in that case.
+  template <typename U = T, typename = std::enable_if_t<!std::is_convertible<U, Result>::value>>
+  // This is not explicit on purpose
+  // NOLINTNEXTLINE(google-explicit-constructor)
   ResultT(Result&& other) : _result(std::move(other)) {
     // .ok() is not allowed here, as _val should be expected to be initialized
     // iff .ok() is true.
     TRI_ASSERT(other.fail());
   }
 
-  // These are not explicit on purpose
+  // This is disabled if U is implicitly convertible to Result
+  // (e.g., if U = int) to avoid ambiguous construction.
+  // Use ::success() or ::error() instead in that case.
+  template <typename U = T, typename = std::enable_if_t<!std::is_convertible<U, Result>::value>>
+  // This is not explicit on purpose
+  // NOLINTNEXTLINE(google-explicit-constructor)
   ResultT(T&& val) : ResultT(std::move(val), TRI_ERROR_NO_ERROR) {}
 
+  // This is disabled if U is implicitly convertible to Result
+  // (e.g., if U = int) to avoid ambiguous construction.
+  // Use ::success() or ::error() instead in that case.
+  template <typename U = T, typename = std::enable_if_t<!std::is_convertible<U, Result>::value>>
+  // This is not explicit on purpose
+  // NOLINTNEXTLINE(google-explicit-constructor)
   ResultT(T const& val) : ResultT(val, TRI_ERROR_NO_ERROR) {}
 
   ResultT() = delete;
@@ -104,8 +139,6 @@ class ResultT {
     return *this;
   }
 
-  Result copy_result() const { return *this; }
-
   // These would be very convenient, but also make it very easy to accidentally
   // use the value of an error-result. So don't add them.
   //
@@ -124,7 +157,17 @@ class ResultT {
 
   T const&& operator*() const&& { return get(); }
 
-  explicit operator bool() const { return ok(); }
+  // Allow convenient check to allow for code like
+  //   if (res) { /* use res.get() */ } else { /* handle error res.result() */ }
+  // . Is disabled for bools to avoid accidental confusion of
+  //   if (res)
+  // with
+  //   if (res.get())
+  // .
+  template <typename U = T, typename = std::enable_if_t<!std::is_same<U, bool>::value>>
+  explicit operator bool() const {
+    return ok();
+  }
 
   T const& get() const { return _val.get(); }
 
@@ -163,7 +206,7 @@ class ResultT {
   // forwarded methods
   bool ok() const { return _result.ok(); }
   bool fail() const { return _result.fail(); }
-  bool is(uint64_t code) { return _result.is(code); }
+  bool is(int code) { return _result.is(code); }
   int errorNumber() const { return _result.errorNumber(); }
   std::string errorMessage() const { return _result.errorMessage(); }
 
@@ -186,6 +229,12 @@ class ResultT {
 
   ResultT(boost::optional<T> const& val_, int errorNumber, std::string const& errorMessage)
       : _result(errorNumber, errorMessage), _val(val_) {}
+
+  ResultT(boost::optional<T>&& val_, Result result)
+      : _result(std::move(result)), _val(std::move(val_)) {}
+
+  ResultT(boost::optional<T>&& val_, Result&& result)
+      : _result(std::move(result)), _val(std::move(val_)) {}
 };
 
 }  // namespace arangodb

arangod/RestHandler/RestRepairHandler.cpp

@@ -290,7 +290,7 @@ ResultT<bool> RestRepairHandler::jobFinished(std::string const& jobId) {
         break;
 
       case JobStatus::finished:
-        return true;
+        return ResultT<bool>::success(true);
         break;
 
       case JobStatus::failed:
@@ -298,14 +298,14 @@ ResultT<bool> RestRepairHandler::jobFinished(std::string const& jobId) {
             << "RestRepairHandler::jobFinished: "
             << "Job " << jobId << " failed, aborting";
 
-        return Result(TRI_ERROR_CLUSTER_REPAIRS_JOB_FAILED);
+        return ResultT<bool>::error(TRI_ERROR_CLUSTER_REPAIRS_JOB_FAILED);
 
       case JobStatus::missing:
         LOG_TOPIC(ERR, arangodb::Logger::CLUSTER)
             << "RestRepairHandler::jobFinished: "
             << "Job " << jobId << " went missing, aborting";
 
-        return Result(TRI_ERROR_CLUSTER_REPAIRS_JOB_DISAPPEARED);
+        return ResultT<bool>::error(TRI_ERROR_CLUSTER_REPAIRS_JOB_DISAPPEARED);
     }
 
   } else {
@@ -314,10 +314,10 @@ ResultT<bool> RestRepairHandler::jobFinished(std::string const& jobId) {
         << "Failed to get job status: "
         << "[" << jobStatus.errorNumber() << "] " << jobStatus.errorMessage();
 
-    return std::move(std::move(jobStatus).result());
+    return ResultT<bool>::error(std::move(jobStatus.result()));
   }
 
-  return false;
+  return ResultT<bool>::success(false);
 }
 
 Result RestRepairHandler::executeRepairOperations(DatabaseID const& databaseId,
@@ -636,7 +636,7 @@ ResultT<bool> RestRepairHandler::checkReplicationFactor(DatabaseID const& databa
         << "No ClusterInfo instance";
     generateError(rest::ResponseCode::SERVER_ERROR, TRI_ERROR_HTTP_SERVER_ERROR);
 
-    return Result(TRI_ERROR_INTERNAL);
+    return ResultT<bool>::error(TRI_ERROR_INTERNAL);
   }
 
   clusterInfo->loadPlan();
@@ -656,11 +656,11 @@ ResultT<bool> RestRepairHandler::checkReplicationFactor(DatabaseID const& databa
           << "replicationFactor is " << collection->replicationFactor()
           << ", but the shard has " << dbServers.size() << " DBServers.";
 
-      return false;
+      return ResultT<bool>::success(false);
     }
   }
 
-  return true;
+  return ResultT<bool>::success(true);
 }
 
 void RestRepairHandler::generateResult(rest::ResponseCode code,