Change GN to disallow reading args defined in the same declare_args() call.

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
Committed: https://crrev.com/f6fd4d747c77e1c7968eb5af8b195a94a7ed2fac
Cr-Commit-Position: refs/heads/master@{#433944}

[Dirk Pranke, 2016-11-18 03:32:42]

Description was changed from

==========
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 turns out we only did this twice, but both
times were unintentional, so this correctly finds the bugs.

R=brettw@chromium.org, kjellander@chromium.org
BUG=542846
==========

to

==========
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
==========

[Dirk Pranke, 2016-11-18 03:32:51]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2016-11-18 03:32:58]

Patchset #1 (id:20001) has been deleted

[Dirk Pranke, 2016-11-18 03:33:24]

What do you think?

[kjellander_chromium, 2016-11-18 13:25:50]

lgtm. I don't think it'll break WebRTC rolling and if it does that's fine since
the error is so clear.

[kjellander_chromium, 2016-11-18 13:27:32]

On 2016/11/18 13:25:50, kjellander_chromium wrote:
> lgtm. I don't think it'll break WebRTC rolling and if it does that's fine
since
> the error is so clear.

I guess at least one unit test triggering the error and one that reads a
variable declared in a previous scope should be added.

[brettw, 2016-11-18 22:07:43]

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:421: block_scope.SetProperty(&kInDeclareArgsKey, (void
*)&kInDeclareArgsKey);
Are you sure you need the void* cast here? I think any pointer should be
convertable to void*.

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/parse_tree.cc
File tools/gn/parse_tree.cc (right):

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:490: const Scope *found_in_scope = nullptr;
Put the * next to the type like you do for Value below (GN code should be
consistent in this regard).

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:499: // Check that the scope that contains the defined
value and the
I think it would be easier to follow if this was in a helper function
EnsureNotReadingFromDeclareArgs or something. This function can be in
functions.cc so you don't need to have a global declare_args key (I like to keep
these keys narrowly scoped).

[Dirk Pranke, 2016-11-18 22:35:40]

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/functions.cc#n...
tools/gn/functions.cc:421: block_scope.SetProperty(&kInDeclareArgsKey, (void
*)&kInDeclareArgsKey);
On 2016/11/18 22:07:43, brettw (ping on IM after 24h) wrote:
> Are you sure you need the void* cast here? I think any pointer should be
> convertable to void*.

I thought I didn't need it, either, but the compiler kept complaining. I will
double-check.

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/parse_tree.cc
File tools/gn/parse_tree.cc (right):

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:490: const Scope *found_in_scope = nullptr;
On 2016/11/18 22:07:43, brettw (ping on IM after 24h) wrote:
> Put the * next to the type like you do for Value below (GN code should be
> consistent in this regard).

ack.

https://codereview.chromium.org/2509333003/diff/40001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:499: // Check that the scope that contains the defined
value and the
On 2016/11/18 22:07:43, brettw (ping on IM after 24h) wrote:
> I think it would be easier to follow if this was in a helper function
> EnsureNotReadingFromDeclareArgs or something. This function can be in
> functions.cc so you don't need to have a global declare_args key (I like to
keep
> these keys narrowly scoped).

Acknowledged.

[Dirk Pranke, 2016-11-19 03:34:14]

Updated. Please take another look?

[brettw, 2016-11-21 17:23:39]

lgtm

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/functions.cc#n...
tools/gn/functions.cc:73: "Reading a variable defined in the same declare_args()
call.");
Can you put an extended help message here explaining how to fix the problem (use
a second_declare_args block if you want dependent defaults).

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/scope.h
File tools/gn/scope.h (right):

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/scope.h#newcod...
tools/gn/scope.h:139: // should count for unused variable checking.
Can this document what found_in_scope means (in particular that the result can
be null even when it returns success).

[Dirk Pranke, 2016-11-21 17:39:48]

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/functions.cc#n...
tools/gn/functions.cc:73: "Reading a variable defined in the same declare_args()
call.");
On 2016/11/21 17:23:39, brettw (ping on IM after 24h) wrote:
> Can you put an extended help message here explaining how to fix the problem
(use
> a second_declare_args block if you want dependent defaults).

Will do.

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/scope.h
File tools/gn/scope.h (right):

https://codereview.chromium.org/2509333003/diff/80001/tools/gn/scope.h#newcod...
tools/gn/scope.h:139: // should count for unused variable checking.
On 2016/11/21 17:23:39, brettw (ping on IM after 24h) wrote:
> Can this document what found_in_scope means (in particular that the result can
> be null even when it returns success).

Will do.

[Dirk Pranke, 2016-11-22 02:48:53]

Updated, please take another look?

https://codereview.chromium.org/2509333003/diff/120001/tools/gn/functions.cc
File tools/gn/functions.cc (right):

https://codereview.chromium.org/2509333003/diff/120001/tools/gn/functions.cc#...
tools/gn/functions.cc:76: "them in two separate declare_args() calls, one after
the other.\n");
I'm not wild about this wording, but also haven't thought of anything better
yet.

[brettw, 2016-11-22 04:55:57]

Seems good enough to me (still lgtm)

[Dirk Pranke, 2016-11-22 17:06:21]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-11-22 17:06:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@chromium.org
Link to the patchset: https://codereview.chromium.org/2509333003/#ps120001
(title: "update w/ review feedback")

[commit-bot: I haz the power, 2016-11-22 17:06:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 19:43:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479834381277210,
"parent_rev": "12b50a48cf0b441e988d7cd112b50529a43a2eb0", "commit_rev":
"d2b538fadf9a1d6c4e52a03774235f049ce674e8"}

[commit-bot: I haz the power, 2016-11-22 19:43:44]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-11-22 19:43:46]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-11-22 19:45:19]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/f6fd4d747c77e1c7968eb5af8b195a94a7ed2fac
Cr-Commit-Position: refs/heads/master@{#433944}
==========

[commit-bot: I haz the power, 2016-11-22 19:45:20]

Patchset 5 (id:??) landed as
https://crrev.com/f6fd4d747c77e1c7968eb5af8b195a94a7ed2fac
Cr-Commit-Position: refs/heads/master@{#433944}