go/types, types2: inconsistent behavior with recursive generic types that produces deadlock, invalid recursive, unknown field error #60817
Comments
nekomeowww
changed the title
First, thanks for workaround. I know it's invalid. I had a example in issue 1 at error disappeared at first branch which already uses pointer reference for both type parameter and field and the issue is resolved. But the critical thing is:
|
Thanks, and sorry that I forgot to update the reproduction issue to include the reproduction for issue 3 unknown field. |
Hmm, I used the same method as you do in Complete as a workaround for issue 1. However, since the non pointer reference for type param but within package scope case (or issue 1, error disappeared branch, when case 1) still runs and can be compiled, is this something that compiler behaves inconsistently? Both the three obvious issues would not occur when the type definitions and value references live in one package. How do you explain it? |
Do I need to create a separate issue for gopls? Or I should wait for the investigation further on? |
So if am understand correctly, it is expected that the recursive generic type definition and logic can be run successfully within package scope instead of being addressed by compiler, and such behavior is different when used in outside of the package scope? This doesn't sound right to me.
I know. |
|
Change https://go.dev/cl/504195 mentions this issue: |
|
CC @griesemer Thanks for the report. Independent of what the correct behavior is here, we should never deadlock, even in invalid code. |
|
I will fix the deadlock (I think it's my bug), but for 1.22, since presumably this has existed since 1.18. |
findleyr
changed the title
|
@Nasfame thanks, I will take a look. |
|
@Nasfame thanks for that CL, and for your investigation. I am not convinced that CL fixes the problem, because I don't think the tests are actually using the unified export data reader. @mdempsky @griesemer: the problem here is that the unified type loader calls Of course, this is a very tricky API to use correctly. I'm not sure how to proceed. Option 1: make the call to I think we may be able to make this easier to understand by segregating the type API into method that are valid "constructors", and other non-constructors. Constructors can be called in any order, but you must not call any constructors after a non-constructor has been used. That's effectively what we have now, but we have no formality to the schema. Then we would make SetConstraint conform to the rules for constructors by internally deferring its call to Not pretty, but arguably better than what we have now, which is an API that is almost impossible to use outside of the carefully crafted import logic. Option 2: let the lazy type loading logic assume single-threadedness. Since this API is only used by the compiler, and I believe is only used in a single-threaded context, perhaps we can avoid the deadlock by unlocking after nilling out the loader here: https://cs.opensource.google/go/go/+/master:src/cmd/compile/internal/types2/named.go;l=199;drc=5724daa6825db0a9097254060633439e6538d845 That would probably work, but is not obviously correct. |
FWIW, gcimporter's unified reader still defers them too: https://cs.opensource.google/go/go/+/master:src/go/internal/gcimporter/ureader.go;l=618 If we need to defer the compiler's SetConstraint calls too, I think that's doable. Stylistically it's kinda gross, but pragmatically it seems fine to me. The API for importers has always been a little awkward. I'd be surprised if there were any non-trivial implementations outside of std+x/tools. |
|
FWIW, I like the constructor vs non-constructor option. It's certainly very delicate trying to construct cyclic type structures using only the exported APIs. Having some sort of documented guidance on how to use them safely would be handy. |
griesemer
added
NeedsFix
|
@mdempsky thanks. I'm not sure if it's possible to do this properly with the current APIs, since SetConstraint is called from Discussed with @griesemer, and we think we should take the more principled approach (option 1 above) to fix the As suggested by @griesemer, perhaps we should avoid the |
|
Change https://go.dev/cl/504775 mentions this issue: |
|
Change https://go.dev/cl/507956 mentions this issue: |
|
Change https://go.dev/cl/507958 mentions this issue: |
|
This issue is currently labeled as early-in-cycle for Go 1.22. |
|
Change https://go.dev/cl/514255 mentions this issue: |
Dups: #60856 For #60817 Change-Id: Ic0710758e170d6ceed66649fec08ef8054be4d6b GitHub-Last-Rev: 8bbc76a GitHub-Pull-Request: #61664 Reviewed-on: https://go-review.googlesource.com/c/go/+/514255 Reviewed-by: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@google.com> Run-TryBot: Robert Griesemer <gri@google.com> Auto-Submit: Robert Griesemer <gri@google.com>
|
I'm coming here from Stack Overflow, where someone has just reported what seems like a very similar compiler bug. I haven't had time to investigate/reduce the problem, but compilation (with Go 1.21.1, at the very least) of the following invalid program fails with a deadlock: package main
import "whatever/vector"
func main() {
s := vector.New[int]()
println(s)
}
-- go.mod --
module whatever
-- iterator/iterator.go --
package iterator
type Iter[I any, T Iterator[I]] struct {
Iter T
}
func New[I any, T Iterator[I]](iter T) Iter[I, T] {
return Iter[I, T]{
Iter: iter,
}
}
type Iterator[Item any] interface {
HasNext() bool
Next() Item
Iter() Iter[Item, Iterator[Item]]
}
-- vector/vector.go --
package vector
import "whatever/iterator"
type Vector[Item any] struct {
data []Item
// used by iterator.
idx int
}
func (v *Vector[Item]) Get(pos int) Item {
if pos < 0 || pos >= v.Size() {
panic("ouf off range")
}
return v.data[pos]
}
func (v *Vector[Item]) HasNext() bool {
return v.idx < v.Size()
}
func (v *Vector[Item]) Next() Item {
v.idx = v.idx + 1
return v.Get(v.idx - 1)
}
func (v *Vector[Item]) Iter() iterator.Iter[Item, iterator.Iterator[Item]] {
return iterator.Iter[Item, iterator.Iterator[Item]]{
Iter: v,
}
}Output: Here is a Playground where you can reproduce the problem. |
|
Thanks for finding this issue. I'll look into that reproducer when I work on this (which I still plan for 1.22). |
|
Change https://go.dev/cl/531655 mentions this issue: |
|
@jub0bs I think your issue may be only indirectly related, as I can only reproduce it using import/export, not with pure type checking from source (see the above CL for a reproducing test). |
|
This issue is currently labeled as early-in-cycle for Go 1.23. |
and others
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes, since go1.18 had introduced generics.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
I defined a recursive generic type like this:
And trying to discover the possibility of using generics to make the common options assignment easier and more readable by using chained calls pattern.
What did you expect to see?
Either the compiler or the documentations of generics of Golang addresses the restricted usage of recursive generic types or the compiler behaves consistently.
The inconsistent behavior may be vary based on whether it's with in tests or just build, whether it's a single source file or multiple source files, whether it's wrapped with a pointer over pointer or not, whether it's imported the outside package or not.
What did you see instead?
Issue 1:
fatal error: all goroutines are asleep - deadlock!Panic stack
Errored:
go buildorgo test,Error disappeared:
when met the following criteria [playground]:
fmt.Printf("%T")still works as expected,go build -gcflags=-S.when met the following criteria [playground]:
fmt.Printf("%T")still works as expected,go build -gcflags=-S.Issue 2:
invalid recursive type xOutput
errored:
go buildorgo test,innerTas union type recursively,error disappeared
innerTas union type recursively.fmt.Printf("%T")still works as expected,go build -gcflags=-S.Issue 3:
unknown field x in struct literal of type yerrored:
go buildorgo testthe compiler will complainfatal error: all goroutines are asleep - deadlock!errorerror disappeared:
when met the following criteria [playground]:
fmt.Printf("%T")still works as expected,go build -gcflags=-S.when met the following criteria:
fmt.Printf("%T")still works as expected,go build -gcflags=-S.Issue 4:
xredeclared in this blockIt errored with a single valid
file_1.go, and the packagechannelxonly containing theSubContainer,ContainerA,ContainerBtypes.This is the most hard to reproduced one by comparing to the other issues I have faced. I only encountered once when debugging the
invalid recursive typeerror.Further more
I have found a issue that containing the same error message just like me in #49439. However, this doesn't explain why it compiles successfully and run expected when the type definitions and the functions that referenced the type are in a same package.
Later I found #51244 with a ongoing fix at CL 386718 that pending on merge (maybe merge before next cycle of Golang release?). However, just like the same issue, this doesn't explain why it compiles successfully and run expected in some cases while the compiler and static analysis behaves inconsistently.
So, is this such inconsistent behavior a already known issue? At least it jammed me for a while to find out what I have done wrong.
Especially the compiler would optimized the recursive type definition and generate a unexpected assembly code:
Is this abnormal? The normal assembly output doesn't look like this.
Imagine a developer is maintaining a package that is widely used by other users, and the developer never test the code with a test package suffixed with
_test, and developer initially would have no ideas to understand how and why the compiler may complain and throw a panic withfatal error: all goroutines are asleep - deadlock!message when users imported the package with the version contained some type definitions the same as the reproduction codes in playground bellow.By the way, run
go buildandgo testwithGOEXPERIMENT=nounifiedenv flag fixed the following errors I encountered bellow (suggested in #54535).The text was updated successfully, but these errors were encountered: