-
Notifications
You must be signed in to change notification settings - Fork 18k
go/types, types2: data race to interface typeset #61561
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
Comments
Connecting the dots: I think this may be the cause of some confusing diagnostics users have reported in gopls. (e.g. https://gophers.slack.com/archives/C2B4L99RS/p1686512574586319) |
Change https://go.dev/cl/512636 mentions this issue: |
race reports from the TryBot log
|
As I understand it, this is too recent to explain #37602? |
@bcmills this race has existed from the beginning (go 1.18). I'm shocked that it hasn't been uncovered until now, though it does explain multiple confusing observations in gopls, which may be one of the few concurrent users of go/types. |
Change https://go.dev/cl/512955 mentions this issue: |
We were memoizing active packages only after a call to TypeCheck. This means that we may duplicate work if diagnostics, references, or implements requests execute before the first call to TypeCheck for an open package. Fix this by pushing the memoization logic down into forEachPackage. This uncovered a bug in go/types that may have already been affecting users: golang/go#61561. This bug is consistent with user reports on slack of strange errors related to missing methods. Avoid this bug in most cases by ensuring that all instantiated interfaces are completed immediately after type checking. However, this doesn't completely avoid the bug as it may not complete parameterized interface literals elsewhere in the type definition. For example, the following definition is still susceptible to the bug: type T[P any] interface { m() interface{ n(P) } } For golang/go#60926 Fixes golang/go#61005 Change-Id: I324cd13bac7c17b1eb5642973157bdbfb2368650 Reviewed-on: https://go-review.googlesource.com/c/tools/+/512636 Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com>
When importing, it is also possible to produce an incomplete interface instance, which can be racy (as reproduced in the included test). Partially avoid this by completing interface underlyings of instances. For golang/go#61561 Change-Id: I8ca2bdc2d03fa1a46179bbd8596d7ef9baf51600 Reviewed-on: https://go-review.googlesource.com/c/tools/+/512955 gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
I believe I have a fix for this. Unfortunately, these types of fixes (in go/types, at least) tend to be risky, so we must be very careful before cherry-picking. |
I’ve also seen what I believe is a related failure when running lint in CI. I’ll hunt for the logs tomorrow. I’ve only seen it fail once or twice and it went away when I retried the job. |
Change https://go.dev/cl/513015 mentions this issue: |
I can't find it now but what I recall is that type checking failed. I use golangci-lint, and I'm pretty sure it was the 'typecheck' linter (which really means the stdlib's go/* packages, not an actual linter/analyzer) that was complaining. |
@firelizzard18 thanks, that would make sense given the nature of the bug. |
It is the responsibility of go/types to complete any interface it creates, except for those created by the user using NewInterface. However, this was not being done for interfaces created during instantiation. Fix this by (rather carefully) ensuring that all newly created interfaces are eventually completed. Fixes golang/go#61561 Change-Id: I3926e7c9cf80714838d2c1b5f36a2d3221c60c41 Reviewed-on: https://go-review.googlesource.com/c/go/+/513015 Run-TryBot: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Robert Findley <rfindley@google.com> Reviewed-by: Robert Griesemer <gri@google.com>
Discovered in the trybot failures of https://go.dev/cl/512636.
There's a data race to interface type set computation. I don't know how it's gone undetected for so long: the interface type set is unguarded, and instantiated interfaces are not completed.
https://cs.opensource.google/go/go/+/master:src/go/types/interface.go;l=29;drc=97f843b51f69f392bb09b24c077aca505e519551
Whatever the fix, I think we should consider back porting to 1.21+1.20.
CC @griesemer
The text was updated successfully, but these errors were encountered: