It's also wrong here:

var x []int
if len(x) > 0 {
  fmt.Println(x[0])
}

My guess is that we need to handle len(...) > 0 comparisons the same way we handle nil checks, here:
https://cs.opensource.google/go/x/tools/+/master:go/analysis/passes/nilness/nilness.go;l=124;drc=5fcc6273f47e89fc27e8e4adfe4dfc7c336ba637

This comparison is emitted for a range statement, so adding this check should fix both cases.

CC @adonovan @timothy-king

Thanks, there's a missing nilness fact: when you enter a "range slice" loop, you learn that slice is not nil.

It's a little tricky because the RangeStmt has been melted down, so you're left with a recursive equation about the loop index. The easiest fix is to peek at the "rangeindex" phi, but it's a little sleazy.

func f19(slice []int) {
	if slice == nil {
		for i := range slice {
			println(slice[i])
		}
	}
}

/*
   func f19(slice []int):
0:                                                                entry P:0 S:2
	t0 = slice == nil:[]int                                            bool
	if t0 goto 1 else 2
1:                                                              if.then P:1 S:1                   # fact: slice == nil
	t1 = len(slice)                                                     int                  # consequence: t1 = 0
	jump 3
2:                                                              if.done P:2 S:0
	return
3:                                                      rangeindex.loop P:2 S:2
	t2 = phi [1: -1:int, 4: t3] #rangeindex                             int    # well this looks awfully like a loop entered from block 1...
	t3 = t2 + 1:int                                                     int
	t4 = t3 < t1                                                       bool
	if t4 goto 4 else 2
4:                                                      rangeindex.body P:1 S:1
	t5 = &slice[t3]                                                    *int   # t3 is a rangeloop index var bounded by t1 which we know is zero.
	t6 = *t5                                                            int
	t7 = println(t6)                                                     ()
	jump 3

*/

Change https://go.dev/cl/563476 mentions this issue: go/analysis/passes/nilness: improve "for range []T(nil)" error

I think of these as quite similar to nilness's "impossible condition:" warnings.

For this example #65674 (comment)

if len(x) > 0 { // warn that the comparison is always false.

This is solvable by inferring from x == nil that len(x) == 0.

For this example #65674 (comment) :

    for i, j := range a { // warn that a is always nil (and the body is deadcode?)

The relevant comparison that is 'impossible' in the example is:

	t4 = t3 < t1                                                       bool
	if t4 goto 4 else 2

I do not have a non-'sleazy' suggestion for inferring t3 >= 0 quickly (i.e. cheaper than an interval analysis fixed point). I feel like this is an argument for just having an improved message that requires knowing this is a 'rangeindex' https://go-review.git.corp.google.com/c/tools/+/563476 .

(Hit the wrong buttons while editing. Reopening)

I think of these as quite similar to nilness's "impossible condition:" warnings.

For this example #65674 (comment)

if len(x) > 0 { // warn that the comparison is always false.

I agree; it seems in keeping with the existing scope.

This is solvable by inferring from x == nil that len(x) == 0.

For this example #65674 (comment) :

That's true, but nilness doesn't concern itself with lengths of things, and I don't think it should start down that road just to fix this issue.

    for i, j := range a { // warn that a is always nil (and the body is deadcode?)

The attached CL does warn that the slice is nil; see these test cases:

		if len(slice) > 3 {
			_ = slice[2] // want "indexing a nil slice"
		}
		for i := 0; i < len(slice); i++ {
			_ = slice[i] // want "indexing a nil slice"
		}

But it's not clear what "body" means in SSA; there's only a control flow graph.

The relevant comparison that is 'impossible' in the example is:

	t4 = t3 < t1                                                       bool
	if t4 goto 4 else 2

I do not have a non-'sleazy' suggestion for inferring t3 >= 0 quickly (i.e. cheaper than an interval analysis fixed point). I feel like this is an argument for just having an improved message that requires knowing this is a 'rangeindex' https://go-review.git.corp.google.com/c/tools/+/563476 .

Exactly; it's a slippery slope that leads to static bounds checking, and I want nilness to touch it.

(BTW: Don't forget to convert URLs to https://go.dev/cl/563476 form for those not on our network.)

Lasse suggested not visiting the range loop when the operand is a nil slice. The logic for that would be something like this:

		// Detect loop-over-nil-slice: a "{for,rangeindex}.loop"
		// block whose condition is "x < len(y)" where y is nil.
		// Don't visit the loop, to avoid printing "indexing a nil slice"
		// for operation in the loop body.
		if If, ok := b.Instrs[len(b.Instrs)-1].(*ssa.If); ok && b.Comment == "rangeindex.loop" || b.Comment == "for.loop" {
			if binop, ok := If.Cond.(*ssa.BinOp); ok && binop.Op == token.LSS {
				if call, ok := binop.Y.(*ssa.Call); ok {
					common := call.Common()
					if blt, ok := common.Value.(*ssa.Builtin); ok && blt.Name() == "len" {
						if nilnessOf(stack, common.Args[0]) == isnil {
							tsucc := b.Succs[0]
							for _, d := range b.Dominees() {
								if d != tsucc {
									visit(d, stack)
								}
							}
							return
						}
					}
				}
			}
		}

This logic is subtle--since it peeks at block comments--and incomplete; for example it can't handle a for-loop with a condition such as len(slice) && cond.

Its effect would be to suppress all the range- and for-loop errors:

		for range slice { // nothing to report here
		}
		for _, v := range slice { // want "indexing a nil slice"
			_ = v
		}
		for i := range slice {
			_ = slice[i] // want "indexing a nil slice"
		}
		for i := range slice {
			if i < len(slice) {
				_ = slice[i] // want "indexing a nil slice"
			}
		}
		for i := 0; i < len(slice); i++ {
			_ = slice[i] // want "indexing a nil slice"
		}

but to keep reporting an error in this case:

		if len(slice) > 3 {
			_ = slice[2] // want "indexing a nil slice"
		}

Personally I don't like the complexity of the extra logic, and would rather be made aware of all of these errors since they indicate an authorial mistake (or that code to populate the slice has been temporarily commented out, but that's ok).

Another option is simply to never report an error when indexing a nil slice.

What do you think?

In my opinion it is fine to report diagnostics here but be clear about what they mean. For example:

for _, v := range slice { // want "indexing a nil slice"
	_ = v
}

I agree that this is clearly an authorial mistake but I don't think the diagnostic message is very clear about what exactly the user has been doing wrong.

There is not even an indexing operation in that line. The compiler under the hood might generate an indexing operation here but is this some that should be reported to the user (and something we can expect every user to know)?

While I was able to figure out what was wrong fairly fast (partially because my example was very simple), it suprised me to see the nilness analyzer reporting a diagnostic.

In the end it is your decision (and I can't judge the cost of the extra complexity) but I think there is room for improvement to make the finding less surprising.

How about changing the message to "index/range of nil slice"?

#65674 (comment)
2cents: I think all of those lines deserve a warning. Preferably placing the warning on the slice in range slice or the range slice together. I think @lfolger has a point that we want avoid mentioning indexing if unneeded to emphasize the "range" part with the diagnostic that we give. "range of nil slice" seems okay to me.

		for range slice { // want "range of nil slice"
		}
		for _, v := range slice { // want "range of nil slice"
			_ = v
		}
		for i := range slice {  // want "range of nil slice"
			_ = slice[i]
		}
		for i := range slice {  // want "range of nil slice"
			if i < len(slice) {
				_ = slice[i] 
			}
		}
		for i := 0; i < len(slice); i++ {
			_ = slice[i] // want "indexing a nil slice"
		}