gopls/internal/analysis/modernize: appendclipped: unclip

The appendclipped pass must ascertain that the first argument
to append(x, y...) is clipped, so that we don't eliminate
possible intended side effects on x, but in some cases:
  - append(x[:len(x):len(x)], y...)
  - append(slices.Clip(x), y...)
we can further simplify the first argument to its unclipped
version (just x in both cases), so that the result is:
    slices.Concat(x, y)

+ test

Fixes golang/go#71296

Change-Id: I89cc4350b5dbd57c88c35c0b4459b23347814441
Reviewed-on: https://go-review.googlesource.com/c/tools/+/647796
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/analysis/modernize/modernize.go

@@ -130,6 +130,7 @@ var (
 	builtinAppend  = types.Universe.Lookup("append")
 	builtinBool    = types.Universe.Lookup("bool")
 	builtinFalse   = types.Universe.Lookup("false")
+	builtinLen     = types.Universe.Lookup("len")
 	builtinMake    = types.Universe.Lookup("make")
 	builtinNil     = types.Universe.Lookup("nil")
 	builtinTrue    = types.Universe.Lookup("true")

gopls/internal/analysis/modernize/slices.go

@@ -52,18 +52,21 @@ func appendclipped(pass *analysis.Pass) {
 		// Only appends whose base is a clipped slice can be simplified:
 		// We must conservatively assume an append to an unclipped slice
 		// such as append(y[:0], x...) is intended to have effects on y.
-		clipped, empty := isClippedSlice(info, base)
-		if !clipped {
+		clipped, empty := clippedSlice(info, base)
+		if clipped == nil {
 			return
 		}
 
 		// If the (clipped) base is empty, it may be safely ignored.
-		// Otherwise treat it as just another arg (the first) to Concat.
+		// Otherwise treat it (or its unclipped subexpression, if possible)
+		// as just another arg (the first) to Concat.
 		if !empty {
-			sliceArgs = append(sliceArgs, base)
+			sliceArgs = append(sliceArgs, clipped)
 		}
 		slices.Reverse(sliceArgs)
 
+		// TODO(adonovan): simplify sliceArgs[0] further: slices.Clone(s) -> s
+
 		// Concat of a single (non-trivial) slice degenerates to Clone.
 		if len(sliceArgs) == 1 {
 			s := sliceArgs[0]
@@ -111,11 +114,6 @@ func appendclipped(pass *analysis.Pass) {
 		}
 
 		// append(append(append(base, a...), b..., c...) -> slices.Concat(base, a, b, c)
-		//
-		// TODO(adonovan): simplify sliceArgs[0] further:
-		// - slices.Clone(s)   -> s
-		// - s[:len(s):len(s)] -> s
-		// - slices.Clip(s)    -> s
 		_, prefix, importEdits := analysisinternal.AddImport(info, file, "slices", "slices", "Concat", call.Pos())
 		pass.Report(analysis.Diagnostic{
 			Pos:      call.Pos(),
@@ -172,46 +170,57 @@ func appendclipped(pass *analysis.Pass) {
 	}
 }
 
-// isClippedSlice reports whether e denotes a slice that is definitely
-// clipped, that is, its len(s)==cap(s).
+// clippedSlice returns res != nil if e denotes a slice that is
+// definitely clipped, that is, its len(s)==cap(s).
+//
+// The value of res is either the same as e or is a subexpression of e
+// that denotes the same slice but without the clipping operation.
 //
-// In addition, it reports whether the slice is definitely empty.
+// In addition, it reports whether the slice is definitely empty,
 //
 // Examples of clipped slices:
 //
 //	x[:0:0]				(empty)
 //	[]T(nil)			(empty)
 //	Slice{}				(empty)
-//	x[:len(x):len(x)]		(nonempty)
+//	x[:len(x):len(x)]		(nonempty)  res=x
 //	x[:k:k]	 	         	(nonempty)
-//	slices.Clip(x)			(nonempty)
-func isClippedSlice(info *types.Info, e ast.Expr) (clipped, empty bool) {
+//	slices.Clip(x)			(nonempty)  res=x
+func clippedSlice(info *types.Info, e ast.Expr) (res ast.Expr, empty bool) {
 	switch e := e.(type) {
 	case *ast.SliceExpr:
-		// x[:0:0], x[:len(x):len(x)], x[:k:k], x[:0]
-		clipped = e.Slice3 && e.High != nil && e.Max != nil && equalSyntax(e.High, e.Max) // x[:k:k]
-		empty = e.High != nil && isZeroLiteral(e.High)                                    // x[:0:*]
+		// x[:0:0], x[:len(x):len(x)], x[:k:k]
+		if e.Slice3 && e.High != nil && e.Max != nil && equalSyntax(e.High, e.Max) { // x[:k:k]
+			res = e
+			empty = isZeroLiteral(e.High) // x[:0:0]
+			if call, ok := e.High.(*ast.CallExpr); ok &&
+				typeutil.Callee(info, call) == builtinLen &&
+				equalSyntax(call.Args[0], e.X) {
+				res = e.X // x[:len(x):len(x)] -> x
+			}
+			return
+		}
 		return
 
 	case *ast.CallExpr:
 		// []T(nil)?
 		if info.Types[e.Fun].IsType() &&
 			is[*ast.Ident](e.Args[0]) &&
 			info.Uses[e.Args[0].(*ast.Ident)] == builtinNil {
-			return true, true
+			return e, true
 		}
 
 		// slices.Clip(x)?
 		obj := typeutil.Callee(info, e)
 		if analysisinternal.IsFunctionNamed(obj, "slices", "Clip") {
-			return true, false
+			return e.Args[0], false // slices.Clip(x) -> x
 		}
 
 	case *ast.CompositeLit:
 		// Slice{}?
 		if len(e.Elts) == 0 {
-			return true, true
+			return e, true
 		}
 	}
-	return false, false
+	return nil, false
 }

gopls/internal/analysis/modernize/testdata/src/appendclipped/appendclipped.go.golden

@@ -20,7 +20,7 @@ func _(s, other []string) {
 	print(slices.Concat(Bytes{1, 2, 3}, Bytes{4, 5, 6}))               // want "Replace append with slices.Concat"
 	print(slices.Concat(s, other, other))                              // want "Replace append with slices.Concat"
 	print(slices.Concat(os.Environ(), other, other))                   // want "Replace append with slices.Concat"
-	print(slices.Concat(other[:len(other):len(other)], s, other))      // want "Replace append with slices.Concat"
-	print(slices.Concat(slices.Clip(other), s, other))                 // want "Replace append with slices.Concat"
+	print(slices.Concat(other, s, other))                              // want "Replace append with slices.Concat"
+	print(slices.Concat(other, s, other))                              // want "Replace append with slices.Concat"
 	print(append(append(append(other[:0], s...), other...), other...)) // nope: intent may be to mutate other
 }