Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/go/ssa: cannot convert slice to generic type that is both array and pointer to array #56849

Closed
dominikh opened this issue Nov 19, 2022 · 9 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@dominikh
Copy link
Member

The Go compiler at tip accepts this function

func baz[T [2]int | *[2]int](x []int) {
	y := T(x)
	fmt.Printf("%T\n", y)
}

but x/tools/go/ssa fails to build a generic function for it:

panic: in command-line-arguments.baz: cannot convert *t0 ([]int) to T

goroutine 1 [running]:
golang.org/x/tools/go/ssa.emitConv(0xc00073ef00, {0x747e20, 0xc00011df80}, {0x745570?, 0xc00021de30})
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/emit.go:293 +0xa4c
golang.org/x/tools/go/ssa.(*builder).expr0(0xc000295bc8, 0xc00073ef00, {0x745ac8?, 0xc000328400?}, {0x7, {0x745570, 0xc00021de30}, {0x0, 0x0}})
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:677 +0x765
golang.org/x/tools/go/ssa.(*builder).expr(0xc00011df20?, 0xc00073ef00, {0x745ac8, 0xc000328400})
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:625 +0x17b
golang.org/x/tools/go/ssa.(*builder).assign(0xc00073ef00?, 0xc00073ef00?, {0x7466e8?, 0xc000750780}, {0x745ac8?, 0xc000328400?}, 0x5b?, 0xc000295520)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:598 +0x3db
golang.org/x/tools/go/ssa.(*builder).assignStmt(0xc000295570?, 0xc00073ef00, {0xc000232360, 0x1, 0x4bf1de?}, {0xc000232380, 0x1, 0x0?}, 0x1)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:1205 +0x3d4
golang.org/x/tools/go/ssa.(*builder).stmt(0xc000295758?, 0xc00073ef00, {0x745948?, 0xc000328440?})
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2181 +0x425
golang.org/x/tools/go/ssa.(*builder).stmtList(0xc000295701?, 0xc000750750?, {0xc00021e6c0?, 0x2, 0xc000233d20?})
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:946 +0x45
golang.org/x/tools/go/ssa.(*builder).stmt(0xc00073ef00?, 0xc00073ef00, {0x745a68?, 0xc00021c750?})
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2277 +0x859
golang.org/x/tools/go/ssa.(*builder).buildFunctionBody(0x634831?, 0xc00073ef00)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2391 +0x437
golang.org/x/tools/go/ssa.(*builder).buildFunction(0x634880?, 0xc00073ef00)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2326 +0x2e
golang.org/x/tools/go/ssa.(*builder).buildCreated(0xc000295bc8)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2413 +0x25
golang.org/x/tools/go/ssa.(*Package).build(0xc0000e8200)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2606 +0xc86
sync.(*Once).doSlow(0xc000254268?, 0x1?)
	/home/dominikh/prj/go/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
	/home/dominikh/prj/go/src/sync/once.go:65
golang.org/x/tools/go/ssa.(*Package).Build(...)
	/home/dominikh/prj/src/golang.org/x/tools/go/ssa/builder.go:2477
main.doMain()
	/home/dominikh/prj/src/golang.org/x/tools/cmd/ssadump/main.go:152 +0xa45
main.main()
	/home/dominikh/prj/src/golang.org/x/tools/cmd/ssadump/main.go:65 +0x19

/cc @timothy-king

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 19, 2022
@gopherbot gopherbot added this to the Unreleased milestone Nov 19, 2022
@dominikh
Copy link
Member Author

One can trigger the same issue on Go 1.19 with array pointers and slices:

func baz[T []int | *[2]int](x []int) {
	y := T(x)
	fmt.Printf("%T\n", y)
}

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 19, 2022
@timothy-king timothy-king self-assigned this Nov 22, 2022
@timothy-king timothy-king added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 22, 2022
@timothy-king
Copy link
Contributor

Thanks for the report. We can also take strings into account.

So I am aware of 25 sequence conversion cases with type parameters that type check at tip:

func f00[S string, D string](s S)                               { _ = D(s) }
func f01[S string, D []rune](s S)                               { _ = D(s) }
func f02[S string, D []rune | string](s S)                      { _ = D(s) }
func f03[S [2]rune, D [2]rune](s S)                             { _ = D(s) }
func f04[S *[2]rune, D *[2]rune](s S)                           { _ = D(s) }
func f05[S []rune, D string](s S)                               { _ = D(s) }
func f06[S []rune, D [2]rune](s S)                              { _ = D(s) }
func f07[S []rune, D [2]rune | string](s S)                     { _ = D(s) }
func f08[S []rune, D *[2]rune](s S)                             { _ = D(s) }
func f09[S []rune, D *[2]rune | string](s S)                    { _ = D(s) }
func f10[S []rune, D *[2]rune | [2]rune](s S)                   { _ = D(s) }
func f11[S []rune, D *[2]rune | [2]rune | string](s S)          { _ = D(s) }
func f12[S []rune, D []rune](s S)                               { _ = D(s) }
func f13[S []rune, D []rune | string](s S)                      { _ = D(s) }
func f14[S []rune, D []rune | [2]rune](s S)                     { _ = D(s) }
func f15[S []rune, D []rune | [2]rune | string](s S)            { _ = D(s) }
func f16[S []rune, D []rune | *[2]rune](s S)                    { _ = D(s) }
func f17[S []rune, D []rune | *[2]rune | string](s S)           { _ = D(s) }
func f18[S []rune, D []rune | *[2]rune | [2]rune](s S)          { _ = D(s) }
func f19[S []rune, D []rune | *[2]rune | [2]rune | string](s S) { _ = D(s) }
func f20[S []rune | string, D string](s S)                      { _ = D(s) }
func f21[S []rune | string, D []rune](s S)                      { _ = D(s) }
func f22[S []rune | string, D []rune | string](s S)             { _ = D(s) }
func f23[S []rune | [2]rune, D [2]rune](s S)                    { _ = D(s) }
func f24[S []rune | *[2]rune, D *[2]rune](s S)                  { _ = D(s) }

Might still be missing something.

Some of the above like func f19[S []rune, D []rune | *[2]rune | [2]rune | string](s S) { _ = D(s) } do not seem well served by Convert, ChangeType or SliceToArrayPointer as they are today.

@timothy-king
Copy link
Contributor

My main point is that representing something like

func f[S []rune, D ~[]rune | *[2]rune | [2]rune | ~string](s S) { d := D(s); _ = d }

is challenging with the current set of x/tools/go/ssa Instructions. The []rune to *[2]rune case requires a SliceToArrayPointer (which can panic). [2]rune is roughly a dereference of a SliceToArrayPointer. While []rune to string, is a Convert instruction (which cannot panic). We have to pick something to emit in ssa, and nothing at the moment seems to fit the bill.

A possible solution is to have a "MagicConvert" instruction that just handles all of these cases. MagicConvert could just be a relaxation of the current Convert instruction, or something new. Another possible solutions is use a type switch of the type of D. This seems to require TypeAssert to have a mode for looking at the underlying type instead of an exact match. #45380 is related.

A less important point is 'that is a lot of sequence conversion cases'. I found this slightly surprising.

@dominikh
Copy link
Member Author

I've considered a type switch approach, but I am worried that this will make analysis of generic functions more difficult for end users, as the SSA will be more removed from the source representation. The switch statement will introduce new phi nodes, and more (impossible) paths through the program, for example if the type switch is inside a loop.

I haven't yet come up with anything better than MagicConvert (GenericConvert?). It's probably also worth keeping ChangeType in mind.

@timothy-king
Copy link
Contributor

I've considered a type switch approach, but I am worried that this will make analysis of generic functions more difficult for end users, as the SSA will be more removed from the source representation.

All fair points.

My gut reaction from issue is that maybe SliceToArrayPointer was added too hastily and we should try to aim for more fundamental instructions [and smaller code deltas?] in the future. If #45380 is accepted, the type switch approach is the smallest user interface change (delta would be 0). Kinda speculative, but that is a point in favor of the type switch.

I'm not really sure which approach will be better in the long term.

GenericConvert

This is a good name and maybe a decent way to approach scoping what the instruction would do.

It's probably also worth keeping ChangeType in mind.

+1. I don't think ChangeType should be touched. Tough maybe it is emitted in fewer places.

@dominikh
Copy link
Member Author

My gut reaction from issue is that maybe SliceToArrayPointer was added too hastily and we should try to aim for more fundamental instructions

In our defense, we didn't expect generics to be this generic :P And I think the instruction is still very useful in combination with instantiation, or with type sets where all types permit SliceToArrayPointer to be used. I don't think that our existing instructions are too specific, only that we may not have ones that are general enough for generic functions.

I don't think ChangeType should be touched. Tough maybe it is emitted in fewer places.

I wasn't thinking of changing it. My point was more that ChangeType is one more semantic that GenericConvert might have if we extend the type sets above, in addition to SliceToArrayPointer, SliceToArrayPointer + dereference, and Convert.

@gopherbot
Copy link

Change https://go.dev/cl/457335 mentions this issue: go/ssa: emit conversion by type switch

@gopherbot
Copy link

Change https://go.dev/cl/457436 mentions this issue: go/ssa: add GenericConvert instruction

@mdempsky
Copy link
Member

My 2c: GenericConvert seems simpler, generates less code, and feels consistent with how other operations have been extended. E.g., append and copy on bytestring don't type switch to separately handle []byte and string.

gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2023
Adds a new MultiConvert instruction. MultiConvert instructions
are a catch all for conversions involving a typeparameter that
would result in multiple different types of conversion
instruction [sequences].

Updates golang/go#56849

Change-Id: I6c2f53ccef1b933406096d6ca2867f1007a13bd3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/457436
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Tim King <taking@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants