Clean up native_object API

Summary:
Currently the `native_object` API can be "map-like" or "array-like" but for some reason makes "map-like" the default. All `native_object` implementations must implement `lookup_property` even if it only supports "array-like" iteration.

This diff changes it so that both "map-like" and "array-like" follow the same interface structure. See the doc blocks in/around`native_object` for more details.

Reviewed By: vitaut

Differential Revision: D67049574

fbshipit-source-id: 6ef8cc860229b50e2b08d17e65261ae8992aadbf

third-party/thrift/src/thrift/compiler/whisker/eval_context.cc

@@ -38,7 +38,10 @@ const object* find_property(const object& o, std::string_view identifier) {
       [](boolean) -> result { return nullptr; },
       [](const array&) -> result { return nullptr; },
       [identifier](const native_object::ptr& o) -> result {
-        return o->lookup_property(identifier);
+        if (auto map_like = o->as_map_like()) {
+          return map_like->lookup_property(identifier);
+        }
+        return nullptr;
       },
       [identifier](const map& m) -> result {
         if (auto it = m.find(identifier); it != m.end()) {
@@ -55,11 +58,18 @@ const object* find_property(const object& o, std::string_view identifier) {
  * This could be a w::map but for debugging purposes, a native_object with a
  * custom print_to function is beneficial.
  */
-class global_scope_object : public native_object {
+class global_scope_object
+    : public native_object,
+      public native_object::map_like,
+      public std::enable_shared_from_this<global_scope_object> {
  public:
   explicit global_scope_object(map properties)
       : properties_(std::move(properties)) {}
 
+  std::shared_ptr<const native_object::map_like> as_map_like() const override {
+    return shared_from_this();
+  }
+
   const object* lookup_property(std::string_view identifier) const override {
     if (auto property = properties_.find(identifier);
         property != properties_.end()) {

third-party/thrift/src/thrift/compiler/whisker/mstch_compat.cc

@@ -35,19 +35,15 @@ namespace {
  */
 class mstch_array_proxy final
     : public native_object,
-      public native_object::sequence,
+      public native_object::array_like,
       public std::enable_shared_from_this<mstch_array_proxy> {
  public:
   explicit mstch_array_proxy(mstch_array&& array)
       : proxied_(std::move(array)) {}
 
  private:
-  const object* lookup_property(std::string_view) const override {
-    // Arrays have no named properties.
-    return nullptr;
-  }
-
-  std::shared_ptr<const native_object::sequence> as_sequence() const override {
+  std::shared_ptr<const native_object::array_like> as_array_like()
+      const override {
     return shared_from_this();
   }
 
@@ -102,11 +98,18 @@ class mstch_array_proxy final
  *
  * Properties are lazily marshaled to whisker::object on first access by name.
  */
-class mstch_map_proxy final : public native_object {
+class mstch_map_proxy final
+    : public native_object,
+      public native_object::map_like,
+      public std::enable_shared_from_this<mstch_map_proxy> {
  public:
   explicit mstch_map_proxy(mstch_map&& map) : proxied_(std::move(map)) {}
 
  private:
+  std::shared_ptr<const native_object::map_like> as_map_like() const override {
+    return shared_from_this();
+  }
+
   const object* lookup_property(std::string_view id) const override {
     if (auto cached = converted_.find(id); cached != converted_.end()) {
       return &cached->second;
@@ -165,11 +168,18 @@ class mstch_map_proxy final : public native_object {
  * Property lookups are NOT cached as the underlying property on the
  * mstch::object may be volatile.
  */
-class mstch_object_proxy : public native_object {
+class mstch_object_proxy
+    : public native_object,
+      public native_object::map_like,
+      public std::enable_shared_from_this<mstch_object_proxy> {
  public:
   explicit mstch_object_proxy(std::shared_ptr<mstch_object>&& obj)
       : proxied_(std::move(obj)) {}
 
+  std::shared_ptr<const native_object::map_like> as_map_like() const override {
+    return shared_from_this();
+  }
+
   const object* lookup_property(std::string_view id) const override {
     // mstch does not support heterogenous lookups, so we need a temporary
     // std::string.

third-party/thrift/src/thrift/compiler/whisker/object.h

@@ -24,7 +24,6 @@
 #include <limits>
 #include <map>
 #include <memory>
-#include <optional>
 #include <string>
 #include <string_view>
 #include <utility>
@@ -107,67 +106,87 @@ struct object_print_options {
 
 /**
  * A native_object is the most powerful type in Whisker. Its properties and
- * behavior are fully defined by user C++ code.
+ * behavior are defined by highly customizable C++ code.
  *
- * A native_object implementation may perform arbitrary computation in C++ to
- * resolve a property name as long as the produced result can be marshaled back
- * to a whisker::object type (including possibly another native_object!).
+ * A native_object can work as a "map":
+ *   as_map_like() can be implemented to resolve a property names using
+ *   arbitrary C++ code, which Whisker's renderer will perform lookups on.
  *
- * Whisker also does not require that all property names exposed by a
- * native_object are statically (or finitely) enumerable. This allows a few
- * freedoms for implementers:
- *   - Properties can be lazily computed at lookup time.
- *   - Property lookup can have side-effects (not recommended, but possible).
+ * A native_object can work as an "array":
+ *   as_array_like() can be implemented to return a sequence of objects, which
+ *   Whisker's renderer will iterate over like an array.
+ *
+ * A native_object can work as both "map" and "array" at the same time!
  */
 class native_object {
  public:
   using ptr = std::shared_ptr<native_object>;
   virtual ~native_object() = default;
 
   /**
-   * Searches for a property on the object referred to by the provided
-   * identifier, returning a non-empty optional if present, or std::nullopt
-   * otherwise.
-   *
-   * The returned object is by value because it may outlive this native_object
-   * instance. For most whisker::object types, this is fine because they are
-   * self-contained.
-   * If this property lookup returns a native_object, `foo`, and that object
-   * depends on `this`, then `foo` should keep a shared_ptr` to `this` to ensure
-   * that `this` outlives `foo`. The Whisker runtime does not provide any
-   * lifetime guarantees for `this`.
+   * A class that allows "map-like" named property access over an underlying
+   * C++ object.
    *
-   * Preconditions:
-   *   - The provided string is a valid Whisker identifier
+   * This interface is strictly more capable than the built-in map. For example:
+   *   - Property names do not need to be finitely enumerable.
+   *   - Property values can be lazily computed at lookup time.
    */
-  virtual const object* lookup_property(std::string_view identifier) const = 0;
-
+  class map_like {
+   public:
+    virtual ~map_like() = default;
+    /**
+     * Searches for a property on an object whose name matches the provided
+     * identifier, returning a non-null pointer if present.
+     *
+     * The returned object is by value because it may outlive this native_object
+     * instance. For most whisker::object types, this is fine because they are
+     * self-contained.
+     *
+     * If this property lookup returns a native_object, `foo`, and that object
+     * depends on `this`, then `foo` should keep a `shared_ptr` to `this` to
+     * ensure that `this` outlives `foo`. The Whisker runtime does not provide
+     * any lifetime guarantees for `this`.
+     *
+     * Preconditions:
+     *   - The provided string is a valid Whisker identifier
+     */
+    virtual const object* lookup_property(
+        std::string_view identifier) const = 0;
+  };
   /**
-   * Creates a tree-like string representation of this object, primarily for
-   * debugging and diagnostic purposes.
+   * Returns an implementation of map_list if this object supports map-like
+   * property lookups. Otherwise, returns nullptr.
    *
-   * The provided tree_printer::scope object abstracts away the details of tree
-   * printing as well as encodes the location in an existing print tree where
-   * this object should be inline. See its documentation for more details.
-   */
-  virtual void print_to(tree_printer::scope, const object_print_options&) const;
-
-  /**
-   * Determines if this native_object compares equal to the other object. It is
-   * the implementers's responsibility to ensure that the other object also
-   * compares equal to this one and maintains the commutative property.
+   * When this function returns a non-null pointer, the returned object can be
+   * used in Whisker property lookups, such as section blocks.
    *
-   * The default implementation compares by object identity.
+   * This function returns a shared_ptr so that the returned object can be this
+   * object itself (which is expected to be stored as a native_object::ptr).
+   * Doing so prevents an extra heap allocation in favor of an atomic increment.
+   *
+   *     class my_object : public native_object,
+   *                       public native_object::map_like,
+   *                       public std::enable_shared_from_this<my_object> {
+   *      public:
+   *       std::shared_ptr<const native_object::map_like> as_map_like()
+   *           const override {
+   *         return shared_from_this();
+   *       }
+   *
+   *       // Implement map-like functions...
+   *     };
    */
-  virtual bool operator==(const native_object& other) const;
+  virtual std::shared_ptr<const native_object::map_like> as_map_like() const {
+    return nullptr;
+  }
 
   /**
    * A class that allows "array-like" random access over an underlying sequence
    * of objects.
    */
-  class sequence {
+  class array_like {
    public:
-    virtual ~sequence() = default;
+    virtual ~array_like() = default;
     /**
      * Returns the number of elements in the sequence.
      */
@@ -181,7 +200,7 @@ class native_object {
     virtual const object& at(std::size_t index) const = 0;
   };
   /**
-   * Returns an implementation of sequence if this object supports array-like
+   * Returns an implementation of array_like if this object supports array-like
    * iteration. Otherwise, returns nullptr.
    *
    * When this function returns a non-null pointer, the returned object can be
@@ -192,23 +211,40 @@ class native_object {
    * Doing so prevents an extra heap allocation in favor of an atomic increment.
    *
    *     class my_object : public native_object,
-   *                       public native_object::sequence,
+   *                       public native_object::array_like,
    *                       public std::enable_shared_from_this<my_object> {
    *      public:
-   *       std::shared_ptr<const sequence> as_sequence() const override {
+   *       std::shared_ptr<const native_object::array_like> as_array_like()
+   *           const override {
    *         return shared_from_this();
    *       }
    *
-   *       // Other functions...
+   *       // Implement array-like functions...
    *     };
-   *
-   * An array-like object is still allowed to have named properties supported
-   * via lookup_property(). This function opts in to strictly additional
-   * capabilities.
    */
-  virtual std::shared_ptr<const sequence> as_sequence() const {
+  virtual std::shared_ptr<const native_object::array_like> as_array_like()
+      const {
     return nullptr;
   }
+
+  /**
+   * Creates a tree-like string representation of this object, primarily for
+   * debugging and diagnostic purposes.
+   *
+   * The provided tree_printer::scope object abstracts away the details of tree
+   * printing as well as encodes the location in an existing print tree where
+   * this object should be inline. See its documentation for more details.
+   */
+  virtual void print_to(tree_printer::scope, const object_print_options&) const;
+
+  /**
+   * Determines if this native_object compares equal to the other object. It is
+   * the implementers's responsibility to ensure that the other object also
+   * compares equal to this one and maintains the commutative property.
+   *
+   * The default implementation compares by object identity.
+   */
+  virtual bool operator==(const native_object& other) const;
 };
 
 namespace detail {

third-party/thrift/src/thrift/compiler/whisker/render.cc

@@ -180,10 +180,13 @@ bool coerce_to_boolean(const array& value) {
   return !value.empty();
 }
 bool coerce_to_boolean(const native_object::ptr& value) {
-  if (auto sequence = value->as_sequence(); sequence != nullptr) {
-    return sequence->size() != 0;
+  if (auto array_like = value->as_array_like(); array_like != nullptr) {
+    return array_like->size() != 0;
   }
-  return true;
+  if (auto map_like = value->as_map_like(); map_like != nullptr) {
+    return true;
+  }
+  return false;
 }
 bool coerce_to_boolean(const map&) {
   return true;
@@ -438,25 +441,36 @@ class render_engine {
             // This native_object is being used as a conditional
             maybe_report_coercion();
             if (!coerce_to_boolean(value)) {
-              // Empty sequences are falsy
+              // Empty array-like objects are falsy
               do_visit(whisker::make::null);
             }
             return;
           }
-          // native_object can behave like a map or an array, depending on its
-          // implementation. The "default" implementation is map-like because
-          // lookup_property() must always be implemented.
-          // native::object::sequence is an optional extension.
-          // Our implementation here gives preference for the more specialized
-          // sequence interface.
-          if (auto sequence = value->as_sequence()) {
-            const std::size_t size = sequence->size();
+          // When used as a section_block, a native_object which is both
+          // "map"-like and "array"-like is ambiguous. We arbitrarily choose
+          // "array"-like as the winner. In practice, a native_object is most
+          // likely to be one or the other.
+          //
+          // This is one of the reasons that section blocks are deprecated in
+          // favor of `{{#each}}` and `{{#with}}`.
+          if (auto array_like = value->as_array_like()) {
+            const std::size_t size = array_like->size();
             for (std::size_t i = 0; i < size; ++i) {
-              do_visit(sequence->at(i));
+              do_visit(array_like->at(i));
             }
             return;
           }
-          do_visit(section_variable);
+          if (auto map_like = value->as_map_like()) {
+            do_visit(section_variable);
+            return;
+          }
+
+          // Since this native_object is neither array-like nor map-like, it is
+          // being used as a conditional
+          maybe_report_coercion();
+          if (coerce_to_boolean(value)) {
+            do_visit(whisker::make::null);
+          }
         },
         [&](const map&) {
           if (section.inverted) {

third-party/thrift/src/thrift/compiler/whisker/render.h

@@ -70,6 +70,9 @@ struct render_options {
    *     - ±0.0, NaN (f64)
    *     - empty string
    *     - empty array
+   *     - native_object which is...
+   *       - array-like with size of 0
+   *       - neither map-like nor array-like
    *   - All other values are considered "truthy".
    *
    * This *mostly* matches "falsy" values in JavaScript for the subset of its