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

cmd/go2go: constraint type inference example from proposal causes panic #41216

Closed
rogpeppe opened this issue Sep 4, 2020 · 8 comments
Closed

cmd/go2go: constraint type inference example from proposal causes panic #41216

rogpeppe opened this issue Sep 4, 2020 · 8 comments
Assignees
Labels
Milestone

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Sep 4, 2020

I copied and pasted the constraint type inference example from the proposal, and it causes the translator to panic.

https://go2goplay.golang.org/p/JvUmICGpndk

panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
go/go2go.typeArgsFromFields(0xc000157950, 0xc000082dd0, 0x1, 0x1, 0xc000082de0, 0x1, 0x1, 0xc000082720, 0x2, 0x2, ...)
	/usr/local/go-faketime/src/go/go2go/instantiate.go:48 +0x3db
go/go2go.(*translator).instantiateFunction(0xc000157950, 0x0, 0xc00009a200, 0xc000082dd0, 0x1, 0x1, 0xc000082de0, 0x1, 0x1, 0x657901, ...)
	/usr/local/go-faketime/src/go/go2go/instantiate.go:104 +0x185
go/go2go.(*translator).translateFunctionInstantiation(0xc000157950, 0xc0000b13b0)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:906 +0x3ff
go/go2go.(*translator).translateExpr(0xc000157950, 0xc0000b13b0)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:671 +0x54f
go/go2go.(*translator).translateExpr(0xc000157950, 0xc000082550)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:692 +0x385
go/go2go.(*translator).translateExprList(0xc000157950, 0xc000082550, 0x1, 0x1)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:865 +0x46
go/go2go.(*translator).translateStmt(0xc000157950, 0xc00009a360)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:586 +0x29d
go/go2go.(*translator).translateBlockStmt(0xc000157950, 0xc000084c00)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:547 +0x57
go/go2go.(*translator).translateFuncDecl(0xc000157950, 0xc0000c2190)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:538 +0xdb
go/go2go.(*translator).translate(0xc000157950, 0xc0000b82d0)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:466 +0x391
go/go2go.rewriteAST(0xc0000ae280, 0xc0000aa360, 0x0, 0x0, 0xc0000b1bd0, 0xc0000b82d0, 0x1, 0xc0000b1360, 0xc0000b1bd0)
	/usr/local/go-faketime/src/go/go2go/rewrite.go:257 +0xe5
go/go2go.RewriteBuffer(0xc0000aa360, 0x7fff8c2cedec, 0x1e, 0xc0000fa000, 0x3fd, 0x5fd, 0x0, 0xc000084a50, 0xc000084a20, 0xc0000849f0, ...)
	/usr/local/go-faketime/src/go/go2go/go2go.go:144 +0x2c5
main.translateFile(0xc0000aa360, 0x7fff8c2cedec, 0x1e)
	/usr/local/go-faketime/src/cmd/go2go/translate.go:26 +0xa9
main.main()
	/usr/local/go-faketime/src/cmd/go2go/main.go:74 +0x309
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 5, 2020

@griesemer This code is using constraint type inference, with one type argument specified in the code, and the other type argument inferred. Does the type checker make the inferred type available? Right now I'm seeing that info.Inferred[x] is nil.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 7, 2020

Change https://golang.org/cl/253258 mentions this issue: [dev.go2go] go/types: add some tests checking types inferred via constraint type inference

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 7, 2020

@ianlancetaylor Judging from the code (go/types/call.go:400) inferred types should be recorded no matter the inference mechanism. Added some go/types tests (CL 253258) to confirm this.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 7, 2020

@ianlancetaylor I have a suspicion (to be confirmed): In the example, the expression FromStrings2[X] looks like an index expression - the parser cannot know that we have a function instantiation. When the type checker figures it out, it creates a new ast.CallExpr node on the fly (so we can re-use existing machinery). If that is the node that is recorded, there is no way to find it later...

Also, in go/go2go/rewrite.go, in the method translator.instantiationTypes(line 1042), for this example, the switch statement appears to see an ast.IndexExpr and thus does not look up t.importer.info.Inferred[x].

Need to investigate some more.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 7, 2020

@ianlancetaylor 1) Constraint type inference did indeed not record the inferred signature if inference was applied to a partially provided (rather than inferred, from the value arguments) type parameter list. 2) Because sometimes we don't have an ast.CallExpr node, e.g. in this example FromStrings2[X] which is now an ast.IndexExpr node (but used to be an ast.CallExpr node in the past when we wrote FromStrings2(X)), we need to update which key is recorded in the Inferred map (best approach TBD).

In short, this is a bug in go/types, caused by the recent change of syntax. Once fixed in go/types, it may require some minimal adjustments in go/go2go.

@griesemer griesemer self-assigned this Sep 7, 2020
gopherbot pushed a commit that referenced this issue Sep 7, 2020
… params are provided

The existing code didn't record the results of constraint type inference
when it was applied to a partial list of (provided) type parameters.

Changed Info.Inferred map to accept *ast.CallExpr and *ast.IndexExpr
as keys ("call" may be f(x) or f[T]).

Updates #41216.

Change-Id: I5da7467d01643bf74fb78f6c6728209cbd4006c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/253258
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 7, 2020

This should now be fixed in go/types. Final fix will need some adjustments in the go/go2go package.

@griesemer griesemer assigned ianlancetaylor and unassigned griesemer Sep 7, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 9, 2020

Translation tool is fixed on the dev.go2go branch.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 9, 2020

Change https://golang.org/cl/253818 mentions this issue: [dev.go2go] go/go2go: handle inferred arguments for index expressions

gopherbot pushed a commit that referenced this issue Sep 9, 2020
Fixes #41216

Change-Id: Iff109f0a7f047742c5f90d64db201712c04f3259
Reviewed-on: https://go-review.googlesource.com/c/go/+/253818
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.