code review 92880043: go.tools/go.ssa: positions for ssa.Store

go.tools/go.ssa: positions for ssa.Store

I am doing some analysis on ssa.Store instructions and I need the positions of those for proper error reporting. However, the position is 0 if the ssa.Store was generated from an ast.AssignStmt without an ast.StarExpr (the common case).

I first tried to pass the position through with several new pos parameters (as it is done for other instructions), but this quickly lead to a lot of changed lines. Then I saw that there is the currentBlock field in ssa.Function, which already is state information of the builder. That's why I decided to add the currentPosition field to pass the position from builder.stmt to the emitStore function. What do you think of this approach, since it is not consistent with how positions for other instructions are handled?

EDIT: New version uses pos parameters as described by Alan.

[neelance, 2014-04-30 12:43:40]

Hello golang-codereviews@googlegroups.com,

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

[gobot, 2014-05-07 14:16:49]

R=adonovan@google.com (assigned by crawshaw@golang.org)

[adonovan, 2014-05-07 14:51:06]

On 2014/05/07 14:16:49, gobot wrote:
> mailto:R=adonovan@google.com (assigned by mailto:crawshaw@golang.org)

Hi Richard,

Thanks for sending this.  I think adding position info to Stores is a great
idea---I have wanted the same thing myself---and your code change is
impressively concise, but I'm concerned that the currentPos trick merely hides
complexity rather than eliminates it.  Adding a token.Pos parameter to emitStore
seems like the right approach since it allows us to see in all cases exactly
what position is associated with each Store, to control it more precisely than
currentPos allows us, and to document it usefully.

Here are my suggested positions for each kind of store:

- keep using ast.StarExpr.Star when explicit in the source.
- for n:1 var initializers, use the lhs ast.Ident.Pos()
- for stores to the varargs slice, use ast.CallExpr.Args[i].Pos().
- for composite literals stores to array/slice/maps, use
ast.CompositeLiteral.Lbrace.
- for typecase temporaries, use obj.pos(), i.e. y in "switch y := x.(type) {".
- for copying of named return parameters, use ast.ReturnStmt.Return
- for range loop index variables, use ast.RangeStmt.Range.
- for synthetic stores in init and testmain, use NoPos.

Would you care to make this slightly larger change?  Try to think of a good
test; I will too.

cheers
alan

PS: I'm sure you're aware the tree is currently frozen, so we won't be able to
commit this CL till June.

[adonovan, 2014-05-07 16:42:58]

Oh, I forgot you'll also need to provide the pos field each time you
construct an "address" struct, which happens in addr() and compLit().
The relevant positions to use are:
- ast.Ident.Pos(), if its the address of a var.
- compLit: ast.CompositeLit.Lbrace for the whole, or e.Pos() for each
element
- ast.SelectorExpr.Sel.Pos
- ast.IndexExpr.Lbrack

In a statement such as:
  y = x
we prefer to associate the store with 'y' not with '='.  This is simpler to
implement, and clearer when there are multiple stores such as in:
   a, b = c
or
   a, b = c, d

In many stores such as x++ and x[i]++ there is no '=' token anyway.  (The
Store positions would be those of 'x' and '[', respectively).


On 7 May 2014 10:51, <adonovan@google.com> wrote:

> On 2014/05/07 14:16:49, gobot wrote:
>
>> mailto:R=adonovan@google.com (assigned by mailto:crawshaw@golang.org)
>>
>
> Hi Richard,
>
> Thanks for sending this.  I think adding position info to Stores is a
> great idea---I have wanted the same thing myself---and your code change
> is impressively concise, but I'm concerned that the currentPos trick
> merely hides complexity rather than eliminates it.  Adding a token.Pos
> parameter to emitStore seems like the right approach since it allows us
> to see in all cases exactly what position is associated with each Store,
> to control it more precisely than currentPos allows us, and to document
> it usefully.
>
> Here are my suggested positions for each kind of store:
>
> - keep using ast.StarExpr.Star when explicit in the source.
> - for n:1 var initializers, use the lhs ast.Ident.Pos()
> - for stores to the varargs slice, use ast.CallExpr.Args[i].Pos().
> - for composite literals stores to array/slice/maps, use
> ast.CompositeLiteral.Lbrace.
> - for typecase temporaries, use obj.pos(), i.e. y in "switch y :=
> x.(type) {".
> - for copying of named return parameters, use ast.ReturnStmt.Return
> - for range loop index variables, use ast.RangeStmt.Range.
> - for synthetic stores in init and testmain, use NoPos.
>
> Would you care to make this slightly larger change?  Try to think of a
> good test; I will too.
>
> cheers
> alan
>
> PS: I'm sure you're aware the tree is currently frozen, so we won't be
> able to commit this CL till June.
>
>
> https://codereview.appspot.com/92880043/
>

[mail_richard-musiol.de, 2014-05-08 11:00:37]

That's exactly why I was refraining from doing the larger change, because I
did not have the in-depth knowledge of all those cases where store is used.
I will work on the larger change now, thanks for your help.


2014-05-07 18:42 GMT+02:00 Alan Donovan <adonovan@google.com>:

> Oh, I forgot you'll also need to provide the pos field each time you
> construct an "address" struct, which happens in addr() and compLit().
> The relevant positions to use are:
> - ast.Ident.Pos(), if its the address of a var.
> - compLit: ast.CompositeLit.Lbrace for the whole, or e.Pos() for each
> element
> - ast.SelectorExpr.Sel.Pos
> - ast.IndexExpr.Lbrack
>
> In a statement such as:
>   y = x
> we prefer to associate the store with 'y' not with '='.  This is simpler
> to implement, and clearer when there are multiple stores such as in:
>    a, b = c
> or
>    a, b = c, d
>
> In many stores such as x++ and x[i]++ there is no '=' token anyway.  (The
> Store positions would be those of 'x' and '[', respectively).
>
>
> On 7 May 2014 10:51, <adonovan@google.com> wrote:
>
>> On 2014/05/07 14:16:49, gobot wrote:
>>
>>> mailto:R=adonovan@google.com (assigned by mailto:crawshaw@golang.org)
>>>
>>
>> Hi Richard,
>>
>> Thanks for sending this.  I think adding position info to Stores is a
>> great idea---I have wanted the same thing myself---and your code change
>> is impressively concise, but I'm concerned that the currentPos trick
>> merely hides complexity rather than eliminates it.  Adding a token.Pos
>> parameter to emitStore seems like the right approach since it allows us
>> to see in all cases exactly what position is associated with each Store,
>> to control it more precisely than currentPos allows us, and to document
>> it usefully.
>>
>> Here are my suggested positions for each kind of store:
>>
>> - keep using ast.StarExpr.Star when explicit in the source.
>> - for n:1 var initializers, use the lhs ast.Ident.Pos()
>> - for stores to the varargs slice, use ast.CallExpr.Args[i].Pos().
>> - for composite literals stores to array/slice/maps, use
>> ast.CompositeLiteral.Lbrace.
>> - for typecase temporaries, use obj.pos(), i.e. y in "switch y :=
>> x.(type) {".
>> - for copying of named return parameters, use ast.ReturnStmt.Return
>> - for range loop index variables, use ast.RangeStmt.Range.
>> - for synthetic stores in init and testmain, use NoPos.
>>
>> Would you care to make this slightly larger change?  Try to think of a
>> good test; I will too.
>>
>> cheers
>> alan
>>
>> PS: I'm sure you're aware the tree is currently frozen, so we won't be
>> able to commit this CL till June.
>>
>>
>> https://codereview.appspot.com/92880043/
>>
>
>

[neelance, 2014-08-24 15:24:51]

Hi Alan,

I finally took the time to implement the position for ssa.Store in the way you
described. Sorry that it took so long, your explanations were very exhaustive
and helpful, it just stayed so long on my todo list.

The implementation is according to what you wrote, except that there is no
ast.RangeStmt.Range, so I used ast.RangeStmt.For instead.

I created a test file along the way, currently checking the output by hand. I
could add comments that describe the positions of the ssa.Store instructions and
an automatic test could validate those. Good approach?

Cheers,
Richard

On 2014/05/08 11:00:37, mail_richard-musiol.de wrote:
> That's exactly why I was refraining from doing the larger change, because I
> did not have the in-depth knowledge of all those cases where store is used.
> I will work on the larger change now, thanks for your help.
> 
> 
> 2014-05-07 18:42 GMT+02:00 Alan Donovan <mailto:adonovan@google.com>:
> 
> > Oh, I forgot you'll also need to provide the pos field each time you
> > construct an "address" struct, which happens in addr() and compLit().
> > The relevant positions to use are:
> > - ast.Ident.Pos(), if its the address of a var.
> > - compLit: ast.CompositeLit.Lbrace for the whole, or e.Pos() for each
> > element
> > - ast.SelectorExpr.Sel.Pos
> > - ast.IndexExpr.Lbrack
> >
> > In a statement such as:
> >   y = x
> > we prefer to associate the store with 'y' not with '='.  This is simpler
> > to implement, and clearer when there are multiple stores such as in:
> >    a, b = c
> > or
> >    a, b = c, d
> >
> > In many stores such as x++ and x[i]++ there is no '=' token anyway.  (The
> > Store positions would be those of 'x' and '[', respectively).
> >
> >
> > On 7 May 2014 10:51, <mailto:adonovan@google.com> wrote:
> >
> >> On 2014/05/07 14:16:49, gobot wrote:
> >>
> >>> mailto:R=adonovan@google.com (assigned by mailto:crawshaw@golang.org)
> >>>
> >>
> >> Hi Richard,
> >>
> >> Thanks for sending this.  I think adding position info to Stores is a
> >> great idea---I have wanted the same thing myself---and your code change
> >> is impressively concise, but I'm concerned that the currentPos trick
> >> merely hides complexity rather than eliminates it.  Adding a token.Pos
> >> parameter to emitStore seems like the right approach since it allows us
> >> to see in all cases exactly what position is associated with each Store,
> >> to control it more precisely than currentPos allows us, and to document
> >> it usefully.
> >>
> >> Here are my suggested positions for each kind of store:
> >>
> >> - keep using ast.StarExpr.Star when explicit in the source.
> >> - for n:1 var initializers, use the lhs ast.Ident.Pos()
> >> - for stores to the varargs slice, use ast.CallExpr.Args[i].Pos().
> >> - for composite literals stores to array/slice/maps, use
> >> ast.CompositeLiteral.Lbrace.
> >> - for typecase temporaries, use obj.pos(), i.e. y in "switch y :=
> >> x.(type) {".
> >> - for copying of named return parameters, use ast.ReturnStmt.Return
> >> - for range loop index variables, use ast.RangeStmt.Range.
> >> - for synthetic stores in init and testmain, use NoPos.
> >>
> >> Would you care to make this slightly larger change?  Try to think of a
> >> good test; I will too.
> >>
> >> cheers
> >> alan
> >>
> >> PS: I'm sure you're aware the tree is currently frozen, so we won't be
> >> able to commit this CL till June.
> >>
> >>
> >> https://codereview.appspot.com/92880043/
> >>
> >
> >

[adonovan, 2014-08-25 15:30:44]

On 2014/08/24 15:24:51, neelance wrote:
> Hi Alan,
> 
> I finally took the time to implement the position for ssa.Store in the way you
> described. Sorry that it took so long, your explanations were very exhaustive
> and helpful, it just stayed so long on my todo list.
> 
> The implementation is according to what you wrote, except that there is no
> ast.RangeStmt.Range, so I used ast.RangeStmt.For instead.
> 
> I created a test file along the way, currently checking the output by hand. I
> could add comments that describe the positions of the ssa.Store instructions
and
> an automatic test could validate those. Good approach?
> 
> Cheers,
> Richard
> 
> On 2014/05/08 11:00:37, mail_richard-musiol.de wrote:
> > That's exactly why I was refraining from doing the larger change, because I
> > did not have the in-depth knowledge of all those cases where store is used.
> > I will work on the larger change now, thanks for your help.
> > 
> > 
> > 2014-05-07 18:42 GMT+02:00 Alan Donovan <mailto:adonovan@google.com>:
> > 
> > > Oh, I forgot you'll also need to provide the pos field each time you
> > > construct an "address" struct, which happens in addr() and compLit().
> > > The relevant positions to use are:
> > > - ast.Ident.Pos(), if its the address of a var.
> > > - compLit: ast.CompositeLit.Lbrace for the whole, or e.Pos() for each
> > > element
> > > - ast.SelectorExpr.Sel.Pos
> > > - ast.IndexExpr.Lbrack
> > >
> > > In a statement such as:
> > >   y = x
> > > we prefer to associate the store with 'y' not with '='.  This is simpler
> > > to implement, and clearer when there are multiple stores such as in:
> > >    a, b = c
> > > or
> > >    a, b = c, d
> > >
> > > In many stores such as x++ and x[i]++ there is no '=' token anyway.  (The
> > > Store positions would be those of 'x' and '[', respectively).
> > >
> > >
> > > On 7 May 2014 10:51, <mailto:adonovan@google.com> wrote:
> > >
> > >> On 2014/05/07 14:16:49, gobot wrote:
> > >>
> > >>> mailto:R=adonovan@google.com (assigned by mailto:crawshaw@golang.org)
> > >>>
> > >>
> > >> Hi Richard,
> > >>
> > >> Thanks for sending this.  I think adding position info to Stores is a
> > >> great idea---I have wanted the same thing myself---and your code change
> > >> is impressively concise, but I'm concerned that the currentPos trick
> > >> merely hides complexity rather than eliminates it.  Adding a token.Pos
> > >> parameter to emitStore seems like the right approach since it allows us
> > >> to see in all cases exactly what position is associated with each Store,
> > >> to control it more precisely than currentPos allows us, and to document
> > >> it usefully.
> > >>
> > >> Here are my suggested positions for each kind of store:
> > >>
> > >> - keep using ast.StarExpr.Star when explicit in the source.
> > >> - for n:1 var initializers, use the lhs ast.Ident.Pos()
> > >> - for stores to the varargs slice, use ast.CallExpr.Args[i].Pos().
> > >> - for composite literals stores to array/slice/maps, use
> > >> ast.CompositeLiteral.Lbrace.
> > >> - for typecase temporaries, use obj.pos(), i.e. y in "switch y :=
> > >> x.(type) {".
> > >> - for copying of named return parameters, use ast.ReturnStmt.Return
> > >> - for range loop index variables, use ast.RangeStmt.Range.
> > >> - for synthetic stores in init and testmain, use NoPos.
> > >>
> > >> Would you care to make this slightly larger change?  Try to think of a
> > >> good test; I will too.
> > >>
> > >> cheers
> > >> alan
> > >>
> > >> PS: I'm sure you're aware the tree is currently frozen, so we won't be
> > >> able to commit this CL till June.
> > >>
> > >>
> > >> https://codereview.appspot.com/92880043/
> > >>
> > >
> > >

Great!  Can you upload the change?  The diff seems as it always was.

cheers
alan

[neelance, 2014-08-25 15:38:06]

On 2014/08/25 15:30:44, adonovan wrote:
> On 2014/08/24 15:24:51, neelance wrote:
> > Hi Alan,
> > 
> > I finally took the time to implement the position for ssa.Store in the way
you
> > described. Sorry that it took so long, your explanations were very
exhaustive
> > and helpful, it just stayed so long on my todo list.
> > 
> > The implementation is according to what you wrote, except that there is no
> > ast.RangeStmt.Range, so I used ast.RangeStmt.For instead.
> > 
> > I created a test file along the way, currently checking the output by hand.
I
> > could add comments that describe the positions of the ssa.Store instructions
> and
> > an automatic test could validate those. Good approach?
> > 
> > Cheers,
> > Richard
> > 
> > On 2014/05/08 11:00:37, mail_richard-musiol.de wrote:
> > > That's exactly why I was refraining from doing the larger change, because
I
> > > did not have the in-depth knowledge of all those cases where store is
used.
> > > I will work on the larger change now, thanks for your help.
> > > 
> > > 
> > > 2014-05-07 18:42 GMT+02:00 Alan Donovan <mailto:adonovan@google.com>:
> > > 
> > > > Oh, I forgot you'll also need to provide the pos field each time you
> > > > construct an "address" struct, which happens in addr() and compLit().
> > > > The relevant positions to use are:
> > > > - ast.Ident.Pos(), if its the address of a var.
> > > > - compLit: ast.CompositeLit.Lbrace for the whole, or e.Pos() for each
> > > > element
> > > > - ast.SelectorExpr.Sel.Pos
> > > > - ast.IndexExpr.Lbrack
> > > >
> > > > In a statement such as:
> > > >   y = x
> > > > we prefer to associate the store with 'y' not with '='.  This is simpler
> > > > to implement, and clearer when there are multiple stores such as in:
> > > >    a, b = c
> > > > or
> > > >    a, b = c, d
> > > >
> > > > In many stores such as x++ and x[i]++ there is no '=' token anyway. 
(The
> > > > Store positions would be those of 'x' and '[', respectively).
> > > >
> > > >
> > > > On 7 May 2014 10:51, <mailto:adonovan@google.com> wrote:
> > > >
> > > >> On 2014/05/07 14:16:49, gobot wrote:
> > > >>
> > > >>> mailto:R=adonovan@google.com (assigned by mailto:crawshaw@golang.org)
> > > >>>
> > > >>
> > > >> Hi Richard,
> > > >>
> > > >> Thanks for sending this.  I think adding position info to Stores is a
> > > >> great idea---I have wanted the same thing myself---and your code change
> > > >> is impressively concise, but I'm concerned that the currentPos trick
> > > >> merely hides complexity rather than eliminates it.  Adding a token.Pos
> > > >> parameter to emitStore seems like the right approach since it allows us
> > > >> to see in all cases exactly what position is associated with each
Store,
> > > >> to control it more precisely than currentPos allows us, and to document
> > > >> it usefully.
> > > >>
> > > >> Here are my suggested positions for each kind of store:
> > > >>
> > > >> - keep using ast.StarExpr.Star when explicit in the source.
> > > >> - for n:1 var initializers, use the lhs ast.Ident.Pos()
> > > >> - for stores to the varargs slice, use ast.CallExpr.Args[i].Pos().
> > > >> - for composite literals stores to array/slice/maps, use
> > > >> ast.CompositeLiteral.Lbrace.
> > > >> - for typecase temporaries, use obj.pos(), i.e. y in "switch y :=
> > > >> x.(type) {".
> > > >> - for copying of named return parameters, use ast.ReturnStmt.Return
> > > >> - for range loop index variables, use ast.RangeStmt.Range.
> > > >> - for synthetic stores in init and testmain, use NoPos.
> > > >>
> > > >> Would you care to make this slightly larger change?  Try to think of a
> > > >> good test; I will too.
> > > >>
> > > >> cheers
> > > >> alan
> > > >>
> > > >> PS: I'm sure you're aware the tree is currently frozen, so we won't be
> > > >> able to commit this CL till June.
> > > >>
> > > >>
> > > >> https://codereview.appspot.com/92880043/
> > > >>
> > > >
> > > >
> 
> Great!  Can you upload the change?  The diff seems as it always was.
> 
> cheers
> alan

Damn, I just did "hg change" and saw that the description changed, but I did not
check the diffs. Now I found "hg upload". Please look again. :-)

Cheers,
Richard

[adonovan, 2014-08-25 16:23:38]

Looks good.

Could you change the documentation for ssa.Store to read:

// Pos() returns the position of the source-level construct most closely
associated with the memory store operation.
// Since implicit memory stores are numerous and varied and depend upon
implementation choices, the details are not specified.

and ssa.UnOp:

// Pos() returns the ast.UnaryExpr.OpPos or ast.RangeStmt.TokPos (for
// the implicit ARROW op in ranging over a channel), if explicit in the source.
// For implicit memory loads (STAR), Pos() returns the position of the most
closely
// associated source-level construct; the details are not specified.

[or "RangeStmt.For", if that's what we decide]

https://codereview.appspot.com/92880043/diff/50001/go/ssa/builder.go
File go/ssa/builder.go (right):

https://codereview.appspot.com/92880043/diff/50001/go/ssa/builder.go#newcode348
go/ssa/builder.go:348: fmt.Println(e)
delete

https://codereview.appspot.com/92880043/diff/50001/go/ssa/builder.go#newcode1046
go/ssa/builder.go:1046: e = kv.Value
pos = kv.Colon 
?

https://codereview.appspot.com/92880043/diff/50001/go/ssa/builder.go#newcode1058
go/ssa/builder.go:1058: b.exprInPlace(fn, &address{addr: faddr, pos: e.Pos(),
expr: e}, e, isZero)
Use kv.Colon for named fields?

https://codereview.appspot.com/92880043/diff/50001/go/ssa/builder.go#newcode1805
go/ssa/builder.go:1805: k, v, loop, done = b.rangeIndexed(fn, x, tv, s.For)
Perhaps we should be consistently using RangeStmt.TokPos or .For in all three
cases here.  Any thoughts?

[neelance, 2014-09-18 22:57:03]

>
https://codereview.appspot.com/92880043/diff/50001/go/ssa/builder.go#newcode1805
> go/ssa/builder.go:1805: k, v, loop, done = b.rangeIndexed(fn, x, tv, s.For)
> Perhaps we should be consistently using RangeStmt.TokPos or .For in all three
> cases here.  Any thoughts?

I think .For would be better, since there is no .TokPos in case of "for range c
{ ... }", especially since this syntax is now possible for channels, too (
https://code.google.com/p/go/issues/detail?id=6102 ).

I've changed the documentation and addressed your other comments.

[adonovan, 2014-09-19 17:01:14]

On 2014/09/18 22:57:03, neelance wrote:
> >
>
https://codereview.appspot.com/92880043/diff/50001/go/ssa/builder.go#newcode1805
> > go/ssa/builder.go:1805: k, v, loop, done = b.rangeIndexed(fn, x, tv, s.For)
> > Perhaps we should be consistently using RangeStmt.TokPos or .For in all
three
> > cases here.  Any thoughts?
> 
> I think .For would be better, since there is no .TokPos in case of "for range
c
> { ... }", especially since this syntax is now possible for channels, too (
> https://code.google.com/p/go/issues/detail?id=6102 ).

Good point.


> I've changed the documentation and addressed your other comments.

LG.  I can't think of a good robust test.

This CL will have to wait until the tree thaws for Go 1.4 before we submit it.

cheers
alan

[gobot, 2014-12-19 05:18:58]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/92880043 is best) in the description in your
new CL.

Thanks very much.