go/analysis/passes/nilness: improve "for range []T(nil)" error

Previously, the error message referred to a "nil dereference
in index operation", which is true, but the index operation
is unreachable because the range loop over a nil slice is
degenerate. Rephrase it to "index of nil slice",
which doesn't promise that the operation will panic,
or "range over nil slice" if we can infer that the
IndexAddr operation came from range-over-slice lowering.

Also, add cases for range over a nil map, *array, and chan,
with tests.

Fixes golang/go#65674

Change-Id: I1bf89bd1118b191a493bc2f230cb1388ff7a9b3b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563476
Reviewed-by: Lasse Folger <[email protected]>
Reviewed-by: Tim King <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

go/analysis/passes/nilness/nilness.go

@@ -52,7 +52,7 @@ func runFunc(pass *analysis.Pass, fn *ssa.Function) {
 	// notNil reports an error if v is provably nil.
 	notNil := func(stack []fact, instr ssa.Instruction, v ssa.Value, descr string) {
 		if nilnessOf(stack, v) == isnil {
-			reportf("nilderef", instr.Pos(), "nil dereference in "+descr)
+			reportf("nilderef", instr.Pos(), descr)
 		}
 	}
 
@@ -77,29 +77,50 @@ func runFunc(pass *analysis.Pass, fn *ssa.Function) {
 				// A nil receiver may be okay for type params.
 				cc := instr.Common()
 				if !(cc.IsInvoke() && typeparams.IsTypeParam(cc.Value.Type())) {
-					notNil(stack, instr, cc.Value, cc.Description())
+					notNil(stack, instr, cc.Value, "nil dereference in "+cc.Description())
 				}
 			case *ssa.FieldAddr:
-				notNil(stack, instr, instr.X, "field selection")
+				notNil(stack, instr, instr.X, "nil dereference in field selection")
 			case *ssa.IndexAddr:
-				notNil(stack, instr, instr.X, "index operation")
+				switch typeparams.CoreType(instr.X.Type()).(type) {
+				case *types.Pointer: // *array
+					notNil(stack, instr, instr.X, "nil dereference in array index operation")
+				case *types.Slice:
+					// This is not necessarily a runtime error, because
+					// it is usually dominated by a bounds check.
+					if isRangeIndex(instr) {
+						notNil(stack, instr, instr.X, "range of nil slice")
+					} else {
+						notNil(stack, instr, instr.X, "index of nil slice")
+					}
+				}
 			case *ssa.MapUpdate:
-				notNil(stack, instr, instr.Map, "map update")
+				notNil(stack, instr, instr.Map, "nil dereference in map update")
+			case *ssa.Range:
+				// (Not a runtime error, but a likely mistake.)
+				notNil(stack, instr, instr.X, "range over nil map")
 			case *ssa.Slice:
 				// A nilcheck occurs in ptr[:] iff ptr is a pointer to an array.
-				if _, ok := instr.X.Type().Underlying().(*types.Pointer); ok {
-					notNil(stack, instr, instr.X, "slice operation")
+				if is[*types.Pointer](instr.X.Type().Underlying()) {
+					notNil(stack, instr, instr.X, "nil dereference in slice operation")
 				}
 			case *ssa.Store:
-				notNil(stack, instr, instr.Addr, "store")
+				notNil(stack, instr, instr.Addr, "nil dereference in store")
 			case *ssa.TypeAssert:
 				if !instr.CommaOk {
-					notNil(stack, instr, instr.X, "type assertion")
+					notNil(stack, instr, instr.X, "nil dereference in type assertion")
 				}
 			case *ssa.UnOp:
-				if instr.Op == token.MUL { // *X
-					notNil(stack, instr, instr.X, "load")
+				switch instr.Op {
+				case token.MUL: // *X
+					notNil(stack, instr, instr.X, "nil dereference in load")
+				case token.ARROW: // <-ch
+					// (Not a runtime error, but a likely mistake.)
+					notNil(stack, instr, instr.X, "receive from nil channel")
 				}
+			case *ssa.Send:
+				// (Not a runtime error, but a likely mistake.)
+				notNil(stack, instr, instr.Chan, "send to nil channel")
 			}
 		}
 
@@ -416,3 +437,39 @@ func isNillable(t types.Type) bool {
 	}
 	return false
 }
+
+// isRangeIndex reports whether the instruction is a slice indexing
+// operation slice[i] within a "for range slice" loop. The operation
+// could be explicit, such as slice[i] within (or even after) the
+// loop, or it could be implicit, such as "for i, v := range slice {}".
+// (These cannot be reliably distinguished.)
+func isRangeIndex(instr *ssa.IndexAddr) bool {
+	// Here we reverse-engineer the go/ssa lowering of range-over-slice:
+	//
+	//      n = len(x)
+	//      jump loop
+	// loop:                                                "rangeindex.loop"
+	//      phi = φ(-1, incr) #rangeindex
+	//      incr = phi + 1
+	//      cond = incr < n
+	//      if cond goto body else done
+	// body:                                                "rangeindex.body"
+	//      instr = &x[incr]
+	//      ...
+	// done:
+	if incr, ok := instr.Index.(*ssa.BinOp); ok && incr.Op == token.ADD {
+		if b := incr.Block(); b.Comment == "rangeindex.loop" {
+			if If, ok := b.Instrs[len(b.Instrs)-1].(*ssa.If); ok {
+				if cond := If.Cond.(*ssa.BinOp); cond.X == incr && cond.Op == token.LSS {
+					if call, ok := cond.Y.(*ssa.Call); ok {
+						common := call.Common()
+						if blt, ok := common.Value.(*ssa.Builtin); ok && blt.Name() == "len" {
+							return common.Args[0] == instr.X
+						}
+					}
+				}
+			}
+		}
+	}
+	return false
+}

go/analysis/passes/nilness/testdata/src/a/a.go

@@ -54,7 +54,7 @@ func f2(ptr *[3]int, i interface{}) {
 	}
 }
 
-func g() error
+func g() error { return nil }
 
 func f3() error {
 	err := g()
@@ -242,3 +242,64 @@ func f18(x any) {
 	}
 	println(*ptr)
 }
+
+// Regression test for https://github.com/golang/go/issues/65674:
+// spurious "nil deference in slice index operation" when the
+// index was subject to a range loop.
+func f19(slice []int, array *[2]int, m map[string]int, ch chan int) {
+	if slice == nil {
+		// A range over a nil slice is dynamically benign,
+		// but still signifies a programmer mistake.
+		//
+		// Since SSA has melted down the control structure,
+		// so we can only report a diagnostic about the
+		// index operation, with heuristics for "range".
+
+		for range slice { // nothing to report here
+		}
+		for _, v := range slice { // want "range of nil slice"
+			_ = v
+		}
+		for i := range slice {
+			_ = slice[i] // want "range of nil slice"
+		}
+		{
+			var i int
+			for i = range slice {
+			}
+			_ = slice[i] // want "index of nil slice"
+		}
+		for i := range slice {
+			if i < len(slice) {
+				_ = slice[i] // want "range of nil slice"
+			}
+		}
+		if len(slice) > 3 {
+			_ = slice[2] // want "index of nil slice"
+		}
+		for i := 0; i < len(slice); i++ {
+			_ = slice[i] // want "index of nil slice"
+		}
+	}
+
+	if array == nil {
+		// (The v var is necessary, otherwise the SSA
+		// code doesn't dereference the pointer.)
+		for _, v := range array { // want "nil dereference in array index operation"
+			_ = v
+		}
+	}
+
+	if m == nil {
+		for range m { // want "range over nil map"
+		}
+		m["one"] = 1 // want "nil dereference in map update"
+	}
+
+	if ch == nil {
+		for range ch { // want "receive from nil channel"
+		}
+		<-ch    // want "receive from nil channel"
+		ch <- 0 // want "send to nil channel"
+	}
+}