Support private values in GN.

This adds special handling for variables that begin with underscores, which is inspired by Dart. Such variables are not imported when doing an import, which gives .gni files a way to have intermediate private variables that won't pollute the scopes of the files that include them.

This also applies to the root build config, which can have private values now.

Adds some missing unused variable checks. This was disabled because processing imports would mean all imported variables were unused, and files not using all of them would get unused variable errors. This adds the option to mark merged values on a scope as used, which is used for imported values.

BUG=341738
R=cjhopman@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271078 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 301 insertions, 139 deletions

diff --git a/build/config/BUILDCONFIG.gn b/build/config/BUILDCONFIG.gn
index d4d585a..c1146b7 100644
--- a/build/config/BUILDCONFIG.gn
+++ b/build/config/BUILDCONFIG.gn
@@ -180,10 +180,6 @@ if (is_win) {
 # to each assignment or appending to the sources variable and matches are
 # automatcally removed.
 #
-# We define lists of filters for each platform for all builds so they can
-# be used by individual targets if necessary (a target can always change
-# sources_assignment_filter on itself if it needs something more specific).
-#
 # Note that the patterns are NOT regular expressions. Only "*" and "\b" (path
 # boundary = end of string or slash) are supported, and the entire string
 # muct match the pattern (so you need "*.cc" to match all .cc files, for
@@ -191,102 +187,91 @@ if (is_win) {
 
 # DO NOT ADD MORE PATTERNS TO THIS LIST, see set_sources_assignment_filter call
 # below.
-windows_sources_filters = [
-  "*_win.cc",
-  "*_win.h",
-  "*_win_unittest.cc",
-  "*\bwin/*",
-  "*.rc",
-]
-mac_sources_filters = [
-  "*_mac.h",
-  "*_mac.cc",
-  "*_mac.mm",
-  "*_mac_unittest.h",
-  "*_mac_unittest.cc",
-  "*_mac_unittest.mm",
-  "*\bmac/*",
-  "*_cocoa.h",
-  "*_cocoa.cc",
-  "*_cocoa.mm",
-  "*_cocoa_unittest.h",
-  "*_cocoa_unittest.cc",
-  "*_cocoa_unittest.mm",
-  "*\bcocoa/*",
-]
-ios_sources_filters = [
-  "*_ios.h",
-  "*_ios.cc",
-  "*_ios.mm",
-  "*_ios_unittest.h",
-  "*_ios_unittest.cc",
-  "*_ios_unittest.mm",
-  "*\bios/*",
-]
-objective_c_sources_filters = [
-  "*.mm",
-]
-linux_sources_filters = [
-  "*_linux.h",
-  "*_linux.cc",
-  "*_linux_unittest.h",
-  "*_linux_unittest.cc",
-  "*\blinux/*",
-]
-android_sources_filters = [
-  "*_android.h",
-  "*_android.cc",
-  "*_android_unittest.h",
-  "*_android_unittest.cc",
-  "*\bandroid/*",
-]
-posix_sources_filters = [
-  "*_posix.h",
-  "*_posix.cc",
-  "*_posix_unittest.h",
-  "*_posix_unittest.cc",
-  "*\bposix/*",
-]
-chromeos_sources_filters = [
-  "*_chromeos.h",
-  "*_chromeos.cc",
-  "*_chromeos_unittest.h",
-  "*_chromeos_unittest.cc",
-  "*\bchromeos/*",
-]
-# DO NOT ADD MORE PATTERNS TO THIS LIST, see set_sources_assignment_filter call
-# below.
-
-# Construct the full list of sources we're using for this platform.
 sources_assignment_filter = []
-if (is_win) {
-  sources_assignment_filter += posix_sources_filters
-} else {
-  sources_assignment_filter += windows_sources_filters
+if (!is_posix) {
+  sources_assignment_filter += [
+    "*_posix.h",
+    "*_posix.cc",
+    "*_posix_unittest.h",
+    "*_posix_unittest.cc",
+    "*\bposix/*",
+  ]
+}
+if (!is_win) {
+  sources_assignment_filter += [
+    "*_win.cc",
+    "*_win.h",
+    "*_win_unittest.cc",
+    "*\bwin/*",
+    "*.rc",
+  ]
 }
 if (!is_mac) {
-  sources_assignment_filter += mac_sources_filters
+  sources_assignment_filter += [
+    "*_mac.h",
+    "*_mac.cc",
+    "*_mac.mm",
+    "*_mac_unittest.h",
+    "*_mac_unittest.cc",
+    "*_mac_unittest.mm",
+    "*\bmac/*",
+    "*_cocoa.h",
+    "*_cocoa.cc",
+    "*_cocoa.mm",
+    "*_cocoa_unittest.h",
+    "*_cocoa_unittest.cc",
+    "*_cocoa_unittest.mm",
+    "*\bcocoa/*",
+  ]
 }
 if (!is_ios) {
-  sources_assignment_filter += ios_sources_filters
+  sources_assignment_filter += [
+    "*_ios.h",
+    "*_ios.cc",
+    "*_ios.mm",
+    "*_ios_unittest.h",
+    "*_ios_unittest.cc",
+    "*_ios_unittest.mm",
+    "*\bios/*",
+  ]
 }
 if (!is_mac && !is_ios) {
-  sources_assignment_filter += objective_c_sources_filters
+  sources_assignment_filter += [
+    "*.mm",
+  ]
 }
 if (!is_linux) {
-  sources_assignment_filter += linux_sources_filters
+  sources_assignment_filter += [
+    "*_linux.h",
+    "*_linux.cc",
+    "*_linux_unittest.h",
+    "*_linux_unittest.cc",
+    "*\blinux/*",
+  ]
 }
 if (!is_android) {
-  sources_assignment_filter += android_sources_filters
+  sources_assignment_filter += [
+    "*_android.h",
+    "*_android.cc",
+    "*_android_unittest.h",
+    "*_android_unittest.cc",
+    "*\bandroid/*",
+  ]
 }
 if (!is_chromeos) {
-  sources_assignment_filter += chromeos_sources_filters
+  sources_assignment_filter += [
+    "*_chromeos.h",
+    "*_chromeos.cc",
+    "*_chromeos_unittest.h",
+    "*_chromeos_unittest.cc",
+    "*\bchromeos/*",
+  ]
 }
+# DO NOT ADD MORE PATTERNS TO THIS LIST, see set_sources_assignment_filter call
+# below.
 
 # Actually save this list.
 #
-# DO NOT ADD MORE PATTERNS TO THIS LIST.
-#
 # These patterns are executed for every file in the source tree of every run.
 # Therefore, adding more patterns slows down the build for everybody. We should
 # only add automatic patterns for configurations affecting hundreds of files
@@ -326,7 +311,7 @@ if (!is_clang && (is_asan || is_lsan || is_tsan || is_msan)) {
 
 # Holds all configs used for making native executables and libraries, to avoid
 # duplication in each target below.
-native_compiler_configs = [
+_native_compiler_configs = [
   "//build/config:feature_flags",
 
   "//build/config/compiler:compiler",
@@ -336,25 +321,25 @@ native_compiler_configs = [
   "//build/config/compiler:runtime_library",
 ]
 if (is_win) {
-  native_compiler_configs += [
+  _native_compiler_configs += [
     "//build/config/win:lean_and_mean",
     "//build/config/win:sdk",
     "//build/config/win:unicode",
   ]
 } else if (is_linux) {
-  native_compiler_configs += [ "//build/config/linux:sdk", ]
+  _native_compiler_configs += [ "//build/config/linux:sdk", ]
 } else if (is_mac) {
-  native_compiler_configs += [ "//build/config/mac:sdk", ]
+  _native_compiler_configs += [ "//build/config/mac:sdk", ]
 } else if (is_ios) {
-  native_compiler_configs += [ "//build/config/ios:sdk", ]
+  _native_compiler_configs += [ "//build/config/ios:sdk", ]
 } else if (is_android) {
-  native_compiler_configs += [ "//build/config/android:sdk", ]
+  _native_compiler_configs += [ "//build/config/android:sdk", ]
 }
 if (!is_win) {
-  native_compiler_configs += [ "//build/config/gcc:symbol_visibility_hidden" ]
+  _native_compiler_configs += [ "//build/config/gcc:symbol_visibility_hidden" ]
 }
 if (is_clang) {
-  native_compiler_configs += [
+  _native_compiler_configs += [
     "//build/config/clang:find_bad_constructs",
     "//build/config/clang:extra_warnings",
   ]
@@ -362,13 +347,13 @@ if (is_clang) {
 
 # Optimizations and debug checking.
 if (is_debug) {
-  native_compiler_configs += [ "//build/config:debug" ]
-  default_optimization_config = "//build/config/compiler:no_optimize"
+  _native_compiler_configs += [ "//build/config:debug" ]
+  _default_optimization_config = "//build/config/compiler:no_optimize"
 } else {
-  native_compiler_configs += [ "//build/config:release" ]
-  default_optimization_config = "//build/config/compiler:optimize"
+  _native_compiler_configs += [ "//build/config:release" ]
+  _default_optimization_config = "//build/config/compiler:optimize"
 }
-native_compiler_configs += [ default_optimization_config ]
+_native_compiler_configs += [ _default_optimization_config ]
 
 # Symbol setup.
 if (is_clang && (is_linux || is_android)) {
@@ -376,29 +361,29 @@ if (is_clang && (is_linux || is_android)) {
   # For now, don't create debug information with clang.
   # See http://crbug.com/70000
   # TODO(brettw) This just copies GYP. Why not do this on Mac as well?
-  default_symbols_config = "//build/config/compiler:no_symbols"
+  _default_symbols_config = "//build/config/compiler:no_symbols"
 } else if (symbol_level == 2) {
-  default_symbols_config = "//build/config/compiler:symbols"
+  _default_symbols_config = "//build/config/compiler:symbols"
 } else if (symbol_level == 1) {
-  default_symbols_config = "//build/config/compiler:minimal_symbols"
+  _default_symbols_config = "//build/config/compiler:minimal_symbols"
 } else if (symbol_level == 0) {
-  default_symbols_config = "//build/config/compiler:no_symbols"
+  _default_symbols_config = "//build/config/compiler:no_symbols"
 } else {
   assert(false, "Bad value for symbol_level.")
 }
-native_compiler_configs += [ default_symbols_config ]
+_native_compiler_configs += [ _default_symbols_config ]
 
 # Windows linker setup for EXEs and DLLs.
 if (is_win) {
   if (is_debug) {
-    default_incremental_linking_config =
+    _default_incremental_linking_config =
       "//build/config/win:incremental_linking"
   } else {
-    default_incremental_linking_config =
+    _default_incremental_linking_config =
       "//build/config/win:no_incremental_linking"
   }
-  windows_linker_configs = [
-    default_incremental_linking_config,
+  _windows_linker_configs = [
+    _default_incremental_linking_config,
     "//build/config/win:sdk_link",
     "//build/config/win:common_linker_setup",
     # Default to console-mode apps. Most of our targets are tests and such
@@ -408,11 +393,11 @@ if (is_win) {
 }
 
 set_defaults("executable") {
-  configs = native_compiler_configs + [
+  configs = _native_compiler_configs + [
     "//build/config:default_libs",
   ]
   if (is_win) {
-    configs += windows_linker_configs
+    configs += _windows_linker_configs
   } else if (is_mac) {
     configs += [
       "//build/config/mac:mac_dynamic_flags",
@@ -423,22 +408,22 @@ set_defaults("executable") {
 }
 
 set_defaults("static_library") {
-  configs = native_compiler_configs
+  configs = _native_compiler_configs
 }
 
 set_defaults("shared_library") {
-  configs = native_compiler_configs + [
+  configs = _native_compiler_configs + [
     "//build/config:default_libs",
   ]
   if (is_win) {
-    configs += windows_linker_configs
+    configs += _windows_linker_configs
   } else if (is_mac) {
     configs += [ "//build/config/mac:mac_dynamic_flags" ]
   }
 }
 
 set_defaults("source_set") {
-  configs = native_compiler_configs
+  configs = _native_compiler_configs
 }
 
 # ==============================================================================
diff --git a/tools/gn/function_set_defaults.cc b/tools/gn/function_set_defaults.cc
index ac61fe9..7851113 100644
--- a/tools/gn/function_set_defaults.cc
+++ b/tools/gn/function_set_defaults.cc
@@ -84,7 +84,7 @@ Value RunSetDefaults(Scope* scope,
   // Now copy the values set on the scope we made into the free-floating one
   // (with no containing scope) used to hold the target defaults.
   Scope* dest = scope->MakeTargetDefaults(target_type);
-  block_scope.NonRecursiveMergeTo(dest, false, function,
+  block_scope.NonRecursiveMergeTo(dest, Scope::MergeOptions(), function,
                                   "<SHOULD NOT FAIL>", err);
   return Value();
 }
diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc
index b2787a3..97c2592 100644
--- a/tools/gn/functions.cc
+++ b/tools/gn/functions.cc
@@ -61,8 +61,10 @@ bool FillTargetBlockScope(const Scope* scope,
   // the block in.
   const Scope* default_scope = scope->GetTargetDefaults(target_type);
   if (default_scope) {
-    if (!default_scope->NonRecursiveMergeTo(block_scope, false, function,
-                                            "target defaults", err))
+    Scope::MergeOptions merge_options;
+    merge_options.skip_private_vars = true;
+    if (!default_scope->NonRecursiveMergeTo(block_scope, merge_options,
+                                            function, "target defaults", err))
       return false;
   }
 
@@ -446,6 +448,10 @@ const char kImport_Help[] =
     "  different value), a runtime error will be thrown. Therefore, it's good\n"
     "  practice to minimize the stuff that an imported file defines.\n"
     "\n"
+    "  Variables and templates beginning with an underscore '_' are\n"
+    "  considered private and will not be imported. Imported files can use\n"
+    "  such variables for internal computation without affecting other files.\n"
+    "\n"
     "Examples:\n"
     "\n"
     "  import(\"//build/rules/idl_compilation_rule.gni\")\n"
diff --git a/tools/gn/import_manager.cc b/tools/gn/import_manager.cc
index 4bb4b63..842577c 100644
--- a/tools/gn/import_manager.cc
+++ b/tools/gn/import_manager.cc
@@ -86,6 +86,9 @@ bool ImportManager::DoImport(const SourceFile& file,
     }
   }
 
-  return imported_scope->NonRecursiveMergeTo(scope, false, node_for_err,
+  Scope::MergeOptions options;
+  options.skip_private_vars = true;
+  options.mark_used = true;  // Don't require all imported values be used.
+  return imported_scope->NonRecursiveMergeTo(scope, options, node_for_err,
                                              "import", err);
 }
diff --git a/tools/gn/loader.cc b/tools/gn/loader.cc
index 4cca2ce..32879ea 100644
--- a/tools/gn/loader.cc
+++ b/tools/gn/loader.cc
@@ -245,6 +245,9 @@ void LoaderImpl::BackgroundLoadFile(const Settings* settings,
   if (err.has_error())
     g_scheduler->FailWithError(err);
 
+  if (!our_scope.CheckForUnusedVars(&err))
+    g_scheduler->FailWithError(err);
+
   // Pass all of the items that were defined off to the builder.
   for (size_t i = 0; i < collected_items.size(); i++)
     settings->build_settings()->ItemDefined(collected_items[i]->Pass());
@@ -285,6 +288,11 @@ void LoaderImpl::BackgroundLoadBuildConfig(
   Err err;
   root_block->ExecuteBlockInScope(base_config, &err);
 
+  // Clear all private variables left in the scope. We want the root build
+  // config to be like a .gni file in that variables beginning with an
+  // underscore aren't exported.
+  base_config->RemovePrivateIdentifiers();
+
   trace.Done();
 
   if (err.has_error())
diff --git a/tools/gn/parse_tree.cc b/tools/gn/parse_tree.cc
index 4015fd1..00fca54 100644
--- a/tools/gn/parse_tree.cc
+++ b/tools/gn/parse_tree.cc
@@ -217,7 +217,7 @@ Value BlockNode::Execute(Scope* containing_scope, Err* err) const {
       return Value();
 
     // Check for unused vars in the scope.
-    //our_scope.CheckForUnusedVars(err);
+    our_scope.CheckForUnusedVars(err);
     return ret;
   }
   return ExecuteBlockInScope(containing_scope, err);
diff --git a/tools/gn/parse_tree_unittest.cc b/tools/gn/parse_tree_unittest.cc
index 4f46232..fc06c32 100644
--- a/tools/gn/parse_tree_unittest.cc
+++ b/tools/gn/parse_tree_unittest.cc
@@ -48,3 +48,32 @@ TEST(ParseTree, Accessor) {
   ASSERT_EQ(Value::INTEGER, result.type());
   EXPECT_EQ(kBValue, result.int_value());
 }
+
+TEST(ParseTree, BlockUnusedVars) {
+  TestWithScope setup;
+
+  // Printing both values should be OK.
+  TestParseInput input_all_used(
+      "{\n"
+      "  a = 12\n"
+      "  b = 13\n"
+      "  print(\"$a $b\")\n"
+      "}");
+  EXPECT_FALSE(input_all_used.has_error());
+
+  Err err;
+  input_all_used.parsed()->Execute(setup.scope(), &err);
+  EXPECT_FALSE(err.has_error());
+
+  // Skipping one should throw an unused var error.
+  TestParseInput input_unused(
+      "{\n"
+      "  a = 12\n"
+      "  b = 13\n"
+      "  print(\"$a\")\n"
+      "}");
+  EXPECT_FALSE(input_unused.has_error());
+
+  input_unused.parsed()->Execute(setup.scope(), &err);
+  EXPECT_TRUE(err.has_error());
+}
diff --git a/tools/gn/scope.cc b/tools/gn/scope.cc
index ebaf0ef..4e049eb 100644
--- a/tools/gn/scope.cc
+++ b/tools/gn/scope.cc
@@ -16,6 +16,13 @@ namespace {
 const unsigned kProcessingBuildConfigFlag = 1;
 const unsigned kProcessingImportFlag = 2;
 
+// Returns true if this variable name should be considered private. Private
+// values start with an underscore, and are not imported from "gni" files
+// when processing an import.
+bool IsPrivateVar(const base::StringPiece& name) {
+  return name.empty() || name[0] == '_';
+}
+
 }  // namespace
 
 Scope::Scope(const Settings* settings)
@@ -129,6 +136,21 @@ void Scope::RemoveIdentifier(const base::StringPiece& ident) {
     values_.erase(found);
 }
 
+void Scope::RemovePrivateIdentifiers() {
+  // Do it in two phases to avoid mutating while iterating. Our hash map is
+  // currently backed by several different vendor-specific implementations and
+  // I'm not sure if all of them support mutating while iterating. Since this
+  // is not perf-critical, do the safe thing.
+  std::vector<base::StringPiece> to_remove;
+  for (RecordMap::const_iterator i = values_.begin(); i != values_.end(); ++i) {
+    if (IsPrivateVar(i->first))
+      to_remove.push_back(i->first);
+  }
+
+  for (size_t i = 0; i < to_remove.size(); i++)
+    values_.erase(to_remove[i]);
+}
+
 bool Scope::AddTemplate(const std::string& name, const Template* templ) {
   if (GetTemplate(name))
     return false;
@@ -201,14 +223,17 @@ void Scope::GetCurrentScopeValues(KeyValueMap* output) const {
 }
 
 bool Scope::NonRecursiveMergeTo(Scope* dest,
-                                bool clobber_existing,
+                                const MergeOptions& options,
                                 const ParseNode* node_for_err,
                                 const char* desc_for_err,
                                 Err* err) const {
   // Values.
   for (RecordMap::const_iterator i = values_.begin(); i != values_.end(); ++i) {
+    if (options.skip_private_vars && IsPrivateVar(i->first))
+      continue;  // Skip this private var.
+
     const Value& new_value = i->second.value;
-    if (!clobber_existing) {
+    if (!options.clobber_existing) {
       const Value* existing_value = dest->GetValue(i->first);
       if (existing_value && new_value != *existing_value) {
         // Value present in both the source and the dest.
@@ -225,12 +250,15 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
       }
     }
     dest->values_[i->first] = i->second;
+
+    if (options.mark_used)
+      dest->MarkUsed(i->first);
   }
 
   // Target defaults are owning pointers.
   for (NamedScopeMap::const_iterator i = target_defaults_.begin();
        i != target_defaults_.end(); ++i) {
-    if (!clobber_existing) {
+    if (!options.clobber_existing) {
       if (dest->GetTargetDefaults(i->first)) {
         // TODO(brettw) it would be nice to know the origin of a
         // set_target_defaults so we can give locations for the colliding target
@@ -251,13 +279,13 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
     if (*dest_scope)
       delete *dest_scope;
     *dest_scope = new Scope(settings_);
-    i->second->NonRecursiveMergeTo(*dest_scope, clobber_existing, node_for_err,
+    i->second->NonRecursiveMergeTo(*dest_scope, options, node_for_err,
                                    "<SHOULDN'T HAPPEN>", err);
   }
 
   // Sources assignment filter.
   if (sources_assignment_filter_) {
-    if (!clobber_existing) {
+    if (!options.clobber_existing) {
       if (dest->GetSourcesAssignmentFilter()) {
         // Sources assignment filter present in both the source and the dest.
         std::string desc_string(desc_for_err);
@@ -274,7 +302,10 @@ bool Scope::NonRecursiveMergeTo(Scope* dest,
   // Templates.
   for (TemplateMap::const_iterator i = templates_.begin();
        i != templates_.end(); ++i) {
-    if (!clobber_existing) {
+    if (options.skip_private_vars && IsPrivateVar(i->first))
+      continue;  // Skip this private template.
+
+    if (!options.clobber_existing) {
       const Template* existing_template = dest->GetTemplate(i->first);
       if (existing_template) {
         // Rule present in both the source and the dest.
@@ -314,9 +345,14 @@ scoped_ptr<Scope> Scope::MakeClosure() const {
     result.reset(new Scope(settings_));
   }
 
+  // Want to clobber since we've flattened some nested scopes, and our parent
+  // scope may have a duplicate value set.
+  MergeOptions options;
+  options.clobber_existing = true;
+
   // Add in our variables and we're done.
   Err err;
-  NonRecursiveMergeTo(result.get(), true, NULL, "<SHOULDN'T HAPPEN>", &err);
+  NonRecursiveMergeTo(result.get(), options, NULL, "<SHOULDN'T HAPPEN>", &err);
   DCHECK(!err.has_error());
   return result.Pass();
 }
diff --git a/tools/gn/scope.h b/tools/gn/scope.h
index 53b3252..035d45b 100644
--- a/tools/gn/scope.h
+++ b/tools/gn/scope.h
@@ -65,6 +65,37 @@ class Scope {
     Scope* 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_used(false) {
+    }
+
+    // When set, all existing avlues in the destination scope will be
+    // overwritten.
+    //
+    // When false, it will be an error to merge a variable into another scope
+    // where a variable with the same name is already set. The exception is
+    // if both of the variables have the same value (which happens if you
+    // somehow multiply import the same file, for example). This case will be
+    // ignored since there is nothing getting lost.
+    bool clobber_existing;
+
+    // When true, private variables (names beginning with an underscore) will
+    // be copied to the destination scope. When false, private values will be
+    // skipped.
+    bool skip_private_vars;
+
+    // When set, values copied to the destination scope will be marked as used
+    // so won't trigger an unused variable warning. You want this when doing an
+    // import, for example, or files that don't need a variable from the .gni
+    // file will throw an error.
+    bool mark_used;
+  };
+
   // Creates an empty toplevel scope.
   Scope(const Settings* settings);
 
@@ -138,6 +169,10 @@ class Scope {
   // scope. This does not search recursive scopes. Does nothing if not found.
   void RemoveIdentifier(const base::StringPiece& ident);
 
+  // Removes from this scope all identifiers and templates that are considered
+  // private.
+  void RemovePrivateIdentifiers();
+
   // Templates associated with this scope. A template can only be set once, so
   // AddTemplate will fail and return false if a rule with that name already
   // exists. GetTemplate returns NULL if the rule doesn't exist, and it will
@@ -169,30 +204,22 @@ class Scope {
   // copied, neither will the reference to the containing scope (this is why
   // it's "non-recursive").
   //
-  // If clobber_existing is true, any existing values will be overwritten. In
-  // this mode, this function will never fail.
-  //
-  // If clobber_existing is false, it will be an error to merge a variable into
-  // a scope that already has something with that name in scope (meaning in
-  // that scope or in any of its containing scopes). If this happens, the error
-  // will be set and the function will return false.
-  //
   // This is used in different contexts. When generating the error, the given
   // parse node will be blamed, and the given desc will be used to describe
   // the operation that doesn't support doing this. For example, desc_for_err
   // would be "import" when doing an import, and the error string would say
   // something like "The import contains...".
   bool NonRecursiveMergeTo(Scope* dest,
-                           bool clobber_existing,
+                           const MergeOptions& options,
                            const ParseNode* node_for_err,
                            const char* desc_for_err,
                            Err* err) const;
 
   // Constructs a scope that is a copy of the current one. Nested scopes will
-  // be collapsed until we reach a const containing scope. The resulting
-  // closure will reference the const containing scope as its containing scope
-  // (since we assume the const scope won't change, we don't have to copy its
-  // values).
+  // be collapsed until we reach a const containing scope. Private values will
+  // be included. The resulting closure will reference the const containing
+  // scope as its containing scope (since we assume the const scope won't
+  // change, we don't have to copy its values).
   scoped_ptr<Scope> MakeClosure() const;
 
   // Makes an empty scope with the given name. Returns NULL if the name is
diff --git a/tools/gn/scope_unittest.cc b/tools/gn/scope_unittest.cc
index 5585e50..53d4689 100644
--- a/tools/gn/scope_unittest.cc
+++ b/tools/gn/scope_unittest.cc
@@ -36,6 +36,8 @@ TEST(Scope, NonRecursiveMergeTo) {
 
   Value old_value(&assignment, "hello");
   setup.scope()->SetValue("v", old_value, &assignment);
+  base::StringPiece private_var_name("_private");
+  setup.scope()->SetValue(private_var_name, old_value, &assignment);
 
   // Detect collisions of values' values.
   {
@@ -45,7 +47,8 @@ TEST(Scope, NonRecursiveMergeTo) {
 
     Err err;
     EXPECT_FALSE(setup.scope()->NonRecursiveMergeTo(
-        &new_scope, false, &assignment, "error", &err));
+        &new_scope, Scope::MergeOptions(),
+        &assignment, "error", &err));
     EXPECT_TRUE(err.has_error());
   }
 
@@ -56,8 +59,10 @@ TEST(Scope, NonRecursiveMergeTo) {
     new_scope.SetValue("v", new_value, &assignment);
 
     Err err;
+    Scope::MergeOptions options;
+    options.clobber_existing = true;
     EXPECT_TRUE(setup.scope()->NonRecursiveMergeTo(
-        &new_scope, true, &assignment, "error", &err));
+        &new_scope, options, &assignment, "error", &err));
     EXPECT_FALSE(err.has_error());
 
     const Value* found_value = new_scope.GetValue("v");
@@ -72,8 +77,61 @@ TEST(Scope, NonRecursiveMergeTo) {
     new_scope.SetValue("v", new_value, &assignment);
 
     Err err;
+    Scope::MergeOptions options;
+    options.clobber_existing = true;
     EXPECT_TRUE(setup.scope()->NonRecursiveMergeTo(
-        &new_scope, false, &assignment, "error", &err));
+        &new_scope, options, &assignment, "error", &err));
+    EXPECT_FALSE(err.has_error());
+  }
+
+  // Copy private values.
+  {
+    Scope new_scope(setup.settings());
+
+    Err err;
+    EXPECT_TRUE(setup.scope()->NonRecursiveMergeTo(
+        &new_scope, Scope::MergeOptions(), &assignment, "error", &err));
+    EXPECT_FALSE(err.has_error());
+    EXPECT_TRUE(new_scope.GetValue(private_var_name));
+  }
+
+  // Skip private values.
+  {
+    Scope new_scope(setup.settings());
+
+    Err err;
+    Scope::MergeOptions options;
+    options.skip_private_vars = true;
+    EXPECT_TRUE(setup.scope()->NonRecursiveMergeTo(
+        &new_scope, options, &assignment, "error", &err));
+    EXPECT_FALSE(err.has_error());
+    EXPECT_FALSE(new_scope.GetValue(private_var_name));
+  }
+
+  // Don't mark used.
+  {
+    Scope new_scope(setup.settings());
+
+    Err err;
+    Scope::MergeOptions options;
+    EXPECT_TRUE(setup.scope()->NonRecursiveMergeTo(
+        &new_scope, options, &assignment, "error", &err));
+    EXPECT_FALSE(err.has_error());
+    EXPECT_FALSE(new_scope.CheckForUnusedVars(&err));
+    EXPECT_TRUE(err.has_error());
+  }
+
+  // Mark used.
+  {
+    Scope new_scope(setup.settings());
+
+    Err err;
+    Scope::MergeOptions options;
+    options.mark_used = true;
+    EXPECT_TRUE(setup.scope()->NonRecursiveMergeTo(
+        &new_scope, options, &assignment, "error", &err));
+    EXPECT_FALSE(err.has_error());
+    EXPECT_TRUE(new_scope.CheckForUnusedVars(&err));
     EXPECT_FALSE(err.has_error());
   }
 }
@@ -167,3 +225,13 @@ TEST(Scope, GetMutableValue) {
   ASSERT_TRUE(mutable2_result);
   EXPECT_TRUE(*mutable2_result == value);
 }
+
+TEST(Scope, RemovePrivateIdentifiers) {
+  TestWithScope setup;
+  setup.scope()->SetValue("a", Value(NULL, true), NULL);
+  setup.scope()->SetValue("_b", Value(NULL, true), NULL);
+
+  setup.scope()->RemovePrivateIdentifiers();
+  EXPECT_TRUE(setup.scope()->GetValue("a"));
+  EXPECT_FALSE(setup.scope()->GetValue("_b"));
+}