staticcheck: thoughts on improving SA4023 and validating non-nil returns

[ainar-g]

(Just sharing some halfrandom thoughts I've had after testing SA4023 on some code.)

Consider these pieces of code:

package foo

// …

// ValueFromCtx returns a value from the context.  If there is no value it
// returns DefaultT.
func ValueFromCtx(ctx context.Context) (value *T) {
	var v = ctx.Value(ctxKeyValue)
	if v == nil {
		return DefaultT
	}

	var ok bool
	if value, ok := v.(*T); !ok {
		panic("invalid value")
	} else if value == nil {
		return DefaultT
	}

	return value
}

package bar

// …

func f(ctx context.Context) {
	var v = foo.ValueFromCtx(ctx)
	if v == nil {
		return
	}

	// …
}

staticcheck (without --checks all) correctly flags the nil check in bar.f as redundant. This code is bad in that it can confuse readers into thinking that foo.ValueFromCtx can ever return nil, so this check is likely to proliferate. On the other hand, unless foo is a stdlib package or an otherwise widely known package, the behaviour may change in the future, so the nil check could be thought of as an example of good code simply being defensive.

Maybe in the future staticcheck could support a directive or a magic comment for that? The directive would make staticcheck validate that the function doesn't actually ever return a nil and also make staticcheck consider that at call sites.

In the meantime, if this actually becomes an issue for a lot of people, it may be worth considering not running this check by default and leaving it to the people who always run with --checks all to deal with it in their own way, I guess?

(Or perhaps there is a solution I'm not seeing, heh.)