GN: forward_variables_from shouldn't clobber vars.
The documentation for forward_variables_from specifies that it will give an error if the variable already exists in the target scope. But this was not implemented. Instead, the value would be silently overwritten.
This change implements the error, and fixes the times this happens in the Linux and Android builds.
Review-Url: https://codereview.chromium.org/1943583002
Cr-Original-Commit-Position: refs/heads/master@{#391136}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 0785c950dce7221b10c21e911795e181076b90db
diff --git a/tools/gn/function_forward_variables_from.cc b/tools/gn/function_forward_variables_from.cc
index dd3629b..8f3b90d 100644
--- a/tools/gn/function_forward_variables_from.cc
+++ b/tools/gn/function_forward_variables_from.cc
@@ -54,6 +54,18 @@
return;
}
+ // Don't allow clobbering existing values.
+ const Value* existing_value = dest->GetValue(storage_key);
+ if (existing_value) {
+ *err = Err(cur, "Clobbering existing value.",
+ "The current scope already defines a value \"" +
+ cur.string_value() + "\".\nforward_variables_from() won't clobber "
+ "existing values. If you want to\nmerge lists, you'll need to "
+ "do this explicitly.");
+ err->AppendSubErr(Err(*existing_value, "value being clobbered."));
+ return;
+ }
+
// Keep the origin information from the original value. The normal
// usage is for this to be used in a template, and if there's an error,
// the user expects to see the line where they set the variable
diff --git a/tools/gn/function_forward_variables_from_unittest.cc b/tools/gn/function_forward_variables_from_unittest.cc
index e5137a4..2e6f5a2 100644
--- a/tools/gn/function_forward_variables_from_unittest.cc
+++ b/tools/gn/function_forward_variables_from_unittest.cc
@@ -8,28 +8,46 @@
TEST(FunctionForwardVariablesFrom, List) {
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\"])\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"
- "}\n");
-
- ASSERT_FALSE(input.has_error());
-
Err err;
- input.parsed()->Execute(setup.scope(), &err);
- ASSERT_FALSE(err.has_error()) << err.message();
+ std::string program =
+ "template(\"a\") {\n"
+ " forward_variables_from(invoker, [\"x\", \"y\", \"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"
+ "}\n";
- EXPECT_EQ("target, 1, 2\n", setup.print_output());
- setup.print_output().clear();
+ {
+ TestWithScope setup;
+
+ // Defines a template and copy the two x and y, and z values out.
+ TestParseInput input(program);
+ ASSERT_FALSE(input.has_error());
+
+ input.parsed()->Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error()) << err.message();
+
+ EXPECT_EQ("target, 1, 2\n", setup.print_output());
+ setup.print_output().clear();
+ }
+
+ {
+ TestWithScope setup;
+
+ // Test that the same input but forwarding a variable with the name of
+ // something in the given scope throws an error rather than clobbering it.
+ // This uses the same known-good program as before, but adds another
+ // variable in the scope before it.
+ TestParseInput clobber("x = 1\n" + program);
+ ASSERT_FALSE(clobber.has_error());
+
+ clobber.parsed()->Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error()); // Should thow a clobber error.
+ EXPECT_EQ("Clobbering existing value.", err.message());
+ }
}
TEST(FunctionForwardVariablesFrom, ListWithExclusion) {
