From 6c25ba624fd032a20dcfa94f9f9f0ae32c57c54b Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 1 Jun 2022 12:54:00 -0400 Subject: [PATCH 1/7] go/token: delete unused File.set field This field is only used for a sanity check in a test. Change-Id: I868ed10131ec33994ebb1b1d88f6740956824bd7 Reviewed-on: https://go-review.googlesource.com/c/go/+/409834 Run-TryBot: Robert Findley TryBot-Result: Gopher Robot Reviewed-by: Robert Findley Reviewed-by: Robert Griesemer --- src/go/token/position.go | 3 +-- src/go/token/serialize.go | 1 - src/go/token/serialize_test.go | 6 ------ 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/go/token/position.go b/src/go/token/position.go index 00f24535bf..bd9ae07b28 100644 --- a/src/go/token/position.go +++ b/src/go/token/position.go @@ -92,7 +92,6 @@ func (p Pos) IsValid() bool { // A File is a handle for a file belonging to a FileSet. // A File has a name, size, and line offset table. type File struct { - set *FileSet name string // file name as provided to AddFile base int // Pos value range for this file is [base...base+size] size int // file size as provided to AddFile @@ -418,7 +417,7 @@ func (s *FileSet) AddFile(filename string, base, size int) *File { panic(fmt.Sprintf("invalid size %d (should be >= 0)", size)) } // base >= s.base && size >= 0 - f := &File{set: s, name: filename, base: base, size: size, lines: []int{0}} + f := &File{name: filename, base: base, size: size, lines: []int{0}} base += size + 1 // +1 because EOF also has a position if base < 0 { panic("token.Pos offset overflow (> 2G of source code in file set)") diff --git a/src/go/token/serialize.go b/src/go/token/serialize.go index ffb69908b9..38c10ebd47 100644 --- a/src/go/token/serialize.go +++ b/src/go/token/serialize.go @@ -31,7 +31,6 @@ func (s *FileSet) Read(decode func(any) error) error { for i := 0; i < len(ss.Files); i++ { f := &ss.Files[i] files[i] = &File{ - set: s, name: f.Name, base: f.Base, size: f.Size, diff --git a/src/go/token/serialize_test.go b/src/go/token/serialize_test.go index 4aa0b0da26..8d9799547a 100644 --- a/src/go/token/serialize_test.go +++ b/src/go/token/serialize_test.go @@ -35,12 +35,6 @@ func equal(p, q *FileSet) error { for i, f := range p.files { g := q.files[i] - if f.set != p { - return fmt.Errorf("wrong fileset for %q", f.name) - } - if g.set != q { - return fmt.Errorf("wrong fileset for %q", g.name) - } if f.name != g.name { return fmt.Errorf("different filenames: %q != %q", f.name, g.name) } From dd2d00f9d57ba6aafb084d83ffc7fb35f97b8c84 Mon Sep 17 00:00:00 2001 From: Dmitri Goutnik Date: Fri, 17 Jun 2022 12:10:09 -0500 Subject: [PATCH 2/7] net: fix flaky *TimeoutMustNotReturn tests The tester goroutine doesn't always gets a chance to run before the timeout expires. Wait for the goroutine to start and set deadlines before staring the timer. Fixes #36796 Change-Id: Iffed6259de31340c3f66e34da473826a1d09fcde Reviewed-on: https://go-review.googlesource.com/c/go/+/412858 Reviewed-by: Damien Neil Run-TryBot: Bryan Mills Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot --- src/net/timeout_test.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/net/timeout_test.go b/src/net/timeout_test.go index d1cfbf853c..3ad026c490 100644 --- a/src/net/timeout_test.go +++ b/src/net/timeout_test.go @@ -243,8 +243,7 @@ func TestAcceptTimeoutMustNotReturn(t *testing.T) { ln := newLocalListener(t, "tcp") defer ln.Close() - max := time.NewTimer(100 * time.Millisecond) - defer max.Stop() + maxch := make(chan *time.Timer) ch := make(chan error) go func() { if err := ln.(*TCPListener).SetDeadline(time.Now().Add(-5 * time.Second)); err != nil { @@ -253,10 +252,14 @@ func TestAcceptTimeoutMustNotReturn(t *testing.T) { if err := ln.(*TCPListener).SetDeadline(noDeadline); err != nil { t.Error(err) } + maxch <- time.NewTimer(100 * time.Millisecond) _, err := ln.Accept() ch <- err }() + max := <-maxch + defer max.Stop() + select { case err := <-ch: if perr := parseAcceptError(err); perr != nil { @@ -348,8 +351,7 @@ func TestReadTimeoutMustNotReturn(t *testing.T) { } defer c.Close() - max := time.NewTimer(100 * time.Millisecond) - defer max.Stop() + maxch := make(chan *time.Timer) ch := make(chan error) go func() { if err := c.SetDeadline(time.Now().Add(-5 * time.Second)); err != nil { @@ -361,11 +363,15 @@ func TestReadTimeoutMustNotReturn(t *testing.T) { if err := c.SetReadDeadline(noDeadline); err != nil { t.Error(err) } + maxch <- time.NewTimer(100 * time.Millisecond) var b [1]byte _, err := c.Read(b[:]) ch <- err }() + max := <-maxch + defer max.Stop() + select { case err := <-ch: if perr := parseReadError(err); perr != nil { @@ -517,8 +523,7 @@ func TestWriteTimeoutMustNotReturn(t *testing.T) { } defer c.Close() - max := time.NewTimer(100 * time.Millisecond) - defer max.Stop() + maxch := make(chan *time.Timer) ch := make(chan error) go func() { if err := c.SetDeadline(time.Now().Add(-5 * time.Second)); err != nil { @@ -530,6 +535,7 @@ func TestWriteTimeoutMustNotReturn(t *testing.T) { if err := c.SetWriteDeadline(noDeadline); err != nil { t.Error(err) } + maxch <- time.NewTimer(100 * time.Millisecond) var b [1]byte for { if _, err := c.Write(b[:]); err != nil { @@ -539,6 +545,9 @@ func TestWriteTimeoutMustNotReturn(t *testing.T) { } }() + max := <-maxch + defer max.Stop() + select { case err := <-ch: if perr := parseWriteError(err); perr != nil { From 9e2f2897546c51863bf860c30622fbe9e3359391 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 17 Jun 2022 12:34:30 -0400 Subject: [PATCH 3/7] cmd/go/internal/work: log clearer detail for subprocess errors in (*Builder).toolID For #52647. Change-Id: Ic12123769d339c2df677500ed59f15a4ee5037d3 Reviewed-on: https://go-review.googlesource.com/c/go/+/412954 Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot Auto-Submit: Bryan Mills Reviewed-by: Ian Lance Taylor --- src/cmd/go/internal/work/buildid.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/internal/work/buildid.go b/src/cmd/go/internal/work/buildid.go index 846e2c8b77..a5b5570e05 100644 --- a/src/cmd/go/internal/work/buildid.go +++ b/src/cmd/go/internal/work/buildid.go @@ -164,13 +164,16 @@ func (b *Builder) toolID(name string) string { cmd.Stdout = &stdout cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - base.Fatalf("%s: %v\n%s%s", desc, err, stdout.Bytes(), stderr.Bytes()) + if stderr.Len() > 0 { + os.Stderr.Write(stderr.Bytes()) + } + base.Fatalf("go: error obtaining buildID for %s: %v", desc, err) } line := stdout.String() f := strings.Fields(line) if len(f) < 3 || f[0] != name && path != VetTool || f[1] != "version" || f[2] == "devel" && !strings.HasPrefix(f[len(f)-1], "buildID=") { - base.Fatalf("%s -V=full: unexpected output:\n\t%s", desc, line) + base.Fatalf("go: parsing buildID from %s -V=full: unexpected output:\n\t%s", desc, line) } if f[2] == "devel" { // On the development branch, use the content ID part of the build ID. From d42a48828f3cff4e57cefaf72bc88cef7d355fd6 Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Tue, 14 Jun 2022 21:29:36 -0700 Subject: [PATCH 4/7] sync: add more notes about Cond behavior Cond is difficult to use correctly (I was just bitten by it in a production app that I inherited). While several proposals have come up to improve or remove sync.Cond, no action has so far been taken. Update the documentation to discourage use of sync.Cond, and point people in the direction of preferred alternatives. I believe this will help encourage behavior we want (less use of sync.Cond and more use of channels), while also paving the way for, potentially, removing Cond in a future version of the language. Thanks very much to Bryan Mills and Sean Liao for discussion and recommendations. Updates #20491. Updates #21165. Change-Id: Ib4d0631c79d4c4d0a30027255cd43bc47cddebd3 Reviewed-on: https://go-review.googlesource.com/c/go/+/412237 Run-TryBot: Ian Lance Taylor Auto-Submit: Ian Lance Taylor Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Ian Lance Taylor --- src/sync/cond.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/sync/cond.go b/src/sync/cond.go index 19f986e478..cbf5ba6071 100644 --- a/src/sync/cond.go +++ b/src/sync/cond.go @@ -22,6 +22,17 @@ import ( // In the terminology of the Go memory model, Cond arranges that // a call to Broadcast or Signal “synchronizes before” any Wait call // that it unblocks. +// +// For many simple use cases, users will be better off using channels than a +// Cond (Broadcast corresponds to closing a channel, and Signal corresponds to +// sending on a channel). +// +// For more on replacements for sync.Cond, see [Roberto Clapis's series on +// advanced concurrency patterns], as well as [Bryan Mills's talk on concurrency +// patterns]. +// +// [Roberto Clapis's series on advanced concurrency patterns]: https://blogtitle.github.io/categories/concurrency/ +// [Bryan Mills's talk on concurrency patterns]: https://drive.google.com/file/d/1nPdvhB0PutEJzdCq5ms6UI58dp50fcAN/view type Cond struct { noCopy noCopy @@ -64,6 +75,9 @@ func (c *Cond) Wait() { // // It is allowed but not required for the caller to hold c.L // during the call. +// +// Signal() does not affect goroutine scheduling priority; if other goroutines +// are attempting to lock c.L, they may be awoken before a "waiting" goroutine. func (c *Cond) Signal() { c.checker.check() runtime_notifyListNotifyOne(&c.notify) From 103cc661f1906837d02133e9c65d0475ac49799c Mon Sep 17 00:00:00 2001 From: Josh Powers Date: Fri, 17 Jun 2022 18:22:12 +0000 Subject: [PATCH 5/7] cmd/go/internal/modfetch: prevent duplicate hashes in go.sum To write go.sum, each module and then each hash is looped through. The hashes are kept in a slice and there is no check to ensure that hashes were not added or already exist in the file. Therefore, unique the hashes of each module before writing to prevent duplicates. Fixes: #28456 Change-Id: I1cf7e7cdee3e7530a0ee605cd76d738627be1e0d GitHub-Last-Rev: 0ed02e9591e966fe5f6ba275635c3974daa2656e GitHub-Pull-Request: golang/go#53291 Reviewed-on: https://go-review.googlesource.com/c/go/+/411154 Auto-Submit: Bryan Mills Run-TryBot: Bryan Mills Reviewed-by: Ian Lance Taylor Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot --- src/cmd/go/internal/modfetch/fetch.go | 1 + .../testdata/script/mod_tidy_duplicates.txt | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 src/cmd/go/testdata/script/mod_tidy_duplicates.txt diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index a7c8c2c769..426df9bc04 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -833,6 +833,7 @@ Outer: for _, m := range mods { list := goSum.m[m] sort.Strings(list) + str.Uniq(&list) for _, h := range list { st := goSum.status[modSum{m, h}] if (!st.dirty || (st.used && keep[m])) && !sumInWorkspaceModulesLocked(m) { diff --git a/src/cmd/go/testdata/script/mod_tidy_duplicates.txt b/src/cmd/go/testdata/script/mod_tidy_duplicates.txt new file mode 100644 index 0000000000..d454c8dc82 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_tidy_duplicates.txt @@ -0,0 +1,38 @@ +env GO111MODULE=on + +# Regression test for golang.org/issue/28456: +# 'go mod tidy' should not leave duplicate lines when re-writing the file. + +go mod tidy +cmp go.sum golden.sum + +-- go.mod -- +module use + +go 1.16 + +require rsc.io/quote v1.5.2 + +-- go.sum -- +rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0= +rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0= +rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= +rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= +rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/testonly v1.0.0 h1:K/VWHdO+Jv7woUXG0GzVNx1czBXUt3Ib1deaMn+xk64= +rsc.io/testonly v1.0.0/go.mod h1:OqmGbIFOcF+XrFReLOGZ6BhMM7uMBiQwZsyNmh74SzY= +-- golden.sum -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0= +rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= +rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/testonly v1.0.0 h1:K/VWHdO+Jv7woUXG0GzVNx1czBXUt3Ib1deaMn+xk64= +rsc.io/testonly v1.0.0/go.mod h1:OqmGbIFOcF+XrFReLOGZ6BhMM7uMBiQwZsyNmh74SzY= +-- main.go -- +package use + +import _ "rsc.io/quote" From ec58e3f3271de385cf976a805e611d3da09c3a0e Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Fri, 17 Jun 2022 15:19:59 -0700 Subject: [PATCH 6/7] test: add regress test for #53419 This currently works with GOEXPERIMENT=unified. Add a regress test to make sure it stays that way. Updates #53419. Change-Id: I2ea1f9039c59807fbd497d69a0420771f8d6d035 Reviewed-on: https://go-review.googlesource.com/c/go/+/413014 Run-TryBot: Matthew Dempsky Auto-Submit: Matthew Dempsky Reviewed-by: Robert Griesemer TryBot-Result: Gopher Robot --- src/go/internal/gcimporter/gcimporter_test.go | 1 + test/run.go | 1 + test/typeparam/issue53419.go | 28 +++++++++++++++++++ 3 files changed, 30 insertions(+) create mode 100644 test/typeparam/issue53419.go diff --git a/src/go/internal/gcimporter/gcimporter_test.go b/src/go/internal/gcimporter/gcimporter_test.go index 6dced31ffb..9aca6216a7 100644 --- a/src/go/internal/gcimporter/gcimporter_test.go +++ b/src/go/internal/gcimporter/gcimporter_test.go @@ -177,6 +177,7 @@ func TestImportTypeparamTests(t *testing.T) { "equal.go": "inconsistent embedded sorting", // TODO(rfindley): investigate this. "nested.go": "fails to compile", // TODO(rfindley): investigate this. "issue50417.go": "inconsistent interface member sorting", + "issue53419.go": "fails to compile", } for _, entry := range list { diff --git a/test/run.go b/test/run.go index cb1622ccc9..8934e23b38 100644 --- a/test/run.go +++ b/test/run.go @@ -1966,6 +1966,7 @@ var types2Failures32Bit = setOf( var go118Failures = setOf( "typeparam/nested.go", // 1.18 compiler doesn't support function-local types with generics "typeparam/issue51521.go", // 1.18 compiler produces bad panic message and link error + "typeparam/issue53419.go", // 1.18 compiler mishandles generic selector resolution ) // In all of these cases, the 1.17 compiler reports reasonable errors, but either the diff --git a/test/typeparam/issue53419.go b/test/typeparam/issue53419.go new file mode 100644 index 0000000000..62a226ff9f --- /dev/null +++ b/test/typeparam/issue53419.go @@ -0,0 +1,28 @@ +// run + +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +type T1 struct{} +type T2 struct{} +type Both struct { + T1 + T2 +} + +func (T1) m() { panic("FAIL") } +func (T2) m() { panic("FAIL") } +func (Both) m() {} + +func f[T interface{ m() }](c T) { + c.m() +} + +func main() { + var b Both + b.m() + f(b) +} From 527ace0ffa81d59698d3a78ac3545de7295ea76b Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Thu, 16 Jun 2022 23:04:25 +0700 Subject: [PATCH 7/7] cmd/compile: skip substituting closures in unsafe builtins arguments For unsafe.{Alignof,Offsetof,Sizeof}, subster will transform them them to OLITERAL nodes, and discard their arguments. However, any closure in their children nodes were already processed and added to declaration queue. Thus, we lack of information for generating instantiation for the closure. To fix it, just skip substituting the closures if we are going to edit the children nodes of unsafe builtins. Fixes #53390 Change-Id: Ia815cd05af9dc0491f10faac4399f378ac53dec6 Reviewed-on: https://go-review.googlesource.com/c/go/+/412794 Reviewed-by: Keith Randall TryBot-Result: Gopher Robot Reviewed-by: Keith Randall Run-TryBot: Cuong Manh Le Reviewed-by: Matthew Dempsky --- src/cmd/compile/internal/noder/stencil.go | 27 ++++++++++++++++++----- test/typeparam/issue53390.go | 20 +++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 test/typeparam/issue53390.go diff --git a/src/cmd/compile/internal/noder/stencil.go b/src/cmd/compile/internal/noder/stencil.go index 3f12aa3cbd..eeb503811c 100644 --- a/src/cmd/compile/internal/noder/stencil.go +++ b/src/cmd/compile/internal/noder/stencil.go @@ -721,11 +721,12 @@ func (g *genInst) getInstantiation(nameNode *ir.Name, shapes []*types.Type, isMe // Struct containing info needed for doing the substitution as we create the // instantiation of a generic function with specified type arguments. type subster struct { - g *genInst - isMethod bool // If a method is being instantiated - newf *ir.Func // Func node for the new stenciled function - ts typecheck.Tsubster - info *instInfo // Place to put extra info in the instantiation + g *genInst + isMethod bool // If a method is being instantiated + newf *ir.Func // Func node for the new stenciled function + ts typecheck.Tsubster + info *instInfo // Place to put extra info in the instantiation + skipClosure bool // Skip substituting closures // Map from non-nil, non-ONAME node n to slice of all m, where m.Defn = n defnMap map[ir.Node][]**ir.Name @@ -978,7 +979,20 @@ func (subst *subster) node(n ir.Node) ir.Node { } } + old := subst.skipClosure + // For unsafe.{Alignof,Offsetof,Sizeof}, subster will transform them to OLITERAL nodes, + // and discard their arguments. However, their children nodes were already process before, + // thus if they contain any closure, the closure was still be added to package declarations + // queue for processing later. Thus, genInst will fail to generate instantiation for the + // closure because of lacking dictionary information, see issue #53390. + if call, ok := m.(*ir.CallExpr); ok && call.X.Op() == ir.ONAME { + switch call.X.Name().BuiltinOp { + case ir.OALIGNOF, ir.OOFFSETOF, ir.OSIZEOF: + subst.skipClosure = true + } + } ir.EditChildren(m, edit) + subst.skipClosure = old m.SetTypecheck(1) @@ -1123,6 +1137,9 @@ func (subst *subster) node(n ir.Node) ir.Node { } case ir.OCLOSURE: + if subst.skipClosure { + break + } // We're going to create a new closure from scratch, so clear m // to avoid using the ir.Copy by accident until we reassign it. m = nil diff --git a/test/typeparam/issue53390.go b/test/typeparam/issue53390.go new file mode 100644 index 0000000000..52098c520b --- /dev/null +++ b/test/typeparam/issue53390.go @@ -0,0 +1,20 @@ +// compile + +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package p + +import "unsafe" + +func F[T any](v T) uintptr { + return unsafe.Alignof(func() T { + func(any) {}(struct{ _ T }{}) + return v + }()) +} + +func f() { + F(0) +}