Revert GN declare_args() change.
We've seen a bunch of bug reports for flaky build failures
complaining about unused or undeclared arguments; it seems
possible that this CL might be the cause, so I'm speculatively
reverting it and we can see if things get better; reverting the
change makes GN more permissive, so this should be reasonably safe.
This reverts r433944 (f6fd4d747c77e1c7968eb5af8b195a94a7ed2fac),
i.e. https://codereview.chromium.org/2509333003. This CL leaves
the build files changes that were in that CL because they
are backwards-compatible and improvements regardless.
TBR=brettw@chromium.org
BUG=674213
Review-Url: https://codereview.chromium.org/2586073002
Cr-Original-Commit-Position: refs/heads/master@{#439366}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 8d1f54effdc07f29a6fd00411e453ae3d67f9a86
diff --git a/tools/gn/docs/reference.md b/tools/gn/docs/reference.md
index ab3233a..1aa56f1 100644
--- a/tools/gn/docs/reference.md
+++ b/tools/gn/docs/reference.md
@@ -1641,9 +1641,8 @@
The precise behavior of declare args is:
- 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).
+ 1. The declare_arg block executes. Any variables in the enclosing scope are
+ available for reading.
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
@@ -1662,10 +1661,12 @@
like [], "", or -1, and after the declare_args block, call exec_script if
the value is unset by the user.
- - 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:
+ - 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:
declare_args() {
enable_foo = true
diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc
index 41be19b..e01a25c 100644
--- a/tools/gn/functions.cc
+++ b/tools/gn/functions.cc
@@ -45,38 +45,8 @@
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) {
@@ -387,9 +357,8 @@
The precise behavior of declare args is:
- 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).
+ 1. The declare_arg block executes. Any variables in the enclosing scope are
+ available for reading.
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
@@ -408,10 +377,12 @@
like [], "", or -1, and after the declare_args block, call exec_script if
the value is unset by the user.
- - 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:
+ - 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:
declare_args() {
enable_foo = true
@@ -444,7 +415,6 @@
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 401384a..37519d8 100644
--- a/tools/gn/functions.h
+++ b/tools/gn/functions.h
@@ -415,16 +415,6 @@
// 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,
@@ -473,7 +463,7 @@
const FunctionCallNode* function,
const std::string& name);
-// Some types of blocks can't be nested inside other ones. For such cases,
+// Some tyesp 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 949b844..7156747 100644
--- a/tools/gn/functions_unittest.cc
+++ b/tools/gn/functions_unittest.cc
@@ -125,48 +125,3 @@
"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 d1de9cb..ecc438b 100644
--- a/tools/gn/parse_tree.cc
+++ b/tools/gn/parse_tree.cc
@@ -487,18 +487,13 @@
}
Value IdentifierNode::Execute(Scope* scope, Err* err) const {
- const Scope* found_in_scope = nullptr;
- const Value* value = scope->GetValueWithScope(value_.value(), true,
- &found_in_scope);
+ const Value* value = scope->GetValue(value_.value(), true);
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 650c64a..fbe73f4 100644
--- a/tools/gn/scope.cc
+++ b/tools/gn/scope.cc
@@ -78,37 +78,25 @@
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) {
- *found_in_scope = nullptr;
+ if (v)
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_->GetValueWithScope(ident, found_in_scope);
- if (mutable_containing_) {
- return mutable_containing_->GetValueWithScope(ident, counts_as_used,
- found_in_scope);
- }
+ return const_containing_->GetValue(ident);
+ if (mutable_containing_)
+ return mutable_containing_->GetValue(ident, counts_as_used);
return nullptr;
}
@@ -143,19 +131,11 @@
}
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()) {
- *found_in_scope = this;
+ if (found != values_.end())
return &found->second.value;
- }
if (containing())
- return containing()->GetValueWithScope(ident, found_in_scope);
+ return containing()->GetValue(ident);
return nullptr;
}
diff --git a/tools/gn/scope.h b/tools/gn/scope.h
index 31bac62..e2c64e6 100644
--- a/tools/gn/scope.h
+++ b/tools/gn/scope.h
@@ -109,7 +109,7 @@
const Settings* settings() const { return settings_; }
- // See the const_/mutable_containing_ var declarations below. Yes, it's a
+ // See the const_/mutable_containing_ var declaraions 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_; }
@@ -134,18 +134,9 @@
//
// 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
