Skip to content

Commit

Permalink
go/analysis/passes/nilness: handle type assertions too
Browse files Browse the repository at this point in the history
This change causes nilness to detect patterns of the form:

	ptr, ok := x.(T)
	if !ok {
		println(*ptr) // nil dereference
	}

It found three bugs in k8s, e.g.

	// pkg/controller/volume/persistentvolume/pv_controller.go
	claim, ok = obj.(*v1.PersistentVolumeClaim)
	if !ok {
		return fmt.Errorf("cannot convert object from volume cache to volume %q!?: %#v", claim.Spec.VolumeName, obj)
	}

Fixes golang/go#47938

Change-Id: Idb4588ce037cb898ccc3feb460ea0b67f65803df
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560075
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
  • Loading branch information
adonovan committed Feb 7, 2024
1 parent e3f1180 commit 5fcc627
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 0 deletions.
56 changes: 56 additions & 0 deletions go/analysis/passes/nilness/nilness.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,42 @@ func runFunc(pass *analysis.Pass, fn *ssa.Function) {
}
}

// In code of the form:
//
// if ptr, ok := x.(*T); ok { ... } else { fsucc }
//
// the fsucc block learns that ptr == nil,
// since that's its zero value.
if If, ok := b.Instrs[len(b.Instrs)-1].(*ssa.If); ok {
// Handle "if ok" and "if !ok" variants.
cond, fsucc := If.Cond, b.Succs[1]
if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT {
cond, fsucc = unop.X, b.Succs[0]
}

// Match pattern:
// t0 = typeassert (pointerlike)
// t1 = extract t0 #0 // ptr
// t2 = extract t0 #1 // ok
// if t2 goto tsucc, fsucc
if extract1, ok := cond.(*ssa.Extract); ok && extract1.Index == 1 {
if assert, ok := extract1.Tuple.(*ssa.TypeAssert); ok &&
isNillable(assert.AssertedType) {
for _, pinstr := range *assert.Referrers() {
if extract0, ok := pinstr.(*ssa.Extract); ok &&
extract0.Index == 0 &&
extract0.Tuple == extract1.Tuple {
for _, d := range b.Dominees() {
if len(d.Preds) == 1 && d == fsucc {
visit(d, append(stack, fact{extract0, isnil}))
}
}
}
}
}
}
}

for _, d := range b.Dominees() {
visit(d, stack)
}
Expand Down Expand Up @@ -360,3 +396,23 @@ func (ff facts) negate() facts {
}
return nn
}

func is[T any](x any) bool {
_, ok := x.(T)
return ok
}

func isNillable(t types.Type) bool {
switch t := typeparams.CoreType(t).(type) {
case *types.Pointer,
*types.Map,
*types.Signature,
*types.Chan,
*types.Interface,
*types.Slice:
return true
case *types.Basic:
return t == types.Typ[types.UnsafePointer]
}
return false
}
26 changes: 26 additions & 0 deletions go/analysis/passes/nilness/testdata/src/a/a.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,29 @@ func f14() {
print(x)
}
}

func f15(x any) {
ptr, ok := x.(*int)
if ok {
return
}
println(*ptr) // want "nil dereference in load"
}

func f16(x any) {
ptr, ok := x.(*int)
if !ok {
println(*ptr) // want "nil dereference in load"
return
}
println(*ptr)
}

func f18(x any) {
ptr, ok := x.(*int)
if ok {
println(ptr)
// falls through
}
println(*ptr)
}

0 comments on commit 5fcc627

Please sign in to comment.