go/types: no way to construct the signature of append(s, "string"...) via the API

[findleyr]

Discovered in a discussion with @timothy-king about #54946: when type checking the following package, we record the type of append as func([]byte, string...) []byte:

package p

func _() {
  var d []byte
  var s string
  _ = append(d, s...)
}

However, there is no way to construct func([]byte, string...) []byte via go/types APIs. NewSignatureType panics with variadic parameter must be of unnamed slice type. This has never mattered before because this type cannot appear in export data, but it matters for x/tools/go/ssa, which must do substitution inside function bodies.

What should we do about this?

1. add a new API to go/types?
2. remove this panic from go/types?
3. work around this limitation in x/tools/go/ssa, for example by inserting a synthetic conversion?
4. some other hack, for example the following rather ugly hack (which ignores error handling, etc)?

expr, _ := parser.ParseExpr("append([]byte(nil), \"\"...)")
info := Info{Types: make(map[ast.Expr]TypeAndValue)}
CheckExpr(fset, pkg, appendPos, expr, &info) // fset, pkg, appendPos from an existing package
k := expr.(*ast.CallExpr).Fun.(*ast.Ident)
T := info.Types[k].Type

Let's discuss. I would lean toward (2) or (4). This doesn't seem worth a new API (though the limitation should perhaps be more clearly documented).

CC @griesemer @adonovan @timothy-king

[adonovan]

I agree that (2) is the best solution, and (4) is a good short-term workaround.

[gopherbot]

Change https://go.dev/cl/430455 mentions this issue: go/types, types2: allow (string...) signature with NewSignatureType

[bcmills]

Or: maybe the built-in append (like the untyped nil) doesn't really have a well-defined type in isolation?
(https://go.dev/play/p/t_YtAQyAS3u)

Just as there are cases where you can use an untyped nil where you could not use a variable of any well-defined type, there are cases where you can use an untyped append where you could not use a function of any well-defined type.

That suggests, perhaps, adding a BasicKind of UntypedAppend, and a BasicInfo value IsAppend. 🤔

Then there wouldn't be a corresponding call to NewSignatureType, because the type of the append expression, while syntactically a function call, would not involve a variable of function type.

[griesemer]

This is about creating a signature for append in context. If that's correct, then I don't think we need to make this anymore complicated than necessary; simply allowing that specific type seems like a fine solution.

[findleyr]

This is about creating a signature for append in context. If that's correct, then I don't think we need to make this anymore complicated than necessary; simply allowing that specific type seems like a fine solution.

Agreed, upon further consideration I think that's the clear best solution.

[visualfc]

Change https://go.dev/cl/430455 mentions this issue: go/types, types2: allow (string...) signature with NewSignatureType

this PR alias string T not support for x/tools/go/ssa

package main

func add[S ~string | ~[]byte](buf *[]byte, s S) {
	*buf = append(*buf, s...)
}

type T string
func main() {
	var buf []byte
	add(&buf, T("foo"))
}

[findleyr]

@visualfc I'm not sure I understand. That CL fixes the panic in go/types, by allowing constructing the signature of append([]byte, string...) []byte.

[visualfc]

@visualfc I'm not sure I understand. That CL fixes the panic in go/types, by allowing constructing the signature of append([]byte, string...) []byte.

this PR allow basic string. but type T string not allow on this PR.

package main


func add[S ~string | ~[]byte](buf *[]byte, s S) {
	*buf = append(*buf, s...)
}

type T string
func main() {
	var buf []byte
	add(&buf, T("foo")) // error check for x/tools/go/ssa
}

ssadump -build=G main.go

panic: variadic parameter must be of unnamed slice type or string

[findleyr]

@visualfc I see now, thanks. You're right, this CL needs to consider the core type.

[visualfc]

@findleyr test this PR ~[]byte panic. coreType(last) is []byte.

package main

func add[S ~string | ~[]byte](buf *[]byte, s S) {
	*buf = append(*buf, s...)
}

type T1 string
type T2 []byte

func main() {
	var buf []byte
	add(&buf, T1("foo")) // pass coreType is string
	add(&buf, T2("foo")) // panic coreType is []byte
}

ssadump -build=G main.go
panic: variadic parameter must be of unnamed slice type or string

goroutine 1 [running]:
go/types.NewSignatureType(0x0, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, 0xc00000d890, 0xc00000d770, 0x1)

[findleyr]

@visualfc thank you, you are right. Please comment on the CL though.

[timothy-king]

Do we know which versions the fix will land in go/types? If it is in 1.20 and later, x/tools/go/ssa will need a workaround to support 1.19 and 1.18.

[griesemer]

The current plan is 1.20 but we could easily do a back port, I think.

[findleyr]

Thinking through the backport question:

If we backport to, say, 1.18, then x/tools/go/ssa will deterministically panic at 1.18 versions before 1.18.8. It's sort of like go/ssa is using an API that was added in 1.18.8...

...but what is the alternative? Generating different instructions (option 3) is a non-starter, I think, now that we've decided to fix go/types. The hack in option 4 is liable to be fragile. Doing nothing means go/ssa can panic on valid generic code before go 1.20.

None of the alternatives is desirable, and the backport is trivially safe, so I vote yes to backport.

[findleyr]

@gopherbot please consider this for backport to 1.18 and 1.19, it is a bug that NewSignatureType panics on valid signatures.

[gopherbot]

Backport issue(s) opened: #55148 (for 1.18), #55149 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[findleyr]

@griesemer if you agree, can you please create cherry-picks of your CL to 1.18 and 1.19? Thanks.