Change GN to disallow reading args defined in the same declare_args() call.
If you have a declare_args() block like:
declare_args() {
use_foo = true
use_bar = use_foo
}
currently this will probably produce behavior different than you might
expect. In the normal case (where you don't use use_foo=false in args.gn),
both use_foo and use_bar will bet set to true, but if you do set
use_foo=false in args.gn, use_foo will be set to false, but use_bar
will still be set to true. This happens because GN evaluates the value
of foo inside the block *before* it looks for arg overrides.
We've decided that this is bad enough to flat-out disallow. Now if you
try to do this GN will just return an error. If you want to have one
declared arg depend on the value of another, you need to put them in
separate declare_arg() calls.
This is changing the behavior of GN in a way that may break existing
builds, but hopefully that will happen only rarely and if it does will
expose the surprises that might otherwise be lurking undetected.
In the Chromium build, it turned out we only did this a few times and
they were all unintentional, so this correctly found bugs.
R=brettw@chromium.org, kjellander@chromium.org
BUG=542846
Review-Url: https://codereview.chromium.org/2509333003
Cr-Original-Commit-Position: refs/heads/master@{#433944}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: f6fd4d747c77e1c7968eb5af8b195a94a7ed2fac
diff --git a/tools/gn/docs/reference.md b/tools/gn/docs/reference.md
index 1aa56f1..ab3233a 100644
--- a/tools/gn/docs/reference.md
+++ b/tools/gn/docs/reference.md
@@ -1641,8 +1641,9 @@
The precise behavior of declare args is:
- 1. The declare_arg block executes. Any variables in the enclosing scope are
- available for reading.
+ 1. The declare_args() block executes. Any variable defined in the enclosing
+ scope is available for reading, but any variable defined earlier in
+ the current scope is not (since the overrides haven't been applied yet).
2. At the end of executing the block, any variables set within that scope
are saved globally as build arguments, with their current values being
@@ -1661,12 +1662,10 @@
like [], "", or -1, and after the declare_args block, call exec_script if
the value is unset by the user.
- - Any code inside of the declare_args block will see the default values of
- previous variables defined in the block rather than the user-overridden
- value. This can be surprising because you will be used to seeing the
- overridden value. If you need to make the default value of one arg
- dependent on the possibly-overridden value of another, write two separate
- declare_args blocks:
+ - Because you cannot read the value of a variable defined in the same
+ block, if you need to make the default value of one arg depend
+ on the possibly-overridden value of another, write two separate
+ declare_args() blocks:
declare_args() {
enable_foo = true
diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc
index e01a25c..41be19b 100644
--- a/tools/gn/functions.cc
+++ b/tools/gn/functions.cc
@@ -45,8 +45,38 @@
return false;
}
+// This key is set as a scope property on the scope of a declare_args() block,
+// in order to prevent reading a variable defined earlier in the same call
+// (see `gn help declare_args` for more).
+const void *kInDeclareArgsKey = nullptr;
+
} // namespace
+
+bool EnsureNotReadingFromSameDeclareArgs(const ParseNode* node,
+ const Scope* cur_scope,
+ const Scope* val_scope,
+ Err* err) {
+ // If the value didn't come from a scope at all, we're safe.
+ if (!val_scope)
+ return true;
+
+ const Scope* val_args_scope = nullptr;
+ val_scope->GetProperty(&kInDeclareArgsKey, &val_args_scope);
+
+ const Scope* cur_args_scope = nullptr;
+ cur_scope->GetProperty(&kInDeclareArgsKey, &cur_args_scope);
+ if (!val_args_scope || !cur_args_scope || (val_args_scope != cur_args_scope))
+ return true;
+
+ *err = Err(node,
+ "Reading a variable defined in the same declare_args() call.\n"
+ "\n"
+ "If you need to set the value of one arg based on another, put\n"
+ "them in two separate declare_args() calls, one after the other.\n");
+ return false;
+}
+
bool EnsureNotProcessingImport(const ParseNode* node,
const Scope* scope,
Err* err) {
@@ -357,8 +387,9 @@
The precise behavior of declare args is:
- 1. The declare_arg block executes. Any variables in the enclosing scope are
- available for reading.
+ 1. The declare_args() block executes. Any variable defined in the enclosing
+ scope is available for reading, but any variable defined earlier in
+ the current scope is not (since the overrides haven't been applied yet).
2. At the end of executing the block, any variables set within that scope
are saved globally as build arguments, with their current values being
@@ -377,12 +408,10 @@
like [], "", or -1, and after the declare_args block, call exec_script if
the value is unset by the user.
- - Any code inside of the declare_args block will see the default values of
- previous variables defined in the block rather than the user-overridden
- value. This can be surprising because you will be used to seeing the
- overridden value. If you need to make the default value of one arg
- dependent on the possibly-overridden value of another, write two separate
- declare_args blocks:
+ - Because you cannot read the value of a variable defined in the same
+ block, if you need to make the default value of one arg depend
+ on the possibly-overridden value of another, write two separate
+ declare_args() blocks:
declare_args() {
enable_foo = true
@@ -415,6 +444,7 @@
return Value();
Scope block_scope(scope);
+ block_scope.SetProperty(&kInDeclareArgsKey, &block_scope);
block->Execute(&block_scope, err);
if (err->has_error())
return Value();
diff --git a/tools/gn/functions.h b/tools/gn/functions.h
index 9668658..0c97700 100644
--- a/tools/gn/functions.h
+++ b/tools/gn/functions.h
@@ -416,6 +416,16 @@
// Helper functions -----------------------------------------------------------
+// Validates that the scope that a value is defined in is not the scope
+// of the current declare_args() call, if that's what we're in. It is
+// illegal to read a value from inside the same declare_args() call, since
+// the overrides will not have been applied yet (see `gn help declare_args`
+// for more).
+bool EnsureNotReadingFromSameDeclareArgs(const ParseNode* node,
+ const Scope* cur_scope,
+ const Scope* val_scope,
+ Err* err);
+
// Verifies that the current scope is not processing an import. If it is, it
// will set the error, blame the given parse node for it, and return false.
bool EnsureNotProcessingImport(const ParseNode* node,
@@ -464,7 +474,7 @@
const FunctionCallNode* function,
const std::string& name);
-// Some tyesp of blocks can't be nested inside other ones. For such cases,
+// Some types of blocks can't be nested inside other ones. For such cases,
// instantiate this object upon entering the block and Enter() will fail if
// there is already another non-nestable block on the stack.
class NonNestableBlock {
diff --git a/tools/gn/functions_unittest.cc b/tools/gn/functions_unittest.cc
index 7156747..949b844 100644
--- a/tools/gn/functions_unittest.cc
+++ b/tools/gn/functions_unittest.cc
@@ -125,3 +125,48 @@
"rounding = [[1, 2], [3, 4], [5], [6]]\n",
setup.print_output());
}
+
+TEST(Functions, DeclareArgs) {
+ TestWithScope setup;
+ Err err;
+
+ // It is not legal to read the value of an argument declared in a
+ // declare_args() from inside the call, but outside the call and in
+ // a separate call should work.
+
+ TestParseInput reading_from_same_call(R"(
+ declare_args() {
+ foo = true
+ bar = foo
+ })");
+ reading_from_same_call.parsed()->Execute(setup.scope(), &err);
+ ASSERT_TRUE(err.has_error());
+
+ TestParseInput reading_from_outside_call(R"(
+ declare_args() {
+ foo = true
+ }
+
+ bar = foo
+ assert(bar)
+ )");
+ err = Err();
+ reading_from_outside_call.parsed()->Execute(setup.scope(), &err);
+ ASSERT_FALSE(err.has_error());
+
+ TestParseInput reading_from_different_call(R"(
+ declare_args() {
+ foo = true
+ }
+
+ declare_args() {
+ bar = foo
+ }
+
+ assert(bar)
+ )");
+ err = Err();
+ TestWithScope setup2;
+ reading_from_different_call.parsed()->Execute(setup2.scope(), &err);
+ ASSERT_FALSE(err.has_error());
+}
diff --git a/tools/gn/parse_tree.cc b/tools/gn/parse_tree.cc
index ecc438b..d1de9cb 100644
--- a/tools/gn/parse_tree.cc
+++ b/tools/gn/parse_tree.cc
@@ -487,13 +487,18 @@
}
Value IdentifierNode::Execute(Scope* scope, Err* err) const {
- const Value* value = scope->GetValue(value_.value(), true);
+ const Scope* found_in_scope = nullptr;
+ const Value* value = scope->GetValueWithScope(value_.value(), true,
+ &found_in_scope);
Value result;
if (!value) {
*err = MakeErrorDescribing("Undefined identifier");
return result;
}
+ if (!EnsureNotReadingFromSameDeclareArgs(this, scope, found_in_scope, err))
+ return result;
+
result = *value;
result.set_origin(this);
return result;
diff --git a/tools/gn/scope.cc b/tools/gn/scope.cc
index fbe73f4..650c64a 100644
--- a/tools/gn/scope.cc
+++ b/tools/gn/scope.cc
@@ -78,25 +78,37 @@
const Value* Scope::GetValue(const base::StringPiece& ident,
bool counts_as_used) {
+ const Scope* found_in_scope = nullptr;
+ return GetValueWithScope(ident, counts_as_used, &found_in_scope);
+}
+
+const Value* Scope::GetValueWithScope(const base::StringPiece& ident,
+ bool counts_as_used,
+ const Scope** found_in_scope) {
// First check for programmatically-provided values.
for (auto* provider : programmatic_providers_) {
const Value* v = provider->GetProgrammaticValue(ident);
- if (v)
+ if (v) {
+ *found_in_scope = nullptr;
return v;
+ }
}
RecordMap::iterator found = values_.find(ident);
if (found != values_.end()) {
if (counts_as_used)
found->second.used = true;
+ *found_in_scope = this;
return &found->second.value;
}
// Search in the parent scope.
if (const_containing_)
- return const_containing_->GetValue(ident);
- if (mutable_containing_)
- return mutable_containing_->GetValue(ident, counts_as_used);
+ return const_containing_->GetValueWithScope(ident, found_in_scope);
+ if (mutable_containing_) {
+ return mutable_containing_->GetValueWithScope(ident, counts_as_used,
+ found_in_scope);
+ }
return nullptr;
}
@@ -131,11 +143,19 @@
}
const Value* Scope::GetValue(const base::StringPiece& ident) const {
+ const Scope *found_in_scope = nullptr;
+ return GetValueWithScope(ident, &found_in_scope);
+}
+
+const Value* Scope::GetValueWithScope(const base::StringPiece& ident,
+ const Scope** found_in_scope) const {
RecordMap::const_iterator found = values_.find(ident);
- if (found != values_.end())
+ if (found != values_.end()) {
+ *found_in_scope = this;
return &found->second.value;
+ }
if (containing())
- return containing()->GetValue(ident);
+ return containing()->GetValueWithScope(ident, found_in_scope);
return nullptr;
}
diff --git a/tools/gn/scope.h b/tools/gn/scope.h
index b87afcd..36de088 100644
--- a/tools/gn/scope.h
+++ b/tools/gn/scope.h
@@ -112,7 +112,7 @@
const Settings* settings() const { return settings_; }
- // See the const_/mutable_containing_ var declaraions below. Yes, it's a
+ // See the const_/mutable_containing_ var declarations below. Yes, it's a
// bit weird that we can have a const pointer to the "mutable" one.
Scope* mutable_containing() { return mutable_containing_; }
const Scope* mutable_containing() const { return mutable_containing_; }
@@ -137,9 +137,18 @@
//
// counts_as_used should be set if the variable is being read in a way that
// should count for unused variable checking.
+ //
+ // found_in_scope is set to the scope that contains the definition of the
+ // ident. If the value was provided programmatically (like host_cpu),
+ // found_in_scope will be set to null.
const Value* GetValue(const base::StringPiece& ident,
bool counts_as_used);
const Value* GetValue(const base::StringPiece& ident) const;
+ const Value* GetValueWithScope(const base::StringPiece& ident,
+ const Scope** found_in_scope) const;
+ const Value* GetValueWithScope(const base::StringPiece& ident,
+ bool counts_as_used,
+ const Scope** found_in_scope);
// Returns the requested value as a mutable one if possible. If the value
// is not found in a mutable scope, then returns null. Note that the value
