Opentelemetry-go: Inconsistency with pointer and concrete type recievers

[rakyll]

core.Number has a mix of pointer and concrete receivers. E.g.

func (n *Number) AddFloat64(f float64)
func (n Number) IsNegative(kind NumberKind) bool

We don't do this in Go for the user's convenience even if we don't mutate any fields in the methods.

[jmacd]

Agreed. Number should use concrete receivers.

[iredelmeier]

This is also true for the Value methods.

[elsesiy]

I'd like to tackle this one to get familiar with the code base. The goal is to have all value receivers correct? This would break a couple test cases that currently evaluate pointer equality if I'm not mistaken..

[iredelmeier]

@elsesiy awesome! Yes re: that being the goal, at least for Number and Value.

[jmacd]

To clarify, we shouldn't change all types across the code base, just those which are commonly used as as values and are used in chained expressions. Number and Value should be fixed, for sure.

[elsesiy]

@jmacd @iredelmeier Yes I'm only focusing on Value and Number. Once I have something presentable I'll open a draft PR for you to provide feedback early on. FYI: I noticed right now is that make test fails with a bunch of race conditions.

[jmacd]

Please post details about any race conditions you observe. The tests should be passing, including with the race detector.

[elsesiy]

@jmacd I created #339 to track this with some more info.

[elsesiy]

@rakyll As I started making changes I noticed that the change is a bit more involved than anticipated.

Changing e.g. func (n *Number) AddFloat64(f float64) to a value receiver would imply that we need to change the method signature to func (n Number) AddFloat64(f float64) Number to return the result unless there's another way to mutate n but I can't think of any. I spoke briefly to @jmacd and he mentioned that you can probably shed some light on the exact things to be done to make it more idiomatic.

I think the scope of this change is limited to the following candidates in number:

• AddRaw
• AddNumber*
• AddRawAtomic
• AddNumberAtomic*

All swapX, setX and asXPtr actually seem to rightfully use a pointer receiver but I'm interested in all other opinions!

* Incl. primitive type specific operations e.g. AddFloat64

Thanks!

[jmacd]

I wonder if @rakyll could expand upon "We don't do this in Go for the user's convenience even if we don't mutate any fields in the methods."

I'm not sure I get its meaning.

[ferhatelmas]

IMHO, it means that be consistent such that if a pointer receiver is needed, always use a pointer for convenience, otherwise default to value receiver.

[jmacd]

I'm still not sure how this gives convenience. If I use a pointer receiver for a type that does not mutate the object, then I'm limiting convenience because I can't chain an expression of such things, so

  thing.MethodThatReturnsNumber().NonMutatingMethod()

is a real convenience. Mutating methods are also a convenience.

How does being consistent add convenience?

[ferhatelmas]

It should always be passed down to have a fluent API like jQuery.

For example

thing.
  Add().
  Mul().
  ReportDoesntMutate().
  Add().
  ReportDoesntMutate().
  Divide().
  End() // a finalizer to have a value, now we have the number

This is what I understood and was thinking while writing the above comment. The original comment might mean totally a different thing though.

[elsesiy]

@iredelmeier @jmacd Any idea on how to proceed here? I'm not too sure if there's anything to be done until @rakyll can elaborate a bit more on the idiomatic way..

[rakyll]

The rationale is it makes it harder for those who want to operate on their interface types. The chances are low for this package to be highly wrapped by the user, but it is a general convention and might be useful to follow for less mental overhead.

For example, the following code won't compile:

package main

func main() {
    var a Methoder = A{}
    a.Method1()
    a.Method2()

}

type Methoder interface {
    Method1()
    Method2()
}

type A struct{}

func (a *A) Method1() {}

func (a A) Method2() {}

This is why a mixture of pointer and concrete types in Go is not preferred. We should mention this at https://github.com/golang/go/wiki/CodeReviewComments#receiver-type, I'm on it.

[jmacd]

Thank you @rakyll.

For the types in question, I think we should probably use pointer receivers.

[jmacd]

(This is a fine justification.)