False positive: Optional proto-fields being flagged and fixed if checking if the field is nil

[muffehazard]

I have a multiple proto messages with optional fields defined, for example:

message Result {
  string data = 1;
  optional int32 parent = 2;
}

This generates a getter for the optional field like this:

func (x *Result) GetParent() int32 {
    if x != nil && x.Parent != nil {
        return *x.Parent
    }
    return 0
}

The linter flags and fixes accessing this field to use this getter method as follows:

if result.GetParent() != nil {
    ....
}

This obviously doesn't work. Checking for zero here doesn't work, as zero is a valid value for this field and is for that reason marked as optional so that I can check if the field is nil instead.

Should the linter only fix cases where we are directly de-referencing the value from the pointer?

So it could fix this:

val := *result.Parent

But not this:

val := result.Parent

[ghostiam]

Hello, thanks for the issue!

Yes, I discovered a problem with optional fields and am trying to solve it.

Linter suggests using getter if it exists, because if you try to get a nested field, even one that is not a reference field, since the parent message may be nil.

But in this case it is a bug in the linter, I will try to fix it as soon as possible.

[ghostiam]

Please check the new linter update https://github.com/ghostiam/protogetter/releases/latest
I tried to fix the behavior when comparing with nil.
The problem with assignment was fixed here #7 and is also available in the new version.

[muffehazard]

The new version works well, no more unnecessary linter warnings 👍

[arvidfm]

Hi, sorry to dig up an old issue, but it sounds like this issue was supposed to address what I'm trying to do, and I'm still having issues with optional fields.

I have a message like this:

message MyMessage {
  optional int32 my_field = 1;
}

and I want to write a function that returns my_field as an *int if it's set, and nil if it's unset:

func getValue(msg *MyMessage) *int {
	if msg != nil && msg.MyField != nil {
		value := int(*msg.MyField)
		return &value
	}
	return nil
}

but this results in the following error:

avoid direct access to proto field *msg.MyField, use msg.GetMyField() instead (protogetter)
                value := int(*msg.MyField)

But GetMyField() returns an int32 instead of an *int32, so there's no way to tell whether the field was set or not, and I need to distinguish between an unset field and a field set to 0.

How should I write this code to make the linter happy? I'm using golangci-lint 1.57.2 which I believe uses protogetter 0.3.5.

Incidentally, I get the same warning with code like this:

var value *int32
if msg != nil {
	value = msg.MyField
}

[ghostiam]

@arvidfm, Hi, thanks for the question.
The purpose of this linter is to avoid the use of nil values and possible panic.

You can silence the linter using a comment:

var value *int32
if msg != nil {
	//nolint:protogetter
	value = msg.MyField
}

Also, the linter does not prohibit you from checking the field for nil, just use a getter if you want to get the value:

func getValue(msg *MyMessage) *int {
	if msg != nil && msg.MyField != nil {
-		value := int(*msg.MyField)
+		value := int(msg.GetMyField())
		return &value
	}
	return nil
}

[arvidfm]

@ghostiam A-ha! Thank you, the second suggestion did the trick! I didn't realise that the if was still fine.