Support for excluding variable from forwarding via forward_variables_form.

Some templates wants to forward all their parameters to the underlying
target except a select few they use. Previously they had to list all
the variables the underlying target supported which was error prone and
brittle if the underlying target is changed.

With this change they can just use the following:

  forward_variables_from(invoker, "*", ["my_extra_variable"])
  # my_extra_variable is not forwarded, all other variables are.

BUG=580293

Review URL: https://codereview.chromium.org/1632573002

Cr-Commit-Position: refs/heads/master@{#371492}

tools/gn/docs/reference.md

@@ -1327,7 +1327,8 @@
 ## **forward_variables_from**: Copies variables from a different scope.
 
 ```
-  forward_variables_from(from_scope, variable_list_or_star)
+  forward_variables_from(from_scope, variable_list_or_star,
+                         variable_to_not_forward_list = [])
 
   Copies the given variables from the given scope to the local scope
   if they exist. This is normally used in the context of templates to
@@ -1354,6 +1355,10 @@
   is never applied by this function. It's assumed than any desired
   filtering was already done when sources was set on the from_scope.
 
+  If variables_to_not_forward_list is non-empty, then it must contains
+  a list of variable names that will not be forwarded. This is mostly
+  useful when variable_list_or_star has a value of "*".
+
 ```
 
 ### **Examples**
@@ -1383,7 +1388,19 @@
     target(my_wrapper_target_type, target_name) {
       forward_variables_from(invoker, "*")
     }
- }
+  }
+
+  # A template that wraps another. It adds behavior based on one
+  # variable, and forwards all others to the nested target.
+  template("my_ios_test_app") {
+    ios_test_app(target_name) {
+      forward_variables_from(invoker, "*", ["test_bundle_name"])
+      if (!defined(extra_substitutions)) {
+        extra_substitutions = []
+      }
+      extra_substitutions += [ "BUNDLE_ID_TEST_NAME=$test_bundle_name" ]
+    }
+  }
 
 
 ```

tools/gn/function_forward_variables_from.cc

@@ -14,6 +14,7 @@ namespace {
 void ForwardAllValues(const FunctionCallNode* function,
                       Scope* source,
                       Scope* dest,
+                      const std::set<std::string>& exclusion_set,
                       Err* err) {
   Scope::MergeOptions options;
   // This function needs to clobber existing for it to be useful. It will be
@@ -23,6 +24,7 @@ void ForwardAllValues(const FunctionCallNode* function,
   options.clobber_existing = true;
   options.skip_private_vars = true;
   options.mark_dest_used = false;
+  options.excluded_values = exclusion_set;
   source->NonRecursiveMergeTo(dest, options, function,
                               "source scope", err);
   source->MarkAllUsed();
@@ -31,10 +33,13 @@ void ForwardAllValues(const FunctionCallNode* function,
 void ForwardValuesFromList(Scope* source,
                            Scope* dest,
                            const std::vector<Value>& list,
+                           const std::set<std::string>& exclusion_set,
                            Err* err) {
   for (const Value& cur : list) {
     if (!cur.VerifyTypeIs(Value::STRING, err))
       return;
+    if (exclusion_set.find(cur.string_value()) != exclusion_set.end())
+      continue;
     const Value* value = source->GetValue(cur.string_value(), true);
     if (value) {
       // Use the storage key for the original value rather than the string in
@@ -66,7 +71,8 @@ const char kForwardVariablesFrom_HelpShort[] =
 const char kForwardVariablesFrom_Help[] =
     "forward_variables_from: Copies variables from a different scope.\n"
     "\n"
-    "  forward_variables_from(from_scope, variable_list_or_star)\n"
+    "  forward_variables_from(from_scope, variable_list_or_star,\n"
+    "                         variable_to_not_forward_list = [])\n"
     "\n"
     "  Copies the given variables from the given scope to the local scope\n"
     "  if they exist. This is normally used in the context of templates to\n"
@@ -94,6 +100,10 @@ const char kForwardVariablesFrom_Help[] =
     "  is never applied by this function. It's assumed than any desired\n"
     "  filtering was already done when sources was set on the from_scope.\n"
     "\n"
+    "  If variables_to_not_forward_list is non-empty, then it must contains\n"
+    "  a list of variable names that will not be forwarded. This is mostly\n"
+    "  useful when variable_list_or_star has a value of \"*\".\n"
+    "\n"
     "Examples\n"
     "\n"
     "  # This is a common action template. It would invoke a script with\n"
@@ -121,7 +131,20 @@ const char kForwardVariablesFrom_Help[] =
     "    target(my_wrapper_target_type, target_name) {\n"
     "      forward_variables_from(invoker, \"*\")\n"
     "    }\n"
-    " }\n";
+    "  }\n"
+    "\n"
+    "  # A template that wraps another. It adds behavior based on one \n"
+    "  # variable, and forwards all others to the nested target.\n"
+    "  template(\"my_ios_test_app\") {\n"
+    "    ios_test_app(target_name) {\n"
+    "      forward_variables_from(invoker, \"*\", [\"test_bundle_name\"])\n"
+    "      if (!defined(extra_substitutions)) {\n"
+    "        extra_substitutions = []\n"
+    "      }\n"
+    "      extra_substitutions += [ \"BUNDLE_ID_TEST_NAME=$test_bundle_name\" "
+                                                                          "]\n"
+    "    }\n"
+    "  }\n";
 
 // This function takes a ListNode rather than a resolved vector of values
 // both avoid copying the potentially-large source scope, and so the variables
@@ -131,9 +154,9 @@ Value RunForwardVariablesFrom(Scope* scope,
                               const ListNode* args_list,
                               Err* err) {
   const std::vector<const ParseNode*>& args_vector = args_list->contents();
-  if (args_vector.size() != 2) {
+  if (args_vector.size() != 2 && args_vector.size() != 3) {
     *err = Err(function, "Wrong number of arguments.",
-               "Expecting exactly two.");
+               "Expecting two or three arguments.");
     return Value();
   }
 
@@ -157,18 +180,40 @@ Value RunForwardVariablesFrom(Scope* scope,
     return Value();
   Scope* source = value->scope_value();
 
+  // Extract the exclusion list if defined.
+  std::set<std::string> exclusion_set;
+  if (args_vector.size() == 3) {
+    Value exclusion_value = args_vector[2]->Execute(scope, err);
+    if (err->has_error())
+      return Value();
+
+    if (exclusion_value.type() != Value::LIST) {
+      *err = Err(exclusion_value, "Not a valid list of variables to exclude.",
+                 "Expecting a list of strings.");
+      return Value();
+    }
+
+    for (const Value& cur : exclusion_value.list_value()) {
+      if (!cur.VerifyTypeIs(Value::STRING, err))
+        return Value();
+
+      exclusion_set.insert(cur.string_value());
+    }
+  }
+
   // Extract the list. If all_values is not set, the what_value will be a list.
   Value what_value = args_vector[1]->Execute(scope, err);
   if (err->has_error())
     return Value();
   if (what_value.type() == Value::STRING) {
     if (what_value.string_value() == "*") {
-      ForwardAllValues(function, source, scope, err);
+      ForwardAllValues(function, source, scope, exclusion_set, err);
       return Value();
     }
   } else {
     if (what_value.type() == Value::LIST) {
-      ForwardValuesFromList(source, scope, what_value.list_value(), err);
+      ForwardValuesFromList(source, scope, what_value.list_value(),
+                            exclusion_set, err);
       return Value();
     }
   }

tools/gn/function_forward_variables_from_unittest.cc

@@ -32,6 +32,34 @@ TEST(FunctionForwardVariablesFrom, List) {
   setup.print_output().clear();
 }
 
+TEST(FunctionForwardVariablesFrom, ListWithExclusion) {
+  Scheduler scheduler;
+  TestWithScope setup;
+
+  // Defines a template and copy the two x and y, and z values out.
+  TestParseInput input(
+    "template(\"a\") {\n"
+    "  forward_variables_from(invoker, [\"x\", \"y\", \"z\"], [\"z\"])\n"
+    "  assert(!defined(z))\n"  // "z" should still be undefined.
+    "  print(\"$target_name, $x, $y\")\n"
+    "}\n"
+    "a(\"target\") {\n"
+    "  x = 1\n"
+    "  y = 2\n"
+    "  z = 3\n"
+    "  print(\"$z\")\n"
+    "}\n");
+
+  ASSERT_FALSE(input.has_error());
+
+  Err err;
+  input.parsed()->Execute(setup.scope(), &err);
+  ASSERT_FALSE(err.has_error()) << err.message();
+
+  EXPECT_EQ("3\ntarget, 1, 2\n", setup.print_output());
+  setup.print_output().clear();
+}
+
 TEST(FunctionForwardVariablesFrom, ErrorCases) {
   Scheduler scheduler;
   TestWithScope setup;
@@ -65,19 +93,61 @@ TEST(FunctionForwardVariablesFrom, ErrorCases) {
   EXPECT_TRUE(err.has_error());
   EXPECT_EQ("Not a valid list of variables to copy.", err.message());
 
+  // Type check the exclusion list.
+  TestParseInput invalid_exclusion_list(
+    "template(\"c\") {\n"
+    "  forward_variables_from(invoker, \"*\", 42)\n"
+    "  print(\"$target_name\")\n"
+    "}\n"
+    "c(\"target\") {\n"
+    "}\n");
+  ASSERT_FALSE(invalid_exclusion_list.has_error());
+  err = Err();
+  invalid_exclusion_list.parsed()->Execute(setup.scope(), &err);
+  EXPECT_TRUE(err.has_error());
+  EXPECT_EQ("Not a valid list of variables to exclude.", err.message());
+
   // Programmatic values should error.
   TestParseInput prog(
-    "template(\"c\") {\n"
+    "template(\"d\") {\n"
     "  forward_variables_from(invoker, [\"root_out_dir\"])\n"
     "  print(\"$target_name\")\n"
     "}\n"
-    "c(\"target\") {\n"
+    "d(\"target\") {\n"
     "}\n");
   ASSERT_FALSE(prog.has_error());
   err = Err();
   prog.parsed()->Execute(setup.scope(), &err);
   EXPECT_TRUE(err.has_error());
   EXPECT_EQ("This value can't be forwarded.", err.message());
+
+  // Not enough arguments.
+  TestParseInput not_enough_arguments(
+    "template(\"e\") {\n"
+    "  forward_variables_from(invoker)\n"
+    "  print(\"$target_name\")\n"
+    "}\n"
+    "e(\"target\") {\n"
+    "}\n");
+  ASSERT_FALSE(not_enough_arguments.has_error());
+  err = Err();
+  not_enough_arguments.parsed()->Execute(setup.scope(), &err);
+  EXPECT_TRUE(err.has_error());
+  EXPECT_EQ("Wrong number of arguments.", err.message());
+
+  // Too many arguments.
+  TestParseInput too_many_arguments(
+    "template(\"f\") {\n"
+    "  forward_variables_from(invoker, \"*\", [], [])\n"
+    "  print(\"$target_name\")\n"
+    "}\n"
+    "f(\"target\") {\n"
+    "}\n");
+  ASSERT_FALSE(too_many_arguments.has_error());
+  err = Err();
+  too_many_arguments.parsed()->Execute(setup.scope(), &err);
+  EXPECT_TRUE(err.has_error());
+  EXPECT_EQ("Wrong number of arguments.", err.message());
 }
 
 TEST(FunctionForwardVariablesFrom, Star) {
@@ -106,3 +176,33 @@ TEST(FunctionForwardVariablesFrom, Star) {
   EXPECT_EQ("target, 1, 2\n", setup.print_output());
   setup.print_output().clear();
 }
+
+
+TEST(FunctionForwardVariablesFrom, StarWithExclusion) {
+  Scheduler scheduler;
+  TestWithScope setup;
+
+  // Defines a template and copy all values except z value. The "*" behavior
+  // should clobber existing variables with the same name.
+  TestParseInput input(
+    "template(\"a\") {\n"
+    "  x = 1000000\n"  // Should be clobbered.
+    "  forward_variables_from(invoker, \"*\", [\"z\"])\n"
+    "  print(\"$target_name, $x, $y\")\n"
+    "}\n"
+    "a(\"target\") {\n"
+    "  x = 1\n"
+    "  y = 2\n"
+    "  z = 3\n"
+    "  print(\"$z\")\n"
+    "}\n");
+
+  ASSERT_FALSE(input.has_error());
+
+  Err err;
+  input.parsed()->Execute(setup.scope(), &err);
+  ASSERT_FALSE(err.has_error()) << err.message();
+
+  EXPECT_EQ("3\ntarget, 1, 2\n", setup.print_output());
+  setup.print_output().clear();
+}

tools/gn/scope.cc

@@ -25,6 +25,15 @@ bool IsPrivateVar(const base::StringPiece& name) {
 
 }  // namespace
 
+// Defaults to all false, which are the things least likely to cause errors.
+Scope::MergeOptions::MergeOptions()
+    : clobber_existing(false),
+      skip_private_vars(false),
+      mark_dest_used(false) {
+}
+
+Scope::MergeOptions::~MergeOptions() {
+}
 
 Scope::ProgrammaticProvider::~ProgrammaticProvider() {
   scope_->RemoveProvider(this);
@@ -249,17 +258,23 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
                                 Err* err) const {
   // Values.
   for (const auto& pair : values_) {
-    if (options.skip_private_vars && IsPrivateVar(pair.first))
+    const base::StringPiece& current_name = pair.first;
+    if (options.skip_private_vars && IsPrivateVar(current_name))
       continue;  // Skip this private var.
+    if (!options.excluded_values.empty() &&
+        options.excluded_values.find(current_name.as_string()) !=
+            options.excluded_values.end()) {
+      continue;  // Skip this excluded value.
+    }
 
     const Value& new_value = pair.second.value;
     if (!options.clobber_existing) {
-      const Value* existing_value = dest->GetValue(pair.first);
+      const Value* existing_value = dest->GetValue(current_name);
       if (existing_value && new_value != *existing_value) {
         // Value present in both the source and the dest.
         std::string desc_string(desc_for_err);
         *err = Err(node_for_err, "Value collision.",
-            "This " + desc_string + " contains \"" + pair.first.as_string() +
+            "This " + desc_string + " contains \"" + current_name.as_string() +
             "\"");
         err->AppendSubErr(Err(pair.second.value, "defined here.",
             "Which would clobber the one in your current scope"));
@@ -269,23 +284,30 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
         return false;
       }
     }
-    dest->values_[pair.first] = pair.second;
+    dest->values_[current_name] = pair.second;
 
     if (options.mark_dest_used)
-      dest->MarkUsed(pair.first);
+      dest->MarkUsed(current_name);
   }
 
   // Target defaults are owning pointers.
   for (const auto& pair : target_defaults_) {
+    const std::string& current_name = pair.first;
+    if (!options.excluded_values.empty() &&
+        options.excluded_values.find(current_name) !=
+            options.excluded_values.end()) {
+      continue;  // Skip the excluded value.
+    }
+
     if (!options.clobber_existing) {
-      if (dest->GetTargetDefaults(pair.first)) {
+      if (dest->GetTargetDefaults(current_name)) {
         // TODO(brettw) it would be nice to know the origin of a
         // set_target_defaults so we can give locations for the colliding target
         // defaults.
         std::string desc_string(desc_for_err);
         *err = Err(node_for_err, "Target defaults collision.",
             "This " + desc_string + " contains target defaults for\n"
-            "\"" + pair.first + "\" which would clobber one for the\n"
+            "\"" + current_name + "\" which would clobber one for the\n"
             "same target type in your current scope. It's unfortunate that I'm "
             "too stupid\nto tell you the location of where the target defaults "
             "were set. Usually\nthis happens in the BUILDCONFIG.gn file.");
@@ -294,7 +316,7 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
     }
 
     // Be careful to delete any pointer we're about to clobber.
-    Scope** dest_scope = &dest->target_defaults_[pair.first];
+    Scope** dest_scope = &dest->target_defaults_[current_name];
     if (*dest_scope)
       delete *dest_scope;
     *dest_scope = new Scope(settings_);
@@ -320,11 +342,17 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
 
   // Templates.
   for (const auto& pair : templates_) {
-    if (options.skip_private_vars && IsPrivateVar(pair.first))
+    const std::string& current_name = pair.first;
+    if (options.skip_private_vars && IsPrivateVar(current_name))
       continue;  // Skip this private template.
+    if (!options.excluded_values.empty() &&
+        options.excluded_values.find(current_name) !=
+            options.excluded_values.end()) {
+      continue;  // Skip the excluded value.
+    }
 
     if (!options.clobber_existing) {
-      const Template* existing_template = dest->GetTemplate(pair.first);
+      const Template* existing_template = dest->GetTemplate(current_name);
       // Since templates are refcounted, we can check if it's the same one by
       // comparing pointers.
       if (existing_template && pair.second.get() != existing_template) {
@@ -333,7 +361,7 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
         std::string desc_string(desc_for_err);
         *err = Err(node_for_err, "Template collision.",
             "This " + desc_string + " contains a template \"" +
-            pair.first + "\"");
+            current_name + "\"");
         err->AppendSubErr(Err(pair.second->GetDefinitionRange(),
             "defined here.",
             "Which would clobber the one in your current scope"));
@@ -346,7 +374,7 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
     }
 
     // Be careful to delete any pointer we're about to clobber.
-    dest->templates_[pair.first] = pair.second;
+    dest->templates_[current_name] = pair.second;
   }
 
   return true;

tools/gn/scope.h

@@ -65,12 +65,8 @@ class Scope {
 
   // Options for configuring scope merges.
   struct MergeOptions {
-    // Defaults to all false, which are the things least likely to cause errors.
-    MergeOptions()
-        : clobber_existing(false),
-          skip_private_vars(false),
-          mark_dest_used(false) {
-    }
+    MergeOptions();
+    ~MergeOptions();
 
     // When set, all existing avlues in the destination scope will be
     // overwritten.
@@ -92,6 +88,9 @@ class Scope {
     // import, for example, or files that don't need a variable from the .gni
     // file will throw an error.
     bool mark_dest_used;
+
+    // When set, those variables are not merged.
+    std::set<std::string> excluded_values;
   };
 
   // Creates an empty toplevel scope.