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

go/types, types2: type inference should unify interface types #51593

Closed
cr7pt0gr4ph7 opened this issue Mar 10, 2022 · 14 comments
Closed

go/types, types2: type inference should unify interface types #51593

cr7pt0gr4ph7 opened this issue Mar 10, 2022 · 14 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. TypeInference Issue is related to generic type inference
Milestone

Comments

@cr7pt0gr4ph7
Copy link

cr7pt0gr4ph7 commented Mar 10, 2022

What version of Go are you using (go version)?

$ go version
go version devel go1.19-5a040c5a36 Thu Mar 10 09:12:04 2022 +0000

Does this issue reproduce with the latest release?

Probably also with 1.18rc1, haven't had a chance to test it yet.

What operating system and processor architecture are you using (go env)?

Gotip Playground.

What did you do?

Experimenting with type inference and anonymous interface types as constraints and/or inference inputs.

package main

type BasicAnon = interface {
	M() int
}

func InferAnon[I interface{ M() R }, R any](arg I) {
}

func main() {
	var a BasicAnon
	InferAnon(a) // crash due to this line
	_ = a
}

https://gotipplay.golang.org/p/lJOnm9LexpD

What did you expect to see?

Either a compiler error, or a successful compilation.

What did you see instead?

A compiler crash due to an assertion failure in (*cycleFinder).typ on this line:

case *Signature:
	// There are no "method types" so we should never see a recv.
	assert(t.recv == nil)
	...

The assumption made in the comment is obviously not true with respect to interface method types, for which (*Signature).Recv() is set to the declaring interface's type.

@findleyr
Copy link
Contributor

CC @griesemer

@griesemer griesemer self-assigned this Mar 10, 2022
@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 10, 2022
@griesemer griesemer added this to the Go1.18 milestone Mar 10, 2022
@griesemer
Copy link
Contributor

Conservatively marking as a release blocker.

@griesemer
Copy link
Contributor

Slightly simplified reproducer:

package p

func f[P interface{ m() R }, R any](P) {}

func _() {
	var x interface { m() int }
	f(x)
}

@cr7pt0gr4ph7
Copy link
Author

I've been experimenting a bit, and made the following discoveries (using a modified version of go/types):

  • After disabling the assertion, the example code given above causes the compiler error cannot infer R, as should probably be expected.

  • But the following code then infers & typechecks without problems, although it ought not to, given my current understanding of the intended behavior of type inference (cmd/compile (types2, go/types): cannot infer generic interface types #41176):

package main

type BasicAnon = interface {
	M() int
}

func InferAnon[I []interface{ M() R }, R any](arg I) {
}

func main() {
	var a []BasicAnon
	InferAnon(a)
	_ = b
}

So, it seems that the implementation already kind-of supports type inference via interface methods, probably as an artifact of the unification algorithm mirroring the type equality/assignability algorithm.

@griesemer
Copy link
Contributor

The assertion is clearly wrong and needs to be removed - we can see signatures of interface methods in that code. The constraint type inference fails at the moment for R because there's no "core term" in this case (implementation-wise). I have a 3-line fix but I am not confident yet that it is correct under all circumstances.

In any case, thanks for this bug report. At the very least we will address the crash for 1.18.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/391615 mentions this issue: go/types, types2: remove incorrect assertion (don't crash)

@cr7pt0gr4ph7
Copy link
Author

cr7pt0gr4ph7 commented Mar 10, 2022

As mentioned above, I'd caution that it seems that the generics type checking code currently is a bit inconsistent in that it allows unification of some constructs (e.g. []interface{M() int} unifies with []interface{M() R} (with R unified to int), but interface{M() int} does not unify with interface{M() R}). The spec isn't very clear in this regard, except for the definition of "(adjusted) core type", which disallows the latter, and kind of allows the former by bot expressly forbidding it.

How bad this is with respect to the release depends on whether such "structural inference" (for the lack of a better word) for interfaces is intended to be allowed in the future for all cases, or forbidden for all cases. In the former case, one should probably also allow unification of interface{M() int;N()} with interface{M() T} during constraint type inference.

I've played around a bit at https://github.com/cr7pt0gr4ph7/go-types-experiments and https://gotipplay.golang.org/p/W-HYHRcmIXD and got the extended inference to work quite trivially, but that is definitely out of scope for this issue.

gopherbot pushed a commit that referenced this issue Mar 10, 2022
The removed assertion was never incorrect, as signatures may
be from methods in interfaces, and (some) interfaces set the
receivers of their methods (so we have a position for error
reporting).

This CL changes the issue below from a release blocker to an
issue for Go 1.19.

For #51593.

Change-Id: I0c5f2913b397b9ab557ed74a80cc7a715e840412
Reviewed-on: https://go-review.googlesource.com/c/go/+/391615
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@griesemer
Copy link
Contributor

griesemer commented Mar 10, 2022

The spec section on type inference will eventually need to be refined/rewritten. We're not going to change anything else here besides fixing the crash for 1.18. As long as we are not inferring incorrect types, we're ok. Eventually we would like to infer these "obvious" cases. As a consequence, code that may not be valid now (inference fails) may become valid in the future (which is ok), but hopefully not the other war around (which would not be ok).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/391796 mentions this issue: [release-branch.go1.18] go/types, types2: remove incorrect assertion (don't crash)

@griesemer
Copy link
Contributor

Once this is cherry-picked into the 1.18 release branch, this is not a release blocker anymore and can be marked for Go 1.19.

gopherbot pushed a commit that referenced this issue Mar 14, 2022
…(don't crash)

The removed assertion was never incorrect, as signatures may
be from methods in interfaces, and (some) interfaces set the
receivers of their methods (so we have a position for error
reporting).

This CL changes the issue below from a release blocker to an
issue for Go 1.19.

For #51593.

Change-Id: I0c5f2913b397b9ab557ed74a80cc7a715e840412
Reviewed-on: https://go-review.googlesource.com/c/go/+/391615
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 914195c)
Reviewed-on: https://go-review.googlesource.com/c/go/+/391796
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@dmitshur
Copy link
Contributor

dmitshur commented Mar 14, 2022

The cherry-pick commit 8706c09 (CL 391796) has landed on release-branch.go1.18.
Per comment above, removing release-blocker and moving to Go1.19 milestone.

@dmitshur dmitshur modified the milestones: Go1.18, Go1.19 Mar 14, 2022
@griesemer griesemer changed the title go/types, types2: compiler crashes with assertion failure on type unification of unnamed interface types go/types, types2: type inference should unify against interface types Mar 14, 2022
@griesemer griesemer changed the title go/types, types2: type inference should unify against interface types go/types, types2: type inference should unify interface types Mar 14, 2022
@griesemer griesemer modified the milestones: Go1.19, Go1.20 Jun 23, 2022
@findleyr
Copy link
Contributor

Moving to 1.21 as it likely requires a proposal.

@findleyr findleyr modified the milestones: Go1.20, Go1.21 Nov 10, 2022
@griesemer griesemer added the TypeInference Issue is related to generic type inference label Feb 28, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/472298 mentions this issue: go/types, types2: consider methods when unifying type parameters and constraints

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/499282 mentions this issue: doc/go1.21: document type inference changes

gopherbot pushed a commit that referenced this issue May 31, 2023
For #39661.
For #41176.
For #51593.
For #52397.
For #57192.
For #58645.
For #58650.
For #58671.
For #59338.
For #59750.
For #60353.

Change-Id: Ib731c9f2879beb541f44cb10e40c36a8677d3ad4
Reviewed-on: https://go-review.googlesource.com/c/go/+/499282
TryBot-Bypass: Robert Griesemer <gri@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@golang golang locked and limited conversation to collaborators May 30, 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. TypeInference Issue is related to generic type inference
Projects
Status: Done
Development

No branches or pull requests

5 participants