Consolidate uses of maybe_managed_ptr → managed_object_ptr

Summary:
The name `maybe_managed_ptr<T>` got kind of confusing because the type was being used for 3 different things.

All these cases are now documented in doc blocks and we have `object::as_static`, `object::owned`, and `object::derive` as helper functions to make intentions clearer at call-sites.

Furthermore, `maybe_managed_ptr<T>` name has been dropped in favor of `managed_object_ptr<T>`.

Change is no-op. This just improves readability.

Reviewed By: thedavekwon

Differential Revision: D67928745

fbshipit-source-id: e0ed7f74b67499cb7462e9e3ec7fd885fc323977

thrift/compiler/whisker/object.cc

@@ -146,7 +146,8 @@ bool native_object::operator==(const native_object& other) const {
 native_function::context::array_like::array_like(
     native_object::array_like::ptr&& arr)
     : which_(std::move(arr)) {}
-native_function::context::array_like::array_like(maybe_managed_ptr<array>&& arr)
+native_function::context::array_like::array_like(
+    managed_object_ptr<array>&& arr)
     : which_(std::move(arr)) {}
 
 native_function::context::array_like::~array_like() noexcept = default;
@@ -175,21 +176,20 @@ const object& native_function::context::array_like::at(
       [&](const native_object::array_like::ptr& arr) -> const object& {
         return arr->at(index);
       },
-      [&](const maybe_managed_ptr<array>& arr) -> const object& {
+      [&](const managed_object_ptr<array>& arr) -> const object& {
         return (*arr)[index];
       });
 }
 
 native_function::context::map_like::map_like(native_object::map_like::ptr&& m)
     : which_(std::move(m)) {}
-native_function::context::map_like::map_like(maybe_managed_ptr<map>&& m)
+native_function::context::map_like::map_like(managed_object_ptr<map>&& m)
     : which_(std::move(m)) {}
 
 /* static */ std::optional<native_function::context::array_like>
 native_function::context::array_like::try_from(const object::ptr& o) {
   if (o->is_array()) {
-    return array_like(
-        maybe_managed_ptr<array>(o, std::addressof(o->as_array())));
+    return array_like(object::derive(o, o->as_array()));
   }
   if (o->is_native_object()) {
     if (native_object::array_like::ptr arr =
@@ -219,7 +219,7 @@ const object* native_function::context::map_like::lookup_property(
       [&](const native_object::map_like::ptr& m) -> const object* {
         return m->lookup_property(identifier);
       },
-      [&](const maybe_managed_ptr<map>& m) -> const object* {
+      [&](const managed_object_ptr<map>& m) -> const object* {
         if (auto it = m->find(identifier); it != m->end()) {
           return &it->second;
         }
@@ -230,7 +230,7 @@ const object* native_function::context::map_like::lookup_property(
 /* static */ std::optional<native_function::context::map_like>
 native_function::context::map_like::try_from(const object::ptr& o) {
   if (o->is_map()) {
-    return map_like(maybe_managed_ptr<map>(o, std::addressof(o->as_map())));
+    return map_like(object::derive(o, o->as_map()));
   }
   if (o->is_native_object()) {
     if (native_object::map_like::ptr m = o->as_native_object()->as_map_like()) {

thrift/compiler/whisker/object.h

@@ -113,19 +113,18 @@ struct object_print_options {
 };
 
 /**
- * A shared_ptr which represents a (possibly) managed object.
+ * A managed_object_ptr<T> (where T is whisker::object or any of its
+ * alternatives) represents one of:
+ *   1. A "static"-ally managed object. See object::as_static(...).
+ *   2. An owned instance of T. See object::owned(...).
+ *   3. A derived instance of T, that is a sub-object of another managed object.
+ *      See object::derive(...).
  *
- * When storing a raw pointer, maybe_managed_ptr does not manage the pointer's
- * lifetime, i.e. never calls `free` on the pointer.
- *
- * When storing a shared_ptr, maybe_managed_ptr will release the shared_ptr
- * upon its own destruction.
- *
- * See `folly::MaybeManagedPtr`.
+ * All three cases are represented by shared_ptr. See `folly::MaybeManagedPtr`.
+ * (1) and (3) use the aliasing constructor of shared_ptr.
  */
-template <typename T>
-using maybe_managed_ptr = std::shared_ptr<const T>;
-using object_ptr = maybe_managed_ptr<object>;
+template <typename T = object>
+using managed_object_ptr = std::shared_ptr<const T>;
 
 /**
  * A native_object is the most powerful type in Whisker. Its properties and
@@ -156,7 +155,7 @@ class native_object {
    */
   class map_like {
    public:
-    using ptr = maybe_managed_ptr<const map_like>;
+    using ptr = managed_object_ptr<map_like>;
     virtual ~map_like() = default;
     /**
      * Searches for a property on an object whose name matches the provided
@@ -207,7 +206,7 @@ class native_object {
    */
   class array_like {
    public:
-    using ptr = maybe_managed_ptr<const array_like>;
+    using ptr = managed_object_ptr<array_like>;
     virtual ~array_like() = default;
     /**
      * Returns the number of elements in the sequence.
@@ -273,7 +272,7 @@ namespace detail {
  * argument<T>(...) or named_argument<T>(...).
  *
  * Small primitive types (i64, f64, boolean) are returned by value.
- * The larger primitive type (string) is returned as a maybe_managed_ptr<T>.
+ * The larger primitive type (string) is returned as a managed_object_ptr<T>.
  *
  * Maps and arrays are wrapped by helper classes, in order to abstract away
  * differences with native_object::array_like and native_object::map_like
@@ -314,7 +313,7 @@ class native_function {
    *         ctx.declare_named_arguments({});
    *         i64 a = ctx.argument<i64>(0);
    *         i64 b = ctx.argument<i64>(1);
-   *         return object::managed(whisker::make::boolean(a == b));
+   *         return object::owned(whisker::make::boolean(a == b));
    *       }
    *     };
    *
@@ -337,7 +336,7 @@ class native_function {
    *           }
    *           result += *ctx.argument<string>(i);
    *         }
-   *         return object::managed(
+   *         return object::owned(
    *             whisker::make::string(std::move(result)));
    *       }
    *     };
@@ -352,7 +351,7 @@ class native_function {
    * Postconditions:
    *  - The returned object is non-null.
    */
-  virtual object_ptr invoke(context) = 0;
+  virtual managed_object_ptr<> invoke(context) = 0;
 
   /**
    * An exception that can be thrown to indicate a fatal error in function
@@ -431,13 +430,13 @@ class native_function {
        * Tries to marshal the provided object into an array-like object, if the
        * underlying type matches. Otherwise, returns an empty optional.
        */
-      static std::optional<array_like> try_from(const object_ptr&);
+      static std::optional<array_like> try_from(const managed_object_ptr<>&);
 
      private:
       explicit array_like(native_object::array_like::ptr&& arr);
-      explicit array_like(maybe_managed_ptr<array>&& arr);
+      explicit array_like(managed_object_ptr<array>&& arr);
 
-      std::variant<native_object::array_like::ptr, maybe_managed_ptr<array>>
+      std::variant<native_object::array_like::ptr, managed_object_ptr<array>>
           which_;
 
       friend class context;
@@ -465,13 +464,14 @@ class native_function {
        * Tries to marshal the provided object into an map-like object, if the
        * underlying type matches. Otherwise, returns an empty optional.
        */
-      static std::optional<map_like> try_from(const object_ptr&);
+      static std::optional<map_like> try_from(const managed_object_ptr<>&);
 
      private:
       explicit map_like(native_object::map_like::ptr&& m);
-      explicit map_like(maybe_managed_ptr<map>&& m);
+      explicit map_like(managed_object_ptr<map>&& m);
 
-      std::variant<native_object::map_like::ptr, maybe_managed_ptr<map>> which_;
+      std::variant<native_object::map_like::ptr, managed_object_ptr<map>>
+          which_;
 
       friend class context;
     };
@@ -514,7 +514,7 @@ class native_function {
      * If the argument is not present and presence is required, then this throws
      * an error. Otherwise, returns nullptr.
      */
-    object_ptr named_argument(
+    managed_object_ptr<> named_argument(
         std::string_view name,
         named_argument_presence = named_argument_presence::required) const;
 
@@ -620,7 +620,7 @@ struct native_function_argument_result<boolean> {
 };
 template <>
 struct native_function_argument_result<string> {
-  using type = maybe_managed_ptr<string>;
+  using type = managed_object_ptr<string>;
   using optional_type = type;
 };
 template <>
@@ -689,28 +689,38 @@ class object final : private detail::object_base<object> {
     return std::get<T>(*this);
   }
 
-  using ptr = object_ptr;
+  using ptr = managed_object_ptr<>;
 
   /**
-   * Returns a shared_ptr which is an unmanaged reference to the provided
+   * Returns a pointer which is an unmanaged reference to the provided
    * object.
    *
    * The caller must guarantee that underlying object is kept alive for the
    * duration of an expression evaluation in which the object is used.
    */
-  static ptr as_ref(const object& o) {
-    return ptr(std::shared_ptr<void>(), &o);
+  static ptr as_static(const object& o) {
+    return ptr(managed_object_ptr<void>(), &o);
   }
-  static ptr as_ref(object&&) = delete;
+  static ptr as_static(object&&) = delete;
 
   /**
-   * Returns a shared_ptr which directly owns a copy of the provided object.
+   * Returns a pointer which directly owns the provided object.
    * This allows native_function to return values that are dynamically computed.
    */
-  static ptr managed(object&& o) {
+  static ptr owned(object&& o) {
     return std::make_shared<const object>(std::move(o));
   }
 
+  /**
+   * Returns a pointer where `value` is known to be a sub-object of `from`.
+   * This means that `from` must outlive `value`.
+   */
+  template <typename T, typename U>
+  static managed_object_ptr<T> derive(
+      const managed_object_ptr<U>& from, const T& value) {
+    return managed_object_ptr<T>(from, std::addressof(value));
+  }
+
   /* implicit */ object(null = {}) : base(std::in_place_type<null>) {}
   explicit object(boolean value) : base(bool(value)) {}
   explicit object(i64 value) : base(value) {}
@@ -976,7 +986,7 @@ native_function::context::argument(size_t index) const {
       error("Argument at index {} is of an unexpected type.", index);
     }
     if constexpr (std::is_same_v<T, string>) {
-      return maybe_managed_ptr<T>(o, std::addressof(o->as<T>()));
+      return object::derive<T>(o, o->as<T>());
     } else {
       return o->as<T>();
     }
@@ -1014,7 +1024,7 @@ native_function::context::named_argument(
       error("Named argument '{}' is of an unexpected type.", name);
     }
     if constexpr (std::is_same_v<T, string>) {
-      return maybe_managed_ptr<T>(o, std::addressof(o->as<T>()));
+      return object::derive(o, o->as<T>());
     } else {
       return o->as<T>();
     }

thrift/compiler/whisker/render.cc

@@ -503,22 +503,22 @@ class render_engine {
     return detail::variant_match(
         expr.which,
         [](const expression::string_literal& s) -> object::ptr {
-          return object::managed(whisker::make::string(s.text));
+          return object::owned(whisker::make::string(s.text));
         },
         [](const expression::i64_literal& i) -> object::ptr {
-          return object::managed(whisker::make::i64(i.value));
+          return object::owned(whisker::make::i64(i.value));
         },
         [](const expression::null_literal&) -> object::ptr {
-          return object::as_ref(whisker::make::null);
+          return object::as_static(whisker::make::null);
         },
         [](const expression::true_literal&) -> object::ptr {
-          return object::as_ref(whisker::make::true_);
+          return object::as_static(whisker::make::true_);
         },
         [](const expression::false_literal&) -> object::ptr {
-          return object::as_ref(whisker::make::false_);
+          return object::as_static(whisker::make::false_);
         },
         [&](const ast::variable_lookup& variable_lookup) -> object::ptr {
-          return object::as_ref(lookup_variable(variable_lookup));
+          return object::as_static(lookup_variable(variable_lookup));
         },
         [&](const function_call& func) -> object::ptr {
           return detail::variant_match(
@@ -528,28 +528,28 @@ class render_engine {
                 assert(func.positional_arguments.size() == 1);
                 assert(func.named_arguments.empty());
                 return evaluate_as_bool(func.positional_arguments[0])
-                    ? object::as_ref(whisker::make::false_)
-                    : object::as_ref(whisker::make::true_);
+                    ? object::as_static(whisker::make::false_)
+                    : object::as_static(whisker::make::true_);
               },
               [&](function_call::builtin_and) -> object::ptr {
                 // enforced by the parser
                 assert(func.named_arguments.empty());
                 for (const expression& arg : func.positional_arguments) {
                   if (!evaluate_as_bool(arg)) {
-                    return object::as_ref(whisker::make::false_);
+                    return object::as_static(whisker::make::false_);
                   }
                 }
-                return object::as_ref(whisker::make::true_);
+                return object::as_static(whisker::make::true_);
               },
               [&](function_call::builtin_or) -> object::ptr {
                 // enforced by the parser
                 assert(func.named_arguments.empty());
                 for (const expression& arg : func.positional_arguments) {
                   if (evaluate_as_bool(arg)) {
-                    return object::as_ref(whisker::make::true_);
+                    return object::as_static(whisker::make::true_);
                   }
                 }
-                return object::as_ref(whisker::make::false_);
+                return object::as_static(whisker::make::false_);
               },
               [&](const function_call::user_defined& user_defined)
                   -> object::ptr {

thrift/compiler/whisker/test/render_test.cc

@@ -712,7 +712,7 @@ class add : public native_function {
     for (std::size_t i = 0; i < ctx.arity(); ++i) {
       result += ctx.argument<i64>(i);
     }
-    return object::managed(w::i64(negate ? -result : result));
+    return object::owned(w::i64(negate ? -result : result));
   }
 };
 
@@ -725,7 +725,7 @@ class i64_eq : public native_function {
     ctx.declare_named_arguments({});
     i64 a = ctx.argument<i64>(0);
     i64 b = ctx.argument<i64>(1);
-    return object::managed(w::boolean(a == b));
+    return object::owned(w::boolean(a == b));
   }
 };
 
@@ -747,7 +747,7 @@ class str_concat : public native_function {
       }
       result += *ctx.argument<string>(i);
     }
-    return object::managed(w::string(std::move(result)));
+    return object::owned(w::string(std::move(result)));
   }
 };
 
@@ -759,7 +759,7 @@ class array_len : public native_function {
     ctx.declare_arity(1);
     ctx.declare_named_arguments({});
     auto len = i64(ctx.argument<array>(0).size());
-    return object::managed(w::i64(len));
+    return object::owned(w::i64(len));
   }
 };
 
@@ -775,7 +775,7 @@ class map_get : public native_function {
     auto key = ctx.named_argument<string>("key", context::required);
 
     if (const object* result = m.lookup_property(*key)) {
-      return object::as_ref(*result);
+      return object::as_static(*result);
     }
     ctx.error("Key '{}' not found.", *key);
   }
@@ -959,7 +959,7 @@ TEST_F(RenderTest, user_defined_function_array_like_named_argument) {
       ctx.declare_arity(0);
       ctx.declare_named_arguments({"input"});
       auto len = i64(ctx.named_argument<array>("input")->size());
-      return object::managed(w::string(fmt::format("length is {}", len)));
+      return object::owned(w::string(fmt::format("length is {}", len)));
     }
   };
 
@@ -1041,8 +1041,7 @@ TEST_F(RenderTest, user_defined_function_map_like_named_argument) {
 
       std::string_view result =
           m->lookup_property(*key) == nullptr ? "missing" : "present";
-      return object::managed(
-          w::string(fmt::format("map element is {}", result)));
+      return object::owned(w::string(fmt::format("map element is {}", result)));
     }
   };