GN: Mark all variables used when defining a template.
This exempts file-scoped values from used variable checking when a template is
defined. This avoids spurious unused variable warnings at the expense of
potentially missing unused variables in some cases. These warnings have tripped
people up multiple times.
The potential for missing actual unused values is pretty small, however,
because most templates are defined in .gni files that don't have used variable
checking (since they could be included into many different contexts).
BUG=395883
Review-Url: https://codereview.chromium.org/2233893005
Cr-Original-Commit-Position: refs/heads/master@{#411265}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: ba1286d0e720ef723192e4a2af7e99d0516495d6
diff --git a/tools/gn/BUILD.gn b/tools/gn/BUILD.gn
index 56f3070..e3a21ff 100644
--- a/tools/gn/BUILD.gn
+++ b/tools/gn/BUILD.gn
@@ -289,6 +289,7 @@
"function_get_target_outputs_unittest.cc",
"function_process_file_template_unittest.cc",
"function_rebase_path_unittest.cc",
+ "function_template_unittest.cc",
"function_toolchain_unittest.cc",
"function_write_file_unittest.cc",
"functions_target_unittest.cc",
diff --git a/tools/gn/function_template.cc b/tools/gn/function_template.cc
index 17dda06..6136f41 100644
--- a/tools/gn/function_template.cc
+++ b/tools/gn/function_template.cc
@@ -192,6 +192,25 @@
}
scope->AddTemplate(template_name, new Template(scope, function));
+
+ // The template object above created a closure around the variables in the
+ // current scope. The template code will execute in that context when it's
+ // invoked. But this means that any variables defined above that are used
+ // by the template won't get marked used just by defining the template. The
+ // result can be spurious unused variable errors.
+ //
+ // The "right" thing to do would be to walk the syntax tree inside the
+ // template, find all identifier references, and mark those variables used.
+ // This is annoying and error-prone to implement and takes extra time to run
+ // for this narrow use case.
+ //
+ // Templates are most often defined in .gni files which don't get
+ // used-variable checking anyway, and this case is annoying enough that the
+ // incremental value of unused variable checking isn't worth the
+ // alternatives. So all values in scope before this template definition are
+ // exempted from unused variable checking.
+ scope->MarkAllUsed();
+
return Value();
}
diff --git a/tools/gn/function_template_unittest.cc b/tools/gn/function_template_unittest.cc
new file mode 100644
index 0000000..cd98423
--- /dev/null
+++ b/tools/gn/function_template_unittest.cc
@@ -0,0 +1,29 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "tools/gn/test_with_scope.h"
+
+// Checks that variables used inside template definitions aren't reported
+// unused if they were declared above the template.
+TEST(FunctionTemplate, MarkUsed) {
+ TestWithScope setup;
+ TestParseInput input(
+ "a = 1\n" // Unused outside of template.
+ "template(\"templ\") {\n"
+ " print(a)\n"
+ "}\n");
+ ASSERT_FALSE(input.has_error()) << input.parse_err().message();
+
+ Err err;
+ input.parsed()->Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error()) << err.message();
+
+ // Normally the loader calls CheckForUnusedVars() when it loads a file
+ // since normal blocks don't do this check. To avoid having to make this
+ // test much more complicated, just explicitly do the check to make sure
+ // things are marked properly.
+ setup.scope()->CheckForUnusedVars(&err);
+ EXPECT_FALSE(err.has_error());
+}
