internal/refactor/inline: falcon: only map keys can be variable

The falcon analysis for inlining composite literals allowed considering
arrays and slices for the uniqueness of their keys. But the keys of
array and slice literals must be constant, slices were not handled, and
the logic for arrays looked wrong.

Fix this by only considering the uniqueness of map keys.

Fixes golang/go#74393

Change-Id: Ic7c34942b9b09558b12ef709be839299c347021c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/684295
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

internal/refactor/inline/falcon.go

@@ -423,51 +423,38 @@ func (st *falconState) expr(e ast.Expr) (res any) { // = types.TypeAndValue | as
 			_ = st.expr(e.Type)
 		}
 		t := types.Unalias(typeparams.Deref(tv.Type))
-		var uniques []ast.Expr
+		ct := typeparams.CoreType(t)
+		var mapKeys []ast.Expr // map key expressions; must be distinct if constant
 		for _, elt := range e.Elts {
 			if kv, ok := elt.(*ast.KeyValueExpr); ok {
-				if !is[*types.Struct](t) {
+				if is[*types.Map](ct) {
 					if k := st.expr(kv.Key); k != nil {
-						uniques = append(uniques, st.toExpr(k))
+						mapKeys = append(mapKeys, st.toExpr(k))
 					}
 				}
 				_ = st.expr(kv.Value)
 			} else {
 				_ = st.expr(elt)
 			}
 		}
-		if uniques != nil {
-			// Inv: not a struct.
-
-			// The type T in constraint T{...} depends on the CompLit:
-			// - for a basic-keyed map, use map[K]int;
-			// - for an interface-keyed map, use map[any]int;
-			// - for a slice, use []int;
-			// - for an array or *array, use [n]int.
-			// The last two entail progressively stronger index checks.
-			var ct ast.Expr // type syntax for constraint
-			switch t := typeparams.CoreType(t).(type) {
-			case *types.Map:
-				if types.IsInterface(t.Key()) {
-					ct = &ast.MapType{
-						Key:   makeIdent(st.any),
-						Value: makeIdent(st.int),
-					}
-				} else {
-					ct = &ast.MapType{
-						Key:   makeIdent(st.typename(t.Key())),
-						Value: makeIdent(st.int),
-					}
+		if len(mapKeys) > 0 {
+			// Inlining a map literal may replace variable key expressions by constants.
+			// All such constants must have distinct values.
+			// (Array and slice literals do not permit non-constant keys.)
+			t := ct.(*types.Map)
+			var typ ast.Expr
+			if types.IsInterface(t.Key()) {
+				typ = &ast.MapType{
+					Key:   makeIdent(st.any),
+					Value: makeIdent(st.int),
 				}
-			case *types.Array: // or *array
-				ct = &ast.ArrayType{
-					Len: makeIntLit(t.Len()),
-					Elt: makeIdent(st.int),
+			} else {
+				typ = &ast.MapType{
+					Key:   makeIdent(st.typename(t.Key())),
+					Value: makeIdent(st.int),
 				}
-			default:
-				panic(fmt.Sprintf("%T: %v", t, t))
 			}
-			st.emitUnique(ct, uniques)
+			st.emitUnique(typ, mapKeys)
 		}
 		// definitely non-constant
 

internal/refactor/inline/falcon_test.go

@@ -108,6 +108,30 @@ func TestFalconMapKeys(t *testing.T) {
 	_ = map[int]bool{1: true, x: true}
 }`,
 		},
+		{
+			"Generic map keys",
+			// We have to use a named map type here, because the substituter tries to
+			// wrap the type expr in parens, which are not allowed around the type in
+			// composite literal expressions.
+			`type Map map[int]bool; func f[M Map](x int) { _ = M{1: true, x: true} }`,
+			`func _() { f[Map](1) }`,
+			`func _() {
+	var x int = 1
+	_ = Map{1: true, x: true}
+}`,
+		},
+		{
+			"Array keys", // not a map; shouldn't crash (golang/go$74393)
+			`func f(x int) { _ = [2]int{1: 0} }`,
+			`func _() { f(0) }`,
+			`func _() { _ = [2]int{1: 0} }`,
+		},
+		{
+			"Slice keys", // not a map; shouldn't crash (golang/go$74393)
+			`func f(x int) { _ = []int{1: 0} }`,
+			`func _() { f(0) }`,
+			`func _() { _ = []int{1: 0} }`,
+		},
 		{
 			"Unique map keys (varied built-in types)",
 			`func f(x int16) { _ = map[any]bool{1: true, x: true} }`,

internal/refactor/inline/inline.go

@@ -1429,6 +1429,8 @@ func (st *state) typeArguments(call *ast.CallExpr) []*argument {
 		// ident or qualified ident to prevent "if x == struct{}"
 		// parsing ambiguity, or "T(x)" where T = "*int" or "func()"
 		// from misparsing.
+		// TODO(adonovan): this fails in cases where parens are disallowed, such as
+		// in the composite literal expression T{k: v}.
 		if _, ok := arg.expr.(*ast.Ident); !ok {
 			arg.expr = &ast.ParenExpr{X: arg.expr}
 		}