Refactor lookup_property to operate on variable_component

Summary:
- Refactor `lookup_property` to operate on `variable_component` as a unit, rather than just the `property` identifier component
- Update map and local variable lookups to use the full identifier as it would appear in source (`prototype:property`)
- Pre-compute and return references for the `prototype:property` string to avoid regeneration and copy on every lookup

Reviewed By: praihan

Differential Revision: D79453987

fbshipit-source-id: 50836f776e0be49323784227090ee7bb826ae5a1

Author: hchokshi

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

@@ -23,6 +23,7 @@
 
 #include <cassert>
 #include <iterator>
+#include <sstream>
 
 namespace whisker::ast {
 
@@ -32,12 +33,12 @@ template <typename T, typename ToStringFunc>
 std::string to_joined_string(
     const std::vector<T>& parts, char separator, ToStringFunc&& to_string) {
   assert(!parts.empty());
-  std::string result = to_string(parts.front());
+  std::ostringstream result;
+  result << to_string(parts.front());
   for (auto part = std::next(parts.begin()); part != parts.end(); ++part) {
-    result += separator;
-    result += to_string(*part);
+    result << separator << to_string(*part);
   }
-  return result;
+  return result.str();
 }
 
 } // namespace
@@ -51,19 +52,31 @@ std::string text::joined() const {
   return result;
 }
 
-std::string variable_component::as_string() const {
-  return prototype.has_value()
+variable_component::variable_component(
+    source_range loc_,
+    std::optional<identifier> prototype_,
+    identifier property_)
+    : loc(loc_),
+      prototype(std::move(prototype_)),
+      property(std::move(property_)) {
+  as_string_ = prototype.has_value()
       ? fmt::format("{}:{}", prototype->name, property.name)
       : property.name;
 }
 
+const std::string_view variable_component::as_string() const {
+  return as_string_;
+}
+
 std::string variable_lookup::chain_string() const {
   return detail::variant_match(
       chain,
       [](this_ref) -> std::string { return "this"; },
       [](const std::vector<variable_component>& ids) -> std::string {
         return to_joined_string(
-            ids, '.', [](const variable_component& id) -> std::string {
+            ids,
+            '.',
+            [](const variable_component& id) -> const std::string_view {
               return id.as_string();
             });
       });

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

@@ -179,6 +179,14 @@ struct variable_component {
   std::optional<identifier> prototype;
   identifier property;
 
+  variable_component() = delete;
+
+  // Explicit constructor to ensure string representation is initialized
+  variable_component(
+      source_range loc,
+      std::optional<identifier> prototype,
+      identifier property);
+
   /**
    * Determines if two identifiers are syntactically equivalent, excluding
    * their location in source code.
@@ -190,7 +198,17 @@ struct variable_component {
   // Remove in C++20 which introduces comparison operator synthesis
   WHISKER_DEFINE_OPERATOR_INEQUALITY(variable_component)
 
-  std::string as_string() const;
+  /**
+   * Returns a source-identical string representation of this variable
+   * component.
+   * For a prototype-qualified lookup, it takes the form "prototype:property".
+   * For a raw identifier lookup, it is just "property".
+   */
+  const std::string_view as_string() const;
+
+ private:
+  // Store string representation to avoid regenerating repeatedly
+  std::string as_string_;
 };
 
 /**

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

@@ -66,7 +66,7 @@ namespace detail {
 std::optional<object> find_property(
     diagnostics_engine& diags,
     const object& self,
-    const ast::identifier& identifier) {
+    const ast::variable_component& component) {
   using result = std::optional<object>;
   return self.visit(
       [](null) -> result { return std::nullopt; },
@@ -76,17 +76,22 @@ std::optional<object> find_property(
       [](boolean) -> result { return std::nullopt; },
       [](const array::ptr&) -> result { return std::nullopt; },
       [&](const map::ptr& m) -> result {
-        return m->lookup_property(identifier.name);
+        // A map doesn't have a prototype, so we treat the lookup as just a
+        // simple string key.
+        // Specifically for the case of mstch_compat, mstch_object_proxy is in
+        // fact a Whisker map, so this also provides backwards compatibility for
+        // legacy mstch properties which have colons in the name.
+        return m->lookup_property(component.as_string());
       },
       [](const native_function::ptr&) -> result { return std::nullopt; },
       [&](const native_handle<>& h) -> result {
         if (const auto* descriptor =
-                h.proto()->find_descriptor(identifier.name)) {
+                h.proto()->find_descriptor(component.property.name)) {
           return detail::variant_match(
               *descriptor,
               [&](const prototype<>::property& prop) -> object {
                 return prop.function->invoke(native_function::context{
-                    identifier.loc,
+                    component.loc,
                     diags,
                     self,
                     {} /* positional arguments */,
@@ -104,11 +109,12 @@ std::optional<object> find_property(
 } // namespace detail
 
 std::optional<object> eval_context::lexical_scope::lookup_property(
-    diagnostics_engine& diags, const ast::identifier& identifier) {
-  if (auto local = locals_.find(identifier.name); local != locals_.end()) {
+    diagnostics_engine& diags, const ast::variable_component& component) {
+  if (auto local = locals_.find(component.as_string());
+      local != locals_.end()) {
     return local->second;
   }
-  return detail::find_property(diags, this_ref_, identifier);
+  return detail::find_property(diags, this_ref_, component);
 }
 
 eval_context::eval_context(diagnostics_engine& diags, object globals)
@@ -194,16 +200,17 @@ eval_context::look_up_object(const ast::variable_lookup& lookup) {
               begin,
               end,
               std::back_inserter(result),
-              [](const auto& component) { return component.as_string(); });
+              [](const auto& component) {
+                return std::string{component.as_string()};
+              });
           return result;
         };
 
         std::optional<object> current;
         // Crawl up through the scope stack since names can be shadowed.
         for (auto scope = stack_.rbegin(); scope != stack_.rend(); ++scope) {
           try {
-            if (auto result =
-                    scope->lookup_property(diags_, path.front().property)) {
+            if (auto result = scope->lookup_property(diags_, path.front())) {
               current = result;
               break;
             }
@@ -225,7 +232,7 @@ eval_context::look_up_object(const ast::variable_lookup& lookup) {
              ++component) {
           try {
             std::optional<object> next =
-                detail::find_property(diags_, *current, component->property);
+                detail::find_property(diags_, *current, *component);
             if (!next.has_value()) {
               return unexpected(eval_property_lookup_error(
                   *current, /* missing_from */

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

@@ -149,7 +149,7 @@ namespace detail {
 std::optional<object> find_property(
     diagnostics_engine& diags,
     const object& self,
-    const ast::identifier& identifier);
+    const ast::variable_component& component);
 
 } // namespace detail
 
@@ -337,7 +337,7 @@ class eval_context {
      *     throws
      */
     std::optional<object> lookup_property(
-        diagnostics_engine& diags, const ast::identifier& identifier);
+        diagnostics_engine& diags, const ast::variable_component& component);
 
     // Before C++20, std::unordered_map does not support heterogenous lookups
     using locals_map = std::map<std::string, object, std::less<>>;

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

@@ -207,7 +207,11 @@ class mstch_object_proxy
       return detail::find_property(
           diags_,
           *self_whisker_obj,
-          ast::identifier{source_range{}, std::string(self_property->second)});
+          ast::variable_component{
+              source_range{},
+              std::nullopt /* prototype */,
+              ast::identifier{
+                  source_range{}, std::string(self_property->second)}});
     }
 
     return std::nullopt;