go-tools
go-tools copied to clipboard
staticcheck: thoughts on improving SA4023 and validating non-nil returns
(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.)