From 72d3ba77982d9637bb80d428500ef8ab57e24970 Mon Sep 17 00:00:00 2001 From: Pranjal Raihan Date: Thu, 12 Dec 2024 12:37:32 -0800 Subject: [PATCH] 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 --- .../thrift/compiler/whisker/eval_context.cc | 14 +- .../thrift/compiler/whisker/mstch_compat.cc | 28 ++-- .../src/thrift/compiler/whisker/object.h | 136 +++++++++++------- .../src/thrift/compiler/whisker/render.cc | 42 ++++-- .../src/thrift/compiler/whisker/render.h | 3 + .../whisker/test/eval_context_test.cc | 25 ++-- .../compiler/whisker/test/object_test.cc | 3 - .../compiler/whisker/test/render_test.cc | 56 ++++++-- 8 files changed, 209 insertions(+), 98 deletions(-) diff --git a/third-party/thrift/src/thrift/compiler/whisker/eval_context.cc b/third-party/thrift/src/thrift/compiler/whisker/eval_context.cc index f2db760d046bb6..b769f51c972f52 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/eval_context.cc +++ b/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 { public: explicit global_scope_object(map properties) : properties_(std::move(properties)) {} + std::shared_ptr 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()) { diff --git a/third-party/thrift/src/thrift/compiler/whisker/mstch_compat.cc b/third-party/thrift/src/thrift/compiler/whisker/mstch_compat.cc index 5043d1efc11f11..7510db63786338 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/mstch_compat.cc +++ b/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 { 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 as_sequence() const override { + std::shared_ptr 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 { public: explicit mstch_map_proxy(mstch_map&& map) : proxied_(std::move(map)) {} private: + std::shared_ptr 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 { public: explicit mstch_object_proxy(std::shared_ptr&& obj) : proxied_(std::move(obj)) {} + std::shared_ptr 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. diff --git a/third-party/thrift/src/thrift/compiler/whisker/object.h b/third-party/thrift/src/thrift/compiler/whisker/object.h index c4fac6a627eba4..88d0968c4e2ea7 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/object.h +++ b/third-party/thrift/src/thrift/compiler/whisker/object.h @@ -24,7 +24,6 @@ #include #include #include -#include #include #include #include @@ -107,17 +106,17 @@ 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: @@ -125,49 +124,69 @@ class 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 { + * public: + * std::shared_ptr 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 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 { * public: - * std::shared_ptr as_sequence() const override { + * std::shared_ptr 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 as_sequence() const { + virtual std::shared_ptr 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 { diff --git a/third-party/thrift/src/thrift/compiler/whisker/render.cc b/third-party/thrift/src/thrift/compiler/whisker/render.cc index 21d6e1a0a2783b..4c8b54013ef0c2 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/render.cc +++ b/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) { diff --git a/third-party/thrift/src/thrift/compiler/whisker/render.h b/third-party/thrift/src/thrift/compiler/whisker/render.h index 5ff601547a34ff..4a7eac9e4a7f5b 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/render.h +++ b/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 diff --git a/third-party/thrift/src/thrift/compiler/whisker/test/eval_context_test.cc b/third-party/thrift/src/thrift/compiler/whisker/test/eval_context_test.cc index a2d6c0236205a7..63027891b5708e 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/test/eval_context_test.cc +++ b/third-party/thrift/src/thrift/compiler/whisker/test/eval_context_test.cc @@ -32,18 +32,21 @@ namespace { /** * An object with no properties. */ -class empty_native_object : public native_object { - public: - const object* lookup_property(std::string_view) const override { - return nullptr; - } -}; +class empty_native_object : public native_object {}; /** * When looking up a property, always returns a whisker::string that is the * property name repeated twice. */ -class double_property_name : public native_object { +class double_property_name + : public native_object, + public native_object::map_like, + public std::enable_shared_from_this { + public: + std::shared_ptr as_map_like() const override { + return shared_from_this(); + } + const object* lookup_property(std::string_view id) const override { if (auto cached = cached_.find(id); cached != cached_.end()) { return &cached->second; @@ -61,12 +64,18 @@ class double_property_name : public native_object { * When looking up a property, always returns the same whisker::object as a * reference. */ -class delegate_to : public native_object { +class delegate_to : public native_object, + public native_object::map_like, + public std::enable_shared_from_this { public: explicit delegate_to(whisker::object delegate) : delegate_(std::move(delegate)) {} private: + std::shared_ptr as_map_like() const override { + return shared_from_this(); + } + const object* lookup_property(std::string_view) const override { return &delegate_; } diff --git a/third-party/thrift/src/thrift/compiler/whisker/test/object_test.cc b/third-party/thrift/src/thrift/compiler/whisker/test/object_test.cc index 3b489dce0752db..093a5dfe685f68 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/test/object_test.cc +++ b/third-party/thrift/src/thrift/compiler/whisker/test/object_test.cc @@ -25,9 +25,6 @@ namespace whisker { namespace { class basic_native_object : public native_object { - const object* lookup_property(std::string_view) const override { - return nullptr; - } void print_to( tree_printer::scope scope, const object_print_options&) const override { scope.println(""); diff --git a/third-party/thrift/src/thrift/compiler/whisker/test/render_test.cc b/third-party/thrift/src/thrift/compiler/whisker/test/render_test.cc index 55a5995ef7e079..cbbb87a43d01cc 100644 --- a/third-party/thrift/src/thrift/compiler/whisker/test/render_test.cc +++ b/third-party/thrift/src/thrift/compiler/whisker/test/render_test.cc @@ -26,11 +26,7 @@ namespace w = whisker::make; namespace whisker { namespace { -class empty_native_object : public native_object { - const object* lookup_property(std::string_view) const override { - return nullptr; - } -}; +class empty_native_object : public native_object {}; } // namespace TEST_F(RenderTest, basic) { @@ -171,17 +167,14 @@ TEST_F(RenderTest, section_block_array_asymmetric_nested_scopes) { TEST_F(RenderTest, section_block_array_iterable_native_object) { class array_like_native_object : public native_object, - public native_object::sequence, + public native_object::array_like, public std::enable_shared_from_this { public: explicit array_like_native_object(array values) : values_(std::move(values)) {} - const object* lookup_property(std::string_view) const override { - return nullptr; - } - - std::shared_ptr as_sequence() const override { + std::shared_ptr as_array_like() + const override { return shared_from_this(); } std::size_t size() const override { return values_.size(); } @@ -263,10 +256,18 @@ TEST_F(RenderTest, section_block_map) { } TEST_F(RenderTest, section_block_map_like_native_object) { - class map_like_native_object : public native_object { + class map_like_native_object + : public native_object, + public native_object::map_like, + public std::enable_shared_from_this { public: explicit map_like_native_object(map values) : values_(std::move(values)) {} + std::shared_ptr as_map_like() + const override { + return shared_from_this(); + } + const object* lookup_property(std::string_view id) const override { if (auto value = values_.find(id); value != values_.end()) { return &value->second; @@ -313,6 +314,37 @@ TEST_F(RenderTest, section_block_map_like_native_object) { } } +TEST_F(RenderTest, section_block_pointless_native_object) { + auto context = + w::map({{"pointless", w::make_native_object()}}); + { + strict_boolean_conditional = diagnostic_level::info; + auto result = render( + "{{#pointless}}\n" + "Should not be rendered\n" + "{{/pointless}}", + context); + EXPECT_EQ(*result, ""); + } + { + strict_boolean_conditional = diagnostic_level::error; + auto result = render( + "{{#pointless}}\n" + "Should not be rendered\n" + "{{/pointless}}", + context); + EXPECT_FALSE(result.has_value()); + EXPECT_THAT( + diagnostics(), + testing::ElementsAre(diagnostic( + diagnostic_level::error, + "Condition 'pointless' is not a boolean. The encountered value is:\n" + "\n", + path_to_file, + 1))); + } +} + TEST_F(RenderTest, section_block_empty_array) { auto result = render( "The factorial function looks like:{{#factorials}}\n"