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

Jump to bottom

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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rogpeppe
Copy link
Contributor

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

@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.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 5, 2020
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Sep 5, 2020
@gopherbot
Copy link

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

@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

@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

@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 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 Sep 7, 2020
@griesemer griesemer self-assigned this Sep 7, 2020
gopherbot pushed a commit that referenced this issue Sep 7, 2020
@griesemer
[dev.go2go] go/types: record constraint type inference when some type…
a2c3425 
… 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

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

Translation tool is fixed on the dev.go2go branch.

@gopherbot
Copy link

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
@ianlancetaylor
[dev.go2go] go/go2go: handle inferred arguments for index expressions
e6b8577 
Fixes #41216

Change-Id: Iff109f0a7f047742c5f90d64db201712c04f3259
Reviewed-on: https://go-review.googlesource.com/c/go/+/253818
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@rogpeppe @ianlancetaylor @griesemer @gopherbot