-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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/compile/internal/types: types.Type.LinkString has collisions for anonymous struct types with blank fields #47087
Comments
Thanks. I think I know how to repro and fix this. If you're able to provide instructions for me to repro your exact failure, that would be helpful. But otherwise, I might ask you to test a CL. |
I'm swamped right now so I can't really work on the repro, but i'll gladly test the CL. |
This is a pre-existing issue that types.Type.LinkString fails to package-qualify blank field names, leading to collisions in the linker names: https://play.golang.org/p/kdbwivysffs (should run successfully without panicking) It manifested in GOEXPERIMENT=unified because I included an assertion to check that it's consistent with types.Identical, which it should be but is evidently not. |
Change https://golang.org/cl/333162 mentions this issue: |
Change https://golang.org/cl/333161 mentions this issue: |
Change https://golang.org/cl/333163 mentions this issue: |
The logic in symfmt for deciding how to package-qualify an identifier is easily refactored into a separate function, loosely similar to go/types.Qualifier's API. Passes toolstash -cmp. Updates #47087. Change-Id: Ib3e7cc35a6577dc28df8eca79ba3457c48168e86 Reviewed-on: https://go-review.googlesource.com/c/go/+/333161 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@golang.org>
@OneOfOne I think https://go-review.googlesource.com/c/go/+/333166 should fix the issue. I'd appreciate if you could confirm that for me, thanks. |
Now that symfmt is simpler, we can simply manually inline it into sconv. Importantly, this allows us to avoid allocating a buffer + writing a string + re-interning it when we don't need to qualify the identifier. Passes toolstash -cmp. Updates #47087. Change-Id: I47b57aef22301ba242556a645346f478f0c1a7d1 Reviewed-on: https://go-review.googlesource.com/c/go/+/333162 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@golang.org>
Yep, I can confirm that CL fixed it, thank you! I feel like a kid waiting to open their christmas presents. |
@OneOfOne Great, thank you for filing the issue and the quick test report. |
I know the branch is under heavy development, but I've been using it with
GOEXPERIMENT=unified
for multiple projects and it works fine.Today I was working on an old (massive) code base and I got this error, there are no generics involved.
Unsetting
GOEXPERIMENT
fixes the issue.Tagging @mdempsky since I know IR is his baby, I'll try to work on a reproducer, as far as I can tell it's related to
google.golang.org/genproto/googleapis/cloud/asset/v1
but I still can't have a small repro.The text was updated successfully, but these errors were encountered: