code review 5700068: go spec: inside functions, variables must be evaluated.

go spec: inside functions, variables must be evaluated.

Fixes issue 1612.

[gri, 2012-02-24 23:39:26]

Hello r, rsc, iant, ken2 (cc: golang-dev@googlegroups.com),

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

[r, 2012-02-24 23:49:04]

http://codereview.appspot.com/5700068/diff/1/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5700068/diff/1/doc/go_spec.html#newcode1839
doc/go_spec.html:1839: it is illegal to declare a variable that is never read.
i don't know what "read" means, plus this is vague. it's not obvious how to be
more specific. what about a structure field?
also, is it just the variable that must be read, or a value assigned to it? as
the old compiler once wrote, "set and not used".

in short, i understand what you're saying but it's not clear and i'm not even
sure it's quite what we want to say. but it might be.

[gri, 2012-02-25 01:10:04]

PTAL.

This time w/ a proposal for the notion of an RhsExpr that would permit
a syntactic definition of the notion of variable "use".

If we like this approach, I will complete the change accordingly.

- gri

On Fri, Feb 24, 2012 at 3:49 PM,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/5700068/diff/1/doc/go_spec.html
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/5700068/diff/1/doc/go_spec.html#newcode1839
> doc/go_spec.html:1839: it is illegal to declare a variable that is never
> read.
> i don't know what "read" means, plus this is vague. it's not obvious how
> to be more specific. what about a structure field?
> also, is it just the variable that must be read, or a value assigned to
> it? as the old compiler once wrote, "set and not used".
>
> in short, i understand what you're saying but it's not clear and i'm not
> even sure it's quite what we want to say. but it might be.
>
> http://codereview.appspot.com/5700068/

[r, 2012-02-25 01:35:25]

i like the general idea but feel it's too sweeping a change to do quickly. i'd
rather hold off on that approach.

i think if you change 'read' to 'evaluated' in the previous CL, you get close to
the right meaning, close enough for now.

[gri, 2012-02-25 02:54:15]

On Fri, Feb 24, 2012 at 5:35 PM,  <r@golang.org> wrote:
> i like the general idea but feel it's too sweeping a change to do
> quickly. i'd rather hold off on that approach.

Happy to go back, but the change is pretty simple. Some places where
we use Expression would use RhsExpr and some places where we  use
ExpressionList would become RhsExprList. That's about it.

Waiting for others to chime in.
- gri

>
> i think if you change 'read' to 'evaluated' in the previous CL, you get
> close to the right meaning, close enough for now.
>
>
> http://codereview.appspot.com/5700068/

[remyoudompheng, 2012-02-25 10:27:35]

On 2012/02/25 01:35:25, r wrote:
> i think if you change 'read' to 'evaluated' in the previous CL, you get close
to
> the right meaning, close enough for now.

Is Array "used" in the following snippet?

var Array [2]int
i := len(Array)

[gri, 2012-02-25 17:24:44]

Yes
On Feb 25, 2012 2:27 AM, <remyoudompheng@gmail.com> wrote:

> On 2012/02/25 01:35:25, r wrote:
>
>> i think if you change 'read' to 'evaluated' in the previous CL, you
>>
> get close to
>
>> the right meaning, close enough for now.
>>
>
> Is Array "used" in the following snippet?
>
> var Array [2]int
> i := len(Array)
>
>
>
http://codereview.appspot.com/**5700068/<http://codereview.appspot.com/5700068/>
>

[gri, 2012-02-27 23:44:59]

Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org,
remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2012-02-27 23:46:04]

Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org,
remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-02-27 23:49:56]

LGTM
i like this simplicity and think it works

[rsc, 2012-02-28 15:38:33]

http://codereview.appspot.com/5700068/diff/4002/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5700068/diff/4002/doc/go_spec.html#newcode1839
doc/go_spec.html:1839: it is illegal to declare a variable that is never
evaluated.
and never has its address taken?

func f() *int {
    var x int
    return &x
}

[gri, 2012-02-28 19:31:42]

PTAL.

Also added missing links to (short) variable declarations where
variables are declared so that it's clear the rule doesn't just apply
to this form of variable declaration.
- gri

On Tue, Feb 28, 2012 at 7:38 AM,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/5700068/diff/4002/doc/go_spec.html
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/5700068/diff/4002/doc/go_spec.html#newcode1839
> doc/go_spec.html:1839: it is illegal to declare a variable that is never
> evaluated.
> and never has its address taken?

Of course.

>
> func f() *int {
>    var x int
>    return &x
> }
>
> http://codereview.appspot.com/5700068/

[rsc, 2012-02-28 19:52:53]

LGTM but wait for Ian.

I am not completely confident about this, but it seems good enough to try and
see.

[remyoudompheng, 2012-02-28 20:38:37]

I may be a bit stubborn but still annoyed by the case of

var a [2]int
_ = len(a)

where the spec explicitly says it does not evaluate nor take the address of a.
Maybe the compiler should complain, because declaring n array just to take its
length is probably a mistake.

[rsc, 2012-02-28 20:41:29]

> var a [2]int
> _ = len(a)

I have no problem with making this an error.
If others agree, please file an issue.

[gri, 2012-02-28 20:43:56]

I think it would be fine given the existing spec and this rule to make
this an error.
- gri

On Tue, Feb 28, 2012 at 12:38 PM,  <remyoudompheng@gmail.com> wrote:
> I may be a bit stubborn but still annoyed by the case of
>
> var a [2]int
> _ = len(a)
>
> where the spec explicitly says it does not evaluate nor take the address
> of a. Maybe the compiler should complain, because declaring n array just
> to take its length is probably a mistake.
>
> http://codereview.appspot.com/5700068/

[ken3, 2012-02-28 20:56:54]

i am uncomfortable with the increasing
cross-product of normal functions being
diagnosed as special cases. especially errors.
all of _, =, len, and array are well defined.
it does what each suggests. it seems to me
that if this is illegal, then
    _ = 5
and a million more should also be illegal.

On Tue, Feb 28, 2012 at 12:43 PM, Robert Griesemer <gri@golang.org> wrote:
> I think it would be fine given the existing spec and this rule to make
> this an error.
> - gri
>
> On Tue, Feb 28, 2012 at 12:38 PM,  <remyoudompheng@gmail.com> wrote:
>> I may be a bit stubborn but still annoyed by the case of
>>
>> var a [2]int
>> _ = len(a)
>>
>> where the spec explicitly says it does not evaluate nor take the address
>> of a. Maybe the compiler should complain, because declaring n array just
>> to take its length is probably a mistake.
>>
>> http://codereview.appspot.com/5700068/

[rsc, 2012-02-28 21:02:55]

On Tue, Feb 28, 2012 at 15:56, Ken Thompson <ken@google.com> wrote:
> i am uncomfortable with the increasing
> cross-product of normal functions being
> diagnosed as special cases. especially errors.
> all of _, =, len, and array are well defined.
> it does what each suggests. it seems to me
> that if this is illegal, then
>    _ = 5
> and a million more should also be illegal.

It's not this rule but len on arrays that is the special case.

x := []int{}
_ = len(x)

is still okay.  The special case is that when you have

a := [5]int{}
_ = len(a)

the spec explicitly defines that len(a) is a compile-time
constant and therefore does not evaluate a.

We could enumerate the constructs where you can write
something that looks like, but is not, a use of an argument.
I believe they are unsafe.Sizeof, unsafe.Offsetof, unsafe.Alignof,
len, and cap.

We could say in the spec the error applies to
variables that are never evaluated, never addressed,
and never used during the computation of a compile-time
constant.

Russ

[r2, 2012-02-28 21:09:17]

i wonder (not yet suggest) if we should make set-and-not-used be like the return
thing and just say, compilers can do this. i realize this leaves an unsettled
pocket in the language and does not define program behavior, so it's worse than
return, but i observe two things:

1) my attempt to simplify by saying "vars must be evaluated" failed because
people want to turn it into a spec
2) ken is right that it's getting messy because we're trying to spec it

so my solomonic pondering is whether we should stop trying to spec it. for now.

-rob

[gri, 2012-02-28 21:16:54]

There is also the alternative route - which I have abandoned because
it was a larger change - to introduce a syntactic production RhsExpr.

Then one can simply say that it is an error if a locally declared
variable never appears in an RhsExpr. It would solve the len(a) issue
because in this case a would appear in an RhsExpr and thus it would be
legal (contrary to what happens now).

The change is that almost all Expressions would become RhsExpr (and
ExpressionLists would become RhsExprLists), except for a just a couple
of expressions on the lhs. The alternative could be to leave
Expression and ExpressionList alone and instead change the lhs, by
calling it LExpression, LExpr, or the like (Lvalue here we come...).
The latter would be a fairly small change and may even give us a
better handle to refer to variables on the lhs.

- gri



On Tue, Feb 28, 2012 at 1:09 PM, Rob 'Commander' Pike <r@google.com> wrote:
> i wonder (not yet suggest) if we should make set-and-not-used be like the
return thing and just say, compilers can do this. i realize this leaves an
unsettled pocket in the language and does not define program behavior, so it's
worse than return, but i observe two things:
>
> 1) my attempt to simplify by saying "vars must be evaluated" failed because
people want to turn it into a spec
> 2) ken is right that it's getting messy because we're trying to spec it
>
> so my solomonic pondering is whether we should stop trying to spec it. for
now.
>
> -rob
>

[r2, 2012-02-28 21:32:41]

On 29/02/2012, at 8:16 AM, Robert Griesemer wrote:

> There is also the alternative route - which I have abandoned because
> it was a larger change - to introduce a syntactic production RhsExpr.
> 
> Then one can simply say that it is an error if a locally declared
> variable never appears in an RhsExpr. It would solve the len(a) issue
> because in this case a would appear in an RhsExpr and thus it would be
> legal (contrary to what happens now).
> 
> The change is that almost all Expressions would become RhsExpr (and
> ExpressionLists would become RhsExprLists), except for a just a couple
> of expressions on the lhs. The alternative could be to leave
> Expression and ExpressionList alone and instead change the lhs, by
> calling it LExpression, LExpr, or the like (Lvalue here we come...).
> The latter would be a fairly small change and may even give us a
> better handle to refer to variables on the lhs.

as i said before, that sounds like a good idea but is too big a change for now.

-rob

[gri, 2012-02-28 21:35:57]

I am inclined to leave the CL as is.

I am not bothered by the len(a) case. It's minor compared to some
other irregularities we have, and arguably a bug to declare an array
that is never used but for its length.

- gri

On Tue, Feb 28, 2012 at 1:32 PM, Rob 'Commander' Pike <r@google.com> wrote:
>
> On 29/02/2012, at 8:16 AM, Robert Griesemer wrote:
>
>> There is also the alternative route - which I have abandoned because
>> it was a larger change - to introduce a syntactic production RhsExpr.
>>
>> Then one can simply say that it is an error if a locally declared
>> variable never appears in an RhsExpr. It would solve the len(a) issue
>> because in this case a would appear in an RhsExpr and thus it would be
>> legal (contrary to what happens now).
>>
>> The change is that almost all Expressions would become RhsExpr (and
>> ExpressionLists would become RhsExprLists), except for a just a couple
>> of expressions on the lhs. The alternative could be to leave
>> Expression and ExpressionList alone and instead change the lhs, by
>> calling it LExpression, LExpr, or the like (Lvalue here we come...).
>> The latter would be a fairly small change and may even give us a
>> better handle to refer to variables on the lhs.
>
> as i said before, that sounds like a good idea but is too big a change for
now.
>
> -rob
>
>

[rsc, 2012-02-28 21:37:32]

> I am not bothered by the len(a) case. It's minor compared to some
> other irregularities we have, and arguably a bug to declare an array
> that is never used but for its length.

Maybe in that case, but what about

var x int
y := unsafe.Sizeof(x)

Are we really going to require _ = x here?

?

[r2, 2012-02-28 21:41:53]

On 29/02/2012, at 8:37 AM, Russ Cox wrote:

>> I am not bothered by the len(a) case. It's minor compared to some
>> other irregularities we have, and arguably a bug to declare an array
>> that is never used but for its length.
> 
> Maybe in that case, but what about
> 
> var x int
> y := unsafe.Sizeof(x)
> 
> Are we really going to require _ = x here?

Why not? unsafe does not need special compensation for usability.

-rob

[rsc, 2012-02-28 21:42:42]

I'm leaning toward leaving this unspecified now too.

[r2, 2012-02-28 21:45:08]

On 29/02/2012, at 8:42 AM, Russ Cox wrote:

> I'm leaning toward leaving this unspecified now too.

that does not preclude us specifying it later

-rob

[iant, 2012-02-28 21:46:23]

http://codereview.appspot.com/5700068/diff/22002/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5700068/diff/22002/doc/go_spec.html#newcode1838
doc/go_spec.html:1838: Inside a <a href="#Function_declarations">function</a> <a
href="#Body">body</a>,
where is the "Body" anchor?

http://codereview.appspot.com/5700068/diff/22002/doc/go_spec.html#newcode1840
doc/go_spec.html:1840: <a href="#Address_operators">address</a> taken.
In gccgo I implement a slightly different rule: it is illegal to declare a
variable that is not named again anywhere in its scope.  I count any reference
to the variable as a use, even if the reference is not actually evaluated.

I don't know which rule is best.

[gri, 2012-02-28 21:51:30]

PTAL. Rephrased as implementation restriction. Perhaps this is better?
- gri

On Tue, Feb 28, 2012 at 1:46 PM,  <iant@golang.org> wrote:
>
> http://codereview.appspot.com/5700068/diff/22002/doc/go_spec.html
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/5700068/diff/22002/doc/go_spec.html#newcode1838
> doc/go_spec.html:1838: Inside a <a
> href="#Function_declarations">function</a> <a href="#Body">body</a>,
> where is the "Body" anchor?

The Body anchor is generated automatically when serving the spec via
godoc (which introduces
links for all ebnf productions).

- gri

>
> http://codereview.appspot.com/5700068/diff/22002/doc/go_spec.html#newcode1840
> doc/go_spec.html:1840: <a href="#Address_operators">address</a> taken.
> In gccgo I implement a slightly different rule: it is illegal to declare
> a variable that is not named again anywhere in its scope.  I count any
> reference to the variable as a use, even if the reference is not
> actually evaluated.
>
> I don't know which rule is best.
>
> http://codereview.appspot.com/5700068/

[rsc, 2012-02-28 21:52:30]

http://codereview.appspot.com/5700068/diff/26002/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5700068/diff/26002/doc/go_spec.html#newcode1840
doc/go_spec.html:1840: if the variable is never evaluated and never has its
if the variable is never used.

(And leave "used" undefined.)

[gri, 2012-02-28 21:54:02]

PTAL.
- gri

On Tue, Feb 28, 2012 at 1:52 PM,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/5700068/diff/26002/doc/go_spec.html
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/5700068/diff/26002/doc/go_spec.html#newcode1840
> doc/go_spec.html:1840: if the variable is never evaluated and never has
> its
> if the variable is never used.
>
> (And leave "used" undefined.)
>
> http://codereview.appspot.com/5700068/

[rsc, 2012-02-28 21:58:37]

LGTM

[r2, 2012-02-28 22:05:08]

LGTM

On Wednesday, February 29, 2012, wrote:

> LGTM
>
>
>
http://codereview.appspot.com/**5700068/<http://codereview.appspot.com/5700068/>
>

[iant, 2012-02-28 22:10:44]

LGTM

[gri, 2012-02-29 01:44:27]

*** Submitted as http://code.google.com/p/go/source/detail?r=7ff2bf9b131a ***

go spec: inside functions, variables must be evaluated.

Fixes issue 1612.

R=r, rsc, iant, ken, remyoudompheng, ken, r
CC=golang-dev
http://codereview.appspot.com/5700068

[rog, 2012-02-29 12:36:16]

http://codereview.appspot.com/5700068/diff/25003/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5700068/diff/25003/doc/go_spec.html#newcode4248
doc/go_spec.html:4248: The iteration variables may be declared by the "range"
using a form of
this looks like a mistake.

[gri, 2012-02-29 16:21:57]

http://codereview.appspot.com/5700068/diff/25003/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5700068/diff/25003/doc/go_spec.html#newcode4248
doc/go_spec.html:4248: The iteration variables may be declared by the "range"
using a form of
On 2012/02/29 12:36:16, rog wrote:
> this looks like a mistake.

See: http://codereview.appspot.com/5706065

[rog, 2012-02-29 17:08:58]

sorry, i missed that.

On 29 February 2012 16:21,  <gri@golang.org> wrote:
>
> http://codereview.appspot.com/5700068/diff/25003/doc/go_spec.html
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/5700068/diff/25003/doc/go_spec.html#newcode4248
> doc/go_spec.html:4248: The iteration variables may be declared by the
> "range" using a form of
> On 2012/02/29 12:36:16, rog wrote:
>>
>> this looks like a mistake.
>
>
> See: http://codereview.appspot.com/5706065
>
> http://codereview.appspot.com/5700068/