When positions doesn't matter

[lindell]

I've come across a problem where I want to copy everything except the position.
To solve this problem I cloned astcopy.go and made changes to all functions.

For example, this is how the UnaryExpr is now copied:

func UnaryExpr(x *ast.UnaryExpr) *ast.UnaryExpr {
	if x == nil {
		return nil
	}
	return &ast.UnaryExpr{
		Op: x.Op,
		X:  copyExpr(x.X),
	}
}

Is this something you would like to incorporate into this repo. Otherwise I will clone this repo and make an alternative version of it.

[quasilyte]

This is a good point.
I need to think about it at least a little more.
I guess not copying the position info by default makes sense, but I need to check packages that depend on the astcopy to see whether they break otherwise.

It could also be beneficial if you'll share your use case where copied position info makes things worse, just for the context. Thank you.

[quasilyte]

CC @cristaloleg

[lindell]

I do not suggest that we break the current implementation, just that we maybe ad (in some way) this one.

[lindell]

My use case is that I'm trying to create a Stub/Mock creator and therefore want to copy arguments and results of function calls. When I've built the syntax tree, I format it with format.Node. When I do this without removing any position, this is the result:

func// Func2 mock
(m *Mocked) Func2(str string, str2 string) {
        return m.Func2Func(str, str2)
}

Which is technically correct go code. But obviusly not what I want. When I strip any positions the result is

// Func2 mock
func (m *Mocked) Func2(str string, str2 string) {
        return m.Func2Func(str, str2)
}

[quasilyte]

Got it.
I think use cases inside go-critic linter don't require position info copies as well.
One option is to release v2 which doesn't copy position info.
If user wants to inherit positions, it's usually as simple as:

cp := astcopy.BinaryExpr(orig)
cp.OpPos = orig.OpPos

Or, if only Pos() result is concerned:

cp := astcopy.BinaryExpr(orig)
cp.X.Pos = orig.X.Pos

Another way is to provide a set of functions that do not copy pos info, so backwards-compatibility is preserved. Although the exported function set will increase in almost 2 times.

Or we can do it in a same way how encoding/binary exports both little and big endian interfaces.
The exported functions will refer to a default copying algorithm that does copy positions. User could re-assign default copying algorithm to one that does not (both of them will be exported). If thread-safety is a concern, one may create an instance of copy making object and use it's methods instead of exported functions.

[quasilyte]

ping @cristaloleg

[cristaloleg]

I'm voting for the:

One option is to release v2 which doesn't copy position info.

I think we can break compatibility for know and see how many users will ask about an ability to copy pos automatically.

Objections?

[cristaloleg]

Ooops. Wanted to ping you :)

[lindell]

I have since long moved on from this problem (Used string templates instead) 🙂 . But I do at least have no objection to this.