From 8d2ea2936b337f6276054dd4ac40999dd0f22dbc Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 17 Oct 2019 16:28:38 -0700 Subject: [PATCH] go/types: don't update the underlying type of an imported type Updating the underlying type of an imported type (even though is was set to the same type again) leads to a race condition if the imported package is imported by separate, concurrently type-checked packages. Fixes #31749. Change-Id: Iabb8e8593eb067eb4816c1df81e545ff52d32c6c Reviewed-on: https://go-review.googlesource.com/c/go/+/201838 Run-TryBot: Robert Griesemer TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- src/go/types/decl.go | 21 ++++++++++++++------- src/go/types/issues_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/go/types/decl.go b/src/go/types/decl.go index a4fb2b81cc..83d40939a8 100644 --- a/src/go/types/decl.go +++ b/src/go/types/decl.go @@ -489,27 +489,34 @@ func (check *Checker) underlying(typ Type) Type { } // Otherwise, follow the forward chain. - seen := map[*Named]int{n0: 0, n: 1} - path := []Object{n0.obj, n.obj} + seen := map[*Named]int{n0: 0} + path := []Object{n0.obj} for { typ = n.underlying - n, _ = typ.(*Named) - if n == nil { + n1, _ := typ.(*Named) + if n1 == nil { break // end of chain } + seen[n] = len(seen) + path = append(path, n.obj) + n = n1 + if i, ok := seen[n]; ok { // cycle check.cycleError(path[i:]) typ = Typ[Invalid] break } - - seen[n] = len(seen) - path = append(path, n.obj) } for n := range seen { + // We should never have to update the underlying type of an imported type; + // those underlying types should have been resolved during the import. + // Also, doing so would lead to a race condition (was issue #31749). + if n.obj.pkg != check.pkg { + panic("internal error: imported type with unresolved underlying type") + } n.underlying = typ } diff --git a/src/go/types/issues_test.go b/src/go/types/issues_test.go index 1d0c0cb08a..f59f905397 100644 --- a/src/go/types/issues_test.go +++ b/src/go/types/issues_test.go @@ -493,3 +493,33 @@ func (h importHelper) Import(path string) (*Package, error) { } return h.pkg, nil } + +// TestIssue34921 verifies that we don't update an imported type's underlying +// type when resolving an underlying type. Specifically, when determining the +// underlying type of b.T (which is the underlying type of a.T, which is int) +// we must not set the underlying type of a.T again since that would lead to +// a race condition if package b is imported elsewhere, in a package that is +// concurrently type-checked. +func TestIssue34921(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Error(r) + } + }() + + var sources = []string{ + `package a; type T int`, + `package b; import "a"; type T a.T`, + } + + var pkg *Package + for _, src := range sources { + f := mustParse(t, src) + conf := Config{Importer: importHelper{pkg}} + res, err := conf.Check(f.Name.Name, fset, []*ast.File{f}, nil) + if err != nil { + t.Errorf("%q failed to typecheck: %v", src, err) + } + pkg = res // res is imported by the next package in this test + } +} -- 2.50.0