Fix variable use tracking for scope subscript accesses.

Accessing a scope field with a subscript expression as in:

  scope["foo"]

Did not mark the 'foo' field in the scope as used, unlike
the corresponding expression `scope.foo`.

CL fixes that sharing the logic for scope member evaluation
between AccessorNode::ExecuteScopeSubscriptAccess() and
AccessorNode::ExecuteScopeAccess().

Change-Id: I97b0efa5a2d24c6a99b1a70a33626d1860747909
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/15920
Commit-Queue: David Turner <digit@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>

src/gn/parse_tree.cc

diff --git a/src/gn/parse_tree.cc b/src/gn/parse_tree.cc
index 28d5313..0f1c27e 100644
--- a/src/gn/parse_tree.cc
+++ b/src/gn/parse_tree.cc
@@ -391,7 +391,7 @@
   if (!key_value.VerifyTypeIs(Value::STRING, err))
     return Value();
   const Value* result =
-      base_value->scope_value()->GetValue(key_value.string_value());
+      ExecuteScopeAccessForMember(scope, key_value.string_value(), err);
   if (!result) {
     *err =
         Err(subscript_.get(), "No value named \"" + key_value.string_value() +
@@ -402,6 +402,21 @@
 }
 
 Value AccessorNode::ExecuteScopeAccess(Scope* scope, Err* err) const {
+  const Value* result =
+      ExecuteScopeAccessForMember(scope, member_->value().value(), err);
+
+  if (!result) {
+    *err = Err(member_.get(), "No value named \"" + member_->value().value() +
+                                  "\" in scope \"" + base_.value() + "\"");
+    return Value();
+  }
+  return *result;
+}
+
+const Value* AccessorNode::ExecuteScopeAccessForMember(
+    Scope* scope,
+    std::string_view member_str,
+    Err* err) const {
   // We jump through some hoops here since ideally a.b will count "b" as
   // accessed in the given scope. The value "a" might be in some normal nested
   // scope and we can modify it, but it might also be inherited from the
@@ -418,30 +433,22 @@
     // Common case: base value is mutable so we can track variable accesses
     // for unused value warnings.
     if (!mutable_base_value->VerifyTypeIs(Value::SCOPE, err))
-      return Value();
-    result = mutable_base_value->scope_value()->GetValue(
-        member_->value().value(), true);
+      return nullptr;
+    result = mutable_base_value->scope_value()->GetValue(member_str, true);
   } else {
     // Fall back to see if the value is on a read-only scope.
     const Value* const_base_value = scope->GetValue(base_.value(), true);
     if (const_base_value) {
       // Read only value, don't try to mark the value access as a "used" one.
       if (!const_base_value->VerifyTypeIs(Value::SCOPE, err))
-        return Value();
-      result =
-          const_base_value->scope_value()->GetValue(member_->value().value());
+        return nullptr;
+      result = const_base_value->scope_value()->GetValue(member_str);
     } else {
       *err = Err(base_, "Undefined identifier.");
-      return Value();
+      return nullptr;
     }
   }
-
-  if (!result) {
-    *err = Err(member_.get(), "No value named \"" + member_->value().value() +
-                                  "\" in scope \"" + base_.value() + "\"");
-    return Value();
-  }
-  return *result;
+  return result;
 }
 
 void AccessorNode::SetNewLocation(int line_number) {

src/gn/parse_tree.h

diff --git a/src/gn/parse_tree.h b/src/gn/parse_tree.h
index 7922cd6..2ed8c54 100644
--- a/src/gn/parse_tree.h
+++ b/src/gn/parse_tree.h
@@ -206,6 +206,9 @@
                                     const Value* base_value,
                                     Err* err) const;
   Value ExecuteScopeAccess(Scope* scope, Err* err) const;
+  const Value* ExecuteScopeAccessForMember(Scope* scope,
+                                           std::string_view member_str,
+                                           Err* err) const;
 
   static constexpr const char* kDumpAccessorKind = "accessor_kind";
   static constexpr const char* kDumpAccessorKindSubscript = "subscript";

src/gn/parse_tree_unittest.cc

diff --git a/src/gn/parse_tree_unittest.cc b/src/gn/parse_tree_unittest.cc
index cc49848..9b684f0 100644
--- a/src/gn/parse_tree_unittest.cc
+++ b/src/gn/parse_tree_unittest.cc
@@ -83,15 +83,20 @@
   EXPECT_EQ(second.type(), Value::INTEGER);
   EXPECT_EQ(second.int_value(), 3);
 
+  const Scope* scope_var = setup.scope()->GetValue("scope")->scope_value();
+  EXPECT_TRUE(scope_var->IsSetButUnused("foo"));
   Value foo = setup.ExecuteExpression("scope[\"foo\"]", &err);
   EXPECT_FALSE(err.has_error());
   EXPECT_EQ(foo.type(), Value::INTEGER);
   EXPECT_EQ(foo.int_value(), 5);
+  EXPECT_FALSE(scope_var->IsSetButUnused("foo"));
 
+  EXPECT_TRUE(scope_var->IsSetButUnused("bar"));
   Value bar = setup.ExecuteExpression("scope[bar_key]", &err);
   EXPECT_FALSE(err.has_error());
   EXPECT_EQ(bar.type(), Value::INTEGER);
   EXPECT_EQ(bar.int_value(), 8);
+  EXPECT_FALSE(scope_var->IsSetButUnused("bar"));
 
   Value invalid1 = setup.ExecuteExpression("scope[second_element_idx]", &err);
   EXPECT_TRUE(err.has_error());