code review 9895044: [release-branch.go1.1] cmd/gc: do not corrupt init() wi...

[release-branch.go1.1] cmd/gc: do not corrupt init() with initializers of _ in closures.

««« CL 9952043 / c42a7c218440
cmd/gc: do not corrupt init() with initializers of _ in closures.

Fixes issue 5607.

R=golang-dev, daniel.morsing, r, dsymonds
CC=golang-dev
https://codereview.appspot.com/9952043
»»»

[adg, 2013-06-04 02:58:28]

Hello DMorsing, dsymonds, r, remyoudompheng (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[adg, 2013-06-04 23:41:14]

Ping. Come on guys, I need to get a bunch of these reviewed before next week.
The quicker we get them in the better they can be tested.

[dsymonds, 2013-06-04 23:45:01]

LGTM

[adg, 2013-06-05 00:01:03]

*** Submitted as https://code.google.com/p/go/source/detail?r=6fd05e0b4f08 ***

[release-branch.go1.1] cmd/gc: do not corrupt init() with initializers of _ in
closures.

««« CL 9952043 / c42a7c218440
cmd/gc: do not corrupt init() with initializers of _ in closures.

Fixes issue 5607.

R=golang-dev, daniel.morsing, r, dsymonds
CC=golang-dev
https://codereview.appspot.com/9952043
»»»

R=daniel.morsing, dsymonds, r, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/9895044

[rsc, 2013-06-10 11:55:30]

I don't understand why this patch is correct.

[DMorsing, 2013-06-10 12:07:44]

On Mon, Jun 10, 2013 at 1:55 PM, Russ Cox <rsc@golang.org> wrote:
> I don't understand why this patch is correct.
>

We assumed that if someone did "var foo, _ = Bar()", that it was on
the top level and should be executed at init. Now we check to make
sure that we're not inside a function.

[rsc, 2013-06-10 15:29:04]

On Mon, Jun 10, 2013 at 8:07 AM, Daniel Morsing
<daniel.morsing@gmail.com>wrote:

> On Mon, Jun 10, 2013 at 1:55 PM, Russ Cox <rsc@golang.org> wrote:
> > I don't understand why this patch is correct.
> >
>
> We assumed that if someone did "var foo, _ = Bar()", that it was on
> the top level and should be executed at init. Now we check to make
> sure that we're not inside a function.
>

Thanks. But what makes sure that Bar is called at all?

Russ

[remyoudompheng, 2013-06-10 20:20:21]

2013/6/10 Russ Cox <rsc@golang.org>:
> On Mon, Jun 10, 2013 at 8:07 AM, Daniel Morsing <daniel.morsing@gmail.com>
> wrote:
>> We assumed that if someone did "var foo, _ = Bar()", that it was on
>> the top level and should be executed at init. Now we check to make
>> sure that we're not inside a function.
>
>
> Thanks. But what makes sure that Bar is called at all?

Called as part of what? It should be compiled normally as part of the
enclosing function.
Is there a pitfall here?

Rémy.

[rsc, 2013-06-10 20:37:19]

On Mon, Jun 10, 2013 at 4:20 PM, Rémy Oudompheng
<remyoudompheng@gmail.com>wrote:

> Called as part of what? It should be compiled normally as part of the
> enclosing function.
> Is there a pitfall here?
>

I agree that the new code is at least as correct as the old code. But if we
have either of:

var foo, _ = Bar()
var _ = Bar()

at top level, what is making sure that the things Bar needs have been
initialized before Bar is called?

The *out = list(*out, n->defn) at line 58 is queueing the call to Bar.
Where is the traversal of the things Bar needs? That is, if we have:

var Bar = func() {}
var x = Bar()

then the stuff at the bottom of the function is what takes care of making
sure that Bar is initialized before x. But if we have

var Bar = func() {}
var _ = Bar()

then that stuff doesn't run. So why is it safe to just return?

Maybe it is, I'm just confused (and I wrote the code initially, but I
haven't had a full night's sleep in months).

Russ

[remyoudompheng, 2013-06-10 20:41:17]

2013/6/10 Russ Cox <rsc@golang.org>:
> On Mon, Jun 10, 2013 at 4:20 PM, Rémy Oudompheng <remyoudompheng@gmail.com>
> wrote:
>>
>> Called as part of what? It should be compiled normally as part of the
>> enclosing function.
>> Is there a pitfall here?
>
>
> I agree that the new code is at least as correct as the old code. But if we
> have either of:
>
> var foo, _ = Bar()
> var _ = Bar()
>
> at top level, what is making sure that the things Bar needs have been
> initialized before Bar is called?
>
> The *out = list(*out, n->defn) at line 58 is queueing the call to Bar. Where
> is the traversal of the things Bar needs? That is, if we have:
>
> var Bar = func() {}
> var x = Bar()
>
> then the stuff at the bottom of the function is what takes care of making
> sure that Bar is initialized before x. But if we have
>
> var Bar = func() {}
> var _ = Bar()
>
> then that stuff doesn't run. So why is it safe to just return?

It is not (well, depending of which church of the Go spec you belong
to). That's why we have issue 5244 and CL8601044
(https://codereview.appspot.com/8601044). If we agree on what it
should do, I can mail out the CL.

[rsc, 2013-06-10 20:50:39]

On Mon, Jun 10, 2013 at 4:41 PM, Rémy Oudompheng
<remyoudompheng@gmail.com>wrote:

> It is not (well, depending of which church of the Go spec you belong
> to). That's why we have issue 5244 and CL8601044
> (https://codereview.appspot.com/8601044). If we agree on what it
> should do, I can mail out the CL.
>

I replied on issue 5244. That seems serious enough that I would have fixed
it for Go 1.1. It's right on the line for Go 1.1.1. Can you convince me
that fixing it won't break any correct programs?

Russ