Repository navigation
staticcheck: thoughts on improving SA4023 and validating non-nil returns #845
Description
(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.)
Activity
dominikh commentedon Oct 10, 2020
That's why the check only flags those "never nil" where it believes that the function might actually return a typed nil. For example, the following won't trigger SA4023:
Under the assumption that fn2 is merely being defensive against future changes in fn.
Specifically, the check requires two conditions to be true: a) the called function never returns an untyped nil b) the called function returns a value that might be nil, such as a slice indexing operation, a map indexing operation, returning a parameter, and so on. Essentially any value we can't statically determine to be non-nil.
The reason your example gets matched is that it returns a global variable, and we don't know the dynamic value of a global variable. We could attempt to handle globals specially, and we might have to.
We can't generally validate that a function never returns a nil value without either forbidding a lot of Go constructs, or requiring redundant nil checks.
That would be a solution, yes. When I ran the new check on some major projects (stdlib, k8s, istio, …) I didn't find any results, neither true positives nor false positives.
dominikh commentedon Oct 10, 2020
5dbe911 adds special handling for global variables.
But looking at your original code, I'm not sure why it ever got flagged to begin with? We only care about comparisons where the lhs has an interface type, but ValueFromCtx returns a concrete type. And I can't actually reproduce a diagnostic.
ainar-g commentedon Oct 10, 2020
Hm, I guess I went too far in simplifying and anonymising the code. Sorry about that! The original code used interfaces, and the default was returned by a function. I did some digging and the results are… weird. Both 9cb6a0a and 5dbe911 mark only functions which:
Here are my silly test cases. The cases where both versions complain are marked in the comments.
dominikh commentedon Oct 10, 2020
I'm not sure we can do any better for functions
AorC. They always both return a typed value (astruct{I}being put in the interface value, andv.(I)guarantees that v is not untyped nil), and may intend to return a (typed) nil, becausev.(I)could be some typed nil pointer. That combination matches our heuristic of "the user may have expected a typed nil value in an interface to equal untyped nil. The function never returns untyped nils, it may intend to return typed nils, and the user is trying to check against untyped nil."SA4023: flag impossible comparisons of interface values with untyped nil
andig commentedon Mar 31, 2024
dominikh commentedon Mar 31, 2024
I'm afraid I have no idea what the type of
lp.circuitis or whatrequire.Nilis.andig commentedon Mar 31, 2024
Sorry for the confusion. staticcheck is actually right.
lp.circuitis pointer to struct. Returning it implies that type (pointer to struct), hence dynamic type is no longer nil even iflp.circuitis nil. Worked around this by checking its value and returning untyped nil:I would never have spotted this bug without staticcheck...