Create safeGet and safeSet for indexable types

CHANGELOG.md

@@ -4,10 +4,12 @@
 
 ### Added (new features/APIs/variables/...)
 - [[PR556]](https://github.com/lanl/singularity-eos/pull/556) Add introspection into types available in the variant
+- [[PR564]](https://github.com/lanl/singularity-eos/pull/564) Removed Get() function from IndexableTypes since it could have unexpected consequences when a type wasn't present
 
 ### Fixed (Repair bugs, etc)
 - [[PR561]](https://github.com/lanl/singularity-eos/pull/561) Fix logic for kokkos-kernels in spackage so that it is only required for closure models on GPU
 - [[PR563]](https://github.com/lanl/singularity-eos/pull/563) Fixed DensityFromPressureTemperature for the Carnahan-Starling EOS.
+- [[PR564]](https://github.com/lanl/singularity-eos/pull/564) Fix logic for numerical vs type indices by adding safeGet(), safeSet(), safeMustGet(), and safeMustSet() helpers
 
 ### Changed (changing behavior/API/variables/...)
 

doc/sphinx/src/using-eos.rst

@@ -699,6 +699,8 @@ of the ``[]`` operator that takes your type. For example:
 
   class MyLambda_t {
   public:
+    // Enable recognition that this is type-indexable
+    constexpr static bool is_type_indexable = true;
     MyLambda_t() = default;
     PORTABLE_FORCEINLINE_FUNCTION
     Real &operator[](const std::size_t idx) const {
@@ -724,17 +726,92 @@ which might be used as
 where ``MeanIonizationState`` is shorthand for index 2, since you
 defined that overload. Note that the ``operator[]`` must be marked
 ``const``. To more easily enable mixing and matching integer-based
-indexing with type-based indexing, the function
+indexing with type-based indexing, the functions
 
 .. code-block:: cpp
 
-  template<typename Name_t, typename Indexer_t>
+  template <typename T, typename Indexer_t>
   PORTABLE_FORCEINLINE_FUNCTION
-  Real &Get(Indexer_t &&lambda, std::size_t idx = 0);
+  bool SafeGet(Indexer_t const &lambda, std::size_t const idx, Real &out);
 
-will return a reference to the value at named index ``Name_t()`` if
-that overload is defined in ``Indexer_t`` and otherwise return a
-reference at index ``idx``.
+.. code-block:: cpp
+
+  template <typename T, typename Indexer_t>
+  PORTABLE_FORCEINLINE_FUNCTION
+  bool SafeGet(Indexer_t const &lambda, Real &out);
+
+.. code-block:: cpp
+
+  template <typename T, typename Indexer_t>
+  PORTABLE_FORCEINLINE_FUNCTION
+  Real SafeMustGet(Indexer_t const &lambda, std::size_t const idx)
+
+.. code-block:: cpp
+
+  template <typename T, typename Indexer_t>
+  PORTABLE_FORCEINLINE_FUNCTION
+  Real SafeMustGet(Indexer_t const &lambda)
+
+will update the value of ``out`` with the value at either the appropriate
+type-based index, ``T``, or the numerical index, ``idx``, if the ``Indexer_t``
+doesn't accept type-based indexing. If the ``Indexer_t`` **does** accept
+type-based indexing but **doesn't** have the requested index, then the
+``out`` value is not updated. The same is true for when ``Indexer_t`` is the
+``nullptr``. The overload that doesn't take a numerical index will *only*
+return the value at a type-based index.
+
+The ``SafeMustGet()`` version is intended to generate errors if a value can't be
+retrieved with four types of errors that can occur:
+
+1. If a null pointer is passed as the indexer, a runtime abort or exception will
+   occur
+2. If a type-based indexer is passed (i.e. one with the ``constexpr static bool``
+   member ``is_type_indexable = true``), but the type doesn't exist then a **static**
+   assertion will fail
+3. If one of the overloads is used where a ``std::size_t`` index is provided, but
+   the indexer can't accept integer indexing, then a **static** assertion will fail
+4. If the indexer can't use type-based indexing but an ``std::size_t`` index
+   wasn't provided, then a **static** assertion will fail
+
+Similarly, the functions
+
+.. code-block:: cpp
+
+  template <typename T, typename Indexer_t>
+  PORTABLE_FORCEINLINE_FUNCTION
+  inline bool SafeSet(Indexer_t &lambda, std::size_t const idx, Real const in);
+
+.. code-block:: cpp
+
+  template <typename T, typename Indexer_t>
+  PORTABLE_FORCEINLINE_FUNCTION
+  inline bool SafeSet(Indexer_t &lambda, Real const in)
+
+.. code-block:: cpp
+
+  template <typename T, typename Indexer_t>
+  PORTABLE_FORCEINLINE_FUNCTION
+  inline bool SafeMustSet(Indexer_t &lambda, std::size_t const idx, Real const in);
+
+.. code-block:: cpp
+
+  template <typename T, typename Indexer_t>
+  PORTABLE_FORCEINLINE_FUNCTION
+  inline bool SafeMustSet(Indexer_t &lambda, Real const in)
+
+can modify the values in the ``Indexer_t`` and behave the same way. In this way,
+if a type-based index isn't present in the container, then a different index won't
+be overwritten. Again, the ``SafeMustSet()`` version will compile-time fail
+or runtime abort if the lambda value can't be modified for the same reasons as
+``SafeMustGet()``.
+
+.. note::
+
+  Previous versions defined a ``Get()`` function that was "unsafe" in the
+  sense that it would fall back on the numerical index even if a type-based
+  indexer was used. This could result in retrieving and overwriting incorrect
+  values in the indexer. We recommend not using this function and instead using
+  the "safe" versions.
 
 As a convenience tool, the struct
 

singularity-eos/base/indexable_types.hpp

@@ -20,45 +20,231 @@
 #include <utility>
 
 #include <ports-of-call/portability.hpp>
+#include <ports-of-call/portable_errors.hpp>
 #include <singularity-eos/base/variadic_utils.hpp>
 
 namespace singularity {
 namespace IndexerUtils {
-// Convenience function for accessing an indexer by either type or
-// natural number index depending on what is available
-template <typename T, typename Indexer_t>
-PORTABLE_FORCEINLINE_FUNCTION auto &Get(Indexer_t &&lambda, std::size_t idx = 0) {
+
+// Identifies an indexer as a type-based indexer
+template <class, class = void>
+struct is_type_indexer : std::false_type {};
+template <class Indexer_t>
+struct is_type_indexer<Indexer_t,
+                       std::void_t<decltype(std::decay_t<Indexer_t>::is_type_indexable)>>
+    : std::bool_constant<std::decay_t<Indexer_t>::is_type_indexable> {};
+template <class Indexer_t>
+constexpr bool is_type_indexer_v = is_type_indexer<Indexer_t>::value;
+
+namespace impl {
+
+// Simple way to switch between pure type indexing or also allowing intergers
+enum class AllowedIndexing { Numeric, TypeOnly };
+
+// The "safe" version of Get(). This function will ONLY return a value IF that
+// type-based index is present in the Indexer OR if the Indexer doesn't support
+// type-based indexing.
+template <AllowedIndexing AI, typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION bool SafeGet(Indexer_t const &lambda, std::size_t const idx,
+                                           Real &out) {
+  // If null then nothing happens
+  if (variadic_utils::is_nullptr(lambda)) {
+    return false;
+  }
+
+  // Return value if type index is available
   if constexpr (variadic_utils::is_indexable_v<Indexer_t, T>) {
-    return lambda[T()];
+    out = lambda[T{}];
+    return true;
+  }
+
+  // Do nothing if lambda has type indexing BUT doesn't have this type index
+  if constexpr (is_type_indexer_v<Indexer_t>) {
+    return false;
+  }
+
+  // Fall back to numeric indexing if allowed
+  if constexpr (AI == AllowedIndexing::Numeric) {
+    if constexpr (variadic_utils::has_int_index_v<Indexer_t>) {
+      out = lambda[idx];
+      return true;
+    }
+  }
+
+  // Something else...
+  return false;
+}
+
+// Break out "Set" functionality from "Get". The original "Get()" did both, but
+// the "safe" version needs to separate that functionality for setting the
+// values in a lambda
+template <AllowedIndexing AI, typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION bool SafeSet(Indexer_t &lambda, std::size_t const idx,
+                                           Real const in) {
+  // If null then nothing happens
+  if (variadic_utils::is_nullptr(lambda)) {
+    return false;
+  }
+
+  // Return value if type index is available
+  if constexpr (variadic_utils::is_indexable_v<Indexer_t, T>) {
+    lambda[T{}] = in;
+    return true;
+  }
+
+  // Do nothing if lambda has type indexing BUT doesn't have this type index
+  if constexpr (is_type_indexer_v<Indexer_t>) {
+    return false;
+  }
+
+  // Fall back to numeric indexing if allowed
+  if constexpr (AI == AllowedIndexing::Numeric) {
+    if constexpr (variadic_utils::has_int_index_v<Indexer_t>) {
+      lambda[idx] = in;
+      return true;
+    }
+  }
+
+  // Something else...
+  return false;
+}
+
+// Same as above but causes an error condition (static or dynamic) if the value
+// can't be obtained. Note that the `decltype(auto)` is intended to preserve the
+// value category of the square bracket operator of the `Indexer_t` type. This
+// allows references to be returned since there is also no possibility of the
+// call doing nothing (i.e. like SafeGet and SafeSet), and thus it can be used
+// for either setting or getting values. This should also allow for `const`
+// correctness downstream in the wrappers where `lambda` is `const`
+template <AllowedIndexing AI, typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION decltype(auto) SafeMustGetSet(Indexer_t &&lambda,
+                                                            std::size_t const idx) {
+  // Error on null pointer
+  PORTABLE_ALWAYS_REQUIRE(!variadic_utils::is_nullptr(lambda),
+                          "Indexer can't be nullptr");
+
+  if constexpr (is_type_indexer_v<Indexer_t>) {
+    // Return type-based index. Static assert that type MUST exist in indexer
+    static_assert(variadic_utils::is_indexable_v<Indexer_t, T>);
+    // Use std::forward to maintain value category for lambda, and use
+    // parentheses to do the same for the output of the lambda[] operation
+    return (std::forward<Indexer_t>(lambda)[T{}]);
+  } else if constexpr (AI == AllowedIndexing::Numeric) {
+    // Fall back to numerical indexing if allowed
+    static_assert(variadic_utils::has_int_index_v<Indexer_t>);
+    // Use std::forward to maintain value category for lambda, and use
+    // parentheses to do the same for the output of the lambda[] operation
+    return (std::forward<Indexer_t>(lambda)[idx]);
   } else {
-    return lambda[idx];
+    // Something else that can't be compiled...
+    static_assert(variadic_utils::dependent_false_v<Indexer_t>,
+                  "Indexer must either be designated as type-based through a "
+                  "`is_type_indexable` boolean data member or SafeGet/SafeSet function "
+                  "must be called with a numerical index");
   }
 }
 
+} // namespace impl
+
+// Overload when numerical index is provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION bool SafeGet(Indexer_t const &lambda, std::size_t const idx,
+                                           Real &out) {
+  return impl::SafeGet<impl::AllowedIndexing::Numeric, T>(lambda, idx, out);
+}
+
+// Overload when numerical index isn't provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION bool SafeGet(Indexer_t const &lambda, Real &out) {
+  std::size_t idx = 0;
+  return impl::SafeGet<impl::AllowedIndexing::TypeOnly, T>(lambda, idx, out);
+}
+
+// Overload when numerical index is provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION bool SafeSet(Indexer_t &lambda, std::size_t const idx,
+                                           Real const in) {
+  return impl::SafeSet<impl::AllowedIndexing::Numeric, T>(lambda, idx, in);
+}
+
+// Overload when numerical index isn't provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION bool SafeSet(Indexer_t &lambda, Real const in) {
+  std::size_t idx = 0;
+  return impl::SafeSet<impl::AllowedIndexing::TypeOnly, T>(lambda, idx, in);
+}
+
+// Overload when numerical index is provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION Real SafeMustGet(Indexer_t const &lambda,
+                                               std::size_t const idx) {
+  return impl::SafeMustGetSet<impl::AllowedIndexing::Numeric, T>(lambda, idx);
+}
+
+// Overload when numerical index isn't provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION Real SafeMustGet(Indexer_t const &lambda) {
+  std::size_t idx = 0;
+  return impl::SafeMustGetSet<impl::AllowedIndexing::TypeOnly, T>(lambda, idx);
+}
+
+// Overload when numerical index is provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION void SafeMustSet(Indexer_t &lambda, std::size_t const idx,
+                                               Real const in) {
+  impl::SafeMustGetSet<impl::AllowedIndexing::Numeric, T>(lambda, idx) = in;
+}
+
+// Overload when numerical index isn't provided
+template <typename T, typename Indexer_t>
+PORTABLE_FORCEINLINE_FUNCTION void SafeMustSet(Indexer_t &lambda, Real const in) {
+  std::size_t idx = 0;
+  impl::SafeMustGetSet<impl::AllowedIndexing::TypeOnly, T>(lambda, idx) = in;
+}
+
 // This is a convenience struct to easily build a small indexer with
 // a set of indexable types.
 template <typename Data_t, typename... Ts>
 class VariadicIndexerBase {
  public:
+  // Any class that wants to be recognized as indexable (so that we don't
+  // accidentally fall back to integer indexing when we don't want to) needs to
+  // include this.
+  constexpr static bool is_type_indexable = true;
+
+  // JHP: another option for the `is_type_indexable` flag is to take the ADL
+  // route. Essentially this would involve defining a friend function that
+  // could be defined in an appropriate namesapce so that theoretically a host
+  // code could use a TPL container with type-based indexing and allow that
+  // container to be flagged in our code as acceptable. This seems like a bit
+  // of a heavy hammer for what we need here though. We can easily change this
+  // if a TPL provides a type that is being used for this purpose.
+
   VariadicIndexerBase() = default;
+
   PORTABLE_FORCEINLINE_FUNCTION
   VariadicIndexerBase(const Data_t &data) : data_(data) {}
+
   template <typename T,
             typename = std::enable_if_t<variadic_utils::contains<T, Ts...>::value>>
   PORTABLE_FORCEINLINE_FUNCTION Real &operator[](const T &t) {
     constexpr std::size_t idx = variadic_utils::GetIndexInTL<T, Ts...>();
     return data_[idx];
   }
+
   PORTABLE_FORCEINLINE_FUNCTION
   Real &operator[](const std::size_t idx) { return data_[idx]; }
+
   template <typename T,
             typename = std::enable_if_t<variadic_utils::contains<T, Ts...>::value>>
   PORTABLE_FORCEINLINE_FUNCTION const Real &operator[](const T &t) const {
     constexpr std::size_t idx = variadic_utils::GetIndexInTL<T, Ts...>();
     return data_[idx];
   }
+
   PORTABLE_FORCEINLINE_FUNCTION
   const Real &operator[](const std::size_t idx) const { return data_[idx]; }
+
   static inline constexpr std::size_t size() { return sizeof...(Ts); }
 
  private:
@@ -70,6 +256,7 @@ using VariadicIndexer = VariadicIndexerBase<std::array<Real, sizeof...(Ts)>, Ts.
 // uses a Real*
 template <typename... Ts>
 using VariadicPointerIndexer = VariadicIndexerBase<Real *, Ts...>;
+
 } // namespace IndexerUtils
 
 namespace IndexableTypes {

singularity-eos/base/variadic_utils.hpp

@@ -24,6 +24,16 @@ namespace variadic_utils {
 // Some generic variatic utilities
 // ======================================================================
 
+// Template parameter dependent boolean suitable for causing static_assert
+// errors within `if constexpr` branches. Essentially the issue is that if the
+// static_assert _always_ evaluates to false, then it will _always_ cause a
+// compile time error even if that branch of the code will never be reached.
+// Making the evaluation (superficially) dependent on the template deduction
+// causes it to be evaluated after the `if constexpr` branching has already been
+// determined. See https://en.cppreference.com/w/cpp/language/if.html#Constexpr_if
+template <class>
+inline constexpr bool dependent_false_v = false;
+
 // Useful for generating nullptr of a specific pointer type
 template <typename T>
 inline constexpr T *np() {
@@ -111,6 +121,15 @@ struct is_indexable<T, Index,
 template <typename T, typename Index>
 constexpr bool is_indexable_v = is_indexable<T, Index>::value;
 
+// Check if a type can accept an int index
+template <class T, class = void>
+struct has_int_index : std::false_type {};
+template <class T>
+struct has_int_index<T, std::void_t<decltype(std::declval<T>()[std::declval<int>()])>>
+    : std::true_type {};
+template <typename T>
+constexpr bool has_int_index_v = has_int_index<T>::value;
+
 // this flattens a typelist of typelists to a single typelist
 
 // first parameter - accumulator