proposal: cmd/vet: warn when shadowing variable with slices.Delete and friends

[Deleplace]

Proposal Details

When calling slices.Delete, shadowing the slice variable is almost certainly a bug:

s := slices.Delete(s, 2, 3)  // !! accidentally using := instead of = !!

Playground

Why

Shadowing comes with surprises when used accidentally. In general, vet cannot warn about it because it does not know if a specific use of shadowing is intentional and legit, or not.

Shadowing keeps the original var intact, so it can be reused outside of the current inner scope where the new var is declared. In the specific case of Delete (and friends), we know that reusing the original value is not desirable:
"When calling these functions (Delete. etc.) we must consider the original slice invalid"

I am not aware of any such shadowing that would be legit and useful. If they do exist, the vet warning would be easy to work around by choosing a different var identifier (no shadowing).

On the other hand, I am very aware of the nasty bugs caused by accidental shadowing in general, and by accidental shadowing in the case of slices.Delete and friends.

Suggestion

vet warning:

./prog.go:11:14: shadowing slice 's' on call to slices.Delete

Same for the friends DeleteFunc, Compact, CompactFunc, Replace.

Like #62729, the goal is to minimize the risks associated with incorrectly using the slices package API.

[ianlancetaylor]

Per cmd/vet/README, vet checks have three criteria: correctness, frequency, and precision. Is this a mistake that is frequent enough to be worth a vet check?

Separately, if we do this, what about s := append(s, ...)?

[Deleplace]

For the frequency, I have yet to collect evidence data.

For append, there is a strong difference. append does not dictate who should own the original slice. The ownership concern is on the user. Many legit patterns are used where the caller "knows" in this case append won't reallocate, or "knows" in this case append will reallocate. append is not destructive for the original contents s[0:len(s)], so in many cases other references to the original slice can still expect its content to be intact, which is fine at least for reading.

slices.Delete is destructive and invalidates all references to the original slice (which then contains previously-undefined garbage at the end, now-defined zeros at the end). De facto, the caller of Delete becomes the only owner of both the original (invalid) slice and the new slice.

[Jorropo]

slices.Delete is destructive and invalidates all references to the original slice

I don't think it's as clear as that. A slice has value beyond it's content, you might need to keep the original slice around because you are reusing it's underlying backing array to reduce allocations in your code.
slices.Delete also does not even write anything before the first i.

[timothy-king]

@Jorropo brings up a good point that one can reuse allocations. We would never want to warn on functions that takes a slice as a buffer to use:

func Foo(x, y []string) {
  s := make([]byte, 0, max(len(x), len(y)))
  PossiblyDeleteWithinSlice(s, x)
  PossiblyDeleteWithinSlice(s, y)
}

Is function scope so different than shadowing in blocks? Well yes, but it is reason to be cautious about precision here.

s := append(s, ...) has more obvious uses. Such as the prefix is still valid in a given scope, but not in the outer scope so a 'pop' is implicit for correctness.

[Deleplace]

Formally, the caller of s := slices.Delete(s, i, j) may assume s[0:i] will be kept unchanged and may want to "share ownership" of s[0:i] with other slice variables. But the original (shadowed) s itself now contains zeroed garbage at the end.

It seems for any useful purpose s = slices.Delete(s, i, j) is better than, or as good as, using :=.

If the original s is a scratch buffer, and the caller wants its size to not be changed, and its exact contents at the end doesn't matter, and the go vet warning is annoying, then using a new identifier t := slices.Delete(s, i, j) should be fine.