From 3a2cef0372ea71a0759d25e4a6d3b4b965a192ce Mon Sep 17 00:00:00 2001
From: Jake Hillion <jakehillion@meta.com>
Date: Thu, 3 Aug 2023 04:07:41 -0700
Subject: [PATCH] features: centralise requirement logic

This has room for conflicting features in future which will be used with
the OIL implementation. For now it makes it much easier to add
requirements.
---
 oi/Features.cpp | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 oi/Features.h   | 11 +++++++----
 oi/OIUtils.cpp  | 35 ++++++-----------------------------
 3 files changed, 61 insertions(+), 33 deletions(-)

diff --git a/oi/Features.cpp b/oi/Features.cpp
index 6ea17f1..705ab00 100644
--- a/oi/Features.cpp
+++ b/oi/Features.cpp
@@ -15,8 +15,12 @@
  */
 #include "Features.h"
 
+#include <glog/logging.h>
+
+#include <cassert>
 #include <map>
 #include <numeric>
+#include <span>
 #include <stdexcept>
 
 namespace oi::detail {
@@ -57,6 +61,22 @@ std::string_view featureHelp(Feature f) {
   }
 }
 
+std::span<const Feature> requirements(Feature f) {
+  switch (f) {
+    case Feature::TypedDataSegment:
+      static constexpr std::array tds = {Feature::TypeGraph};
+      return tds;
+    case Feature::TreeBuilderTypeChecking:
+      static constexpr std::array tc = {Feature::TypedDataSegment};
+      return tc;
+    case Feature::TreeBuilderV2:
+      static constexpr std::array tb2 = {Feature::TreeBuilderTypeChecking};
+      return tb2;
+    default:
+      return {};
+  }
+}
+
 }  // namespace
 
 Feature featureFromStr(std::string_view str) {
@@ -101,4 +121,32 @@ void featuresHelp(std::ostream& out) {
   }
 }
 
+std::optional<FeatureSet> handleFeatureConflicts(FeatureSet enabled,
+                                                 const FeatureSet& disabled) {
+  FeatureSet toCheck = enabled;
+  while (toCheck.any()) {
+    for (const auto f : allFeatures) {
+      if (!toCheck[f])
+        continue;
+      toCheck[f] = false;
+
+      for (const auto r : requirements(f)) {
+        if (enabled[r])
+          continue;
+        if (disabled[r]) {
+          LOG(ERROR) << featureToStr(f) << " feature requires "
+                     << featureToStr(r) << "but it was explicitly disabled!";
+          return std::nullopt;
+        }
+        enabled[r] = true;
+        toCheck[r] = true;
+        LOG(WARNING) << featureToStr(f) << " feature requires "
+                     << featureToStr(r) << ", enabling it now.";
+      }
+    }
+  }
+
+  return enabled;
+}
+
 }  // namespace oi::detail
diff --git a/oi/Features.h b/oi/Features.h
index adfddeb..911f0a9 100644
--- a/oi/Features.h
+++ b/oi/Features.h
@@ -16,6 +16,7 @@
 #pragma once
 
 #include <array>
+#include <optional>
 #include <ostream>
 #include <string_view>
 
@@ -45,10 +46,6 @@ enum class Feature {
 #undef X
 };
 
-Feature featureFromStr(std::string_view);
-const char* featureToStr(Feature);
-void featuresHelp(std::ostream& out);
-
 constexpr std::array allFeatures = {
 #define X(name, _) Feature::name,
     OI_FEATURE_LIST
@@ -58,4 +55,10 @@ constexpr std::array allFeatures = {
 // Use "size+1" to account for UnknownFeature"
 using FeatureSet = EnumBitset<Feature, allFeatures.size() + 1>;
 
+Feature featureFromStr(std::string_view);
+const char* featureToStr(Feature);
+void featuresHelp(std::ostream& out);
+std::optional<FeatureSet> handleFeatureConflicts(FeatureSet enabled,
+                                                 const FeatureSet& disabled);
+
 }  // namespace oi::detail
diff --git a/oi/OIUtils.cpp b/oi/OIUtils.cpp
index 30346c4..b4e5f4c 100644
--- a/oi/OIUtils.cpp
+++ b/oi/OIUtils.cpp
@@ -172,39 +172,16 @@ std::optional<FeatureSet> processConfigFile(
     }
   }
 
-  FeatureSet featuresSet;
+  FeatureSet enabledFeatures;
+  FeatureSet disabledFeatures;
   for (auto [k, v] : featureMap) {
     if (v) {
-      featuresSet[k] = true;
+      enabledFeatures[k] = true;
+    } else {
+      disabledFeatures[k] = true;
     }
   }
-
-  if (featuresSet[Feature::TreeBuilderTypeChecking] &&
-      !featuresSet[Feature::TypedDataSegment]) {
-    if (auto search = featureMap.find(Feature::TypedDataSegment);
-        search != featureMap.end() && !search->second) {
-      LOG(ERROR) << "TreeBuilderTypeChecking feature requires TypedDataSegment "
-                    "feature to be enabled but it was explicitly disabled!";
-      return {};
-    }
-    featuresSet[Feature::TypedDataSegment] = true;
-    LOG(WARNING) << "TreeBuilderTypeChecking feature requires TypedDataSegment "
-                    "feature to be enabled, enabling now.";
-  }
-  if (featuresSet[Feature::TypedDataSegment] &&
-      !featuresSet[Feature::TypeGraph]) {
-    if (auto search = featureMap.find(Feature::TypeGraph);
-        search != featureMap.end() && !search->second) {
-      LOG(ERROR) << "TypedDataSegment feature requires TypeGraph feature to be "
-                    "enabled but it was explicitly disabled!";
-      return {};
-    }
-    featuresSet[Feature::TypeGraph] = true;
-    LOG(WARNING) << "TypedDataSegment feature requires TypeGraph feature to be "
-                    "enabled, enabling now.";
-  }
-
-  return featuresSet;
+  return handleFeatureConflicts(enabledFeatures, disabledFeatures);
 }
 
 }  // namespace oi::detail::utils