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