minimock's generated mocks fail staticcheck's SA5011 (possible nil pointer dereference)

[alexandre-normand]

I was trying to use minimock last week and found that the generated mocks were failing staticcheck's SA5011 - Possible nil pointer dereference.

Example:

cacher_mock_test.go:1921:65: possible nil pointer dereference (SA5011)

I ended up postponing using minimock for that reason but would love to revisit eventually.

Thanks!

[hexdigest]

Hi @alexandre-normand ,

Can you please post some generated code around 1921 line?

P.S. lot's of these static check tools actually give false positives. If it's possible it doesn't mean that this will ever happen.

[alexandre-normand]

Hi @alexandre-normand ,

Can you please post some generated code around 1921 line?

P.S. lot's of these static check tools actually give false positives. If it's possible it doesn't mean that this will ever happen.

Yeah, it's a fair point. I ended up having to redo a test from scratch so it's a bit different but I got the same staticcheck errors (just a different line):

cacher_mock_test.go:259:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:476:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:694:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:912:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1072:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1271:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1487:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1705:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1705:42: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1705:60: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1923:24: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1923:46: possible nil pointer dereference (SA5011)
cacher_mock_test.go:1923:65: possible nil pointer dereference (SA5011)

This is from an interface like this:

//go:generate go run github.com/gojuno/minimock/v3/cmd/minimock@v3.3.6 -i Cacher
type Cacher[K any, V any] interface {
    cache.TTLMultiCacher[K, V]
}

And taking a sample of the generated mock for the first instance of the SA5011 check getting raised:

247:    if mmAdd.AddMock.defaultExpectation != nil {
248:        mm_atomic.AddUint64(&mmAdd.AddMock.defaultExpectation.Counter, 1)
249:        mm_want := mmAdd.AddMock.defaultExpectation.params
250:        mm_got := CacherMockAddParams[K, V]{ctx, key, value}
251:        if mm_want != nil && !minimock.Equal(*mm_want, mm_got) {
252:            mmAdd.t.Errorf("CacherMock.Add got unexpected parameters, want: %#v, got: %#v%s\n", *mm_want, mm_got, minimock.Diff(*mm_want, mm_got))
253:        }
254:
255:        mm_results := mmAdd.AddMock.defaultExpectation.results
256:        if mm_results == nil {
257:            mmAdd.t.Fatal("No results are set for the CacherMock.Add")
258:        }
259:        return (*mm_results).err
260:    }

The return (*mm_results).err is the one that is getting flagged and it's obvious looking at that code that if it's nil, we're going to abort with the mmAdd.t.Fatal("No results are set for the CacherMock.Add") call.

I agree that this is one of those cases where the checker could be better (it has no knowledge of this being a testing.T and how the t.Fatal call is essentially aborting the flow) but it's just a little awkward to deal with this in practice. The workarounds in how those linter checks are integrated for us are:

• Add a //nolint:SA5011 on those lines but this is not really an option for users since that would defeat the purpose of generating code.
• Generate the mock(s) under a generated sub-directory. This isn't terrible except that it now technically makes some mocks exported since they have to be generated without the _test.go suffix in order to be available to the packages needing the mocks for testing.

I think ideally that linter would just ignore all files with the standard // Code generated header but this is beyond my sphere of control. I totally understand if addressing all the possible linter false positives like this one is not desirable / reasonable but I thought I'd report it just in case since I saw that there was some effort put into generating linter compliant code as hinted by this line in the README:

It generates code that passes gometalinter checks.

I'm still hoping to try minimock again when I get a chance but since I didn't know about the generated sub-directory workaround last week, I ended up reverting that code back to gomock. That will likely be the acceptable (even if suboptimal) workaround when I try again.

[zcolleen]

Hey @alexandre-normand! As i see gometalinters are deprecated and its recommended to switch to golangci-lint. I have checked this issue on golangci-lint and it is not reproducing. Seems that this issue is fixed somehow in golangci-lint. May be the best solution here is just to switch to golangci-lint?