From d800dd5e4537f124331711d81c24550044f63b63 Mon Sep 17 00:00:00 2001 From: Pranjal Raihan Date: Fri, 24 Jan 2025 21:46:12 -0800 Subject: [PATCH] Use ordered set for enumerating map property keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: We want two properties of the key set: 1. Unique — each property only needs to be enumerated once 2. Deterministic order — when we print or iterate using `(map.items ...)` we want the order to be deterministic. Otherwise, different runs of whisker could produce different outputs. `std::set` fits both of these requirements. Reviewed By: rmakheja Differential Revision: D68527462 fbshipit-source-id: 3bd51dd21315d5a5d1ad65afccbc598825645aec --- thrift/compiler/whisker/dsl.cc | 9 +++---- thrift/compiler/whisker/dsl.h | 2 +- thrift/compiler/whisker/mstch_compat.cc | 9 +++---- thrift/compiler/whisker/mstch_compat.h | 6 ++--- thrift/compiler/whisker/object.cc | 26 +++++-------------- thrift/compiler/whisker/object.h | 9 ++++--- thrift/compiler/whisker/standard_library.cc | 9 +++++-- thrift/compiler/whisker/test/object_test.cc | 14 +++++----- .../whisker/test/render_test_helpers.h | 7 +++-- 9 files changed, 40 insertions(+), 51 deletions(-) diff --git a/thrift/compiler/whisker/dsl.cc b/thrift/compiler/whisker/dsl.cc index 7f48d1b0f73..daece7c29f3 100644 --- a/thrift/compiler/whisker/dsl.cc +++ b/thrift/compiler/whisker/dsl.cc @@ -68,18 +68,17 @@ object::ptr map_like::lookup_property(std::string_view identifier) const { }); } -std::optional> map_like::keys() const { - using result = std::optional>; +std::optional> map_like::keys() const { + using result = std::optional>; return whisker::detail::variant_match( which_, [&](const native_object::map_like::ptr& m) -> result { return m->keys(); }, [&](const managed_ptr& m) -> result { - std::vector keys; - keys.reserve(m->size()); + std::set keys; for (const auto& [key, _] : *m) { - keys.push_back(key); + keys.insert(key); } return keys; }); diff --git a/thrift/compiler/whisker/dsl.h b/thrift/compiler/whisker/dsl.h index c58fa55d0cb..4c810414175 100644 --- a/thrift/compiler/whisker/dsl.h +++ b/thrift/compiler/whisker/dsl.h @@ -73,7 +73,7 @@ static_assert(std::is_copy_constructible_v); class map_like final : public native_object::map_like { public: object::ptr lookup_property(std::string_view identifier) const final; - std::optional> keys() const final; + std::optional> keys() const final; /** * Tries to marshal the provided object into an map-like object, if the diff --git a/thrift/compiler/whisker/mstch_compat.cc b/thrift/compiler/whisker/mstch_compat.cc index 37c280a23d9..3ac10f2f921 100644 --- a/thrift/compiler/whisker/mstch_compat.cc +++ b/thrift/compiler/whisker/mstch_compat.cc @@ -112,11 +112,10 @@ class mstch_map_proxy final return nullptr; } - std::optional> keys() const override { - std::vector property_names; - property_names.reserve(proxied_.size()); + std::optional> keys() const override { + std::set property_names; for (const auto& [key, _] : proxied_) { - property_names.push_back(key); + property_names.insert(key); } return property_names; } @@ -162,7 +161,7 @@ class mstch_object_proxy [](object::ptr o) -> object::ptr { return o; }); } - std::optional> keys() const override { + std::optional> keys() const override { return proxied_->property_names(); } diff --git a/thrift/compiler/whisker/mstch_compat.h b/thrift/compiler/whisker/mstch_compat.h index 1ca6e1a3f11..2d001dc9211 100644 --- a/thrift/compiler/whisker/mstch_compat.h +++ b/thrift/compiler/whisker/mstch_compat.h @@ -54,10 +54,10 @@ class object_t { return methods_.find(name) != methods_.end(); } - std::vector property_names() const { - std::vector result; + std::set property_names() const { + std::set result; for (const auto& [name, _] : methods_) { - result.push_back(name); + result.insert(name); } return result; } diff --git a/thrift/compiler/whisker/object.cc b/thrift/compiler/whisker/object.cc index 72b9bbeaa22..95381398c6f 100644 --- a/thrift/compiler/whisker/object.cc +++ b/thrift/compiler/whisker/object.cc @@ -194,17 +194,6 @@ bool array_eq( return true; } -namespace { -std::optional> unique_keys( - std::optional> keys) { - if (!keys.has_value()) { - return std::nullopt; - } - return std::unordered_set( - std::make_move_iterator(keys->begin()), - std::make_move_iterator(keys->end())); -} -} // namespace bool map_eq(const map& lhs, const map& rhs) { return lhs == rhs; } @@ -212,7 +201,7 @@ bool map_eq(const map& lhs, const native_object::map_like::ptr& rhs) { if (rhs == nullptr) { return false; } - auto rhs_keys = unique_keys(rhs->keys()); + auto rhs_keys = rhs->keys(); if (!rhs_keys.has_value()) { // Not enumerable return false; @@ -244,8 +233,8 @@ bool map_eq( return false; } - auto lhs_keys = unique_keys(lhs->keys()); - auto rhs_keys = unique_keys(rhs->keys()); + auto lhs_keys = lhs->keys(); + auto rhs_keys = rhs->keys(); const bool keys_equal = lhs_keys.has_value() && rhs_keys.has_value() && *lhs_keys == *rhs_keys; if (!keys_equal) { @@ -264,7 +253,7 @@ bool map_eq( void native_object::map_like::default_print_to( std::string_view name, - std::vector property_names, + const std::set& property_names, tree_printer::scope scope, const object_print_options& options) const { assert(scope.semantic_depth() <= options.max_depth); @@ -430,11 +419,10 @@ object make::proxy(const object::ptr& source) { } return nullptr; } - std::optional> keys() const final { - std::vector property_names; - property_names.reserve(map_->size()); + std::optional> keys() const final { + std::set property_names; for (const auto& [key, _] : *map_) { - property_names.push_back(key); + property_names.insert(key); } return property_names; } diff --git a/thrift/compiler/whisker/object.h b/thrift/compiler/whisker/object.h index 0bb537dc761..9356dabad07 100644 --- a/thrift/compiler/whisker/object.h +++ b/thrift/compiler/whisker/object.h @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -162,16 +163,16 @@ class native_object { std::string_view identifier) const = 0; /** - * Returns an ordered list of finitely enumerable property names of this + * Returns an ordered set of finitely enumerable property names of this * map-like object. * * If property names are not enumerable (i.e. dynamically generated), then * this returns the empty optional. * - * For each name returned in this list, lookup_property must not return + * For each name returned in this set, lookup_property must not return * nullptr. */ - virtual std::optional> keys() const { + virtual std::optional> keys() const { return std::nullopt; } @@ -187,7 +188,7 @@ class native_object { */ void default_print_to( std::string_view name, - std::vector property_names, + const std::set& property_names, tree_printer::scope, const object_print_options&) const; }; diff --git a/thrift/compiler/whisker/standard_library.cc b/thrift/compiler/whisker/standard_library.cc index 79825092ca2..2ebbf283caf 100644 --- a/thrift/compiler/whisker/standard_library.cc +++ b/thrift/compiler/whisker/standard_library.cc @@ -20,6 +20,8 @@ #include +#include + namespace w = whisker::make; namespace whisker { @@ -189,8 +191,11 @@ map::value_type create_map_functions() { throw ctx.make_error( "map-like object does not have enumerable properties."); } - auto view = - w::make_native_object(std::move(m), std::move(*keys)); + auto view = w::make_native_object( + std::move(m), + std::vector( + std::make_move_iterator(keys->begin()), + std::make_move_iterator(keys->end()))); return manage_owned(std::move(view)); }); diff --git a/thrift/compiler/whisker/test/object_test.cc b/thrift/compiler/whisker/test/object_test.cc index f6dc4c3c703..00267957104 100644 --- a/thrift/compiler/whisker/test/object_test.cc +++ b/thrift/compiler/whisker/test/object_test.cc @@ -147,7 +147,7 @@ TEST(ObjectTest, native_object_equality) { struct always_zero_map : native_object, native_object::map_like, std::enable_shared_from_this { - explicit always_zero_map(std::vector keys) + explicit always_zero_map(std::set keys) : keys_(std::move(keys)) {} native_object::map_like::ptr as_map_like() const override { @@ -158,9 +158,7 @@ TEST(ObjectTest, native_object_equality) { return manage_owned(w::i64(0)); } - std::optional> keys() const override { - return keys_; - } + std::optional> keys() const override { return keys_; } void print_to( tree_printer::scope scope, @@ -169,7 +167,7 @@ TEST(ObjectTest, native_object_equality) { } private: - std::vector keys_; + std::set keys_; }; struct always_zero_array : native_object, @@ -198,11 +196,11 @@ TEST(ObjectTest, native_object_equality) { }; native_object::ptr m1 = - std::make_shared(std::vector{"foo"}); + std::make_shared(std::set{"foo"}); native_object::ptr m2 = - std::make_shared(std::vector{"foo"}); + std::make_shared(std::set{"foo"}); native_object::ptr m3 = - std::make_shared(std::vector{"foo", "bar"}); + std::make_shared(std::set{"foo", "bar"}); map raw_m3{{"foo", w::i64(0)}, {"bar", w::i64(0)}}; object o1 = w::native_object(m1); diff --git a/thrift/compiler/whisker/test/render_test_helpers.h b/thrift/compiler/whisker/test/render_test_helpers.h index 9ddeae5ee95..cc47caa276e 100644 --- a/thrift/compiler/whisker/test/render_test_helpers.h +++ b/thrift/compiler/whisker/test/render_test_helpers.h @@ -83,11 +83,10 @@ class RenderTest : public testing::Test { return nullptr; } - std::optional> keys() const override { - std::vector keys; - keys.reserve(values_.size()); + std::optional> keys() const override { + std::set keys; for (const auto& [key, _] : values_) { - keys.push_back(key); + keys.insert(key); } return keys; }