From 070951c5dcc47c9cff2ad4c1ac6170a4060a4d0c Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 2 Feb 2022 13:22:18 -0800 Subject: [PATCH 01/42] constraints: remove package It has moved to golang.org/x/exp/constraints. Perhaps it will move back to the standard library in a future release. For golang/go#45458 Fixes golang/go#50792 Change-Id: I93aa251a7afe7b329a3d3faadc0c5d6388b1f0e9 Reviewed-on: https://go-review.googlesource.com/c/go/+/382460 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor Reviewed-by: Robert Griesemer TryBot-Result: Gopher Robot --- api/go1.18.txt | 6 - doc/go1.18.html | 8 -- .../types2/testdata/fixedbugs/issue47818.go2 | 4 - .../types2/testdata/fixedbugs/issue49705.go2 | 7 +- src/constraints/constraints.go | 50 -------- src/constraints/constraints_test.go | 117 ------------------ .../types/testdata/fixedbugs/issue47818.go2 | 4 - .../types/testdata/fixedbugs/issue49705.go2 | 7 +- test/typeparam/issue50121.dir/a.go | 8 +- test/typeparam/issue50121b.dir/a.go | 9 +- test/typeparam/issue50193.go | 11 +- 11 files changed, 28 insertions(+), 203 deletions(-) delete mode 100644 src/constraints/constraints.go delete mode 100644 src/constraints/constraints_test.go diff --git a/api/go1.18.txt b/api/go1.18.txt index 2d05c3f41c..7a81ce259e 100644 --- a/api/go1.18.txt +++ b/api/go1.18.txt @@ -1,12 +1,6 @@ pkg bufio, method (*Writer) AvailableBuffer() []uint8 pkg bufio, method (ReadWriter) AvailableBuffer() []uint8 pkg bytes, func Cut([]uint8, []uint8) ([]uint8, []uint8, bool) -pkg constraints, type Complex interface {} -pkg constraints, type Float interface {} -pkg constraints, type Integer interface {} -pkg constraints, type Ordered interface {} -pkg constraints, type Signed interface {} -pkg constraints, type Unsigned interface {} pkg crypto/tls, method (*Conn) NetConn() net.Conn pkg debug/buildinfo, func Read(io.ReaderAt) (*debug.BuildInfo, error) pkg debug/buildinfo, func ReadFile(string) (*debug.BuildInfo, error) diff --git a/doc/go1.18.html b/doc/go1.18.html index e69113411e..cb3c2dbac3 100644 --- a/doc/go1.18.html +++ b/doc/go1.18.html @@ -467,14 +467,6 @@ Do not send CLs removing the interior tags from such phrases.

Core library

-

New constraints package

- -

- The new constraints package - defines a set of useful constraints that can be used with type parameters of - generic functions. -

-

New debug/buildinfo package

diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue47818.go2 b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue47818.go2 index 2631118bae..546de1ce31 100644 --- a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue47818.go2 +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue47818.go2 @@ -8,8 +8,6 @@ package go1_17 -import "constraints" - type T[P /* ERROR type parameters require go1\.18 or later */ any /* ERROR undeclared name: any \(requires version go1\.18 or later\) */ ] struct{} // for init (and main, but we're not in package main) we should only get one error @@ -57,5 +55,3 @@ type ( _ = C1 _ = C2 ) - -type Ordered constraints /* ERROR using type constraint constraints\.Ordered requires go1\.18 or later */ .Ordered diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49705.go2 b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49705.go2 index 2b991b8722..5b5fba2a1d 100644 --- a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49705.go2 +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49705.go2 @@ -4,8 +4,11 @@ package p -import "constraints" +type Integer interface { + ~int | ~int8 | ~int16 | ~int32 | ~int64 | + ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | ~uintptr +} -func shl[I constraints.Integer](n int) I { +func shl[I Integer](n int) I { return 1 << n } diff --git a/src/constraints/constraints.go b/src/constraints/constraints.go deleted file mode 100644 index 2c033dff47..0000000000 --- a/src/constraints/constraints.go +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright 2021 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 constraints defines a set of useful constraints to be used -// with type parameters. -package constraints - -// Signed is a constraint that permits any signed integer type. -// If future releases of Go add new predeclared signed integer types, -// this constraint will be modified to include them. -type Signed interface { - ~int | ~int8 | ~int16 | ~int32 | ~int64 -} - -// Unsigned is a constraint that permits any unsigned integer type. -// If future releases of Go add new predeclared unsigned integer types, -// this constraint will be modified to include them. -type Unsigned interface { - ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | ~uintptr -} - -// Integer is a constraint that permits any integer type. -// If future releases of Go add new predeclared integer types, -// this constraint will be modified to include them. -type Integer interface { - Signed | Unsigned -} - -// Float is a constraint that permits any floating-point type. -// If future releases of Go add new predeclared floating-point types, -// this constraint will be modified to include them. -type Float interface { - ~float32 | ~float64 -} - -// Complex is a constraint that permits any complex numeric type. -// If future releases of Go add new predeclared complex numeric types, -// this constraint will be modified to include them. -type Complex interface { - ~complex64 | ~complex128 -} - -// Ordered is a constraint that permits any ordered type: any type -// that supports the operators < <= >= >. -// If future releases of Go add new ordered types, -// this constraint will be modified to include them. -type Ordered interface { - Integer | Float | ~string -} diff --git a/src/constraints/constraints_test.go b/src/constraints/constraints_test.go deleted file mode 100644 index 47d4cba52a..0000000000 --- a/src/constraints/constraints_test.go +++ /dev/null @@ -1,117 +0,0 @@ -// Copyright 2021 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 constraints - -import ( - "bytes" - "fmt" - "internal/testenv" - "os" - "os/exec" - "path/filepath" - "testing" -) - -type ( - testSigned[T Signed] struct{ f T } - testUnsigned[T Unsigned] struct{ f T } - testInteger[T Integer] struct{ f T } - testFloat[T Float] struct{ f T } - testComplex[T Complex] struct{ f T } - testOrdered[T Ordered] struct{ f T } -) - -// TestTypes passes if it compiles. -type TestTypes struct { - _ testSigned[int] - _ testSigned[int64] - _ testUnsigned[uint] - _ testUnsigned[uintptr] - _ testInteger[int8] - _ testInteger[uint8] - _ testInteger[uintptr] - _ testFloat[float32] - _ testComplex[complex64] - _ testOrdered[int] - _ testOrdered[float64] - _ testOrdered[string] -} - -var prolog = []byte(` -package constrainttest - -import "constraints" - -type ( - testSigned[T constraints.Signed] struct{ f T } - testUnsigned[T constraints.Unsigned] struct{ f T } - testInteger[T constraints.Integer] struct{ f T } - testFloat[T constraints.Float] struct{ f T } - testComplex[T constraints.Complex] struct{ f T } - testOrdered[T constraints.Ordered] struct{ f T } -) -`) - -func TestFailure(t *testing.T) { - testenv.MustHaveGoBuild(t) - gocmd := testenv.GoToolPath(t) - tmpdir := t.TempDir() - - if err := os.WriteFile(filepath.Join(tmpdir, "go.mod"), []byte("module constraintest"), 0666); err != nil { - t.Fatal(err) - } - - // Test for types that should not satisfy a constraint. - // For each pair of constraint and type, write a Go file - // var V constraint[type] - // For example, - // var V testSigned[uint] - // This should not compile, as testSigned (above) uses - // constraints.Signed, and uint does not satisfy that constraint. - // Therefore, the build of that code should fail. - for i, test := range []struct { - constraint, typ string - }{ - {"testSigned", "uint"}, - {"testUnsigned", "int"}, - {"testInteger", "float32"}, - {"testFloat", "int8"}, - {"testComplex", "float64"}, - {"testOrdered", "bool"}, - } { - i := i - test := test - t.Run(fmt.Sprintf("%s %d", test.constraint, i), func(t *testing.T) { - t.Parallel() - name := fmt.Sprintf("go%d.go", i) - f, err := os.Create(filepath.Join(tmpdir, name)) - if err != nil { - t.Fatal(err) - } - if _, err := f.Write(prolog); err != nil { - t.Fatal(err) - } - if _, err := fmt.Fprintf(f, "var V %s[%s]\n", test.constraint, test.typ); err != nil { - t.Fatal(err) - } - if err := f.Close(); err != nil { - t.Fatal(err) - } - cmd := exec.Command(gocmd, "build", name) - cmd.Dir = tmpdir - if out, err := cmd.CombinedOutput(); err == nil { - t.Error("build succeeded, but expected to fail") - } else if len(out) > 0 { - t.Logf("%s", out) - const want = "does not implement" - if !bytes.Contains(out, []byte(want)) { - t.Errorf("output does not include %q", want) - } - } else { - t.Error("no error output, expected something") - } - }) - } -} diff --git a/src/go/types/testdata/fixedbugs/issue47818.go2 b/src/go/types/testdata/fixedbugs/issue47818.go2 index 2631118bae..546de1ce31 100644 --- a/src/go/types/testdata/fixedbugs/issue47818.go2 +++ b/src/go/types/testdata/fixedbugs/issue47818.go2 @@ -8,8 +8,6 @@ package go1_17 -import "constraints" - type T[P /* ERROR type parameters require go1\.18 or later */ any /* ERROR undeclared name: any \(requires version go1\.18 or later\) */ ] struct{} // for init (and main, but we're not in package main) we should only get one error @@ -57,5 +55,3 @@ type ( _ = C1 _ = C2 ) - -type Ordered constraints /* ERROR using type constraint constraints\.Ordered requires go1\.18 or later */ .Ordered diff --git a/src/go/types/testdata/fixedbugs/issue49705.go2 b/src/go/types/testdata/fixedbugs/issue49705.go2 index 2b991b8722..5b5fba2a1d 100644 --- a/src/go/types/testdata/fixedbugs/issue49705.go2 +++ b/src/go/types/testdata/fixedbugs/issue49705.go2 @@ -4,8 +4,11 @@ package p -import "constraints" +type Integer interface { + ~int | ~int8 | ~int16 | ~int32 | ~int64 | + ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | ~uintptr +} -func shl[I constraints.Integer](n int) I { +func shl[I Integer](n int) I { return 1 << n } diff --git a/test/typeparam/issue50121.dir/a.go b/test/typeparam/issue50121.dir/a.go index 9918fa38a6..ca11b6b27a 100644 --- a/test/typeparam/issue50121.dir/a.go +++ b/test/typeparam/issue50121.dir/a.go @@ -5,11 +5,15 @@ package a import ( - "constraints" "math/rand" ) -type Builder[T constraints.Integer] struct{} +type Integer interface { + ~int | ~int8 | ~int16 | ~int32 | ~int64 | + ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | ~uintptr +} + +type Builder[T Integer] struct{} func (r Builder[T]) New() T { return T(rand.Int()) diff --git a/test/typeparam/issue50121b.dir/a.go b/test/typeparam/issue50121b.dir/a.go index f2b706e0fd..4ddbb6ea84 100644 --- a/test/typeparam/issue50121b.dir/a.go +++ b/test/typeparam/issue50121b.dir/a.go @@ -4,11 +4,12 @@ package a -import ( - "constraints" -) +type Integer interface { + ~int | ~int8 | ~int16 | ~int32 | ~int64 | + ~uint | ~uint8 | ~uint16 | ~uint32 | ~uint64 | ~uintptr +} -type Builder[T constraints.Integer] struct{} +type Builder[T Integer] struct{} func (r Builder[T]) New() T { return T(42) diff --git a/test/typeparam/issue50193.go b/test/typeparam/issue50193.go index 8dc488244e..76de588e74 100644 --- a/test/typeparam/issue50193.go +++ b/test/typeparam/issue50193.go @@ -7,17 +7,20 @@ package main import ( - "constraints" "fmt" ) -func zero[T constraints.Complex]() T { +type Complex interface { + ~complex64 | ~complex128 +} + +func zero[T Complex]() T { return T(0) } -func pi[T constraints.Complex]() T { +func pi[T Complex]() T { return T(3.14) } -func sqrtN1[T constraints.Complex]() T { +func sqrtN1[T Complex]() T { return T(-1i) } From 1c6426505e640799f2a16d6097eed3f83b372b37 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Tue, 1 Feb 2022 13:27:36 -0800 Subject: [PATCH 02/42] net/netip: correct ipv6 address in ParsePrefix comment Fixes #50950 Change-Id: Iea94dba6e57d7e7985d4ae06a9b59ad126568599 Reviewed-on: https://go-review.googlesource.com/c/go/+/382294 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Tobias Klauser Reviewed-by: Brad Fitzpatrick --- src/net/netip/netip.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net/netip/netip.go b/src/net/netip/netip.go index 591d38abc8..f27984ab57 100644 --- a/src/net/netip/netip.go +++ b/src/net/netip/netip.go @@ -1288,7 +1288,7 @@ func (p Prefix) isZero() bool { return p == Prefix{} } func (p Prefix) IsSingleIP() bool { return p.bits != 0 && int(p.bits) == p.ip.BitLen() } // ParsePrefix parses s as an IP address prefix. -// The string can be in the form "192.168.1.0/24" or "2001::db8::/32", +// The string can be in the form "192.168.1.0/24" or "2001:db8::/32", // the CIDR notation defined in RFC 4632 and RFC 4291. // // Note that masked address bits are not zeroed. Use Masked for that. From 54b2a75406a4e347cff2825b698f910549d6bd04 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 1 Feb 2022 16:35:02 -0500 Subject: [PATCH 03/42] cmd/go/internal/modload: skip deleted entries in UpdateWorkFile Fixes #50958 Change-Id: I25b4f34bea7705525217296471ce97e6a2ab99f6 Reviewed-on: https://go-review.googlesource.com/c/go/+/382245 Trust: Bryan Mills Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Michael Matloob --- src/cmd/go/internal/modload/init.go | 3 +++ .../go/testdata/script/work_use_issue50958.txt | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 src/cmd/go/testdata/script/work_use_issue50958.txt diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go index cdcfbeb8de..23f4efd02a 100644 --- a/src/cmd/go/internal/modload/init.go +++ b/src/cmd/go/internal/modload/init.go @@ -610,6 +610,9 @@ func UpdateWorkFile(wf *modfile.WorkFile) { missingModulePaths := map[string]string{} // module directory listed in file -> abspath modroot for _, d := range wf.Use { + if d.Path == "" { + continue // d is marked for deletion. + } modRoot := d.Path if d.ModulePath == "" { missingModulePaths[d.Path] = modRoot diff --git a/src/cmd/go/testdata/script/work_use_issue50958.txt b/src/cmd/go/testdata/script/work_use_issue50958.txt new file mode 100644 index 0000000000..7a25531f3d --- /dev/null +++ b/src/cmd/go/testdata/script/work_use_issue50958.txt @@ -0,0 +1,17 @@ +go work use -r . +cmp go.work go.work.want + +-- go.mod -- +module example +go 1.18 +-- go.work -- +go 1.18 + +use sub +-- go.work.want -- +go 1.18 + +use . +-- sub/README.txt -- +This directory no longer contains a go.mod file. + From d0a0606841937cd6dd1db7a95ebd9d6e7ad02d96 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 1 Feb 2022 16:59:41 -0500 Subject: [PATCH 04/42] cmd/go: fail 'go work' subcommands with a more helpful error if no go.work file exists Otherwise, the failure mode for these subcommands refers to an empty file path: go: open : no such file or directory Fixes #50964 Change-Id: I8776431a294d2b2246d7d147b6059054f31bc255 Reviewed-on: https://go-review.googlesource.com/c/go/+/382246 Trust: Bryan Mills Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Michael Matloob --- src/cmd/go/internal/workcmd/edit.go | 26 +++++++++++++--------- src/cmd/go/internal/workcmd/sync.go | 6 +++-- src/cmd/go/internal/workcmd/use.go | 3 +++ src/cmd/go/testdata/script/work_nowork.txt | 20 +++++++++++++++++ 4 files changed, 42 insertions(+), 13 deletions(-) create mode 100644 src/cmd/go/testdata/script/work_nowork.txt diff --git a/src/cmd/go/internal/workcmd/edit.go b/src/cmd/go/internal/workcmd/edit.go index 879ddc3b1d..e7b1b13271 100644 --- a/src/cmd/go/internal/workcmd/edit.go +++ b/src/cmd/go/internal/workcmd/edit.go @@ -115,17 +115,6 @@ func init() { } func runEditwork(ctx context.Context, cmd *base.Command, args []string) { - anyFlags := - *editGo != "" || - *editJSON || - *editPrint || - *editFmt || - len(workedits) > 0 - - if !anyFlags { - base.Fatalf("go: no flags specified (see 'go help work edit').") - } - if *editJSON && *editPrint { base.Fatalf("go: cannot use both -json and -print") } @@ -147,6 +136,21 @@ func runEditwork(ctx context.Context, cmd *base.Command, args []string) { } } + if gowork == "" { + base.Fatalf("go: no go.work file found\n\t(run 'go work init' first or specify path using -workfile flag)") + } + + anyFlags := + *editGo != "" || + *editJSON || + *editPrint || + *editFmt || + len(workedits) > 0 + + if !anyFlags { + base.Fatalf("go: no flags specified (see 'go help work edit').") + } + workFile, err := modload.ReadWorkFile(gowork) if err != nil { base.Fatalf("go: errors parsing %s:\n%s", base.ShortPath(gowork), err) diff --git a/src/cmd/go/internal/workcmd/sync.go b/src/cmd/go/internal/workcmd/sync.go index 1cca817517..948fc5d370 100644 --- a/src/cmd/go/internal/workcmd/sync.go +++ b/src/cmd/go/internal/workcmd/sync.go @@ -43,9 +43,11 @@ func init() { } func runSync(ctx context.Context, cmd *base.Command, args []string) { - modload.InitWorkfile() - modload.ForceUseModules = true + modload.InitWorkfile() + if modload.WorkFilePath() == "" { + base.Fatalf("go: no go.work file found\n\t(run 'go work init' first or specify path using -workfile flag)") + } workGraph := modload.LoadModGraph(ctx, "") _ = workGraph diff --git a/src/cmd/go/internal/workcmd/use.go b/src/cmd/go/internal/workcmd/use.go index a5ba6c7133..d3bc1b7d55 100644 --- a/src/cmd/go/internal/workcmd/use.go +++ b/src/cmd/go/internal/workcmd/use.go @@ -49,6 +49,9 @@ func runUse(ctx context.Context, cmd *base.Command, args []string) { modload.InitWorkfile() gowork = modload.WorkFilePath() + if gowork == "" { + base.Fatalf("go: no go.work file found\n\t(run 'go work init' first or specify path using -workfile flag)") + } workFile, err := modload.ReadWorkFile(gowork) if err != nil { base.Fatalf("go: %v", err) diff --git a/src/cmd/go/testdata/script/work_nowork.txt b/src/cmd/go/testdata/script/work_nowork.txt new file mode 100644 index 0000000000..b0320cbccb --- /dev/null +++ b/src/cmd/go/testdata/script/work_nowork.txt @@ -0,0 +1,20 @@ +! go work use +stderr '^go: no go\.work file found\n\t\(run ''go work init'' first or specify path using -workfile flag\)$' + +! go work use . +stderr '^go: no go\.work file found\n\t\(run ''go work init'' first or specify path using -workfile flag\)$' + +! go work edit +stderr '^go: no go\.work file found\n\t\(run ''go work init'' first or specify path using -workfile flag\)$' + +! go work edit -go=1.18 +stderr '^go: no go\.work file found\n\t\(run ''go work init'' first or specify path using -workfile flag\)$' + +! go work sync +stderr '^go: no go\.work file found\n\t\(run ''go work init'' first or specify path using -workfile flag\)$' + +-- go.mod -- +module example +go 1.18 +-- README.txt -- +There is no go.work file here. From 475ce826b75f113aff2810f3d27cb861adee0caa Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 2 Feb 2022 17:09:28 -0500 Subject: [PATCH 05/42] net: remove an arbitrary timeout in TestUDPReadSizeError Looking at the condition actually exercised by the test it seems unnecessary: assuming that the Write succeeds (checked earlier in the test), the Read must have a nonzero number of bytes available to read immediately. (That is not the case in TestUDPZeroByteBuffer, from which this test appears to have been derived.) Fixes #50870 Change-Id: Ia6040a2d5dc320f0b86ec9d6f6b91dc72e8f3b84 Reviewed-on: https://go-review.googlesource.com/c/go/+/382537 Trust: Bryan Mills Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Ian Lance Taylor --- src/net/udpsock_test.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/net/udpsock_test.go b/src/net/udpsock_test.go index 6f82554e56..f8acf6a028 100644 --- a/src/net/udpsock_test.go +++ b/src/net/udpsock_test.go @@ -415,19 +415,14 @@ func TestUDPReadSizeError(t *testing.T) { if n != len(b1) { t.Errorf("got %d; want %d", n, len(b1)) } - c1.SetReadDeadline(time.Now().Add(100 * time.Millisecond)) b2 := make([]byte, len(b1)-1) if genericRead { n, err = c1.(Conn).Read(b2) } else { n, _, err = c1.ReadFrom(b2) } - switch err { - case nil: // ReadFrom succeeds - default: // Read may timeout, it depends on the platform - if nerr, ok := err.(Error); (!ok || !nerr.Timeout()) && runtime.GOOS != "windows" { // Windows returns WSAEMSGSIZE - t.Fatal(err) - } + if err != nil && runtime.GOOS != "windows" { // Windows returns WSAEMSGSIZE + t.Fatal(err) } if n != len(b1)-1 { t.Fatalf("got %d; want %d", n, len(b1)-1) From b00447038a50e0923b12cb0bc3c28f6b842a7f54 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 27 Jan 2022 18:55:29 -0800 Subject: [PATCH 06/42] go/types, types2: shorter list for 2nd phase of function type inference In the 2nd phase of function argument type inference we only consider parameters with types that are single type parameters. Thus there is no need to collect anything else in the first phase. This matches the algorithm description in the forthcoming spec more closely. Change-Id: Ie5c29f30ff43b1e37d719ecbe1688b50ed2177f3 Reviewed-on: https://go-review.googlesource.com/c/go/+/381554 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/infer.go | 24 +++++++++++++----------- src/go/types/infer.go | 24 +++++++++++++----------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/cmd/compile/internal/types2/infer.go b/src/cmd/compile/internal/types2/infer.go index 51d0d22144..51b26eb2aa 100644 --- a/src/cmd/compile/internal/types2/infer.go +++ b/src/cmd/compile/internal/types2/infer.go @@ -179,7 +179,7 @@ func (check *Checker) infer(pos syntax.Pos, tparams []*TypeParam, targs []Type, if arg.mode == invalid { // An error was reported earlier. Ignore this targ // and continue, we may still be able to infer all - // targs resulting in fewer follon-on errors. + // targs resulting in fewer follow-on errors. continue } if targ := arg.typ; isTyped(targ) { @@ -190,7 +190,12 @@ func (check *Checker) infer(pos syntax.Pos, tparams []*TypeParam, targs []Type, errorf("type", par.typ, targ, arg) return nil } - } else { + } else if _, ok := par.typ.(*TypeParam); ok { + // Since default types are all basic (i.e., non-composite) types, an + // untyped argument will never match a composite parameter type; the + // only parameter type it can possibly match against is a *TypeParam. + // Thus, for untyped arguments we only need to look at parameter types + // that are single type parameters. indices = append(indices, i) } } @@ -219,20 +224,17 @@ func (check *Checker) infer(pos syntax.Pos, tparams []*TypeParam, targs []Type, // Some generic parameters with untyped arguments may have been given // a type by now, we can ignore them. for _, i := range indices { - par := params.At(i) - // Since untyped types are all basic (i.e., non-composite) types, an - // untyped argument will never match a composite parameter type; the - // only parameter type it can possibly match against is a *TypeParam. - // Thus, only consider untyped arguments for generic parameters that - // are not of composite types and which don't have a type inferred yet. - if tpar, _ := par.typ.(*TypeParam); tpar != nil && targs[tpar.index] == nil { + tpar := params.At(i).typ.(*TypeParam) // is type parameter by construction of indices + // Only consider untyped arguments for which the corresponding type + // parameter doesn't have an inferred type yet. + if targs[tpar.index] == nil { arg := args[i] targ := Default(arg.typ) // The default type for an untyped nil is untyped nil. We must not // infer an untyped nil type as type parameter type. Ignore untyped // nil by making sure all default argument types are typed. - if isTyped(targ) && !u.unify(par.typ, targ) { - errorf("default type", par.typ, targ, arg) + if isTyped(targ) && !u.unify(tpar, targ) { + errorf("default type", tpar, targ, arg) return nil } } diff --git a/src/go/types/infer.go b/src/go/types/infer.go index 2678da3bf5..6a9a662565 100644 --- a/src/go/types/infer.go +++ b/src/go/types/infer.go @@ -183,7 +183,7 @@ func (check *Checker) infer(posn positioner, tparams []*TypeParam, targs []Type, if arg.mode == invalid { // An error was reported earlier. Ignore this targ // and continue, we may still be able to infer all - // targs resulting in fewer follon-on errors. + // targs resulting in fewer follow-on errors. continue } if targ := arg.typ; isTyped(targ) { @@ -194,7 +194,12 @@ func (check *Checker) infer(posn positioner, tparams []*TypeParam, targs []Type, errorf("type", par.typ, targ, arg) return nil } - } else { + } else if _, ok := par.typ.(*TypeParam); ok { + // Since default types are all basic (i.e., non-composite) types, an + // untyped argument will never match a composite parameter type; the + // only parameter type it can possibly match against is a *TypeParam. + // Thus, for untyped arguments we only need to look at parameter types + // that are single type parameters. indices = append(indices, i) } } @@ -221,20 +226,17 @@ func (check *Checker) infer(posn positioner, tparams []*TypeParam, targs []Type, // Some generic parameters with untyped arguments may have been given // a type by now, we can ignore them. for _, i := range indices { - par := params.At(i) - // Since untyped types are all basic (i.e., non-composite) types, an - // untyped argument will never match a composite parameter type; the - // only parameter type it can possibly match against is a *TypeParam. - // Thus, only consider untyped arguments for generic parameters that - // are not of composite types and which don't have a type inferred yet. - if tpar, _ := par.typ.(*TypeParam); tpar != nil && targs[tpar.index] == nil { + tpar := params.At(i).typ.(*TypeParam) // is type parameter by construction of indices + // Only consider untyped arguments for which the corresponding type + // parameter doesn't have an inferred type yet. + if targs[tpar.index] == nil { arg := args[i] targ := Default(arg.typ) // The default type for an untyped nil is untyped nil. We must not // infer an untyped nil type as type parameter type. Ignore untyped // nil by making sure all default argument types are typed. - if isTyped(targ) && !u.unify(par.typ, targ) { - errorf("default type", par.typ, targ, arg) + if isTyped(targ) && !u.unify(tpar, targ) { + errorf("default type", tpar, targ, arg) return nil } } From fa4d9b8e2bc2612960c80474fca83a4c85a974eb Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 13 Jan 2022 15:38:14 -0500 Subject: [PATCH 07/42] cmd/go/internal/modfetch: do not short-circuit canonical versions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since at least CL 121857, the conversion logic in (*modfetch).codeRepo.Stat has had a short-circuit to use the version requested by the caller if it successfully resolves and is already canonical. However, we should not use that version if it refers to a branch instead of a tag, because branches (unlike tags) usually do not refer to a single, stable release: a branch named "v1.0.0" may be for the development of the v1.0.0 release, or for the development of patches based on v1.0.0, but only one commit (perhaps at the end of that branch — but possibly not even written yet!) can be that specific version. We already have some logic to prefer tags that are semver-equivalent to the version requested by the caller. That more general case suffices for exact equality too — so we can eliminate the special-case, fixing the bug and (happily!) also somewhat simplifying the code. Fixes #35671 Updates #41512 Change-Id: I2fd290190b8a99a580deec7e26d15659b58a50b0 Reviewed-on: https://go-review.googlesource.com/c/go/+/378400 Trust: Bryan Mills Run-TryBot: Bryan Mills Reviewed-by: Russ Cox TryBot-Result: Gopher Robot --- src/cmd/go/internal/modfetch/coderepo.go | 226 ++++++------ src/cmd/go/internal/modfetch/coderepo_test.go | 325 ++++++++++-------- .../testdata/script/mod_invalid_version.txt | 10 +- 3 files changed, 294 insertions(+), 267 deletions(-) diff --git a/src/cmd/go/internal/modfetch/coderepo.go b/src/cmd/go/internal/modfetch/coderepo.go index 79da010809..2206c7c840 100644 --- a/src/cmd/go/internal/modfetch/coderepo.go +++ b/src/cmd/go/internal/modfetch/coderepo.go @@ -298,16 +298,13 @@ func (r *codeRepo) Latest() (*RevInfo, error) { // If statVers is a valid module version, it is used for the Version field. // Otherwise, the Version is derived from the passed-in info and recent tags. func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, error) { - info2 := &RevInfo{ - Name: info.Name, - Short: info.Short, - Time: info.Time, - } - // If this is a plain tag (no dir/ prefix) // and the module path is unversioned, // and if the underlying file tree has no go.mod, // then allow using the tag with a +incompatible suffix. + // + // (If the version is +incompatible, then the go.mod file must not exist: + // +incompatible is not an ongoing opt-out from semantic import versioning.) var canUseIncompatible func() bool canUseIncompatible = func() bool { var ok bool @@ -321,19 +318,12 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e return ok } - invalidf := func(format string, args ...any) error { - return &module.ModuleError{ - Path: r.modPath, - Err: &module.InvalidVersionError{ - Version: info2.Version, - Err: fmt.Errorf(format, args...), - }, - } - } - - // checkGoMod verifies that the go.mod file for the module exists or does not - // exist as required by info2.Version and the module path represented by r. - checkGoMod := func() (*RevInfo, error) { + // checkCanonical verifies that the canonical version v is compatible with the + // module path represented by r, adding a "+incompatible" suffix if needed. + // + // If statVers is also canonical, checkCanonical also verifies that v is + // either statVers or statVers with the added "+incompatible" suffix. + checkCanonical := func(v string) (*RevInfo, error) { // If r.codeDir is non-empty, then the go.mod file must exist: the module // author — not the module consumer, — gets to decide how to carve up the repo // into modules. @@ -344,73 +334,91 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e // r.findDir verifies both of these conditions. Execute it now so that // r.Stat will correctly return a notExistError if the go.mod location or // declared module path doesn't match. - _, _, _, err := r.findDir(info2.Version) + _, _, _, err := r.findDir(v) if err != nil { // TODO: It would be nice to return an error like "not a module". // Right now we return "missing go.mod", which is a little confusing. return nil, &module.ModuleError{ Path: r.modPath, Err: &module.InvalidVersionError{ - Version: info2.Version, + Version: v, Err: notExistError{err: err}, }, } } - // If the version is +incompatible, then the go.mod file must not exist: - // +incompatible is not an ongoing opt-out from semantic import versioning. - if strings.HasSuffix(info2.Version, "+incompatible") { - if !canUseIncompatible() { - if r.pathMajor != "" { - return nil, invalidf("+incompatible suffix not allowed: module path includes a major version suffix, so major version must match") - } else { - return nil, invalidf("+incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required") - } - } - - if err := module.CheckPathMajor(strings.TrimSuffix(info2.Version, "+incompatible"), r.pathMajor); err == nil { - return nil, invalidf("+incompatible suffix not allowed: major version %s is compatible", semver.Major(info2.Version)) + invalidf := func(format string, args ...any) error { + return &module.ModuleError{ + Path: r.modPath, + Err: &module.InvalidVersionError{ + Version: v, + Err: fmt.Errorf(format, args...), + }, } } - return info2, nil + // Add the +incompatible suffix if needed or requested explicitly, and + // verify that its presence or absence is appropriate for this version + // (which depends on whether it has an explicit go.mod file). + + if v == strings.TrimSuffix(statVers, "+incompatible") { + v = statVers + } + base := strings.TrimSuffix(v, "+incompatible") + var errIncompatible error + if !module.MatchPathMajor(base, r.pathMajor) { + if canUseIncompatible() { + v = base + "+incompatible" + } else { + if r.pathMajor != "" { + errIncompatible = invalidf("module path includes a major version suffix, so major version must match") + } else { + errIncompatible = invalidf("module contains a go.mod file, so module path must match major version (%q)", path.Join(r.pathPrefix, semver.Major(v))) + } + } + } else if strings.HasSuffix(v, "+incompatible") { + errIncompatible = invalidf("+incompatible suffix not allowed: major version %s is compatible", semver.Major(v)) + } + + if statVers != "" && statVers == module.CanonicalVersion(statVers) { + // Since the caller-requested version is canonical, it would be very + // confusing to resolve it to anything but itself, possibly with a + // "+incompatible" suffix. Error out explicitly. + if statBase := strings.TrimSuffix(statVers, "+incompatible"); statBase != base { + return nil, &module.ModuleError{ + Path: r.modPath, + Err: &module.InvalidVersionError{ + Version: statVers, + Err: fmt.Errorf("resolves to version %v (%s is not a tag)", v, statBase), + }, + } + } + } + + if errIncompatible != nil { + return nil, errIncompatible + } + + return &RevInfo{ + Name: info.Name, + Short: info.Short, + Time: info.Time, + Version: v, + }, nil } // Determine version. - // - // If statVers is canonical, then the original call was repo.Stat(statVers). - // Since the version is canonical, we must not resolve it to anything but - // itself, possibly with a '+incompatible' annotation: we do not need to do - // the work required to look for an arbitrary pseudo-version. - if statVers != "" && statVers == module.CanonicalVersion(statVers) { - info2.Version = statVers - if module.IsPseudoVersion(info2.Version) { - if err := r.validatePseudoVersion(info, info2.Version); err != nil { - return nil, err - } - return checkGoMod() + if module.IsPseudoVersion(statVers) { + if err := r.validatePseudoVersion(info, statVers); err != nil { + return nil, err } - - if err := module.CheckPathMajor(info2.Version, r.pathMajor); err != nil { - if canUseIncompatible() { - info2.Version += "+incompatible" - return checkGoMod() - } else { - if vErr, ok := err.(*module.InvalidVersionError); ok { - // We're going to describe why the version is invalid in more detail, - // so strip out the existing “invalid version” wrapper. - err = vErr.Err - } - return nil, invalidf("module contains a go.mod file, so major version must be compatible: %v", err) - } - } - - return checkGoMod() + return checkCanonical(statVers) } - // statVers is empty or non-canonical, so we need to resolve it to a canonical - // version or pseudo-version. + // statVers is not a pseudo-version, so we need to either resolve it to a + // canonical version or verify that it is already a canonical tag + // (not a branch). // Derive or verify a version from a code repo tag. // Tag must have a prefix matching codeDir. @@ -441,71 +449,62 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e if v == "" || !strings.HasPrefix(trimmed, v) { return "", false // Invalid or incomplete version (just vX or vX.Y). } - if isRetracted(v) { - return "", false - } if v == trimmed { tagIsCanonical = true } - - if err := module.CheckPathMajor(v, r.pathMajor); err != nil { - if canUseIncompatible() { - return v + "+incompatible", tagIsCanonical - } - return "", false - } - return v, tagIsCanonical } // If the VCS gave us a valid version, use that. if v, tagIsCanonical := tagToVersion(info.Version); tagIsCanonical { - info2.Version = v - return checkGoMod() + if info, err := checkCanonical(v); err == nil { + return info, err + } } // Look through the tags on the revision for either a usable canonical version // or an appropriate base for a pseudo-version. - var pseudoBase string + var ( + highestCanonical string + pseudoBase string + ) for _, pathTag := range info.Tags { v, tagIsCanonical := tagToVersion(pathTag) - if tagIsCanonical { - if statVers != "" && semver.Compare(v, statVers) == 0 { - // The user requested a non-canonical version, but the tag for the - // canonical equivalent refers to the same revision. Use it. - info2.Version = v - return checkGoMod() + if statVers != "" && semver.Compare(v, statVers) == 0 { + // The tag is equivalent to the version requested by the user. + if tagIsCanonical { + // This tag is the canonical form of the requested version, + // not some other form with extra build metadata. + // Use this tag so that the resolved version will match exactly. + // (If it isn't actually allowed, we'll error out in checkCanonical.) + return checkCanonical(v) } else { - // Save the highest canonical tag for the revision. If we don't find a - // better match, we'll use it as the canonical version. + // The user explicitly requested something equivalent to this tag. We + // can't use the version from the tag directly: since the tag is not + // canonical, it could be ambiguous. For example, tags v0.0.1+a and + // v0.0.1+b might both exist and refer to different revisions. // - // NOTE: Do not replace this with semver.Max. Despite the name, - // semver.Max *also* canonicalizes its arguments, which uses - // semver.Canonical instead of module.CanonicalVersion and thereby - // strips our "+incompatible" suffix. - if semver.Compare(info2.Version, v) < 0 { - info2.Version = v - } + // The tag is otherwise valid for the module, so we can at least use it as + // the base of an unambiguous pseudo-version. + // + // If multiple tags match, tagToVersion will canonicalize them to the same + // base version. + pseudoBase = v + } + } + // Save the highest non-retracted canonical tag for the revision. + // If we don't find a better match, we'll use it as the canonical version. + if tagIsCanonical && semver.Compare(highestCanonical, v) < 0 && !isRetracted(v) { + if module.MatchPathMajor(v, r.pathMajor) || canUseIncompatible() { + highestCanonical = v } - } else if v != "" && semver.Compare(v, statVers) == 0 { - // The user explicitly requested something equivalent to this tag. We - // can't use the version from the tag directly: since the tag is not - // canonical, it could be ambiguous. For example, tags v0.0.1+a and - // v0.0.1+b might both exist and refer to different revisions. - // - // The tag is otherwise valid for the module, so we can at least use it as - // the base of an unambiguous pseudo-version. - // - // If multiple tags match, tagToVersion will canonicalize them to the same - // base version. - pseudoBase = v } } - // If we found any canonical tag for the revision, return it. + // If we found a valid canonical tag for the revision, return it. // Even if we found a good pseudo-version base, a canonical version is better. - if info2.Version != "" { - return checkGoMod() + if highestCanonical != "" { + return checkCanonical(highestCanonical) } // Find the highest tagged version in the revision's history, subject to @@ -528,11 +527,10 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e tag, _ = r.code.RecentTag(info.Name, tagPrefix, allowedMajor("v0")) } } - pseudoBase, _ = tagToVersion(tag) // empty if the tag is invalid + pseudoBase, _ = tagToVersion(tag) } - info2.Version = module.PseudoVersion(r.pseudoMajor, pseudoBase, info.Time, info.Short) - return checkGoMod() + return checkCanonical(module.PseudoVersion(r.pseudoMajor, pseudoBase, info.Time, info.Short)) } // validatePseudoVersion checks that version has a major version compatible with @@ -556,10 +554,6 @@ func (r *codeRepo) validatePseudoVersion(info *codehost.RevInfo, version string) } }() - if err := module.CheckPathMajor(version, r.pathMajor); err != nil { - return err - } - rev, err := module.PseudoVersionRev(version) if err != nil { return err diff --git a/src/cmd/go/internal/modfetch/coderepo_test.go b/src/cmd/go/internal/modfetch/coderepo_test.go index 02e399f352..d98ea87da2 100644 --- a/src/cmd/go/internal/modfetch/coderepo_test.go +++ b/src/cmd/go/internal/modfetch/coderepo_test.go @@ -418,171 +418,204 @@ var codeRepoTests = []codeRepoTest{ zipSum: "h1:JItBZ+gwA5WvtZEGEbuDL4lUttGtLrs53lmdurq3bOg=", zipFileHash: "9ea9ae1673cffcc44b7fdd3cc89953d68c102449b46c982dbf085e4f2e394da5", }, + { + // Git branch with a semver name, +incompatible version, and no go.mod file. + vcs: "git", + path: "vcs-test.golang.org/go/mod/gitrepo1", + rev: "v2.3.4+incompatible", + err: `resolves to version v2.0.1+incompatible (v2.3.4 is not a tag)`, + }, + { + // Git branch with a semver name, matching go.mod file, and compatible version. + vcs: "git", + path: "vcs-test.golang.org/git/semver-branch.git", + rev: "v1.0.0", + err: `resolves to version v0.1.1-0.20220202191944-09c4d8f6938c (v1.0.0 is not a tag)`, + }, + { + // Git branch with a semver name, matching go.mod file, and disallowed +incompatible version. + // The version/tag mismatch takes precedence over the +incompatible mismatched. + vcs: "git", + path: "vcs-test.golang.org/git/semver-branch.git", + rev: "v2.0.0+incompatible", + err: `resolves to version v0.1.0 (v2.0.0 is not a tag)`, + }, + { + // Git branch with a semver name, matching go.mod file, and mismatched version. + // The version/tag mismatch takes precedence over the +incompatible mismatched. + vcs: "git", + path: "vcs-test.golang.org/git/semver-branch.git", + rev: "v2.0.0", + err: `resolves to version v0.1.0 (v2.0.0 is not a tag)`, + }, + { + // v3.0.0-devel is the same as tag v4.0.0-beta.1, but v4.0.0-beta.1 would + // not be allowed because it is incompatible and a go.mod file exists. + // The error message should refer to a valid pseudo-version, not the + // unusable semver tag. + vcs: "git", + path: "vcs-test.golang.org/git/semver-branch.git", + rev: "v3.0.0-devel", + err: `resolves to version v0.1.1-0.20220203155313-d59622f6e4d7 (v3.0.0-devel is not a tag)`, + }, } func TestCodeRepo(t *testing.T) { testenv.MustHaveExternalNetwork(t) + tmpdir := t.TempDir() - tmpdir, err := os.MkdirTemp("", "modfetch-test-") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpdir) + for _, tt := range codeRepoTests { + f := func(tt codeRepoTest) func(t *testing.T) { + return func(t *testing.T) { + t.Parallel() + if tt.vcs != "mod" { + testenv.MustHaveExecPath(t, tt.vcs) + } - t.Run("parallel", func(t *testing.T) { - for _, tt := range codeRepoTests { - f := func(tt codeRepoTest) func(t *testing.T) { - return func(t *testing.T) { - t.Parallel() - if tt.vcs != "mod" { - testenv.MustHaveExecPath(t, tt.vcs) - } + repo := Lookup("direct", tt.path) - repo := Lookup("direct", tt.path) + if tt.mpath == "" { + tt.mpath = tt.path + } + if mpath := repo.ModulePath(); mpath != tt.mpath { + t.Errorf("repo.ModulePath() = %q, want %q", mpath, tt.mpath) + } - if tt.mpath == "" { - tt.mpath = tt.path - } - if mpath := repo.ModulePath(); mpath != tt.mpath { - t.Errorf("repo.ModulePath() = %q, want %q", mpath, tt.mpath) - } - - info, err := repo.Stat(tt.rev) - if err != nil { - if tt.err != "" { - if !strings.Contains(err.Error(), tt.err) { - t.Fatalf("repoStat(%q): %v, wanted %q", tt.rev, err, tt.err) - } - return - } - t.Fatalf("repo.Stat(%q): %v", tt.rev, err) - } + info, err := repo.Stat(tt.rev) + if err != nil { if tt.err != "" { - t.Errorf("repo.Stat(%q): success, wanted error", tt.rev) - } - if info.Version != tt.version { - t.Errorf("info.Version = %q, want %q", info.Version, tt.version) - } - if info.Name != tt.name { - t.Errorf("info.Name = %q, want %q", info.Name, tt.name) - } - if info.Short != tt.short { - t.Errorf("info.Short = %q, want %q", info.Short, tt.short) - } - if !info.Time.Equal(tt.time) { - t.Errorf("info.Time = %v, want %v", info.Time, tt.time) + if !strings.Contains(err.Error(), tt.err) { + t.Fatalf("repoStat(%q): %v, wanted %q", tt.rev, err, tt.err) + } + return } + t.Fatalf("repo.Stat(%q): %v", tt.rev, err) + } + if tt.err != "" { + t.Errorf("repo.Stat(%q): success, wanted error", tt.rev) + } + if info.Version != tt.version { + t.Errorf("info.Version = %q, want %q", info.Version, tt.version) + } + if info.Name != tt.name { + t.Errorf("info.Name = %q, want %q", info.Name, tt.name) + } + if info.Short != tt.short { + t.Errorf("info.Short = %q, want %q", info.Short, tt.short) + } + if !info.Time.Equal(tt.time) { + t.Errorf("info.Time = %v, want %v", info.Time, tt.time) + } - if tt.gomod != "" || tt.gomodErr != "" { - data, err := repo.GoMod(tt.version) - if err != nil && tt.gomodErr == "" { - t.Errorf("repo.GoMod(%q): %v", tt.version, err) - } else if err != nil && tt.gomodErr != "" { - if err.Error() != tt.gomodErr { - t.Errorf("repo.GoMod(%q): %v, want %q", tt.version, err, tt.gomodErr) - } - } else if tt.gomodErr != "" { - t.Errorf("repo.GoMod(%q) = %q, want error %q", tt.version, data, tt.gomodErr) - } else if string(data) != tt.gomod { - t.Errorf("repo.GoMod(%q) = %q, want %q", tt.version, data, tt.gomod) - } - } - - needHash := !testing.Short() && (tt.zipFileHash != "" || tt.zipSum != "") - if tt.zip != nil || tt.zipErr != "" || needHash { - f, err := os.CreateTemp(tmpdir, tt.version+".zip.") - if err != nil { - t.Fatalf("os.CreateTemp: %v", err) - } - zipfile := f.Name() - defer func() { - f.Close() - os.Remove(zipfile) - }() - - var w io.Writer - var h hash.Hash - if needHash { - h = sha256.New() - w = io.MultiWriter(f, h) - } else { - w = f - } - err = repo.Zip(w, tt.version) - f.Close() - if err != nil { - if tt.zipErr != "" { - if err.Error() == tt.zipErr { - return - } - t.Fatalf("repo.Zip(%q): %v, want error %q", tt.version, err, tt.zipErr) - } - t.Fatalf("repo.Zip(%q): %v", tt.version, err) - } - if tt.zipErr != "" { - t.Errorf("repo.Zip(%q): success, want error %q", tt.version, tt.zipErr) - } - - if tt.zip != nil { - prefix := tt.path + "@" + tt.version + "/" - z, err := zip.OpenReader(zipfile) - if err != nil { - t.Fatalf("open zip %s: %v", zipfile, err) - } - var names []string - for _, file := range z.File { - if !strings.HasPrefix(file.Name, prefix) { - t.Errorf("zip entry %v does not start with prefix %v", file.Name, prefix) - continue - } - names = append(names, file.Name[len(prefix):]) - } - z.Close() - if !reflect.DeepEqual(names, tt.zip) { - t.Fatalf("zip = %v\nwant %v\n", names, tt.zip) - } - } - - if needHash { - sum, err := dirhash.HashZip(zipfile, dirhash.Hash1) - if err != nil { - t.Errorf("repo.Zip(%q): %v", tt.version, err) - } else if sum != tt.zipSum { - t.Errorf("repo.Zip(%q): got file with sum %q, want %q", tt.version, sum, tt.zipSum) - } else if zipFileHash := hex.EncodeToString(h.Sum(nil)); zipFileHash != tt.zipFileHash { - t.Errorf("repo.Zip(%q): got file with hash %q, want %q (but content has correct sum)", tt.version, zipFileHash, tt.zipFileHash) - } + if tt.gomod != "" || tt.gomodErr != "" { + data, err := repo.GoMod(tt.version) + if err != nil && tt.gomodErr == "" { + t.Errorf("repo.GoMod(%q): %v", tt.version, err) + } else if err != nil && tt.gomodErr != "" { + if err.Error() != tt.gomodErr { + t.Errorf("repo.GoMod(%q): %v, want %q", tt.version, err, tt.gomodErr) } + } else if tt.gomodErr != "" { + t.Errorf("repo.GoMod(%q) = %q, want error %q", tt.version, data, tt.gomodErr) + } else if string(data) != tt.gomod { + t.Errorf("repo.GoMod(%q) = %q, want %q", tt.version, data, tt.gomod) } } - } - t.Run(strings.ReplaceAll(tt.path, "/", "_")+"/"+tt.rev, f(tt)) - if strings.HasPrefix(tt.path, vgotest1git) { - for vcs, alt := range altVgotests { - altTest := tt - altTest.vcs = vcs - altTest.path = alt + strings.TrimPrefix(altTest.path, vgotest1git) - if strings.HasPrefix(altTest.mpath, vgotest1git) { - altTest.mpath = alt + strings.TrimPrefix(altTest.mpath, vgotest1git) + + needHash := !testing.Short() && (tt.zipFileHash != "" || tt.zipSum != "") + if tt.zip != nil || tt.zipErr != "" || needHash { + f, err := os.CreateTemp(tmpdir, tt.version+".zip.") + if err != nil { + t.Fatalf("os.CreateTemp: %v", err) } - var m map[string]string - if alt == vgotest1hg { - m = hgmap + zipfile := f.Name() + defer func() { + f.Close() + os.Remove(zipfile) + }() + + var w io.Writer + var h hash.Hash + if needHash { + h = sha256.New() + w = io.MultiWriter(f, h) + } else { + w = f + } + err = repo.Zip(w, tt.version) + f.Close() + if err != nil { + if tt.zipErr != "" { + if err.Error() == tt.zipErr { + return + } + t.Fatalf("repo.Zip(%q): %v, want error %q", tt.version, err, tt.zipErr) + } + t.Fatalf("repo.Zip(%q): %v", tt.version, err) + } + if tt.zipErr != "" { + t.Errorf("repo.Zip(%q): success, want error %q", tt.version, tt.zipErr) + } + + if tt.zip != nil { + prefix := tt.path + "@" + tt.version + "/" + z, err := zip.OpenReader(zipfile) + if err != nil { + t.Fatalf("open zip %s: %v", zipfile, err) + } + var names []string + for _, file := range z.File { + if !strings.HasPrefix(file.Name, prefix) { + t.Errorf("zip entry %v does not start with prefix %v", file.Name, prefix) + continue + } + names = append(names, file.Name[len(prefix):]) + } + z.Close() + if !reflect.DeepEqual(names, tt.zip) { + t.Fatalf("zip = %v\nwant %v\n", names, tt.zip) + } + } + + if needHash { + sum, err := dirhash.HashZip(zipfile, dirhash.Hash1) + if err != nil { + t.Errorf("repo.Zip(%q): %v", tt.version, err) + } else if sum != tt.zipSum { + t.Errorf("repo.Zip(%q): got file with sum %q, want %q", tt.version, sum, tt.zipSum) + } else if zipFileHash := hex.EncodeToString(h.Sum(nil)); zipFileHash != tt.zipFileHash { + t.Errorf("repo.Zip(%q): got file with hash %q, want %q (but content has correct sum)", tt.version, zipFileHash, tt.zipFileHash) + } } - altTest.version = remap(altTest.version, m) - altTest.name = remap(altTest.name, m) - altTest.short = remap(altTest.short, m) - altTest.rev = remap(altTest.rev, m) - altTest.err = remap(altTest.err, m) - altTest.gomodErr = remap(altTest.gomodErr, m) - altTest.zipErr = remap(altTest.zipErr, m) - altTest.zipSum = "" - altTest.zipFileHash = "" - t.Run(strings.ReplaceAll(altTest.path, "/", "_")+"/"+altTest.rev, f(altTest)) } } } - }) + t.Run(strings.ReplaceAll(tt.path, "/", "_")+"/"+tt.rev, f(tt)) + if strings.HasPrefix(tt.path, vgotest1git) { + for vcs, alt := range altVgotests { + altTest := tt + altTest.vcs = vcs + altTest.path = alt + strings.TrimPrefix(altTest.path, vgotest1git) + if strings.HasPrefix(altTest.mpath, vgotest1git) { + altTest.mpath = alt + strings.TrimPrefix(altTest.mpath, vgotest1git) + } + var m map[string]string + if alt == vgotest1hg { + m = hgmap + } + altTest.version = remap(altTest.version, m) + altTest.name = remap(altTest.name, m) + altTest.short = remap(altTest.short, m) + altTest.rev = remap(altTest.rev, m) + altTest.err = remap(altTest.err, m) + altTest.gomodErr = remap(altTest.gomodErr, m) + altTest.zipErr = remap(altTest.zipErr, m) + altTest.zipSum = "" + altTest.zipFileHash = "" + t.Run(strings.ReplaceAll(altTest.path, "/", "_")+"/"+altTest.rev, f(altTest)) + } + } + } } var hgmap = map[string]string{ diff --git a/src/cmd/go/testdata/script/mod_invalid_version.txt b/src/cmd/go/testdata/script/mod_invalid_version.txt index 428b8aa60e..8385b08d95 100644 --- a/src/cmd/go/testdata/script/mod_invalid_version.txt +++ b/src/cmd/go/testdata/script/mod_invalid_version.txt @@ -194,10 +194,10 @@ cp go.mod.orig go.mod go mod edit -require github.com/pierrec/lz4@v2.0.9-0.20190209155647-9a39efadad3d+incompatible cd outside ! go list -m github.com/pierrec/lz4 -stderr 'go: example.com@v0.0.0 requires\n\tgithub.com/pierrec/lz4@v2.0.9-0.20190209155647-9a39efadad3d\+incompatible: invalid version: \+incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required' +stderr '^go: example.com@v0.0.0 requires\n\tgithub.com/pierrec/lz4@v2.0.9-0.20190209155647-9a39efadad3d\+incompatible: invalid version: module contains a go.mod file, so module path must match major version \("github.com/pierrec/lz4/v2"\)$' cd .. ! go list -m github.com/pierrec/lz4 -stderr 'github.com/pierrec/lz4@v2.0.9-0.20190209155647-9a39efadad3d\+incompatible: invalid version: \+incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required' +stderr '^go: github.com/pierrec/lz4@v2.0.9-0.20190209155647-9a39efadad3d\+incompatible: invalid version: module contains a go.mod file, so module path must match major version \("github.com/pierrec/lz4/v2"\)$' # A +incompatible pseudo-version is valid for a revision of the module # that lacks a go.mod file. @@ -222,7 +222,7 @@ stdout 'github.com/pierrec/lz4 v2.0.5\+incompatible' # not resolve to a pseudo-version with a different major version. cp go.mod.orig go.mod ! go get github.com/pierrec/lz4@v2.0.8 -stderr 'go: github.com/pierrec/lz4@v2.0.8: invalid version: module contains a go.mod file, so major version must be compatible: should be v0 or v1, not v2' +stderr 'go: github.com/pierrec/lz4@v2.0.8: invalid version: module contains a go.mod file, so module path must match major version \("github.com/pierrec/lz4/v2"\)$' # An invalid +incompatible suffix for a canonical version should error out, # not resolve to a pseudo-version. @@ -233,10 +233,10 @@ cp go.mod.orig go.mod go mod edit -require github.com/pierrec/lz4@v2.0.8+incompatible cd outside ! go list -m github.com/pierrec/lz4 -stderr 'github.com/pierrec/lz4@v2.0.8\+incompatible: invalid version: \+incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required' +stderr '^go: github.com/pierrec/lz4@v2.0.8\+incompatible: invalid version: module contains a go.mod file, so module path must match major version \("github.com/pierrec/lz4/v2"\)$' cd .. ! go list -m github.com/pierrec/lz4 -stderr 'github.com/pierrec/lz4@v2.0.8\+incompatible: invalid version: \+incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required' +stderr '^go: github.com/pierrec/lz4@v2.0.8\+incompatible: invalid version: module contains a go.mod file, so module path must match major version \("github.com/pierrec/lz4/v2"\)$' -- go.mod.orig -- module example.com From bec8a108b324b2fc68eafeee7293a479813ec4e2 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 2 Feb 2022 10:19:30 -0500 Subject: [PATCH 08/42] runtime: skip TestSegv failures with "morestack on g0" on darwin/amd64 This failure mode has been present since at least 2020-06-08. We have enough information to diagnose it, and further failures don't seem to be adding any new information at this point: they can only add noise, both on the Go project's builders and in users' own modules (for example, when run as part of 'go test all'). For #39457 Change-Id: I2379631da0c8af69598fa61c0cc5ac0ea6ba8267 Reviewed-on: https://go-review.googlesource.com/c/go/+/382395 Trust: Bryan Mills Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Cherry Mui --- src/runtime/crash_cgo_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go index 9444554d37..dc8f6a7148 100644 --- a/src/runtime/crash_cgo_test.go +++ b/src/runtime/crash_cgo_test.go @@ -596,6 +596,9 @@ func TestSegv(t *testing.T) { t.Log(got) want := "SIGSEGV" if !strings.Contains(got, want) { + if runtime.GOOS == "darwin" && runtime.GOARCH == "amd64" && strings.Contains(got, "fatal: morestack on g0") { + testenv.SkipFlaky(t, 39457) + } t.Errorf("did not see %q in output", want) } From f9b761aa76d0cf439a0c996e1b7c06a0fe49314e Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Mon, 31 Jan 2022 16:37:40 -0500 Subject: [PATCH 09/42] runtime: regression test for issue 50936 Add a regression test for issue 50936 which coerces the runtime into frequent execution of the cgocall dropg/execute curg assignment race by making many concurrent cgo calls eligible for P retake by sysmon. This results in no P during exitsyscall, at which point they will update curg and will crash if SIGPROF arrives in between updating mp.curg and mp.curg.m. This test is conceptually similar to the basic cgo callback test in aprof.go but with additional concurrency and a sleep in C. On my machine this test fails ~5% of the time prior to CL 382079. For #50936. Change-Id: I21b6c7f2594f9a615a64580ef70a88b692505678 Reviewed-on: https://go-review.googlesource.com/c/go/+/382244 Trust: Michael Pratt Run-TryBot: Michael Pratt TryBot-Result: Gopher Robot Reviewed-by: Cherry Mui --- src/runtime/crash_cgo_test.go | 13 +++ src/runtime/testdata/testprogcgo/aprof.go | 7 +- .../testdata/testprogcgo/pprof_callback.go | 89 +++++++++++++++++++ 3 files changed, 107 insertions(+), 2 deletions(-) create mode 100644 src/runtime/testdata/testprogcgo/pprof_callback.go diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go index dc8f6a7148..8c250f72d6 100644 --- a/src/runtime/crash_cgo_test.go +++ b/src/runtime/crash_cgo_test.go @@ -216,6 +216,19 @@ func TestCgoCCodeSIGPROF(t *testing.T) { } } +func TestCgoPprofCallback(t *testing.T) { + t.Parallel() + switch runtime.GOOS { + case "windows", "plan9": + t.Skipf("skipping cgo pprof callback test on %s", runtime.GOOS) + } + got := runTestProg(t, "testprogcgo", "CgoPprofCallback") + want := "OK\n" + if got != want { + t.Errorf("expected %q got %v", want, got) + } +} + func TestCgoCrashTraceback(t *testing.T) { t.Parallel() switch platform := runtime.GOOS + "/" + runtime.GOARCH; platform { diff --git a/src/runtime/testdata/testprogcgo/aprof.go b/src/runtime/testdata/testprogcgo/aprof.go index 44a15b0865..c70d6333bb 100644 --- a/src/runtime/testdata/testprogcgo/aprof.go +++ b/src/runtime/testdata/testprogcgo/aprof.go @@ -7,8 +7,11 @@ package main // Test that SIGPROF received in C code does not crash the process // looking for the C code's func pointer. -// The test fails when the function is the first C function. -// The exported functions are the first C functions, so we use that. +// This is a regression test for issue 14599, where profiling fails when the +// function is the first C function. Exported functions are the first C +// functions, so we use an exported function. Exported functions are created in +// lexigraphical order of source files, so this file is named aprof.go to +// ensure its function is first. // extern void CallGoNop(); import "C" diff --git a/src/runtime/testdata/testprogcgo/pprof_callback.go b/src/runtime/testdata/testprogcgo/pprof_callback.go new file mode 100644 index 0000000000..e34564395e --- /dev/null +++ b/src/runtime/testdata/testprogcgo/pprof_callback.go @@ -0,0 +1,89 @@ +// 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. + +//go:build !plan9 && !windows + +package main + +// Make many C-to-Go callback while collecting a CPU profile. +// +// This is a regression test for issue 50936. + +/* +#include + +void goCallbackPprof(); + +static void callGo() { + // Spent >20us in C so this thread is eligible for sysmon to retake its + // P. + usleep(50); + goCallbackPprof(); +} +*/ +import "C" + +import ( + "fmt" + "os" + "runtime/pprof" + "runtime" + "time" +) + +func init() { + register("CgoPprofCallback", CgoPprofCallback) +} + +//export goCallbackPprof +func goCallbackPprof() { + // No-op. We want to stress the cgocall and cgocallback internals, + // landing as many pprof signals there as possible. +} + +func CgoPprofCallback() { + // Issue 50936 was a crash in the SIGPROF handler when the signal + // arrived during the exitsyscall following a cgocall(back) in dropg or + // execute, when updating mp.curg. + // + // These are reachable only when exitsyscall finds no P available. Thus + // we make C calls from significantly more Gs than there are available + // Ps. Lots of runnable work combined with >20us spent in callGo makes + // it possible for sysmon to retake Ps, forcing C calls to go down the + // desired exitsyscall path. + // + // High GOMAXPROCS is used to increase opportunities for failure on + // high CPU machines. + const ( + P = 16 + G = 64 + ) + runtime.GOMAXPROCS(P) + + f, err := os.CreateTemp("", "prof") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + defer f.Close() + + if err := pprof.StartCPUProfile(f); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + + for i := 0; i < G; i++ { + go func() { + for { + C.callGo() + } + }() + } + + time.Sleep(time.Second) + + pprof.StopCPUProfile() + + fmt.Println("OK") +} From 0003d9da093ce1cb19aebb074da4506fade35a66 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Fri, 19 Nov 2021 14:32:49 -0800 Subject: [PATCH 10/42] doc/go1.18: add crypto/x509 verifier notes Change-Id: If3e835e868ae695ba232b57096c135ce2e73305b Reviewed-on: https://go-review.googlesource.com/c/go/+/365835 Trust: Roland Shoemaker Trust: Filippo Valsorda Reviewed-by: Katie Hockman --- doc/go1.18.html | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/doc/go1.18.html b/doc/go1.18.html index cb3c2dbac3..4d77f14d53 100644 --- a/doc/go1.18.html +++ b/doc/go1.18.html @@ -662,6 +662,46 @@ Do not send CLs removing the interior tags from such phrases. +

crypto/x509
+
+

+ Certificate.Verify + now uses platform APIs to verify certificate validity on macOS and iOS when it + is called with a nil + VerifyOpts.Roots + or when using the root pool returned from + SystemCertPool. +

+ +

+ SystemCertPool + is now available on Windows. +

+ +

+ On Windows, macOS, and iOS, when a + CertPool returned by + SystemCertPool + has additional certificates added to it, + Certificate.Verify + will do two verifications: one using the platform verifier APIs and the + system roots, and one using the Go verifier and the additional roots. + Chains returned by the platform verifier APIs will be prioritized. +

+ +

+ CertPool.Subjects + is deprecated. On Windows, macOS, and iOS the + CertPool returned by + SystemCertPool + will return a pool which does not include system roots in the slice + returned by Subjects, as a static list can't appropriately + represent the platform policies and might not be available at all from the + platform APIs. +

+
+
+
debug/dwarf

From 7f9494c277a471f6f47f4af3036285c0b1419816 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Wed, 2 Feb 2022 09:13:17 -0800 Subject: [PATCH 11/42] crypto/elliptic: make IsOnCurve return false for invalid field elements Thanks to Guido Vranken for reporting this issue. Fixes #50974 Fixes CVE-2022-23806 Change-Id: I0201c2c88f13dd82910985a495973f1683af9259 Reviewed-on: https://go-review.googlesource.com/c/go/+/382455 Run-TryBot: Filippo Valsorda TryBot-Result: Gopher Robot Trust: Filippo Valsorda Reviewed-by: Roland Shoemaker Reviewed-by: Katie Hockman Trust: Katie Hockman --- src/crypto/elliptic/elliptic.go | 5 +++ src/crypto/elliptic/elliptic_test.go | 55 ++++++++++++++++++++++++++++ src/crypto/elliptic/p224.go | 3 ++ src/crypto/elliptic/p384.go | 3 ++ src/crypto/elliptic/p521.go | 3 ++ 5 files changed, 69 insertions(+) diff --git a/src/crypto/elliptic/elliptic.go b/src/crypto/elliptic/elliptic.go index c5c5a906c4..7ead09f8d3 100644 --- a/src/crypto/elliptic/elliptic.go +++ b/src/crypto/elliptic/elliptic.go @@ -89,6 +89,11 @@ func (curve *CurveParams) IsOnCurve(x, y *big.Int) bool { return specific.IsOnCurve(x, y) } + if x.Sign() < 0 || x.Cmp(curve.P) >= 0 || + y.Sign() < 0 || y.Cmp(curve.P) >= 0 { + return false + } + // y² = x³ - 3x + b y2 := new(big.Int).Mul(y, y) y2.Mod(y2, curve.P) diff --git a/src/crypto/elliptic/elliptic_test.go b/src/crypto/elliptic/elliptic_test.go index f5b36f75ca..5481929db1 100644 --- a/src/crypto/elliptic/elliptic_test.go +++ b/src/crypto/elliptic/elliptic_test.go @@ -182,6 +182,61 @@ func testUnmarshalToLargeCoordinates(t *testing.T, curve Curve) { } } +// TestInvalidCoordinates tests big.Int values that are not valid field elements +// (negative or bigger than P). They are expected to return false from +// IsOnCurve, all other behavior is undefined. +func TestInvalidCoordinates(t *testing.T) { + testAllCurves(t, testInvalidCoordinates) +} + +func testInvalidCoordinates(t *testing.T, curve Curve) { + checkIsOnCurveFalse := func(name string, x, y *big.Int) { + if curve.IsOnCurve(x, y) { + t.Errorf("IsOnCurve(%s) unexpectedly returned true", name) + } + } + + p := curve.Params().P + _, x, y, _ := GenerateKey(curve, rand.Reader) + xx, yy := new(big.Int), new(big.Int) + + // Check if the sign is getting dropped. + xx.Neg(x) + checkIsOnCurveFalse("-x, y", xx, y) + yy.Neg(y) + checkIsOnCurveFalse("x, -y", x, yy) + + // Check if negative values are reduced modulo P. + xx.Sub(x, p) + checkIsOnCurveFalse("x-P, y", xx, y) + yy.Sub(y, p) + checkIsOnCurveFalse("x, y-P", x, yy) + + // Check if positive values are reduced modulo P. + xx.Add(x, p) + checkIsOnCurveFalse("x+P, y", xx, y) + yy.Add(y, p) + checkIsOnCurveFalse("x, y+P", x, yy) + + // Check if the overflow is dropped. + xx.Add(x, new(big.Int).Lsh(big.NewInt(1), 535)) + checkIsOnCurveFalse("x+2⁵³⁵, y", xx, y) + yy.Add(y, new(big.Int).Lsh(big.NewInt(1), 535)) + checkIsOnCurveFalse("x, y+2⁵³⁵", x, yy) + + // Check if P is treated like zero (if possible). + // y^2 = x^3 - 3x + B + // y = mod_sqrt(x^3 - 3x + B) + // y = mod_sqrt(B) if x = 0 + // If there is no modsqrt, there is no point with x = 0, can't test x = P. + if yy := new(big.Int).ModSqrt(curve.Params().B, p); yy != nil { + if !curve.IsOnCurve(big.NewInt(0), yy) { + t.Fatal("(0, mod_sqrt(B)) is not on the curve?") + } + checkIsOnCurveFalse("P, y", p, yy) + } +} + func TestMarshalCompressed(t *testing.T) { t.Run("P-256/03", func(t *testing.T) { data, _ := hex.DecodeString("031e3987d9f9ea9d7dd7155a56a86b2009e1e0ab332f962d10d8beb6406ab1ad79") diff --git a/src/crypto/elliptic/p224.go b/src/crypto/elliptic/p224.go index a8533b85ff..8a431c4769 100644 --- a/src/crypto/elliptic/p224.go +++ b/src/crypto/elliptic/p224.go @@ -61,6 +61,9 @@ func p224PointFromAffine(x, y *big.Int) (p *nistec.P224Point, ok bool) { if x.Sign() == 0 && y.Sign() == 0 { return nistec.NewP224Point(), true } + if x.Sign() < 0 || y.Sign() < 0 { + return nil, false + } if x.BitLen() > 224 || y.BitLen() > 224 { return nil, false } diff --git a/src/crypto/elliptic/p384.go b/src/crypto/elliptic/p384.go index 0fb7471850..33a441d090 100644 --- a/src/crypto/elliptic/p384.go +++ b/src/crypto/elliptic/p384.go @@ -66,6 +66,9 @@ func p384PointFromAffine(x, y *big.Int) (p *nistec.P384Point, ok bool) { if x.Sign() == 0 && y.Sign() == 0 { return nistec.NewP384Point(), true } + if x.Sign() < 0 || y.Sign() < 0 { + return nil, false + } if x.BitLen() > 384 || y.BitLen() > 384 { return nil, false } diff --git a/src/crypto/elliptic/p521.go b/src/crypto/elliptic/p521.go index 6c9eed30e5..6a3ade3c36 100644 --- a/src/crypto/elliptic/p521.go +++ b/src/crypto/elliptic/p521.go @@ -71,6 +71,9 @@ func p521PointFromAffine(x, y *big.Int) (p *nistec.P521Point, ok bool) { if x.Sign() == 0 && y.Sign() == 0 { return nistec.NewP521Point(), true } + if x.Sign() < 0 || y.Sign() < 0 { + return nil, false + } if x.BitLen() > 521 || y.BitLen() > 521 { return nil, false } From 1ab827371858e02f864f91e7dc561ae48eb7bbd0 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Thu, 3 Feb 2022 09:44:21 -0800 Subject: [PATCH 12/42] cmd/compile: ensure size is computed for shape types Fixes #50993 Change-Id: I5f1bf5a8375c3da3203083b11de26962523ccb36 Reviewed-on: https://go-review.googlesource.com/c/go/+/382874 Trust: Keith Randall Run-TryBot: Keith Randall TryBot-Result: Gopher Robot Reviewed-by: Dan Scales Trust: Dan Scales --- src/cmd/compile/internal/typecheck/subr.go | 1 + test/typeparam/issue50993.go | 35 ++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 test/typeparam/issue50993.go diff --git a/src/cmd/compile/internal/typecheck/subr.go b/src/cmd/compile/internal/typecheck/subr.go index 93812ebda5..9892471142 100644 --- a/src/cmd/compile/internal/typecheck/subr.go +++ b/src/cmd/compile/internal/typecheck/subr.go @@ -1480,6 +1480,7 @@ func Shapify(t *types.Type, index int, tparam *types.Type) *types.Type { s.SetUnderlying(u) s.SetIsShape(true) s.SetHasShape(true) + types.CalcSize(s) name.SetType(s) name.SetTypecheck(1) submap[u] = s diff --git a/test/typeparam/issue50993.go b/test/typeparam/issue50993.go new file mode 100644 index 0000000000..4d459fd04c --- /dev/null +++ b/test/typeparam/issue50993.go @@ -0,0 +1,35 @@ +// compile -d=checkptr + +// 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 + +import ( + "sync/atomic" + "unsafe" +) + +type Node[T any] struct { + Next *Node[T] + // Prev *Node[T] +} + +func LoadPointer[T any](addr **T) (val *T) { + return (*T)( + atomic.LoadPointer( + (*unsafe.Pointer)(unsafe.Pointer(addr)), + )) +} + +func (q *Node[T]) Pop() { + var tail, head *Node[T] + if head == LoadPointer(&tail) { + } +} + +func main() { + ch := Node[uint64]{} + ch.Pop() +} From 896df422a7cecbace10f5877beeeb1476b6061ae Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Fri, 3 Sep 2021 09:56:23 -0700 Subject: [PATCH 13/42] crypto/x509: support NumericString in DN components Fixes #48171 Change-Id: Ia2e1920c0938a1f8659935a4f725a7e5090ef2c0 Reviewed-on: https://go-review.googlesource.com/c/go/+/347034 Trust: Roland Shoemaker Run-TryBot: Roland Shoemaker TryBot-Result: Gopher Robot Reviewed-by: Filippo Valsorda --- src/crypto/x509/parser.go | 13 ++++- src/crypto/x509/parser_test.go | 102 +++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 src/crypto/x509/parser_test.go diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go index 5e6bd54368..a32a973c68 100644 --- a/src/crypto/x509/parser.go +++ b/src/crypto/x509/parser.go @@ -51,9 +51,9 @@ func isPrintable(b byte) bool { } // parseASN1String parses the ASN.1 string types T61String, PrintableString, -// UTF8String, BMPString, and IA5String. This is mostly copied from the -// respective encoding/asn1.parse... methods, rather than just increasing -// the API surface of that package. +// UTF8String, BMPString, IA5String, and NumericString. This is mostly copied +// from the respective encoding/asn1.parse... methods, rather than just +// increasing the API surface of that package. func parseASN1String(tag cryptobyte_asn1.Tag, value []byte) (string, error) { switch tag { case cryptobyte_asn1.T61String: @@ -93,6 +93,13 @@ func parseASN1String(tag cryptobyte_asn1.Tag, value []byte) (string, error) { return "", errors.New("invalid IA5String") } return s, nil + case cryptobyte_asn1.Tag(asn1.TagNumericString): + for _, b := range value { + if !('0' <= b && b <= '9' || b == ' ') { + return "", errors.New("invalid NumericString") + } + } + return string(value), nil } return "", fmt.Errorf("unsupported string type: %v", tag) } diff --git a/src/crypto/x509/parser_test.go b/src/crypto/x509/parser_test.go new file mode 100644 index 0000000000..d7cf7ea758 --- /dev/null +++ b/src/crypto/x509/parser_test.go @@ -0,0 +1,102 @@ +// Copyright 2021 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 x509 + +import ( + "encoding/asn1" + "testing" + + cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1" +) + +func TestParseASN1String(t *testing.T) { + tests := []struct { + name string + tag cryptobyte_asn1.Tag + value []byte + expected string + expectedErr string + }{ + { + name: "T61String", + tag: cryptobyte_asn1.T61String, + value: []byte{80, 81, 82}, + expected: string("PQR"), + }, + { + name: "PrintableString", + tag: cryptobyte_asn1.PrintableString, + value: []byte{80, 81, 82}, + expected: string("PQR"), + }, + { + name: "PrintableString (invalid)", + tag: cryptobyte_asn1.PrintableString, + value: []byte{1, 2, 3}, + expectedErr: "invalid PrintableString", + }, + { + name: "UTF8String", + tag: cryptobyte_asn1.UTF8String, + value: []byte{80, 81, 82}, + expected: string("PQR"), + }, + { + name: "UTF8String (invalid)", + tag: cryptobyte_asn1.UTF8String, + value: []byte{255}, + expectedErr: "invalid UTF-8 string", + }, + { + name: "BMPString", + tag: cryptobyte_asn1.Tag(asn1.TagBMPString), + value: []byte{80, 81}, + expected: string("偑"), + }, + { + name: "BMPString (invalid length)", + tag: cryptobyte_asn1.Tag(asn1.TagBMPString), + value: []byte{255}, + expectedErr: "invalid BMPString", + }, + { + name: "IA5String", + tag: cryptobyte_asn1.IA5String, + value: []byte{80, 81}, + expected: string("PQ"), + }, + { + name: "IA5String (invalid)", + tag: cryptobyte_asn1.IA5String, + value: []byte{255}, + expectedErr: "invalid IA5String", + }, + { + name: "NumericString", + tag: cryptobyte_asn1.Tag(asn1.TagNumericString), + value: []byte{49, 50}, + expected: string("12"), + }, + { + name: "NumericString (invalid)", + tag: cryptobyte_asn1.Tag(asn1.TagNumericString), + value: []byte{80}, + expectedErr: "invalid NumericString", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + out, err := parseASN1String(tc.tag, tc.value) + if err != nil && err.Error() != tc.expectedErr { + t.Fatalf("parseASN1String returned unexpected error: got %q, want %q", err, tc.expectedErr) + } else if err == nil && tc.expectedErr != "" { + t.Fatalf("parseASN1String didn't fail, expected: %s", tc.expectedErr) + } + if out != tc.expected { + t.Fatalf("parseASN1String returned unexpected value: got %q, want %q", out, tc.expected) + } + }) + } +} From 248ad855b7d0e49839b7b4281d9e60e222368583 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Thu, 3 Feb 2022 12:43:48 -0800 Subject: [PATCH 14/42] misc/cgo/test: disable unhelpful GCC warning GCC 9 warns about a change in the ABI of passing structs with bitfields, but we don't care. Fixes #50987 Change-Id: Ica658d04172a42a7be788f94d31a714bb8c4766f Reviewed-on: https://go-review.googlesource.com/c/go/+/382956 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Benny Siegert Trust: Benny Siegert --- misc/cgo/test/test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/misc/cgo/test/test.go b/misc/cgo/test/test.go index dd81f770a2..109ef987f9 100644 --- a/misc/cgo/test/test.go +++ b/misc/cgo/test/test.go @@ -367,6 +367,11 @@ void init() { // Cgo incorrectly computed the alignment of structs // with no Go accessible fields as 0, and then panicked on // modulo-by-zero computations. + +// issue 50987 +// disable arm64 GCC warnings +#cgo CFLAGS: -Wno-psabi -Wno-unknown-warning-option + typedef struct { } foo; From 4e2410617d7b13e63e80ad77c9b2d44abaf39e9a Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Thu, 3 Feb 2022 15:17:00 -0800 Subject: [PATCH 15/42] cmd/compile: restrict generics test to -G=3 Change-Id: Ifdb4f4f4fab8d45847ca525198b3960f87799f0c Reviewed-on: https://go-review.googlesource.com/c/go/+/383034 Trust: Keith Randall Run-TryBot: Keith Randall TryBot-Result: Gopher Robot Reviewed-by: Dan Scales Trust: Dan Scales --- test/typeparam/issue50993.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/typeparam/issue50993.go b/test/typeparam/issue50993.go index 4d459fd04c..39bdba0772 100644 --- a/test/typeparam/issue50993.go +++ b/test/typeparam/issue50993.go @@ -1,4 +1,4 @@ -// compile -d=checkptr +// compile -G=3 -d=checkptr // Copyright 2022 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style From 4afcc9f35e97b4e96f2f350f2a00ea65f43f4175 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 2 Feb 2022 18:40:27 -0500 Subject: [PATCH 16/42] go/parser, go/types: don't parse type parameters on methods The go/parser package is updated to report an error on method type parameters, and to not store them in the AST. Tests are updated accordingly, and error messages are normalized accross go/parser and the compiler syntax package. Before this CL, go/parser would parse type parameters on method declarations and interface methods, leaving it to go/types to complain. There are several problems with this: - Interface Methods and Method declarations do not have type parameters in the spec. We try to align the parser with the productions in the spec. - Parsing method type parameters means that downstream libraries (go/doc, go/format, etc.) theoretically need to handle them, even though they are not part of the language. - Relatedly, go/types has inconsistent handling of method type parameters due to support being removed, leading to the crasher in #50427. It is more consistent and safer to disallow type parameters on methods in the parser. Fixes #50427 Change-Id: I555766da0c76c4cf1cfe0baa9416863088088b4e Reviewed-on: https://go-review.googlesource.com/c/go/+/382538 Trust: Robert Findley Run-TryBot: Robert Findley Reviewed-by: Robert Griesemer TryBot-Result: Gopher Robot --- src/go/parser/parser.go | 25 +++++++++---- src/go/parser/short_test.go | 26 ++++++++----- src/go/parser/testdata/issue50427.go2 | 19 ++++++++++ src/go/types/errorcodes.go | 5 +-- src/go/types/testdata/check/typeparams.go2 | 37 ++++++++++--------- .../types/testdata/fixedbugs/issue39634.go2 | 2 +- .../types/testdata/fixedbugs/issue50427.go2 | 23 ++++++++++++ 7 files changed, 97 insertions(+), 40 deletions(-) create mode 100644 src/go/parser/testdata/issue50427.go2 create mode 100644 src/go/types/testdata/fixedbugs/issue50427.go2 diff --git a/src/go/parser/parser.go b/src/go/parser/parser.go index e456e2930e..4479adb732 100644 --- a/src/go/parser/parser.go +++ b/src/go/parser/parser.go @@ -977,7 +977,7 @@ func (p *parser) parseFuncType() *ast.FuncType { pos := p.expect(token.FUNC) tparams, params := p.parseParameters(true) if tparams != nil { - p.error(tparams.Pos(), "function type cannot have type parameters") + p.error(tparams.Pos(), "function type must have no type parameters") } results := p.parseResult() @@ -1004,18 +1004,21 @@ func (p *parser) parseMethodSpec() *ast.Field { p.exprLev-- if name0, _ := x.(*ast.Ident); name0 != nil && p.tok != token.COMMA && p.tok != token.RBRACK { // generic method m[T any] - list := p.parseParameterList(name0, token.RBRACK) - rbrack := p.expect(token.RBRACK) - tparams := &ast.FieldList{Opening: lbrack, List: list, Closing: rbrack} + // + // Interface methods do not have type parameters. We parse them for a + // better error message and improved error recovery. + _ = p.parseParameterList(name0, token.RBRACK) + _ = p.expect(token.RBRACK) + p.error(lbrack, "interface method must have no type parameters") + // TODO(rfindley) refactor to share code with parseFuncType. _, params := p.parseParameters(false) results := p.parseResult() idents = []*ast.Ident{ident} typ = &ast.FuncType{ - Func: token.NoPos, - TypeParams: tparams, - Params: params, - Results: results, + Func: token.NoPos, + Params: params, + Results: results, } } else { // embedded instantiated type @@ -2655,6 +2658,12 @@ func (p *parser) parseFuncDecl() *ast.FuncDecl { ident := p.parseIdent() tparams, params := p.parseParameters(true) + if recv != nil && tparams != nil { + // Method declarations do not have type parameters. We parse them for a + // better error message and improved error recovery. + p.error(tparams.Opening, "method must have no type parameters") + tparams = nil + } results := p.parseResult() var body *ast.BlockStmt diff --git a/src/go/parser/short_test.go b/src/go/parser/short_test.go index 90a4ec9ecd..6ea430636e 100644 --- a/src/go/parser/short_test.go +++ b/src/go/parser/short_test.go @@ -94,9 +94,7 @@ var validWithTParamsOnly = []string{ `package p; func _[ /* ERROR "expected '\(', found '\['" */ A, B any](a A) B`, `package p; func _[ /* ERROR "expected '\(', found '\['" */ A, B C](a A) B`, `package p; func _[ /* ERROR "expected '\(', found '\['" */ A, B C[A, B]](a A) B`, - `package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B any](a A) B`, - `package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B C](a A) B`, - `package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B C[A, B]](a A) B`, + `package p; type _[A, /* ERROR "expected ']', found ','" */ B any] interface { _(a A) B }`, `package p; type _[A, /* ERROR "expected ']', found ','" */ B C[A, B]] interface { _(a A) B }`, `package p; func _[ /* ERROR "expected '\(', found '\['" */ T1, T2 interface{}](x T1) T2`, @@ -110,10 +108,10 @@ var validWithTParamsOnly = []string{ `package p; var _ T[ /* ERROR "expected ';', found '\['" */ chan int]`, // TODO(rfindley) this error message could be improved. - `package p; func (_ /* ERROR "mixed named and unnamed parameters" */ R[P]) _[T any](x T)`, - `package p; func (_ /* ERROR "mixed named and unnamed parameters" */ R[ P, Q]) _[T1, T2 any](x T)`, + `package p; func (_ /* ERROR "mixed named and unnamed parameters" */ R[P]) _(x T)`, + `package p; func (_ /* ERROR "mixed named and unnamed parameters" */ R[ P, Q]) _(x T)`, - `package p; func (R[P] /* ERROR "missing element type" */ ) _[T any]()`, + `package p; func (R[P] /* ERROR "missing element type" */ ) _()`, `package p; func _(T[P] /* ERROR "missing element type" */ )`, `package p; func _(T[P1, /* ERROR "expected ']', found ','" */ P2, P3 ])`, `package p; func _(T[P] /* ERROR "missing element type" */ ) T[P]`, @@ -122,7 +120,7 @@ var validWithTParamsOnly = []string{ `package p; type _ interface{int| /* ERROR "expected ';'" */ float32; bool; m(); string;}`, `package p; type I1[T any /* ERROR "expected ']', found any" */ ] interface{}; type I2 interface{ I1[int] }`, `package p; type I1[T any /* ERROR "expected ']', found any" */ ] interface{}; type I2[T any] interface{ I1[T] }`, - `package p; type _ interface { f[ /* ERROR "expected ';', found '\['" */ T any]() }`, + `package p; type _ interface { N[ /* ERROR "expected ';', found '\['" */ T] }`, `package p; type T[P any /* ERROR "expected ']'" */ ] = T0`, } @@ -235,20 +233,28 @@ var invalidNoTParamErrs = []string{ `package p; func _[ /* ERROR "expected '\(', found '\['" */ ]()`, `package p; type _[A, /* ERROR "expected ']', found ','" */] struct{ A }`, `package p; func _[ /* ERROR "expected '\(', found '\['" */ type P, *Q interface{}]()`, + + `package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B any](a A) B`, + `package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B C](a A) B`, + `package p; func (T) _[ /* ERROR "expected '\(', found '\['" */ A, B C[A, B]](a A) B`, } // invalidTParamErrs holds invalid source code examples annotated with the // error messages produced when ParseTypeParams is set. var invalidTParamErrs = []string{ `package p; type _[_ any] int; var _ = T[] /* ERROR "expected operand" */ {}`, - `package p; var _ func[ /* ERROR "cannot have type parameters" */ T any](T)`, + `package p; var _ func[ /* ERROR "must have no type parameters" */ T any](T)`, `package p; func _[]/* ERROR "empty type parameter list" */()`, // TODO(rfindley) a better location would be after the ']' - `package p; type _[A/* ERROR "all type parameters must be named" */,] struct{ A }`, + `package p; type _[A /* ERROR "all type parameters must be named" */ ,] struct{ A }`, // TODO(rfindley) this error is confusing. - `package p; func _[type /* ERROR "all type parameters must be named" */P, *Q interface{}]()`, + `package p; func _[type /* ERROR "all type parameters must be named" */ P, *Q interface{}]()`, + + `package p; func (T) _[ /* ERROR "must have no type parameters" */ A, B any](a A) B`, + `package p; func (T) _[ /* ERROR "must have no type parameters" */ A, B C](a A) B`, + `package p; func (T) _[ /* ERROR "must have no type parameters" */ A, B C[A, B]](a A) B`, } func TestInvalid(t *testing.T) { diff --git a/src/go/parser/testdata/issue50427.go2 b/src/go/parser/testdata/issue50427.go2 new file mode 100644 index 0000000000..15214594e2 --- /dev/null +++ b/src/go/parser/testdata/issue50427.go2 @@ -0,0 +1,19 @@ +// 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 + +type T interface{ m[ /* ERROR "must have no type parameters" */ P any]() } + +func _(t T) { + var _ interface{ m[ /* ERROR "must have no type parameters" */ P any](); n() } = t +} + +type S struct{} + +func (S) m[ /* ERROR "must have no type parameters" */ P any]() {} + +func _(s S) { + var _ interface{ m[ /* ERROR "must have no type parameters" */ P any](); n() } = s +} diff --git a/src/go/types/errorcodes.go b/src/go/types/errorcodes.go index b3796e8919..51f091a9cb 100644 --- a/src/go/types/errorcodes.go +++ b/src/go/types/errorcodes.go @@ -1385,10 +1385,7 @@ const ( // _InvalidMethodTypeParams occurs when methods have type parameters. // - // Example: - // type T int - // - // func (T) m[P any]() {} + // It cannot be encountered with an AST parsed using go/parser. _InvalidMethodTypeParams // _MisplacedTypeParam occurs when a type parameter is used in a place where diff --git a/src/go/types/testdata/check/typeparams.go2 b/src/go/types/testdata/check/typeparams.go2 index 6d63d598d9..d5b9ed6e77 100644 --- a/src/go/types/testdata/check/typeparams.go2 +++ b/src/go/types/testdata/check/typeparams.go2 @@ -329,8 +329,8 @@ func init[P /* ERROR func init must have no type parameters */ any]() {} type T struct {} func (T) m1() {} -func (T) m2[ /* ERROR methods cannot have type parameters */ _ any]() {} -func (T) m3[ /* ERROR methods cannot have type parameters */ P any]() {} +func (T) m2[ /* ERROR method must have no type parameters */ _ any]() {} +func (T) m3[ /* ERROR method must have no type parameters */ P any]() {} // type inference across parameterized types @@ -391,25 +391,28 @@ func _[T any] (x T) { // type parameters in methods (generalization) -type R0 struct{} +// Type Parameter lists are not allowed on methods, and are not produced by +// go/parser. The test cases below are preserved for consistency with types2, +// which produces an error but stores type parameters. +// type R0 struct{} -func (R0) _[ /* ERROR methods cannot have type parameters */ T any](x T) {} -func (R0 /* ERROR invalid receiver */ ) _[ /* ERROR methods cannot have type parameters */ R0 any]() {} // scope of type parameters starts at "func" +// func (R0) _[ /* ERROR methods cannot have type parameters */ T any](x T) {} +// func (R0 /* ERROR invalid receiver */ ) _[ /* ERROR methods cannot have type parameters */ R0 any]() {} // scope of type parameters starts at "func" -type R1[A, B any] struct{} +// type R1[A, B any] struct{} -func (_ R1[A, B]) m0(A, B) -func (_ R1[A, B]) m1[ /* ERROR methods cannot have type parameters */ T any](A, B, T) T { panic(0) } -func (_ R1 /* ERROR not a generic type */ [R1, _]) _() -func (_ R1[A, B]) _[ /* ERROR methods cannot have type parameters */ A /* ERROR redeclared */ any](B) {} +// func (_ R1[A, B]) m0(A, B) +// func (_ R1[A, B]) m1[ /* ERROR methods cannot have type parameters */ T any](A, B, T) T { panic(0) } +// func (_ R1 /* ERROR not a generic type */ [R1, _]) _() +// func (_ R1[A, B]) _[ /* ERROR methods cannot have type parameters */ A /* ERROR redeclared */ any](B) {} -func _() { - var r R1[int, string] - r.m1[rune](42, "foo", 'a') - r.m1[rune](42, "foo", 1.2 /* ERROR cannot use .* as rune .* \(truncated\) */) - r.m1(42, "foo", 1.2) // using type inference - var _ float64 = r.m1(42, "foo", 1.2) -} +// func _() { +// var r R1[int, string] +// r.m1[rune](42, "foo", 'a') +// r.m1[rune](42, "foo", 1.2 /* ERROR cannot use .* as rune .* \(truncated\) */) +// r.m1(42, "foo", 1.2) // using type inference +// var _ float64 = r.m1(42, "foo", 1.2) +// } type I1[A any] interface { m1(A) diff --git a/src/go/types/testdata/fixedbugs/issue39634.go2 b/src/go/types/testdata/fixedbugs/issue39634.go2 index 34ab654f1c..8cba2e735a 100644 --- a/src/go/types/testdata/fixedbugs/issue39634.go2 +++ b/src/go/types/testdata/fixedbugs/issue39634.go2 @@ -85,7 +85,7 @@ func (t T25[A]) m1() {} var x T25 /* ERROR without instantiation */ .m1 // crash 26 -type T26 = interface{ F26[ /* ERROR methods cannot have type parameters */ Z any]() } +type T26 = interface{ F26[ /* ERROR interface method must have no type parameters */ Z any]() } // The error messages on the line below differ from types2 because for backward // compatibility go/parser must produce an IndexExpr with BadExpr index for the // expression F26[]. diff --git a/src/go/types/testdata/fixedbugs/issue50427.go2 b/src/go/types/testdata/fixedbugs/issue50427.go2 new file mode 100644 index 0000000000..d89d63e308 --- /dev/null +++ b/src/go/types/testdata/fixedbugs/issue50427.go2 @@ -0,0 +1,23 @@ +// 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 + +// The parser no longer parses type parameters for methods. +// In the past, type checking the code below led to a crash (#50427). + +type T interface{ m[ /* ERROR "must have no type parameters" */ P any]() } + +func _(t T) { + var _ interface{ m[ /* ERROR "must have no type parameters" */ P any](); n() } = t /* ERROR "does not implement" */ +} + +type S struct{} + +func (S) m[ /* ERROR "must have no type parameters" */ P any]() {} + +func _(s S) { + var _ interface{ m[ /* ERROR "must have no type parameters" */ P any](); n() } = s /* ERROR "does not implement" */ + +} From 6e56fcedfb6338e0a75dadef190a1c342e837cf4 Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Fri, 4 Feb 2022 11:27:17 -0800 Subject: [PATCH 17/42] internal/nettrace: fix spelling error Change-Id: Ibf51c0687197c0d791916b21cba7f8408aa5300a Reviewed-on: https://go-review.googlesource.com/c/go/+/383216 Reviewed-by: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Trust: Kevin Burke --- src/internal/nettrace/nettrace.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/nettrace/nettrace.go b/src/internal/nettrace/nettrace.go index 94f38a71ee..6e0dbe73bb 100644 --- a/src/internal/nettrace/nettrace.go +++ b/src/internal/nettrace/nettrace.go @@ -27,7 +27,7 @@ type Trace struct { DNSStart func(name string) // DNSDone is called after a DNS lookup completes (or fails). - // The coalesced parameter is whether singleflight de-dupped + // The coalesced parameter is whether singleflight de-duped // the call. The addrs are of type net.IPAddr but can't // actually be for circular dependency reasons. DNSDone func(netIPs []any, coalesced bool, err error) From 02224c8ce471b9618081c69d952fb8597f71b7e6 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Fri, 4 Feb 2022 12:22:19 -0800 Subject: [PATCH 18/42] cmd/go: accept "-F dir" in compiler flags Fixes #51008 Change-Id: I5e47c7be59d4aae1d5059d99231422212cffa23a Reviewed-on: https://go-review.googlesource.com/c/go/+/383217 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills --- src/cmd/go/internal/work/security.go | 1 + src/cmd/go/internal/work/security_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/src/cmd/go/internal/work/security.go b/src/cmd/go/internal/work/security.go index e9b9f6c6c0..d1e2c673fa 100644 --- a/src/cmd/go/internal/work/security.go +++ b/src/cmd/go/internal/work/security.go @@ -131,6 +131,7 @@ var validCompilerFlagsWithNextArg = []string{ "-D", "-U", "-I", + "-F", "-framework", "-include", "-isysroot", diff --git a/src/cmd/go/internal/work/security_test.go b/src/cmd/go/internal/work/security_test.go index 8d4be0abfc..d2aeb54e0c 100644 --- a/src/cmd/go/internal/work/security_test.go +++ b/src/cmd/go/internal/work/security_test.go @@ -15,6 +15,7 @@ var goodCompilerFlags = [][]string{ {"-Ufoo"}, {"-Ufoo1"}, {"-F/Qt"}, + {"-F", "/Qt"}, {"-I/"}, {"-I/etc/passwd"}, {"-I."}, From 2aafd9ea179ec308607c131dbda438abac2b9184 Mon Sep 17 00:00:00 2001 From: Brandon Bennett Date: Fri, 4 Feb 2022 12:45:18 -0700 Subject: [PATCH 19/42] cmd/go: preserve LIBRARY_PATH and C_INCLUDE_PATH for script tests In bespoke build environments default libraries may be specificied with LIBRARY_PATH, C_INCLUDE_PATH enviroment variables to overide the system (i.e glibc). Allow them though to allow cgo testing of these enviroments. Fixes #50985 Change-Id: I7497a7715d9b635a6ae97efaab94a7ff01cdf8e2 Reviewed-on: https://go-review.googlesource.com/c/go/+/383334 Reviewed-by: Bryan Mills Run-TryBot: Bryan Mills Trust: Ian Lance Taylor TryBot-Result: Gopher Robot --- src/cmd/go/script_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cmd/go/script_test.go b/src/cmd/go/script_test.go index 722921f74c..55a88e0e0b 100644 --- a/src/cmd/go/script_test.go +++ b/src/cmd/go/script_test.go @@ -142,6 +142,8 @@ var extraEnvKeys = []string{ "SYSTEMROOT", // must be preserved on Windows to find DLLs; golang.org/issue/25210 "WINDIR", // must be preserved on Windows to be able to run PowerShell command; golang.org/issue/30711 "LD_LIBRARY_PATH", // must be preserved on Unix systems to find shared libraries + "LIBRARY_PATH", // allow override of non-standard static library paths + "C_INCLUDE_PATH", // allow override non-standard include paths "CC", // don't lose user settings when invoking cgo "GO_TESTING_GOTOOLS", // for gccgo testing "GCCGO", // for gccgo testing From 56a539724809122ecee26acddc3b8a7b775afed6 Mon Sep 17 00:00:00 2001 From: Katie Hockman Date: Fri, 4 Feb 2022 17:11:18 -0500 Subject: [PATCH 20/42] testing: remove TODO and link to fuzz docs Change-Id: I3f5ee9629b0b0f3f29a021a656dbf3bca27e582d Reviewed-on: https://go-review.googlesource.com/c/go/+/383415 Trust: Katie Hockman Run-TryBot: Katie Hockman Reviewed-by: Bryan Mills TryBot-Result: Gopher Robot --- src/testing/testing.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/testing/testing.go b/src/testing/testing.go index a8c8122aa7..df4dfe4490 100644 --- a/src/testing/testing.go +++ b/src/testing/testing.go @@ -202,8 +202,7 @@ // mode, the fuzz test acts much like a regular test, with subtests started // with F.Fuzz instead of T.Run. // -// TODO(#48255): write and link to documentation that will be helpful to users -// who are unfamiliar with fuzzing. +// See https://go.dev/doc/fuzz for documentation about fuzzing. // // Skipping // From f9763a648bbe4468118b95e147bc5e81268d0341 Mon Sep 17 00:00:00 2001 From: Katie Hockman Date: Fri, 4 Feb 2022 17:02:49 -0500 Subject: [PATCH 21/42] internal/fuzz: change meaning of "total" in output Fixes #51023 Change-Id: I7dd9f7c696e15ba9c6c887d4c4e0f4d281a82b17 Reviewed-on: https://go-review.googlesource.com/c/go/+/383414 Trust: Katie Hockman Reviewed-by: Roland Shoemaker Reviewed-by: Bryan Mills Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot --- src/internal/fuzz/fuzz.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/internal/fuzz/fuzz.go b/src/internal/fuzz/fuzz.go index 0014cde04f..3ccf74745f 100644 --- a/src/internal/fuzz/fuzz.go +++ b/src/internal/fuzz/fuzz.go @@ -597,7 +597,7 @@ type coordinator struct { // interestingCount is the number of unique interesting values which have // been found this execution. - interestingCount int64 + interestingCount int // warmupInputCount is the count of all entries in the corpus which will // need to be received from workers to run once during warmup, but not fuzz. @@ -731,8 +731,8 @@ func (c *coordinator) logStats() { } else { rate := float64(c.count-c.countLastLog) / now.Sub(c.timeLastLog).Seconds() if coverageEnabled { - interestingTotalCount := int64(c.warmupInputCount-len(c.opts.Seed)) + c.interestingCount - fmt.Fprintf(c.opts.Log, "fuzz: elapsed: %s, execs: %d (%.0f/sec), new interesting: %d (total: %d)\n", c.elapsed(), c.count, rate, c.interestingCount, interestingTotalCount) + total := c.warmupInputCount + c.interestingCount + fmt.Fprintf(c.opts.Log, "fuzz: elapsed: %s, execs: %d (%.0f/sec), new interesting: %d (total: %d)\n", c.elapsed(), c.count, rate, c.interestingCount, total) } else { fmt.Fprintf(c.opts.Log, "fuzz: elapsed: %s, execs: %d (%.0f/sec)\n", c.elapsed(), c.count, rate) } From e052044d6b25a76603cafbfb1099cf4196528556 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Mon, 31 Jan 2022 21:25:14 -0800 Subject: [PATCH 22/42] go/types, types2: better error messages for comparisons Refactor Checker.comparison such that its logic is easier to reason about and so that special cases can be handled more directly. Use the appropriate operand (of 1st or 2nd operand) for error reporting (position and type), rather than always using the first operand. Use an extra parameter to indicate a switch case comparison; in this case the error is always reported at the position of the first operand. (The error messages are not yet adjusted for switches; see next CL.) Introduce a new kindString function which is used to print simplified types in error messages (related to comparisons only): instead of printing the details of a struct type, we just print "struct" where the details are not relevant. This matches the 1.17 compiler behavior. Added a "reportf" parameter to the internal comparable function so we can report an error cause in addition to the boolean result. Rather than passing a *string for cause, we pass a function to record the cause so that we can use the *Checker context for printing (needed for proper type qualification). This mechanism reports the same details now as the 1.17 compiler. Adjusted various tests as needed added new test files. Fixes #50918. Change-Id: I1f0e7af22f09db4d31679c667c71a9038a8dc9d2 Reviewed-on: https://go-review.googlesource.com/c/go/+/381964 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/expr.go | 178 +++++++++++++---- src/cmd/compile/internal/types2/predicates.go | 18 +- src/cmd/compile/internal/types2/stmt.go | 2 +- .../internal/types2/testdata/check/expr2.src | 28 +-- .../types2/testdata/fixedbugs/issue48712.go2 | 22 +-- .../types2/testdata/fixedbugs/issue50918.go | 21 ++ .../types2/testdata/spec/comparisons.go2 | 120 ++++++++++++ src/cmd/compile/internal/types2/typeset.go | 2 +- src/go/types/expr.go | 182 ++++++++++++++---- src/go/types/predicates.go | 18 +- src/go/types/stmt.go | 2 +- src/go/types/testdata/check/expr2.src | 28 +-- .../types/testdata/fixedbugs/issue48712.go2 | 22 +-- src/go/types/testdata/fixedbugs/issue50918.go | 21 ++ src/go/types/testdata/spec/comparisons.go2 | 120 ++++++++++++ src/go/types/typeset.go | 2 +- test/fixedbugs/issue11737.go | 2 +- 17 files changed, 643 insertions(+), 145 deletions(-) create mode 100644 src/cmd/compile/internal/types2/testdata/fixedbugs/issue50918.go create mode 100644 src/cmd/compile/internal/types2/testdata/spec/comparisons.go2 create mode 100644 src/go/types/testdata/fixedbugs/issue50918.go create mode 100644 src/go/types/testdata/spec/comparisons.go2 diff --git a/src/cmd/compile/internal/types2/expr.go b/src/cmd/compile/internal/types2/expr.go index 7a668d20f1..442e7121e5 100644 --- a/src/cmd/compile/internal/types2/expr.go +++ b/src/cmd/compile/internal/types2/expr.go @@ -770,52 +770,82 @@ func (check *Checker) implicitTypeAndValue(x *operand, target Type) (Type, const return target, nil, 0 } -func (check *Checker) comparison(x, y *operand, op syntax.Operator) { +// If switchCase is true, the operator op is ignored. +func (check *Checker) comparison(x, y *operand, op syntax.Operator, switchCase bool) { + if switchCase { + op = syntax.Eql + } + + errOp := x // operand for which error is reported, if any + cause := "" // specific error cause, if any + // spec: "In any comparison, the first operand must be assignable // to the type of the second operand, or vice versa." - err := "" - xok, _ := x.assignableTo(check, y.typ, nil) - yok, _ := y.assignableTo(check, x.typ, nil) - if xok || yok { - equality := false - defined := false - switch op { - case syntax.Eql, syntax.Neq: - // spec: "The equality operators == and != apply to operands that are comparable." - equality = true - defined = Comparable(x.typ) && Comparable(y.typ) || x.isNil() && hasNil(y.typ) || y.isNil() && hasNil(x.typ) - case syntax.Lss, syntax.Leq, syntax.Gtr, syntax.Geq: - // spec: The ordering operators <, <=, >, and >= apply to operands that are ordered." - defined = allOrdered(x.typ) && allOrdered(y.typ) - default: - unreachable() + ok, _ := x.assignableTo(check, y.typ, nil) + if !ok { + ok, _ = y.assignableTo(check, x.typ, nil) + } + if !ok { + // Report the error on the 2nd operand since we only + // know after seeing the 2nd operand whether we have + // a type mismatch. + errOp = y + // For now, if we're not running the compiler, use the + // position of x to minimize changes to existing tests. + if !check.conf.CompilerErrorMessages { + errOp = x } - if !defined { - if equality && (isTypeParam(x.typ) || isTypeParam(y.typ)) { - typ := x.typ - if isTypeParam(y.typ) { - typ = y.typ - } - err = check.sprintf("%s is not comparable", typ) - } else { - typ := x.typ - if x.isNil() { - typ = y.typ - } - err = check.sprintf("operator %s not defined on %s", op, typ) + cause = check.sprintf("mismatched types %s and %s", x.typ, y.typ) + goto Error + } + + // check if comparison is defined for operands + switch op { + case syntax.Eql, syntax.Neq: + // spec: "The equality operators == and != apply to operands that are comparable." + switch { + case x.isNil() || y.isNil(): + // Comparison against nil requires that the other operand type has nil. + typ := x.typ + if x.isNil() { + typ = y.typ } + if !hasNil(typ) { + // This case should only be possible for "nil == nil". + // Report the error on the 2nd operand since we only + // know after seeing the 2nd operand whether we have + // an invalid comparison. + errOp = y + goto Error + } + + case !Comparable(x.typ): + errOp = x + cause = check.incomparableCause(x.typ) + goto Error + + case !Comparable(y.typ): + errOp = y + cause = check.incomparableCause(y.typ) + goto Error } - } else { - err = check.sprintf("mismatched types %s and %s", x.typ, y.typ) - } - - if err != "" { - // TODO(gri) better error message for cases where one can only compare against nil - check.errorf(x, invalidOp+"cannot compare %s %s %s (%s)", x.expr, op, y.expr, err) - x.mode = invalid - return + + case syntax.Lss, syntax.Leq, syntax.Gtr, syntax.Geq: + // spec: The ordering operators <, <=, >, and >= apply to operands that are ordered." + switch { + case !allOrdered(x.typ): + errOp = x + goto Error + case !allOrdered(y.typ): + errOp = y + goto Error + } + + default: + unreachable() } + // comparison is ok if x.mode == constant_ && y.mode == constant_ { x.val = constant.MakeBool(constant.Compare(x.val, op2tok[op], y.val)) // The operands are never materialized; no need to update @@ -833,6 +863,74 @@ func (check *Checker) comparison(x, y *operand, op syntax.Operator) { // spec: "Comparison operators compare two operands and yield // an untyped boolean value." x.typ = Typ[UntypedBool] + return + +Error: + // We have an offending operand errOp and possibly an error cause. + if cause == "" { + if isTypeParam(x.typ) || isTypeParam(y.typ) { + // TODO(gri) should report the specific type causing the problem, if any + if !isTypeParam(x.typ) { + errOp = y + } + cause = check.sprintf("type parameter %s is not comparable with %s", errOp.typ, op) + } else { + cause = check.sprintf("operator %s not defined on %s", op, check.kindString(errOp.typ)) // catch-all + } + } + // For switches, report errors on the first (case) operand. + // TODO(gri) adjust error message in that case + if switchCase { + errOp = x + } + if check.conf.CompilerErrorMessages { + check.errorf(errOp, invalidOp+"%s %s %s (%s)", x.expr, op, y.expr, cause) + } else { + check.errorf(errOp, invalidOp+"cannot compare %s %s %s (%s)", x.expr, op, y.expr, cause) + } + x.mode = invalid +} + +// incomparableCause returns a more specific cause why typ is not comparable. +// If there is no more specific cause, the result is "". +func (check *Checker) incomparableCause(typ Type) string { + switch under(typ).(type) { + case *Slice, *Signature, *Map: + return check.kindString(typ) + " can only be compared to nil" + } + // see if we can extract a more specific error + var cause string + comparable(typ, nil, func(format string, args ...interface{}) { + cause = check.sprintf(format, args...) + }) + return cause +} + +// kindString returns the type kind as a string. +func (check *Checker) kindString(typ Type) string { + switch under(typ).(type) { + case *Array: + return "array" + case *Slice: + return "slice" + case *Struct: + return "struct" + case *Pointer: + return "pointer" + case *Signature: + return "func" + case *Interface: + if isTypeParam(typ) { + return check.sprintf("type parameter %s", typ) + } + return "interface" + case *Map: + return "map" + case *Chan: + return "chan" + default: + return check.sprintf("%s", typ) // catch-all + } } // If e != nil, it must be the shift expression; it may be nil for non-constant shifts. @@ -1034,7 +1132,7 @@ func (check *Checker) binary(x *operand, e syntax.Expr, lhs, rhs syntax.Expr, op } if isComparison(op) { - check.comparison(x, &y, op) + check.comparison(x, &y, op, false) return } diff --git a/src/cmd/compile/internal/types2/predicates.go b/src/cmd/compile/internal/types2/predicates.go index 003e58db38..279d0775bd 100644 --- a/src/cmd/compile/internal/types2/predicates.go +++ b/src/cmd/compile/internal/types2/predicates.go @@ -102,10 +102,11 @@ func isGeneric(t Type) bool { // Comparable reports whether values of type T are comparable. func Comparable(T Type) bool { - return comparable(T, nil) + return comparable(T, nil, nil) } -func comparable(T Type, seen map[Type]bool) bool { +// If reportf != nil, it may be used to report why T is not comparable. +func comparable(T Type, seen map[Type]bool, reportf func(string, ...interface{})) bool { if seen[T] { return true } @@ -123,13 +124,22 @@ func comparable(T Type, seen map[Type]bool) bool { return true case *Struct: for _, f := range t.fields { - if !comparable(f.typ, seen) { + if !comparable(f.typ, seen, nil) { + if reportf != nil { + reportf("struct containing %s cannot be compared", f.typ) + } return false } } return true case *Array: - return comparable(t.elem, seen) + if !comparable(t.elem, seen, nil) { + if reportf != nil { + reportf("%s cannot be compared", t) + } + return false + } + return true case *Interface: return !isTypeParam(T) || t.typeSet().IsComparable(seen) } diff --git a/src/cmd/compile/internal/types2/stmt.go b/src/cmd/compile/internal/types2/stmt.go index b23d7aeef2..633ee31551 100644 --- a/src/cmd/compile/internal/types2/stmt.go +++ b/src/cmd/compile/internal/types2/stmt.go @@ -239,7 +239,7 @@ L: } // Order matters: By comparing v against x, error positions are at the case values. res := v // keep original v unchanged - check.comparison(&res, x, syntax.Eql) + check.comparison(&res, x, syntax.Eql, true) if res.mode == invalid { continue L } diff --git a/src/cmd/compile/internal/types2/testdata/check/expr2.src b/src/cmd/compile/internal/types2/testdata/check/expr2.src index 8e5862319e..88781f1189 100644 --- a/src/cmd/compile/internal/types2/testdata/check/expr2.src +++ b/src/cmd/compile/internal/types2/testdata/check/expr2.src @@ -9,8 +9,8 @@ package expr2 func _bool() { const t = true == true const f = true == false - _ = t /* ERROR "cannot compare" */ < f - _ = 0 /* ERROR "mismatched types untyped int and untyped bool" */ == t + _ = t /* ERROR cannot compare */ < f + _ = 0 /* ERROR mismatched types untyped int and untyped bool */ == t var b bool var x, y float32 b = x < y @@ -20,7 +20,7 @@ func _bool() { // corner cases var ( - v0 = nil /* ERROR "cannot compare" */ == nil + v0 = nil == nil // ERROR operator == not defined on untyped nil ) func arrays() { @@ -40,7 +40,7 @@ func arrays() { _ = c /* ERROR mismatched types */ == d var e [10]func() int - _ = e /* ERROR == not defined */ == e + _ = e /* ERROR \[10\]func\(\) int cannot be compared */ == e } func structs() { @@ -79,8 +79,8 @@ func structs() { func pointers() { // nil - _ = nil /* ERROR == not defined */ == nil - _ = nil /* ERROR != not defined */ != nil + _ = nil == nil // ERROR operator == not defined on untyped nil + _ = nil != nil // ERROR operator != not defined on untyped nil _ = nil /* ERROR < not defined */ < nil _ = nil /* ERROR <= not defined */ <= nil _ = nil /* ERROR > not defined */ > nil @@ -211,16 +211,16 @@ func interfaces() { // issue #28164 // testcase from issue - _ = interface /* ERROR cannot compare */ {}(nil) == []int(nil) + _ = interface{}(nil) == [ /* ERROR slice can only be compared to nil */ ]int(nil) // related cases var e interface{} var s []int var x int - _ = e /* ERROR cannot compare */ == s - _ = s /* ERROR cannot compare */ == e - _ = e /* ERROR cannot compare */ < x - _ = x /* ERROR cannot compare */ < e + _ = e == s // ERROR slice can only be compared to nil + _ = s /* ERROR slice can only be compared to nil */ == e + _ = e /* ERROR operator < not defined on interface */ < x + _ = x < e // ERROR operator < not defined on interface } func slices() { @@ -231,7 +231,7 @@ func slices() { _ = s /* ERROR < not defined */ < nil // slices are not otherwise comparable - _ = s /* ERROR == not defined */ == s + _ = s /* ERROR slice can only be compared to nil */ == s _ = s /* ERROR < not defined */ < s } @@ -243,7 +243,7 @@ func maps() { _ = m /* ERROR < not defined */ < nil // maps are not otherwise comparable - _ = m /* ERROR == not defined */ == m + _ = m /* ERROR map can only be compared to nil */ == m _ = m /* ERROR < not defined */ < m } @@ -255,6 +255,6 @@ func funcs() { _ = f /* ERROR < not defined */ < nil // funcs are not otherwise comparable - _ = f /* ERROR == not defined */ == f + _ = f /* ERROR func can only be compared to nil */ == f _ = f /* ERROR < not defined */ < f } diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue48712.go2 b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue48712.go2 index bad8712fda..ab397560a8 100644 --- a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue48712.go2 +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue48712.go2 @@ -10,7 +10,7 @@ func _[P comparable](x, y P) { _ = y == x _ = y == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } func _[P comparable](x P, y any) { @@ -19,23 +19,23 @@ func _[P comparable](x P, y any) { _ = y == x _ = y == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } func _[P any](x, y P) { - _ = x /* ERROR P is not comparable */ == x - _ = x /* ERROR P is not comparable */ == y - _ = y /* ERROR P is not comparable */ == x - _ = y /* ERROR P is not comparable */ == y + _ = x /* ERROR type parameter P is not comparable with == */ == x + _ = x /* ERROR type parameter P is not comparable with == */ == y + _ = y /* ERROR type parameter P is not comparable with == */ == x + _ = y /* ERROR type parameter P is not comparable with == */ == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } func _[P any](x P, y any) { - _ = x /* ERROR P is not comparable */ == x - _ = x /* ERROR P is not comparable */ == y - _ = y /* ERROR P is not comparable */ == x + _ = x /* ERROR type parameter P is not comparable with == */ == x + _ = x /* ERROR type parameter P is not comparable with == */ == y + _ = y == x // ERROR type parameter P is not comparable with == _ = y == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50918.go b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50918.go new file mode 100644 index 0000000000..41604b8bad --- /dev/null +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50918.go @@ -0,0 +1,21 @@ +// 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 + +type thing1 struct { + things []string +} + +type thing2 struct { + things []thing1 +} + +func _() { + var a1, b1 thing1 + _ = a1 /* ERROR struct containing \[\]string cannot be compared */ == b1 + + var a2, b2 thing2 + _ = a2 /* ERROR struct containing \[\]thing1 cannot be compared */ == b2 +} diff --git a/src/cmd/compile/internal/types2/testdata/spec/comparisons.go2 b/src/cmd/compile/internal/types2/testdata/spec/comparisons.go2 new file mode 100644 index 0000000000..62c95d47d7 --- /dev/null +++ b/src/cmd/compile/internal/types2/testdata/spec/comparisons.go2 @@ -0,0 +1,120 @@ +// 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 comparisons + +type ( + B int // basic type representative + A [10]func() + L []byte + S struct{ f []byte } + P *S + F func() + I interface{} + M map[string]int + C chan int +) + +var ( + b B + a A + l L + s S + p P + f F + i I + m M + c C +) + +func _() { + _ = nil == nil // ERROR operator == not defined on untyped nil + _ = b == b + _ = a /* ERROR \[10\]func\(\) cannot be compared */ == a + _ = l /* ERROR slice can only be compared to nil */ == l + _ = s /* ERROR struct containing \[\]byte cannot be compared */ == s + _ = p == p + _ = f /* ERROR func can only be compared to nil */ == f + _ = i == i + _ = m /* ERROR map can only be compared to nil */ == m + _ = c == c + + _ = b /* ERROR mismatched types */ == nil + _ = a /* ERROR mismatched types */ == nil + _ = l == nil + _ = s /* ERROR mismatched types */ == nil + _ = p == nil + _ = f == nil + _ = i == nil + _ = m == nil + _ = c == nil + + _ = nil /* ERROR operator < not defined on untyped nil */ < nil + _ = b < b + _ = a /* ERROR operator < not defined on array */ < a + _ = l /* ERROR operator < not defined on slice */ < l + _ = s /* ERROR operator < not defined on struct */ < s + _ = p /* ERROR operator < not defined on pointer */ < p + _ = f /* ERROR operator < not defined on func */ < f + _ = i /* ERROR operator < not defined on interface */ < i + _ = m /* ERROR operator < not defined on map */ < m + _ = c /* ERROR operator < not defined on chan */ < c +} + +func _[ + B int, + A [10]func(), + L []byte, + S struct{ f []byte }, + P *S, + F func(), + I interface{}, + J comparable, + M map[string]int, + C chan int, +] ( + b B, + a A, + l L, + s S, + p P, + f F, + i I, + j J, + m M, + c C, +) { + _ = b == b + _ = a /* ERROR type parameter A is not comparable with == */ == a + _ = l /* ERROR type parameter L is not comparable with == */ == l + _ = s /* ERROR type parameter S is not comparable with == */ == s + _ = p == p + _ = f /* ERROR type parameter F is not comparable with == */ == f + _ = i /* ERROR type parameter I is not comparable with == */ == i + _ = j == j + _ = m /* ERROR type parameter M is not comparable with == */ == m + _ = c == c + + _ = b /* ERROR mismatched types */ == nil + _ = a /* ERROR mismatched types */ == nil + _ = l == nil + _ = s /* ERROR mismatched types */ == nil + _ = p == nil + _ = f == nil + _ = i /* ERROR mismatched types */ == nil + _ = j /* ERROR mismatched types */ == nil + _ = m == nil + _ = c == nil + + _ = b < b + _ = a /* ERROR type parameter A is not comparable with < */ < a + _ = l /* ERROR type parameter L is not comparable with < */ < l + _ = s /* ERROR type parameter S is not comparable with < */ < s + _ = p /* ERROR type parameter P is not comparable with < */ < p + _ = f /* ERROR type parameter F is not comparable with < */ < f + _ = i /* ERROR type parameter I is not comparable with < */ < i + _ = j /* ERROR type parameter J is not comparable with < */ < j + _ = m /* ERROR type parameter M is not comparable with < */ < m + _ = c /* ERROR type parameter C is not comparable with < */ < c +} diff --git a/src/cmd/compile/internal/types2/typeset.go b/src/cmd/compile/internal/types2/typeset.go index 7a1e1bdf2f..3884276adc 100644 --- a/src/cmd/compile/internal/types2/typeset.go +++ b/src/cmd/compile/internal/types2/typeset.go @@ -39,7 +39,7 @@ func (s *_TypeSet) IsComparable(seen map[Type]bool) bool { return s.comparable } return s.is(func(t *term) bool { - return t != nil && comparable(t.typ, seen) + return t != nil && comparable(t.typ, seen, nil) }) } diff --git a/src/go/types/expr.go b/src/go/types/expr.go index 44e0288d3e..c5b27e84b8 100644 --- a/src/go/types/expr.go +++ b/src/go/types/expr.go @@ -728,54 +728,84 @@ func (check *Checker) implicitTypeAndValue(x *operand, target Type) (Type, const return target, nil, 0 } -func (check *Checker) comparison(x, y *operand, op token.Token) { +// If switchCase is true, the operator op is ignored. +func (check *Checker) comparison(x, y *operand, op token.Token, switchCase bool) { + if switchCase { + op = token.EQL + } + + errOp := x // operand for which error is reported, if any + cause := "" // specific error cause, if any + // spec: "In any comparison, the first operand must be assignable // to the type of the second operand, or vice versa." - err := "" - var code errorCode - xok, _ := x.assignableTo(check, y.typ, nil) - yok, _ := y.assignableTo(check, x.typ, nil) - if xok || yok { - equality := false - defined := false - switch op { - case token.EQL, token.NEQ: - // spec: "The equality operators == and != apply to operands that are comparable." - equality = true - defined = Comparable(x.typ) && Comparable(y.typ) || x.isNil() && hasNil(y.typ) || y.isNil() && hasNil(x.typ) - case token.LSS, token.LEQ, token.GTR, token.GEQ: - // spec: The ordering operators <, <=, >, and >= apply to operands that are ordered." - defined = allOrdered(x.typ) && allOrdered(y.typ) - default: - unreachable() + code := _MismatchedTypes + ok, _ := x.assignableTo(check, y.typ, nil) + if !ok { + ok, _ = y.assignableTo(check, x.typ, nil) + } + if !ok { + // Report the error on the 2nd operand since we only + // know after seeing the 2nd operand whether we have + // a type mismatch. + errOp = y + // For now, if we're not running the compiler, use the + // position of x to minimize changes to existing tests. + if !compilerErrorMessages { + errOp = x } - if !defined { - if equality && (isTypeParam(x.typ) || isTypeParam(y.typ)) { - typ := x.typ - if isTypeParam(y.typ) { - typ = y.typ - } - err = check.sprintf("%s is not comparable", typ) - } else { - typ := x.typ - if x.isNil() { - typ = y.typ - } - err = check.sprintf("operator %s not defined on %s", op, typ) + cause = check.sprintf("mismatched types %s and %s", x.typ, y.typ) + goto Error + } + + // check if comparison is defined for operands + code = _UndefinedOp + switch op { + case token.EQL, token.NEQ: + // spec: "The equality operators == and != apply to operands that are comparable." + switch { + case x.isNil() || y.isNil(): + // Comparison against nil requires that the other operand type has nil. + typ := x.typ + if x.isNil() { + typ = y.typ } - code = _UndefinedOp + if !hasNil(typ) { + // This case should only be possible for "nil == nil". + // Report the error on the 2nd operand since we only + // know after seeing the 2nd operand whether we have + // an invalid comparison. + errOp = y + goto Error + } + + case !Comparable(x.typ): + errOp = x + cause = check.incomparableCause(x.typ) + goto Error + + case !Comparable(y.typ): + errOp = y + cause = check.incomparableCause(y.typ) + goto Error } - } else { - err = check.sprintf("mismatched types %s and %s", x.typ, y.typ) - code = _MismatchedTypes - } - - if err != "" { - check.errorf(x, code, "cannot compare %s %s %s (%s)", x.expr, op, y.expr, err) - x.mode = invalid - return + + case token.LSS, token.LEQ, token.GTR, token.GEQ: + // spec: The ordering operators <, <=, >, and >= apply to operands that are ordered." + switch { + case !allOrdered(x.typ): + errOp = x + goto Error + case !allOrdered(y.typ): + errOp = y + goto Error + } + + default: + unreachable() } + // comparison is ok if x.mode == constant_ && y.mode == constant_ { x.val = constant.MakeBool(constant.Compare(x.val, op, y.val)) // The operands are never materialized; no need to update @@ -793,6 +823,74 @@ func (check *Checker) comparison(x, y *operand, op token.Token) { // spec: "Comparison operators compare two operands and yield // an untyped boolean value." x.typ = Typ[UntypedBool] + return + +Error: + // We have an offending operand errOp and possibly an error cause. + if cause == "" { + if isTypeParam(x.typ) || isTypeParam(y.typ) { + // TODO(gri) should report the specific type causing the problem, if any + if !isTypeParam(x.typ) { + errOp = y + } + cause = check.sprintf("type parameter %s is not comparable with %s", errOp.typ, op) + } else { + cause = check.sprintf("operator %s not defined on %s", op, check.kindString(errOp.typ)) // catch-all + } + } + // For switches, report errors on the first (case) operand. + // TODO(gri) adjust error message in that case + if switchCase { + errOp = x + } + if compilerErrorMessages { + check.invalidOp(errOp, code, "%s %s %s (%s)", x.expr, op, y.expr, cause) + } else { + check.invalidOp(errOp, code, "cannot compare %s %s %s (%s)", x.expr, op, y.expr, cause) + } + x.mode = invalid +} + +// incomparableCause returns a more specific cause why typ is not comparable. +// If there is no more specific cause, the result is "". +func (check *Checker) incomparableCause(typ Type) string { + switch under(typ).(type) { + case *Slice, *Signature, *Map: + return check.kindString(typ) + " can only be compared to nil" + } + // see if we can extract a more specific error + var cause string + comparable(typ, nil, func(format string, args ...interface{}) { + cause = check.sprintf(format, args...) + }) + return cause +} + +// kindString returns the type kind as a string. +func (check *Checker) kindString(typ Type) string { + switch under(typ).(type) { + case *Array: + return "array" + case *Slice: + return "slice" + case *Struct: + return "struct" + case *Pointer: + return "pointer" + case *Signature: + return "func" + case *Interface: + if isTypeParam(typ) { + return check.sprintf("type parameter %s", typ) + } + return "interface" + case *Map: + return "map" + case *Chan: + return "chan" + default: + return check.sprintf("%s", typ) // catch-all + } } // If e != nil, it must be the shift expression; it may be nil for non-constant shifts. @@ -1014,7 +1112,7 @@ func (check *Checker) binary(x *operand, e ast.Expr, lhs, rhs ast.Expr, op token } if isComparison(op) { - check.comparison(x, &y, op) + check.comparison(x, &y, op, false) return } diff --git a/src/go/types/predicates.go b/src/go/types/predicates.go index 9ae6cd51b7..23dcd7274d 100644 --- a/src/go/types/predicates.go +++ b/src/go/types/predicates.go @@ -104,10 +104,11 @@ func isGeneric(t Type) bool { // Comparable reports whether values of type T are comparable. func Comparable(T Type) bool { - return comparable(T, nil) + return comparable(T, nil, nil) } -func comparable(T Type, seen map[Type]bool) bool { +// If reportf != nil, it may be used to report why T is not comparable. +func comparable(T Type, seen map[Type]bool, reportf func(string, ...interface{})) bool { if seen[T] { return true } @@ -125,13 +126,22 @@ func comparable(T Type, seen map[Type]bool) bool { return true case *Struct: for _, f := range t.fields { - if !comparable(f.typ, seen) { + if !comparable(f.typ, seen, nil) { + if reportf != nil { + reportf("struct containing %s cannot be compared", f.typ) + } return false } } return true case *Array: - return comparable(t.elem, seen) + if !comparable(t.elem, seen, nil) { + if reportf != nil { + reportf("%s cannot be compared", t) + } + return false + } + return true case *Interface: return !isTypeParam(T) || t.typeSet().IsComparable(seen) } diff --git a/src/go/types/stmt.go b/src/go/types/stmt.go index 802673567d..5ceae08daa 100644 --- a/src/go/types/stmt.go +++ b/src/go/types/stmt.go @@ -248,7 +248,7 @@ L: } // Order matters: By comparing v against x, error positions are at the case values. res := v // keep original v unchanged - check.comparison(&res, x, token.EQL) + check.comparison(&res, x, token.EQL, true) if res.mode == invalid { continue L } diff --git a/src/go/types/testdata/check/expr2.src b/src/go/types/testdata/check/expr2.src index 8757fd9e48..6133dbb42b 100644 --- a/src/go/types/testdata/check/expr2.src +++ b/src/go/types/testdata/check/expr2.src @@ -9,8 +9,8 @@ package expr2 func _bool() { const t = true == true const f = true == false - _ = t /* ERROR "cannot compare" */ < f - _ = 0 /* ERROR "mismatched types untyped int and untyped bool" */ == t + _ = t /* ERROR cannot compare */ < f + _ = 0 /* ERROR mismatched types untyped int and untyped bool */ == t var b bool var x, y float32 b = x < y @@ -20,7 +20,7 @@ func _bool() { // corner cases var ( - v0 = nil /* ERROR "cannot compare" */ == nil + v0 = nil == nil // ERROR operator == not defined on untyped nil ) func arrays() { @@ -40,7 +40,7 @@ func arrays() { _ = c /* ERROR mismatched types */ == d var e [10]func() int - _ = e /* ERROR == not defined */ == e + _ = e /* ERROR \[10\]func\(\) int cannot be compared */ == e } func structs() { @@ -79,8 +79,8 @@ func structs() { func pointers() { // nil - _ = nil /* ERROR == not defined */ == nil - _ = nil /* ERROR != not defined */ != nil + _ = nil == nil // ERROR operator == not defined on untyped nil + _ = nil != nil // ERROR operator != not defined on untyped nil _ = nil /* ERROR < not defined */ < nil _ = nil /* ERROR <= not defined */ <= nil _ = nil /* ERROR > not defined */ > nil @@ -211,16 +211,16 @@ func interfaces() { // issue #28164 // testcase from issue - _ = interface /* ERROR cannot compare */ {}(nil) == []int(nil) + _ = interface{}(nil) == [ /* ERROR slice can only be compared to nil */ ]int(nil) // related cases var e interface{} var s []int var x int - _ = e /* ERROR cannot compare */ == s - _ = s /* ERROR cannot compare */ == e - _ = e /* ERROR cannot compare */ < x - _ = x /* ERROR cannot compare */ < e + _ = e == s // ERROR slice can only be compared to nil + _ = s /* ERROR slice can only be compared to nil */ == e + _ = e /* ERROR operator < not defined on interface */ < x + _ = x < e // ERROR operator < not defined on interface } func slices() { @@ -231,7 +231,7 @@ func slices() { _ = s /* ERROR < not defined */ < nil // slices are not otherwise comparable - _ = s /* ERROR == not defined */ == s + _ = s /* ERROR slice can only be compared to nil */ == s _ = s /* ERROR < not defined */ < s } @@ -243,7 +243,7 @@ func maps() { _ = m /* ERROR < not defined */ < nil // maps are not otherwise comparable - _ = m /* ERROR == not defined */ == m + _ = m /* ERROR map can only be compared to nil */ == m _ = m /* ERROR < not defined */ < m } @@ -255,6 +255,6 @@ func funcs() { _ = f /* ERROR < not defined */ < nil // funcs are not otherwise comparable - _ = f /* ERROR == not defined */ == f + _ = f /* ERROR func can only be compared to nil */ == f _ = f /* ERROR < not defined */ < f } diff --git a/src/go/types/testdata/fixedbugs/issue48712.go2 b/src/go/types/testdata/fixedbugs/issue48712.go2 index bad8712fda..ab397560a8 100644 --- a/src/go/types/testdata/fixedbugs/issue48712.go2 +++ b/src/go/types/testdata/fixedbugs/issue48712.go2 @@ -10,7 +10,7 @@ func _[P comparable](x, y P) { _ = y == x _ = y == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } func _[P comparable](x P, y any) { @@ -19,23 +19,23 @@ func _[P comparable](x P, y any) { _ = y == x _ = y == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } func _[P any](x, y P) { - _ = x /* ERROR P is not comparable */ == x - _ = x /* ERROR P is not comparable */ == y - _ = y /* ERROR P is not comparable */ == x - _ = y /* ERROR P is not comparable */ == y + _ = x /* ERROR type parameter P is not comparable with == */ == x + _ = x /* ERROR type parameter P is not comparable with == */ == y + _ = y /* ERROR type parameter P is not comparable with == */ == x + _ = y /* ERROR type parameter P is not comparable with == */ == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } func _[P any](x P, y any) { - _ = x /* ERROR P is not comparable */ == x - _ = x /* ERROR P is not comparable */ == y - _ = y /* ERROR P is not comparable */ == x + _ = x /* ERROR type parameter P is not comparable with == */ == x + _ = x /* ERROR type parameter P is not comparable with == */ == y + _ = y == x // ERROR type parameter P is not comparable with == _ = y == y - _ = x /* ERROR operator < not defined on P */ < y + _ = x /* ERROR type parameter P is not comparable with < */ < y } diff --git a/src/go/types/testdata/fixedbugs/issue50918.go b/src/go/types/testdata/fixedbugs/issue50918.go new file mode 100644 index 0000000000..41604b8bad --- /dev/null +++ b/src/go/types/testdata/fixedbugs/issue50918.go @@ -0,0 +1,21 @@ +// 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 + +type thing1 struct { + things []string +} + +type thing2 struct { + things []thing1 +} + +func _() { + var a1, b1 thing1 + _ = a1 /* ERROR struct containing \[\]string cannot be compared */ == b1 + + var a2, b2 thing2 + _ = a2 /* ERROR struct containing \[\]thing1 cannot be compared */ == b2 +} diff --git a/src/go/types/testdata/spec/comparisons.go2 b/src/go/types/testdata/spec/comparisons.go2 new file mode 100644 index 0000000000..62c95d47d7 --- /dev/null +++ b/src/go/types/testdata/spec/comparisons.go2 @@ -0,0 +1,120 @@ +// 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 comparisons + +type ( + B int // basic type representative + A [10]func() + L []byte + S struct{ f []byte } + P *S + F func() + I interface{} + M map[string]int + C chan int +) + +var ( + b B + a A + l L + s S + p P + f F + i I + m M + c C +) + +func _() { + _ = nil == nil // ERROR operator == not defined on untyped nil + _ = b == b + _ = a /* ERROR \[10\]func\(\) cannot be compared */ == a + _ = l /* ERROR slice can only be compared to nil */ == l + _ = s /* ERROR struct containing \[\]byte cannot be compared */ == s + _ = p == p + _ = f /* ERROR func can only be compared to nil */ == f + _ = i == i + _ = m /* ERROR map can only be compared to nil */ == m + _ = c == c + + _ = b /* ERROR mismatched types */ == nil + _ = a /* ERROR mismatched types */ == nil + _ = l == nil + _ = s /* ERROR mismatched types */ == nil + _ = p == nil + _ = f == nil + _ = i == nil + _ = m == nil + _ = c == nil + + _ = nil /* ERROR operator < not defined on untyped nil */ < nil + _ = b < b + _ = a /* ERROR operator < not defined on array */ < a + _ = l /* ERROR operator < not defined on slice */ < l + _ = s /* ERROR operator < not defined on struct */ < s + _ = p /* ERROR operator < not defined on pointer */ < p + _ = f /* ERROR operator < not defined on func */ < f + _ = i /* ERROR operator < not defined on interface */ < i + _ = m /* ERROR operator < not defined on map */ < m + _ = c /* ERROR operator < not defined on chan */ < c +} + +func _[ + B int, + A [10]func(), + L []byte, + S struct{ f []byte }, + P *S, + F func(), + I interface{}, + J comparable, + M map[string]int, + C chan int, +] ( + b B, + a A, + l L, + s S, + p P, + f F, + i I, + j J, + m M, + c C, +) { + _ = b == b + _ = a /* ERROR type parameter A is not comparable with == */ == a + _ = l /* ERROR type parameter L is not comparable with == */ == l + _ = s /* ERROR type parameter S is not comparable with == */ == s + _ = p == p + _ = f /* ERROR type parameter F is not comparable with == */ == f + _ = i /* ERROR type parameter I is not comparable with == */ == i + _ = j == j + _ = m /* ERROR type parameter M is not comparable with == */ == m + _ = c == c + + _ = b /* ERROR mismatched types */ == nil + _ = a /* ERROR mismatched types */ == nil + _ = l == nil + _ = s /* ERROR mismatched types */ == nil + _ = p == nil + _ = f == nil + _ = i /* ERROR mismatched types */ == nil + _ = j /* ERROR mismatched types */ == nil + _ = m == nil + _ = c == nil + + _ = b < b + _ = a /* ERROR type parameter A is not comparable with < */ < a + _ = l /* ERROR type parameter L is not comparable with < */ < l + _ = s /* ERROR type parameter S is not comparable with < */ < s + _ = p /* ERROR type parameter P is not comparable with < */ < p + _ = f /* ERROR type parameter F is not comparable with < */ < f + _ = i /* ERROR type parameter I is not comparable with < */ < i + _ = j /* ERROR type parameter J is not comparable with < */ < j + _ = m /* ERROR type parameter M is not comparable with < */ < m + _ = c /* ERROR type parameter C is not comparable with < */ < c +} diff --git a/src/go/types/typeset.go b/src/go/types/typeset.go index 4598daacb0..9f4831e976 100644 --- a/src/go/types/typeset.go +++ b/src/go/types/typeset.go @@ -37,7 +37,7 @@ func (s *_TypeSet) IsComparable(seen map[Type]bool) bool { return s.comparable } return s.is(func(t *term) bool { - return t != nil && comparable(t.typ, seen) + return t != nil && comparable(t.typ, seen, nil) }) } diff --git a/test/fixedbugs/issue11737.go b/test/fixedbugs/issue11737.go index eb4bfe8964..aa4abbc327 100644 --- a/test/fixedbugs/issue11737.go +++ b/test/fixedbugs/issue11737.go @@ -12,6 +12,6 @@ func f() func s(x interface{}) { switch x { - case f: // ERROR "invalid case f \(type func\(\)\) in switch \(incomparable type\)|cannot compare" + case f: // ERROR "invalid case f \(type func\(\)\) in switch \(incomparable type\)|can only be compared to nil" } } From 63e833154c230e9f46b41f913b12d3c72912cabc Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 1 Feb 2022 21:43:00 -0800 Subject: [PATCH 23/42] go/types, types2: better error messages for expression switches Fixes #50965. Change-Id: I61a74bdb46cf5e72ab94dbe8bd114704282b6211 Reviewed-on: https://go-review.googlesource.com/c/go/+/382354 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/expr.go | 13 ++++++------- .../internal/types2/testdata/check/stmt0.src | 2 +- .../types2/testdata/fixedbugs/issue43110.src | 2 +- .../types2/testdata/fixedbugs/issue50965.go | 17 +++++++++++++++++ src/go/types/expr.go | 13 ++++++------- src/go/types/testdata/check/stmt0.src | 2 +- src/go/types/testdata/fixedbugs/issue43110.src | 2 +- src/go/types/testdata/fixedbugs/issue50965.go | 17 +++++++++++++++++ 8 files changed, 50 insertions(+), 18 deletions(-) create mode 100644 src/cmd/compile/internal/types2/testdata/fixedbugs/issue50965.go create mode 100644 src/go/types/testdata/fixedbugs/issue50965.go diff --git a/src/cmd/compile/internal/types2/expr.go b/src/cmd/compile/internal/types2/expr.go index 442e7121e5..f1696bbe51 100644 --- a/src/cmd/compile/internal/types2/expr.go +++ b/src/cmd/compile/internal/types2/expr.go @@ -878,15 +878,14 @@ Error: cause = check.sprintf("operator %s not defined on %s", op, check.kindString(errOp.typ)) // catch-all } } - // For switches, report errors on the first (case) operand. - // TODO(gri) adjust error message in that case if switchCase { - errOp = x - } - if check.conf.CompilerErrorMessages { - check.errorf(errOp, invalidOp+"%s %s %s (%s)", x.expr, op, y.expr, cause) + check.errorf(x, "invalid case %s in switch on %s (%s)", x.expr, y.expr, cause) // error position always at 1st operand } else { - check.errorf(errOp, invalidOp+"cannot compare %s %s %s (%s)", x.expr, op, y.expr, cause) + if check.conf.CompilerErrorMessages { + check.errorf(errOp, invalidOp+"%s %s %s (%s)", x.expr, op, y.expr, cause) + } else { + check.errorf(errOp, invalidOp+"cannot compare %s %s %s (%s)", x.expr, op, y.expr, cause) + } } x.mode = invalid } diff --git a/src/cmd/compile/internal/types2/testdata/check/stmt0.src b/src/cmd/compile/internal/types2/testdata/check/stmt0.src index ed7ce05327..90ef09511f 100644 --- a/src/cmd/compile/internal/types2/testdata/check/stmt0.src +++ b/src/cmd/compile/internal/types2/testdata/check/stmt0.src @@ -429,7 +429,7 @@ func switches0() { switch int32(x) { case 1, 2: - case x /* ERROR "cannot compare" */ : + case x /* ERROR "invalid case x in switch on int32\(x\) \(mismatched types int and int32\)" */ : } switch x { diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue43110.src b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue43110.src index 4a46945239..8d5c983fd5 100644 --- a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue43110.src +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue43110.src @@ -30,7 +30,7 @@ func _() { } switch (func())(nil) { - case f /* ERROR cannot compare */ : + case f /* ERROR invalid case f in switch on .* \(func can only be compared to nil\) */ : } switch nil /* ERROR use of untyped nil in switch expression */ { diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50965.go b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50965.go new file mode 100644 index 0000000000..bf2dcc93d0 --- /dev/null +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50965.go @@ -0,0 +1,17 @@ +// 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 + +func _(x int, c string) { + switch x { + case c /* ERROR invalid case c in switch on x \(mismatched types string and int\) */ : + } +} + +func _(x, c []int) { + switch x { + case c /* ERROR invalid case c in switch on x \(slice can only be compared to nil\) */ : + } +} diff --git a/src/go/types/expr.go b/src/go/types/expr.go index c5b27e84b8..88a8901b07 100644 --- a/src/go/types/expr.go +++ b/src/go/types/expr.go @@ -838,15 +838,14 @@ Error: cause = check.sprintf("operator %s not defined on %s", op, check.kindString(errOp.typ)) // catch-all } } - // For switches, report errors on the first (case) operand. - // TODO(gri) adjust error message in that case if switchCase { - errOp = x - } - if compilerErrorMessages { - check.invalidOp(errOp, code, "%s %s %s (%s)", x.expr, op, y.expr, cause) + check.errorf(x, code, "invalid case %s in switch on %s (%s)", x.expr, y.expr, cause) // error position always at 1st operand } else { - check.invalidOp(errOp, code, "cannot compare %s %s %s (%s)", x.expr, op, y.expr, cause) + if compilerErrorMessages { + check.invalidOp(errOp, code, "%s %s %s (%s)", x.expr, op, y.expr, cause) + } else { + check.invalidOp(errOp, code, "cannot compare %s %s %s (%s)", x.expr, op, y.expr, cause) + } } x.mode = invalid } diff --git a/src/go/types/testdata/check/stmt0.src b/src/go/types/testdata/check/stmt0.src index ec8bf71013..7795a442aa 100644 --- a/src/go/types/testdata/check/stmt0.src +++ b/src/go/types/testdata/check/stmt0.src @@ -429,7 +429,7 @@ func switches0() { switch int32(x) { case 1, 2: - case x /* ERROR "cannot compare" */ : + case x /* ERROR "invalid case x in switch on int32\(x\) \(mismatched types int and int32\)" */ : } switch x { diff --git a/src/go/types/testdata/fixedbugs/issue43110.src b/src/go/types/testdata/fixedbugs/issue43110.src index 4a46945239..8d5c983fd5 100644 --- a/src/go/types/testdata/fixedbugs/issue43110.src +++ b/src/go/types/testdata/fixedbugs/issue43110.src @@ -30,7 +30,7 @@ func _() { } switch (func())(nil) { - case f /* ERROR cannot compare */ : + case f /* ERROR invalid case f in switch on .* \(func can only be compared to nil\) */ : } switch nil /* ERROR use of untyped nil in switch expression */ { diff --git a/src/go/types/testdata/fixedbugs/issue50965.go b/src/go/types/testdata/fixedbugs/issue50965.go new file mode 100644 index 0000000000..bf2dcc93d0 --- /dev/null +++ b/src/go/types/testdata/fixedbugs/issue50965.go @@ -0,0 +1,17 @@ +// 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 + +func _(x int, c string) { + switch x { + case c /* ERROR invalid case c in switch on x \(mismatched types string and int\) */ : + } +} + +func _(x, c []int) { + switch x { + case c /* ERROR invalid case c in switch on x \(slice can only be compared to nil\) */ : + } +} From d5bd3f9a6c3833d0c12ec45e1c73f8adf32ee2dd Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 2 Feb 2022 13:18:56 -0800 Subject: [PATCH 24/42] go/types, types2: use same method lookup code in both type checkers types2 has some code to handle case-folding when doing method lookups which was missing in go/types. This change is a first step to match the implementations. Specifically: In types2: - remove the lookupMethodFold names in favor of just lookupMethod, but with the foldCase flag (e.g., instead if lookupMethodFold, we just use lookupMethod) - rename checkFold to foldCase everywhere - add foldCase parameter where it was missing - moved foldCase paremeter to the end in lookupFieldOrMethod - no functionality changes In go/types: - match function signatures with types2 use - always provide false as argument for foldCase for now - no functionality changes Preparation for fixing some of the outstanding error reporting issues. Change-Id: If129a5feb89ddf96a3596e8d73b23afa591875a0 Reviewed-on: https://go-review.googlesource.com/c/go/+/382461 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/lookup.go | 45 ++++++------------- src/cmd/compile/internal/types2/methodlist.go | 12 ++--- src/cmd/compile/internal/types2/named.go | 4 +- src/cmd/compile/internal/types2/typeset.go | 5 +-- src/go/types/lookup.go | 25 ++++++----- src/go/types/methodlist.go | 9 ++-- src/go/types/named.go | 4 +- src/go/types/typeset.go | 5 +-- 8 files changed, 47 insertions(+), 62 deletions(-) diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index a71dd409e1..407b8384df 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -55,7 +55,7 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o // not have found it for T (see also issue 8590). if t, _ := T.(*Named); t != nil { if p, _ := t.Underlying().(*Pointer); p != nil { - obj, index, indirect = lookupFieldOrMethod(p, false, false, pkg, name) + obj, index, indirect = lookupFieldOrMethod(p, false, pkg, name, false) if _, ok := obj.(*Func); ok { return nil, nil, false } @@ -63,7 +63,7 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o } } - obj, index, indirect = lookupFieldOrMethod(T, addressable, false, pkg, name) + obj, index, indirect = lookupFieldOrMethod(T, addressable, pkg, name, false) // If we didn't find anything and if we have a type parameter with a structural constraint, // see if there is a matching field (but not a method, those need to be declared explicitly @@ -71,7 +71,7 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o // are ok here because only fields are accepted as results. if obj == nil && isTypeParam(T) { if t := structuralType(T); t != nil { - obj, index, indirect = lookupFieldOrMethod(t, addressable, false, pkg, name) + obj, index, indirect = lookupFieldOrMethod(t, addressable, pkg, name, false) if _, ok := obj.(*Var); !ok { obj, index, indirect = nil, nil, false // accept fields (variables) only } @@ -86,11 +86,11 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o // indirectly via different packages.) // lookupFieldOrMethod should only be called by LookupFieldOrMethod and missingMethod. -// If checkFold is true, the lookup for methods will include looking for any method +// If foldCase is true, the lookup for methods will include looking for any method // which case-folds to the same as 'name' (used for giving helpful error messages). // // The resulting object may not be fully type-checked. -func lookupFieldOrMethod(T Type, addressable, checkFold bool, pkg *Package, name string) (obj Object, index []int, indirect bool) { +func lookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string, foldCase bool) (obj Object, index []int, indirect bool) { // WARNING: The code in this function is extremely subtle - do not modify casually! if name == "_" { @@ -144,7 +144,7 @@ func lookupFieldOrMethod(T Type, addressable, checkFold bool, pkg *Package, name // look for a matching attached method named.resolve(nil) - if i, m := named.lookupMethodFold(pkg, name, checkFold); m != nil { + if i, m := named.lookupMethod(pkg, name, foldCase); m != nil { // potential match // caution: method may not have a proper signature yet index = concat(e.index, i) @@ -191,7 +191,7 @@ func lookupFieldOrMethod(T Type, addressable, checkFold bool, pkg *Package, name case *Interface: // look for a matching method (interface may be a type parameter) - if i, m := lookupMethodFold(t.typeSet().methods, pkg, name, checkFold); m != nil { + if i, m := t.typeSet().LookupMethod(pkg, name, foldCase); m != nil { assert(m.typ != nil) index = concat(e.index, i) if obj != nil || e.multiples { @@ -308,7 +308,7 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, if ityp, _ := under(V).(*Interface); ityp != nil { // TODO(gri) the methods are sorted - could do this more efficiently for _, m := range T.typeSet().methods { - _, f := ityp.typeSet().LookupMethod(m.pkg, m.name) + _, f := ityp.typeSet().LookupMethod(m.pkg, m.name, false) if f == nil { if !static { @@ -339,17 +339,17 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, // A concrete type implements T if it implements all methods of T. for _, m := range T.typeSet().methods { // TODO(gri) should this be calling LookupFieldOrMethod instead (and why not)? - obj, _, _ := lookupFieldOrMethod(V, false, false, m.pkg, m.name) + obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name, false) // Check if *V implements this method of T. if obj == nil { ptr := NewPointer(V) - obj, _, _ = lookupFieldOrMethod(ptr, false, false, m.pkg, m.name) + obj, _, _ = lookupFieldOrMethod(ptr, false, m.pkg, m.name, false) if obj == nil { // If we didn't find the exact method (even with pointer // receiver), look to see if there is a method that // matches m.name with case-folding. - obj, _, _ = lookupFieldOrMethod(V, false, true, m.pkg, m.name) + obj, _, _ = lookupFieldOrMethod(V, false, m.pkg, m.name, true) } if obj != nil { // methods may not have a fully set up signature yet @@ -513,28 +513,11 @@ func fieldIndex(fields []*Var, pkg *Package, name string) int { } // lookupMethod returns the index of and method with matching package and name, or (-1, nil). -func lookupMethod(methods []*Func, pkg *Package, name string) (int, *Func) { +// If foldCase is true, method names are considered equal if they are equal with case folding. +func lookupMethod(methods []*Func, pkg *Package, name string, foldCase bool) (int, *Func) { if name != "_" { for i, m := range methods { - if m.sameId(pkg, name) { - return i, m - } - } - } - return -1, nil -} - -// lookupMethodFold is like lookupMethod, but if checkFold is true, it matches a method -// name if the names are equal with case folding. -func lookupMethodFold(methods []*Func, pkg *Package, name string, checkFold bool) (int, *Func) { - if name != "_" { - for i, m := range methods { - if m.name != name && !(checkFold && strings.EqualFold(m.name, name)) { - continue - } - // Use m.name, since we've already checked that m.name and - // name are equal with folding. - if m.sameId(pkg, m.name) { + if (m.name == name || foldCase && strings.EqualFold(m.name, name)) && m.sameId(pkg, m.name) { return i, m } } diff --git a/src/cmd/compile/internal/types2/methodlist.go b/src/cmd/compile/internal/types2/methodlist.go index ba10159ea2..cd6c06c5fb 100644 --- a/src/cmd/compile/internal/types2/methodlist.go +++ b/src/cmd/compile/internal/types2/methodlist.go @@ -41,20 +41,20 @@ func (l *methodList) isLazy() bool { // panics if the receiver is lazy. func (l *methodList) Add(m *Func) { assert(!l.isLazy()) - if i, _ := lookupMethod(l.methods, m.pkg, m.name); i < 0 { + if i, _ := lookupMethod(l.methods, m.pkg, m.name, false); i < 0 { l.methods = append(l.methods, m) } } -// LookupFold looks up the method identified by pkg and name in the receiver. -// LookupFold panics if the receiver is lazy. If checkFold is true, it matches -// a method name if the names are equal with case folding. -func (l *methodList) LookupFold(pkg *Package, name string, checkFold bool) (int, *Func) { +// Lookup looks up the method identified by pkg and name in the receiver. +// Lookup panics if the receiver is lazy. If foldCase is true, method names +// are considered equal if they are equal with case folding. +func (l *methodList) Lookup(pkg *Package, name string, foldCase bool) (int, *Func) { assert(!l.isLazy()) if l == nil { return -1, nil } - return lookupMethodFold(l.methods, pkg, name, checkFold) + return lookupMethod(l.methods, pkg, name, foldCase) } // Len returns the length of the method list. diff --git a/src/cmd/compile/internal/types2/named.go b/src/cmd/compile/internal/types2/named.go index 5248893a4a..bb522e8fe3 100644 --- a/src/cmd/compile/internal/types2/named.go +++ b/src/cmd/compile/internal/types2/named.go @@ -297,12 +297,12 @@ func (n *Named) setUnderlying(typ Type) { } } -func (n *Named) lookupMethodFold(pkg *Package, name string, checkFold bool) (int, *Func) { +func (n *Named) lookupMethod(pkg *Package, name string, foldCase bool) (int, *Func) { n.resolve(nil) // If n is an instance, we may not have yet instantiated all of its methods. // Look up the method index in orig, and only instantiate method at the // matching index (if any). - i, _ := n.orig.methods.LookupFold(pkg, name, checkFold) + i, _ := n.orig.methods.Lookup(pkg, name, foldCase) if i < 0 { return -1, nil } diff --git a/src/cmd/compile/internal/types2/typeset.go b/src/cmd/compile/internal/types2/typeset.go index 3884276adc..fff348bcf4 100644 --- a/src/cmd/compile/internal/types2/typeset.go +++ b/src/cmd/compile/internal/types2/typeset.go @@ -58,9 +58,8 @@ func (s *_TypeSet) NumMethods() int { return len(s.methods) } func (s *_TypeSet) Method(i int) *Func { return s.methods[i] } // LookupMethod returns the index of and method with matching package and name, or (-1, nil). -func (s *_TypeSet) LookupMethod(pkg *Package, name string) (int, *Func) { - // TODO(gri) s.methods is sorted - consider binary search - return lookupMethod(s.methods, pkg, name) +func (s *_TypeSet) LookupMethod(pkg *Package, name string, foldCase bool) (int, *Func) { + return lookupMethod(s.methods, pkg, name, foldCase) } func (s *_TypeSet) String() string { diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index bee76ccb55..59cec23035 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -55,7 +55,7 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o // not have found it for T (see also issue 8590). if t, _ := T.(*Named); t != nil { if p, _ := t.Underlying().(*Pointer); p != nil { - obj, index, indirect = lookupFieldOrMethod(p, false, pkg, name) + obj, index, indirect = lookupFieldOrMethod(p, false, pkg, name, false) if _, ok := obj.(*Func); ok { return nil, nil, false } @@ -63,7 +63,7 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o } } - obj, index, indirect = lookupFieldOrMethod(T, addressable, pkg, name) + obj, index, indirect = lookupFieldOrMethod(T, addressable, pkg, name, false) // If we didn't find anything and if we have a type parameter with a structural constraint, // see if there is a matching field (but not a method, those need to be declared explicitly @@ -71,7 +71,7 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o // are ok here because only fields are accepted as results. if obj == nil && isTypeParam(T) { if t := structuralType(T); t != nil { - obj, index, indirect = lookupFieldOrMethod(t, addressable, pkg, name) + obj, index, indirect = lookupFieldOrMethod(t, addressable, pkg, name, false) if _, ok := obj.(*Var); !ok { obj, index, indirect = nil, nil, false // accept fields (variables) only } @@ -86,9 +86,11 @@ func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o // indirectly via different packages.) // lookupFieldOrMethod should only be called by LookupFieldOrMethod and missingMethod. +// If foldCase is true, the lookup for methods will include looking for any method +// which case-folds to the same as 'name' (used for giving helpful error messages). // // The resulting object may not be fully type-checked. -func lookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (obj Object, index []int, indirect bool) { +func lookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string, foldCase bool) (obj Object, index []int, indirect bool) { // WARNING: The code in this function is extremely subtle - do not modify casually! if name == "_" { @@ -142,7 +144,7 @@ func lookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o // look for a matching attached method named.resolve(nil) - if i, m := named.lookupMethod(pkg, name); m != nil { + if i, m := named.lookupMethod(pkg, name, foldCase); m != nil { // potential match // caution: method may not have a proper signature yet index = concat(e.index, i) @@ -189,7 +191,7 @@ func lookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (o case *Interface: // look for a matching method (interface may be a type parameter) - if i, m := t.typeSet().LookupMethod(pkg, name); m != nil { + if i, m := t.typeSet().LookupMethod(pkg, name, foldCase); m != nil { assert(m.typ != nil) index = concat(e.index, i) if obj != nil || e.multiples { @@ -301,7 +303,7 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, if ityp, _ := under(V).(*Interface); ityp != nil { // TODO(gri) the methods are sorted - could do this more efficiently for _, m := range T.typeSet().methods { - _, f := ityp.typeSet().LookupMethod(m.pkg, m.name) + _, f := ityp.typeSet().LookupMethod(m.pkg, m.name, false) if f == nil { if !static { @@ -331,12 +333,12 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, // A concrete type implements T if it implements all methods of T. for _, m := range T.typeSet().methods { // TODO(gri) should this be calling lookupFieldOrMethod instead (and why not)? - obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name) + obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name, false) // Check if *V implements this method of T. if obj == nil { ptr := NewPointer(V) - obj, _, _ = lookupFieldOrMethod(ptr, false, m.pkg, m.name) + obj, _, _ = lookupFieldOrMethod(ptr, false, m.pkg, m.name, false) if obj != nil { // methods may not have a fully set up signature yet @@ -502,10 +504,11 @@ func fieldIndex(fields []*Var, pkg *Package, name string) int { } // lookupMethod returns the index of and method with matching package and name, or (-1, nil). -func lookupMethod(methods []*Func, pkg *Package, name string) (int, *Func) { +// If foldCase is true, method names are considered equal if they are equal with case folding. +func lookupMethod(methods []*Func, pkg *Package, name string, foldCase bool) (int, *Func) { if name != "_" { for i, m := range methods { - if m.sameId(pkg, name) { + if (m.name == name || foldCase && strings.EqualFold(m.name, name)) && m.sameId(pkg, m.name) { return i, m } } diff --git a/src/go/types/methodlist.go b/src/go/types/methodlist.go index 10a2a323a8..afe919013d 100644 --- a/src/go/types/methodlist.go +++ b/src/go/types/methodlist.go @@ -41,19 +41,20 @@ func (l *methodList) isLazy() bool { // panics if the receiver is lazy. func (l *methodList) Add(m *Func) { assert(!l.isLazy()) - if i, _ := lookupMethod(l.methods, m.pkg, m.name); i < 0 { + if i, _ := lookupMethod(l.methods, m.pkg, m.name, false); i < 0 { l.methods = append(l.methods, m) } } // Lookup looks up the method identified by pkg and name in the receiver. -// Lookup panics if the receiver is lazy. -func (l *methodList) Lookup(pkg *Package, name string) (int, *Func) { +// Lookup panics if the receiver is lazy. If foldCase is true, method names +// are considered equal if they are equal with case folding. +func (l *methodList) Lookup(pkg *Package, name string, foldCase bool) (int, *Func) { assert(!l.isLazy()) if l == nil { return -1, nil } - return lookupMethod(l.methods, pkg, name) + return lookupMethod(l.methods, pkg, name, foldCase) } // Len returns the length of the method list. diff --git a/src/go/types/named.go b/src/go/types/named.go index 28db26014f..5e84c39776 100644 --- a/src/go/types/named.go +++ b/src/go/types/named.go @@ -299,12 +299,12 @@ func (n *Named) setUnderlying(typ Type) { } } -func (n *Named) lookupMethod(pkg *Package, name string) (int, *Func) { +func (n *Named) lookupMethod(pkg *Package, name string, foldCase bool) (int, *Func) { n.resolve(nil) // If n is an instance, we may not have yet instantiated all of its methods. // Look up the method index in orig, and only instantiate method at the // matching index (if any). - i, _ := n.orig.methods.Lookup(pkg, name) + i, _ := n.orig.methods.Lookup(pkg, name, foldCase) if i < 0 { return -1, nil } diff --git a/src/go/types/typeset.go b/src/go/types/typeset.go index 9f4831e976..e1f73015b9 100644 --- a/src/go/types/typeset.go +++ b/src/go/types/typeset.go @@ -56,9 +56,8 @@ func (s *_TypeSet) NumMethods() int { return len(s.methods) } func (s *_TypeSet) Method(i int) *Func { return s.methods[i] } // LookupMethod returns the index of and method with matching package and name, or (-1, nil). -func (s *_TypeSet) LookupMethod(pkg *Package, name string) (int, *Func) { - // TODO(gri) s.methods is sorted - consider binary search - return lookupMethod(s.methods, pkg, name) +func (s *_TypeSet) LookupMethod(pkg *Package, name string, foldCase bool) (int, *Func) { + return lookupMethod(s.methods, pkg, name, foldCase) } func (s *_TypeSet) String() string { From 727790a2fda02e75844ce91320c05fd2bf1f431d Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 2 Feb 2022 15:04:58 -0800 Subject: [PATCH 25/42] go/types, types2: remove superflous type parameter length checks There is no need to check for length equality of type parameter lists in Checker.missingMethod: the Identical predicate does this check. Furthermore, we don't have methods with their own type parameters. Remove the unnecessary (duplicate) code. Also, update doc string on missingMethod and rename the 2nd result parameter for clarity, and clarify internal comments. For go/types, include the same case-folding code as for types2 but leave it disabled or now. Adjust any other differences in the missingMethod implementation. With this change, the types2 and go/types code of missingMethod is identical again except for the disabled case-folding lookup. No functionality changes. Preparation for fixing some of the outstanding error reporting issues. Change-Id: I4778d006c17f4e084ecc2cac7386c68e86aa49eb Reviewed-on: https://go-review.googlesource.com/c/go/+/382614 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/lookup.go | 54 ++++++---------------- src/go/types/lookup.go | 55 +++++++++-------------- 2 files changed, 33 insertions(+), 76 deletions(-) diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index 407b8384df..80f085803e 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -285,28 +285,21 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b return m, typ != nil } -// If we accept type parameters for methods, (at least) the code -// guarded with this constant will need to be adjusted when such -// methods are used (not just parsed). -const acceptMethodTypeParams = false - -// missingMethod is like MissingMethod but accepts a *Checker as -// receiver and an addressable flag. -// The receiver may be nil if missingMethod is invoked through -// an exported API call (such as MissingMethod), i.e., when all -// methods have been type-checked. -// If the type has the correctly named method, but with the wrong -// signature, the existing method is returned as well. -// To improve error messages, also report the wrong signature -// when the method exists on *V instead of V. -func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, wrongType *Func) { +// missingMethod is like MissingMethod but accepts a *Checker as receiver. +// The receiver may be nil if missingMethod is invoked through an exported +// API call (such as MissingMethod), i.e., when all methods have been type- +// checked. +// +// If a method is missing on T but is found on *T, or if a method is found +// on T when looked up with case-folding, this alternative method is returned +// as the second result. +func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, alt *Func) { // fast path for common case if T.Empty() { return } if ityp, _ := under(V).(*Interface); ityp != nil { - // TODO(gri) the methods are sorted - could do this more efficiently for _, m := range T.typeSet().methods { _, f := ityp.typeSet().LookupMethod(m.pkg, m.name, false) @@ -318,17 +311,7 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, return m, f } - // both methods must have the same number of type parameters - ftyp := f.typ.(*Signature) - mtyp := m.typ.(*Signature) - if ftyp.TypeParams().Len() != mtyp.TypeParams().Len() { - return m, f - } - if !acceptMethodTypeParams && ftyp.TypeParams().Len() > 0 { - panic("method with type parameters") - } - - if !Identical(ftyp, mtyp) { + if !Identical(f.typ, m.typ) { return m, f } } @@ -346,9 +329,8 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, ptr := NewPointer(V) obj, _, _ = lookupFieldOrMethod(ptr, false, m.pkg, m.name, false) if obj == nil { - // If we didn't find the exact method (even with pointer - // receiver), look to see if there is a method that - // matches m.name with case-folding. + // If we didn't find the exact method (even with pointer receiver), + // check if there is a matching method using case-folding. obj, _, _ = lookupFieldOrMethod(V, false, m.pkg, m.name, true) } if obj != nil { @@ -371,17 +353,7 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, check.objDecl(f, nil) } - // both methods must have the same number of type parameters - ftyp := f.typ.(*Signature) - mtyp := m.typ.(*Signature) - if ftyp.TypeParams().Len() != mtyp.TypeParams().Len() { - return m, f - } - if !acceptMethodTypeParams && ftyp.TypeParams().Len() > 0 { - panic("method with type parameters") - } - - if !Identical(ftyp, mtyp) { + if !Identical(f.typ, m.typ) { return m, f } } diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index 59cec23035..b08308088c 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -285,23 +285,22 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b return m, typ != nil } -// missingMethod is like MissingMethod but accepts a *Checker as -// receiver and an addressable flag. -// The receiver may be nil if missingMethod is invoked through -// an exported API call (such as MissingMethod), i.e., when all -// methods have been type-checked. -// If the type has the correctly named method, but with the wrong -// signature, the existing method is returned as well. -// To improve error messages, also report the wrong signature -// when the method exists on *V instead of V. -func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, wrongType *Func) { +// missingMethod is like MissingMethod but accepts a *Checker as receiver. +// The receiver may be nil if missingMethod is invoked through an exported +// API call (such as MissingMethod), i.e., when all methods have been type- +// checked. +// +// If a method is missing on T but is found on *T, or if a method is found +// on T when looked up with case-folding, this alternative method is returned +// as the second result. +// Note: case-folding lookup is currently disabled +func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, alt *Func) { // fast path for common case if T.Empty() { return } if ityp, _ := under(V).(*Interface); ityp != nil { - // TODO(gri) the methods are sorted - could do this more efficiently for _, m := range T.typeSet().methods { _, f := ityp.typeSet().LookupMethod(m.pkg, m.name, false) @@ -309,20 +308,11 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, if !static { continue } + // We don't do any case-fold check if V is an interface. return m, f } - // both methods must have the same number of type parameters - ftyp := f.typ.(*Signature) - mtyp := m.typ.(*Signature) - if ftyp.TypeParams().Len() != mtyp.TypeParams().Len() { - return m, f - } - if ftyp.TypeParams().Len() > 0 { - panic("method with type parameters") - } - - if !Identical(ftyp, mtyp) { + if !Identical(f.typ, m.typ) { return m, f } } @@ -332,14 +322,19 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, // A concrete type implements T if it implements all methods of T. for _, m := range T.typeSet().methods { - // TODO(gri) should this be calling lookupFieldOrMethod instead (and why not)? + // TODO(gri) should this be calling LookupFieldOrMethod instead (and why not)? obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name, false) // Check if *V implements this method of T. if obj == nil { ptr := NewPointer(V) obj, _, _ = lookupFieldOrMethod(ptr, false, m.pkg, m.name, false) - + if obj == nil { + // TODO(gri) enable this code + // If we didn't find the exact method (even with pointer receiver), + // check if there is a matching method using case-folding. + // obj, _, _ = lookupFieldOrMethod(V, false, m.pkg, m.name, true) + } if obj != nil { // methods may not have a fully set up signature yet if check != nil { @@ -360,17 +355,7 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, check.objDecl(f, nil) } - // both methods must have the same number of type parameters - ftyp := f.typ.(*Signature) - mtyp := m.typ.(*Signature) - if ftyp.TypeParams().Len() != mtyp.TypeParams().Len() { - return m, f - } - if ftyp.TypeParams().Len() > 0 { - panic("method with type parameters") - } - - if !Identical(ftyp, mtyp) { + if !Identical(f.typ, m.typ) { return m, f } } From d9eba71a643f31e509dd08884509c5b2c1ab26a4 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 2 Feb 2022 16:31:26 -0800 Subject: [PATCH 26/42] go/types, types2: fix bug in types2.MissingMethod Because Checker.missingMethod also looks up methods matching matching case-folded names, when Checker.missingMethod returns an alternative method, that method does not automatically have the wrong type. It may be a method with a different name. Adjust types2.MissingMethod to check the alternative method name before reporting a wrong type. Add API test that verifies (now correct) behavior for this case. Ported the code also to go/types, though it was not a bug there yet because looking up with case-folding is not yet enabled. Change-Id: Iaa48808535c9265a9879338ea666c6c021e93a2b Reviewed-on: https://go-review.googlesource.com/c/go/+/382634 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/api_test.go | 58 +++++++++++++++++++++ src/cmd/compile/internal/types2/lookup.go | 5 +- src/go/types/api_test.go | 58 +++++++++++++++++++++ src/go/types/lookup.go | 5 +- 4 files changed, 122 insertions(+), 4 deletions(-) diff --git a/src/cmd/compile/internal/types2/api_test.go b/src/cmd/compile/internal/types2/api_test.go index 80e998ebee..094374f7f1 100644 --- a/src/cmd/compile/internal/types2/api_test.go +++ b/src/cmd/compile/internal/types2/api_test.go @@ -2369,3 +2369,61 @@ type Bad Bad // invalid type } } } + +func TestMissingMethodAlternative(t *testing.T) { + const src = ` +package p +type T interface { + m() +} + +type V0 struct{} +func (V0) m() {} + +type V1 struct{} + +type V2 struct{} +func (V2) m() int + +type V3 struct{} +func (*V3) m() + +type V4 struct{} +func (V4) M() +` + + pkg, err := pkgFor("p.go", src, nil) + if err != nil { + t.Fatal(err) + } + + T := pkg.Scope().Lookup("T").Type().Underlying().(*Interface) + lookup := func(name string) (*Func, bool) { + return MissingMethod(pkg.Scope().Lookup(name).Type(), T, true) + } + + // V0 has method m with correct signature. Should not report wrongType. + method, wrongType := lookup("V0") + if method != nil || wrongType { + t.Fatalf("V0: got method = %v, wrongType = %v", method, wrongType) + } + + checkMissingMethod := func(tname string, reportWrongType bool) { + method, wrongType := lookup(tname) + if method == nil || method.Name() != "m" || wrongType != reportWrongType { + t.Fatalf("%s: got method = %v, wrongType = %v", tname, method, wrongType) + } + } + + // V1 has no method m. Should not report wrongType. + checkMissingMethod("V1", false) + + // V2 has method m with wrong signature type (ignoring receiver). Should report wrongType. + checkMissingMethod("V2", true) + + // V3 has no method m but it exists on *V3. Should report wrongType. + checkMissingMethod("V3", true) + + // V4 has no method m but has M. Should not report wrongType. + checkMissingMethod("V4", false) +} diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index 80f085803e..fc6b34941a 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -281,8 +281,9 @@ func lookupType(m map[Type]int, typ Type) (int, bool) { // x is of interface type V). // func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType bool) { - m, typ := (*Checker)(nil).missingMethod(V, T, static) - return m, typ != nil + m, alt := (*Checker)(nil).missingMethod(V, T, static) + // Only report a wrong type if the alternative method has the same name as m. + return m, alt != nil && alt.name == m.name // alt != nil implies m != nil } // missingMethod is like MissingMethod but accepts a *Checker as receiver. diff --git a/src/go/types/api_test.go b/src/go/types/api_test.go index 5f4d48472c..a18ee16c7b 100644 --- a/src/go/types/api_test.go +++ b/src/go/types/api_test.go @@ -2362,3 +2362,61 @@ type Bad Bad // invalid type } } } + +func TestMissingMethodAlternative(t *testing.T) { + const src = ` +package p +type T interface { + m() +} + +type V0 struct{} +func (V0) m() {} + +type V1 struct{} + +type V2 struct{} +func (V2) m() int + +type V3 struct{} +func (*V3) m() + +type V4 struct{} +func (V4) M() +` + + pkg, err := pkgFor("p.go", src, nil) + if err != nil { + t.Fatal(err) + } + + T := pkg.Scope().Lookup("T").Type().Underlying().(*Interface) + lookup := func(name string) (*Func, bool) { + return MissingMethod(pkg.Scope().Lookup(name).Type(), T, true) + } + + // V0 has method m with correct signature. Should not report wrongType. + method, wrongType := lookup("V0") + if method != nil || wrongType { + t.Fatalf("V0: got method = %v, wrongType = %v", method, wrongType) + } + + checkMissingMethod := func(tname string, reportWrongType bool) { + method, wrongType := lookup(tname) + if method == nil || method.Name() != "m" || wrongType != reportWrongType { + t.Fatalf("%s: got method = %v, wrongType = %v", tname, method, wrongType) + } + } + + // V1 has no method m. Should not report wrongType. + checkMissingMethod("V1", false) + + // V2 has method m with wrong signature type (ignoring receiver). Should report wrongType. + checkMissingMethod("V2", true) + + // V3 has no method m but it exists on *V3. Should report wrongType. + checkMissingMethod("V3", true) + + // V4 has no method m but has M. Should not report wrongType. + checkMissingMethod("V4", false) +} diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index b08308088c..77e8fe9df5 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -281,8 +281,9 @@ func lookupType(m map[Type]int, typ Type) (int, bool) { // x is of interface type V). // func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType bool) { - m, typ := (*Checker)(nil).missingMethod(V, T, static) - return m, typ != nil + m, alt := (*Checker)(nil).missingMethod(V, T, static) + // Only report a wrong type if the alternative method has the same name as m. + return m, alt != nil && alt.name == m.name // alt != nil implies m != nil } // missingMethod is like MissingMethod but accepts a *Checker as receiver. From ef843ae49c8a8ad6494da294fc46b0e80147e715 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 2 Feb 2022 21:36:50 -0800 Subject: [PATCH 27/42] go/types, types2: simplify missingMethodReason Added a funcString helper so we don't need to rewrite strings with strings.Replace. Use compiler format for error message about wrong method type; this removes another unnecessary variation. Simplify conditions for pointer-to-interface related error: if one of the involved types is an interface pointer, it can't have any methods. Rewrite logic so we don't need all the else-if branches. Adjusted a test case for types2 accordingly. The go/types version of this test case has a different error because the implementation of Checker.typeAssertion is different in the two type checkers (the types2 version gives errors closer to the 1.17 compiler). Change-Id: I19e604d7063c3e31e8290bd78384a9f38bb0d740 Reviewed-on: https://go-review.googlesource.com/c/go/+/382694 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/lookup.go | 60 ++++++++--------- .../internal/types2/testdata/check/issues.src | 2 +- src/go/types/lookup.go | 64 +++++++++---------- 3 files changed, 62 insertions(+), 64 deletions(-) diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index fc6b34941a..1aeb2beaa0 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -7,6 +7,7 @@ package types2 import ( + "bytes" "strings" ) @@ -364,48 +365,40 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, // missingMethodReason returns a string giving the detailed reason for a missing method m, // where m is missing from V, but required by T. It puts the reason in parentheses, -// and may include more have/want info after that. If non-nil, wrongType is a relevant +// and may include more have/want info after that. If non-nil, alt is a relevant // method that matches in some way. It may have the correct name, but wrong type, or // it may have a pointer receiver, or it may have the correct name except wrong case. -func (check *Checker) missingMethodReason(V, T Type, m, wrongType *Func) string { - var r string +func (check *Checker) missingMethodReason(V, T Type, m, alt *Func) string { var mname string if check.conf.CompilerErrorMessages { mname = m.Name() + " method" } else { mname = "method " + m.Name() } - if wrongType != nil { - if m.Name() != wrongType.Name() { - r = check.sprintf("(missing %s)\n\t\thave %s^^%s\n\t\twant %s^^%s", - mname, wrongType.Name(), wrongType.typ, m.Name(), m.typ) - } else if Identical(m.typ, wrongType.typ) { - r = check.sprintf("(%s has pointer receiver)", mname) - } else { - if check.conf.CompilerErrorMessages { - r = check.sprintf("(wrong type for %s)\n\t\thave %s^^%s\n\t\twant %s^^%s", - mname, wrongType.Name(), wrongType.typ, m.Name(), m.typ) - } else { - r = check.sprintf("(wrong type for %s)\n\thave %s\n\twant %s", - mname, wrongType.typ, m.typ) - } + + if alt != nil { + if m.Name() != alt.Name() { + return check.sprintf("(missing %s)\n\t\thave %s\n\t\twant %s", + mname, check.funcString(alt), check.funcString(m)) } - // This is a hack to print the function type without the leading - // 'func' keyword in the have/want printouts. We could change to have - // an extra formatting option for types2.Type that doesn't print out - // 'func'. - r = strings.Replace(r, "^^func", "", -1) - } else if IsInterface(T) { - if isInterfacePtr(V) { - r = "(" + check.interfacePtrError(V) + ")" + + if Identical(m.typ, alt.typ) { + return check.sprintf("(%s has pointer receiver)", mname) } - } else if isInterfacePtr(T) { - r = "(" + check.interfacePtrError(T) + ")" + + return check.sprintf("(wrong type for %s)\n\t\thave %s\n\t\twant %s", + mname, check.funcString(alt), check.funcString(m)) } - if r == "" { - r = check.sprintf("(missing %s)", mname) + + if isInterfacePtr(V) { + return "(" + check.interfacePtrError(V) + ")" } - return r + + if isInterfacePtr(T) { + return "(" + check.interfacePtrError(T) + ")" + } + + return check.sprintf("(missing %s)", mname) } func isInterfacePtr(T Type) bool { @@ -421,6 +414,13 @@ func (check *Checker) interfacePtrError(T Type) string { return check.sprintf("type %s is pointer to interface, not interface", T) } +// funcString returns a string of the form name + signature for f. +func (check *Checker) funcString(f *Func) string { + buf := bytes.NewBufferString(f.name) + WriteSignature(buf, f.typ.(*Signature), check.qualifier) + return buf.String() +} + // assertableTo reports whether a value of type V can be asserted to have type T. // It returns (nil, false) as affirmative answer. Otherwise it returns a missing // method required by V and whether it is missing or just has the wrong type. diff --git a/src/cmd/compile/internal/types2/testdata/check/issues.src b/src/cmd/compile/internal/types2/testdata/check/issues.src index a19f99b31a..4c49147922 100644 --- a/src/cmd/compile/internal/types2/testdata/check/issues.src +++ b/src/cmd/compile/internal/types2/testdata/check/issues.src @@ -137,7 +137,7 @@ func issue10260() { T1{}.foo /* ERROR cannot call pointer method foo on T1 */ () x.Foo /* ERROR "x.Foo undefined \(type I1 has no field or method Foo, but does have foo\)" */ () - _ = i2. /* ERROR impossible type assertion: i2.\(\*T1\)\n\t\*T1 does not implement I2 \(wrong type for method foo\)\n\thave func\(\)\n\twant func\(x int\) */ (*T1) + _ = i2. /* ERROR impossible type assertion: i2\.\(\*T1\)\n\t\*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ (*T1) i1 = i0 /* ERROR cannot use .* missing method foo */ i1 = t0 /* ERROR cannot use .* missing method foo */ diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index 77e8fe9df5..1b4f953803 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -7,6 +7,7 @@ package types import ( + "bytes" "strings" ) @@ -366,50 +367,40 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, // missingMethodReason returns a string giving the detailed reason for a missing method m, // where m is missing from V, but required by T. It puts the reason in parentheses, -// and may include more have/want info after that. If non-nil, wrongType is a relevant +// and may include more have/want info after that. If non-nil, alt is a relevant // method that matches in some way. It may have the correct name, but wrong type, or -// it may have a pointer receiver. -func (check *Checker) missingMethodReason(V, T Type, m, wrongType *Func) string { - var r string +// it may have a pointer receiver, or it may have the correct name except wrong case. +func (check *Checker) missingMethodReason(V, T Type, m, alt *Func) string { var mname string if compilerErrorMessages { mname = m.Name() + " method" } else { mname = "method " + m.Name() } - if wrongType != nil { - if m.Name() != wrongType.Name() { - // Note: this case can't happen because we don't look for alternative - // method spellings, unlike types2. Keep for symmetry with types2. - r = check.sprintf("(missing %s)\n\t\thave %s^^%s\n\t\twant %s^^%s", - mname, wrongType.Name(), wrongType.typ, m.Name(), m.typ) - } else if Identical(m.typ, wrongType.typ) { - r = check.sprintf("(%s has pointer receiver)", mname) - } else { - if compilerErrorMessages { - r = check.sprintf("(wrong type for %s)\n\t\thave %s^^%s\n\t\twant %s^^%s", - mname, wrongType.Name(), wrongType.typ, m.Name(), m.typ) - } else { - r = check.sprintf("(wrong type for %s)\n\thave %s\n\twant %s", - mname, wrongType.typ, m.typ) - } + + if alt != nil { + if m.Name() != alt.Name() { + return check.sprintf("(missing %s)\n\t\thave %s\n\t\twant %s", + mname, check.funcString(alt), check.funcString(m)) } - // This is a hack to print the function type without the leading - // 'func' keyword in the have/want printouts. We could change to have - // an extra formatting option for types2.Type that doesn't print out - // 'func'. - r = strings.Replace(r, "^^func", "", -1) - } else if IsInterface(T) { - if isInterfacePtr(V) { - r = "(" + check.interfacePtrError(V) + ")" + + if Identical(m.typ, alt.typ) { + return check.sprintf("(%s has pointer receiver)", mname) } - } else if isInterfacePtr(T) { - r = "(" + check.interfacePtrError(T) + ")" + + return check.sprintf("(wrong type for %s)\n\t\thave %s\n\t\twant %s", + mname, check.funcString(alt), check.funcString(m)) } - if r == "" { - r = check.sprintf("(missing %s)", mname) + + if isInterfacePtr(V) { + return "(" + check.interfacePtrError(V) + ")" } - return r + + if isInterfacePtr(T) { + return "(" + check.interfacePtrError(T) + ")" + } + + return check.sprintf("(missing %s)", mname) } func isInterfacePtr(T Type) bool { @@ -425,6 +416,13 @@ func (check *Checker) interfacePtrError(T Type) string { return check.sprintf("type %s is pointer to interface, not interface", T) } +// funcString returns a string of the form name + signature for f. +func (check *Checker) funcString(f *Func) string { + buf := bytes.NewBufferString(f.name) + WriteSignature(buf, f.typ.(*Signature), check.qualifier) + return buf.String() +} + // assertableTo reports whether a value of type V can be asserted to have type T. // It returns (nil, false) as affirmative answer. Otherwise it returns a missing // method required by V and whether it is missing or just has the wrong type. From f1903fd4ecbf7a1e524bf71ddecb8650b9d2ea9f Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 3 Feb 2022 15:03:02 -0800 Subject: [PATCH 28/42] go/types, types2: simplify Checker.typeAssertion, use same code in both type checkers - Remove the xtyp argument from the Checker.typeAssertion parameter list; it was confusing and not needed. Adjusted call sites. - Simplify logic in Checker.typeAssertion. - Use the same code in both types2 and go/types, specifically use the same error positions. - Adjust error messages as needed. This removes another subtle discrepancy between types2 and go/types. The go/types error messages don't have the have/want appendix for the affected error messages yet because we don't use case folding in lookups yet. Change-Id: Id39f5c473da36c9baad60082f85cf1f34dc26c50 Reviewed-on: https://go-review.googlesource.com/c/go/+/383014 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/expr.go | 34 +++++++------------ src/cmd/compile/internal/types2/stmt.go | 16 +++++---- .../internal/types2/testdata/check/issues.src | 4 +-- .../types2/testdata/fixedbugs/issue49005.go | 5 +-- .../types2/testdata/fixedbugs/issue50816.go2 | 4 +-- src/go/types/expr.go | 31 ++++++++--------- src/go/types/stmt.go | 16 +++++---- src/go/types/testdata/check/issues.src | 4 +-- src/go/types/testdata/fixedbugs/issue49005.go | 31 +++++++++++++++++ .../types/testdata/fixedbugs/issue50816.go2 | 4 +-- 10 files changed, 85 insertions(+), 64 deletions(-) create mode 100644 src/go/types/testdata/fixedbugs/issue49005.go diff --git a/src/cmd/compile/internal/types2/expr.go b/src/cmd/compile/internal/types2/expr.go index f1696bbe51..4fdabe754e 100644 --- a/src/cmd/compile/internal/types2/expr.go +++ b/src/cmd/compile/internal/types2/expr.go @@ -1578,8 +1578,7 @@ func (check *Checker) exprInternal(x *operand, e syntax.Expr, hint Type) exprKin check.errorf(x, invalidOp+"cannot use type assertion on type parameter value %s", x) goto Error } - xtyp, _ := under(x.typ).(*Interface) - if xtyp == nil { + if _, ok := under(x.typ).(*Interface); !ok { check.errorf(x, invalidOp+"%s is not an interface", x) goto Error } @@ -1592,7 +1591,7 @@ func (check *Checker) exprInternal(x *operand, e syntax.Expr, hint Type) exprKin if T == Typ[Invalid] { goto Error } - check.typeAssertion(e, x, xtyp, T, false) + check.typeAssertion(e, x, T, false) x.mode = commaok x.typ = T @@ -1733,28 +1732,21 @@ func keyVal(x constant.Value) interface{} { return x } -// typeAssertion checks that x.(T) is legal; xtyp must be the type of x. -func (check *Checker) typeAssertion(e syntax.Expr, x *operand, xtyp *Interface, T Type, typeSwitch bool) { - method, wrongType := check.assertableTo(xtyp, T) +// typeAssertion checks x.(T). The type of x must be an interface. +func (check *Checker) typeAssertion(e syntax.Expr, x *operand, T Type, typeSwitch bool) { + method, alt := check.assertableTo(under(x.typ).(*Interface), T) if method == nil { + return // success + } + + cause := check.missingMethodReason(T, x.typ, method, alt) + + if typeSwitch { + check.errorf(e, "impossible type switch case: %s\n\t%s cannot have dynamic type %s %s", e, x, T, cause) return } - var err error_ - var msg string - if typeSwitch { - err.errorf(e.Pos(), "impossible type switch case: %s", e) - msg = check.sprintf("%s cannot have dynamic type %s %s", x, T, - check.missingMethodReason(T, x.typ, method, wrongType)) - - } else { - err.errorf(e.Pos(), "impossible type assertion: %s", e) - msg = check.sprintf("%s does not implement %s %s", T, x.typ, - check.missingMethodReason(T, x.typ, method, wrongType)) - - } - err.errorf(nopos, msg) - check.report(&err) + check.errorf(e, "impossible type assertion: %s\n\t%s does not implement %s %s", e, T, x.typ, cause) } // expr typechecks expression e and initializes x with the expression value. diff --git a/src/cmd/compile/internal/types2/stmt.go b/src/cmd/compile/internal/types2/stmt.go index 633ee31551..03da98af34 100644 --- a/src/cmd/compile/internal/types2/stmt.go +++ b/src/cmd/compile/internal/types2/stmt.go @@ -274,7 +274,8 @@ func (check *Checker) isNil(e syntax.Expr) bool { return false } -func (check *Checker) caseTypes(x *operand, xtyp *Interface, types []syntax.Expr, seen map[Type]syntax.Expr) (T Type) { +// If the type switch expression is invalid, x is nil. +func (check *Checker) caseTypes(x *operand, types []syntax.Expr, seen map[Type]syntax.Expr) (T Type) { var dummy operand L: for _, e := range types { @@ -305,8 +306,8 @@ L: } } seen[T] = e - if T != nil && xtyp != nil { - check.typeAssertion(e, x, xtyp, T, true) + if x != nil && T != nil { + check.typeAssertion(e, x, T, true) } } return @@ -733,12 +734,13 @@ func (check *Checker) typeSwitchStmt(inner stmtContext, s *syntax.SwitchStmt, gu } // TODO(gri) we may want to permit type switches on type parameter values at some point - var xtyp *Interface + var sx *operand // switch expression against which cases are compared against; nil if invalid if isTypeParam(x.typ) { check.errorf(&x, "cannot use type switch on type parameter value %s", &x) } else { - xtyp, _ = under(x.typ).(*Interface) - if xtyp == nil { + if _, ok := under(x.typ).(*Interface); ok { + sx = &x + } else { check.errorf(&x, "%s is not an interface", &x) } } @@ -758,7 +760,7 @@ func (check *Checker) typeSwitchStmt(inner stmtContext, s *syntax.SwitchStmt, gu } // Check each type in this type switch case. cases := unpackExpr(clause.Cases) - T := check.caseTypes(&x, xtyp, cases, seen) + T := check.caseTypes(sx, cases, seen) check.openScopeUntil(clause, end, "case") // If lhs exists, declare a corresponding variable in the case-local scope. if lhs != nil { diff --git a/src/cmd/compile/internal/types2/testdata/check/issues.src b/src/cmd/compile/internal/types2/testdata/check/issues.src index 4c49147922..3b27e03585 100644 --- a/src/cmd/compile/internal/types2/testdata/check/issues.src +++ b/src/cmd/compile/internal/types2/testdata/check/issues.src @@ -132,12 +132,12 @@ func issue10260() { var x I1 x = T1 /* ERROR cannot use .*: missing method foo \(foo has pointer receiver\) */ {} - _ = x. /* ERROR impossible type assertion: x.\(T1\)\n\tT1 does not implement I1 \(method foo has pointer receiver\) */ (T1) + _ = x /* ERROR impossible type assertion: x\.\(T1\)\n\tT1 does not implement I1 \(method foo has pointer receiver\) */ .(T1) T1{}.foo /* ERROR cannot call pointer method foo on T1 */ () x.Foo /* ERROR "x.Foo undefined \(type I1 has no field or method Foo, but does have foo\)" */ () - _ = i2. /* ERROR impossible type assertion: i2\.\(\*T1\)\n\t\*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ (*T1) + _ = i2 /* ERROR impossible type assertion: i2\.\(\*T1\)\n\t\*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ .(*T1) i1 = i0 /* ERROR cannot use .* missing method foo */ i1 = t0 /* ERROR cannot use .* missing method foo */ diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49005.go b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49005.go index f152e7f55c..7083dc9eef 100644 --- a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49005.go +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49005.go @@ -2,9 +2,6 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// This file is tested when running "go test -run Manual" -// without source arguments. Use for one-off debugging. - package p type T1 interface{ M() } @@ -23,7 +20,7 @@ type T2 interface{ M() } func F2() T2 -var _ = F2(). /* ERROR impossible type assertion: F2\(\).\(\*X2\)\n\t\*X2 does not implement T2 \(missing method M\) */ (*X2) +var _ = F2 /* ERROR impossible type assertion: F2\(\)\.\(\*X2\)\n\t\*X2 does not implement T2 \(missing method M\) */ ().(*X2) type X2 struct{} diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50816.go2 b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50816.go2 index b2bcb45248..e7e31d9192 100644 --- a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50816.go2 +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue50816.go2 @@ -18,6 +18,6 @@ func (T2) foo() string { return "" } func _() { var i I - _ = i./* ERROR impossible type assertion: i.\(T1\)\n\tT1 does not implement I \(missing method Foo\)\n\t\thave foo\(\)\n\t\twant Foo\(\) */ (T1) - _ = i./* ERROR impossible type assertion: i.\(T2\)\n\tT2 does not implement I \(missing method Foo\)\n\t\thave foo\(\) string\n\t\twant Foo\(\) */ (T2) + _ = i /* ERROR impossible type assertion: i\.\(T1\)\n\tT1 does not implement I \(missing method Foo\)\n\t\thave foo\(\)\n\t\twant Foo\(\) */ .(T1) + _ = i /* ERROR impossible type assertion: i\.\(T2\)\n\tT2 does not implement I \(missing method Foo\)\n\t\thave foo\(\) string\n\t\twant Foo\(\) */ .(T2) } diff --git a/src/go/types/expr.go b/src/go/types/expr.go index 88a8901b07..0d21a592f9 100644 --- a/src/go/types/expr.go +++ b/src/go/types/expr.go @@ -1556,8 +1556,7 @@ func (check *Checker) exprInternal(x *operand, e ast.Expr, hint Type) exprKind { check.invalidOp(x, _InvalidAssert, "cannot use type assertion on type parameter value %s", x) goto Error } - xtyp, _ := under(x.typ).(*Interface) - if xtyp == nil { + if _, ok := under(x.typ).(*Interface); !ok { check.invalidOp(x, _InvalidAssert, "%s is not an interface", x) goto Error } @@ -1572,7 +1571,7 @@ func (check *Checker) exprInternal(x *operand, e ast.Expr, hint Type) exprKind { if T == Typ[Invalid] { goto Error } - check.typeAssertion(x, x, xtyp, T) + check.typeAssertion(e, x, T, false) x.mode = commaok x.typ = T @@ -1676,23 +1675,21 @@ func keyVal(x constant.Value) any { return x } -// typeAssertion checks that x.(T) is legal; xtyp must be the type of x. -func (check *Checker) typeAssertion(at positioner, x *operand, xtyp *Interface, T Type) { - method, wrongType := check.assertableTo(xtyp, T) +// typeAssertion checks x.(T). The type of x must be an interface. +func (check *Checker) typeAssertion(e ast.Expr, x *operand, T Type, typeSwitch bool) { + method, alt := check.assertableTo(under(x.typ).(*Interface), T) if method == nil { + return // success + } + + cause := check.missingMethodReason(T, x.typ, method, alt) + + if typeSwitch { + check.errorf(e, _ImpossibleAssert, "impossible type switch case: %s\n\t%s cannot have dynamic type %s %s", e, x, T, cause) return } - var msg string - if wrongType != nil { - if Identical(method.typ, wrongType.typ) { - msg = fmt.Sprintf("missing method %s (%s has pointer receiver)", method.name, method.name) - } else { - msg = fmt.Sprintf("wrong type for method %s (have %s, want %s)", method.name, wrongType.typ, method.typ) - } - } else { - msg = "missing method " + method.name - } - check.errorf(at, _ImpossibleAssert, "%s cannot have dynamic type %s (%s)", x, T, msg) + + check.errorf(e, _ImpossibleAssert, "impossible type assertion: %s\n\t%s does not implement %s %s", e, T, x.typ, cause) } // expr typechecks expression e and initializes x with the expression value. diff --git a/src/go/types/stmt.go b/src/go/types/stmt.go index 5ceae08daa..b32eb18bef 100644 --- a/src/go/types/stmt.go +++ b/src/go/types/stmt.go @@ -281,7 +281,8 @@ func (check *Checker) isNil(e ast.Expr) bool { return false } -func (check *Checker) caseTypes(x *operand, xtyp *Interface, types []ast.Expr, seen map[Type]ast.Expr) (T Type) { +// If the type switch expression is invalid, x is nil. +func (check *Checker) caseTypes(x *operand, types []ast.Expr, seen map[Type]ast.Expr) (T Type) { var dummy operand L: for _, e := range types { @@ -310,8 +311,8 @@ L: } } seen[T] = e - if T != nil && xtyp != nil { - check.typeAssertion(e, x, xtyp, T) + if x != nil && T != nil { + check.typeAssertion(e, x, T, true) } } return @@ -684,12 +685,13 @@ func (check *Checker) stmt(ctxt stmtContext, s ast.Stmt) { return } // TODO(gri) we may want to permit type switches on type parameter values at some point - var xtyp *Interface + var sx *operand // switch expression against which cases are compared against; nil if invalid if isTypeParam(x.typ) { check.errorf(&x, _InvalidTypeSwitch, "cannot use type switch on type parameter value %s", &x) } else { - xtyp, _ = under(x.typ).(*Interface) - if xtyp == nil { + if _, ok := under(x.typ).(*Interface); ok { + sx = &x + } else { check.errorf(&x, _InvalidTypeSwitch, "%s is not an interface", &x) } } @@ -705,7 +707,7 @@ func (check *Checker) stmt(ctxt stmtContext, s ast.Stmt) { continue } // Check each type in this type switch case. - T := check.caseTypes(&x, xtyp, clause.List, seen) + T := check.caseTypes(sx, clause.List, seen) check.openScope(clause, "case") // If lhs exists, declare a corresponding variable in the case-local scope. if lhs != nil { diff --git a/src/go/types/testdata/check/issues.src b/src/go/types/testdata/check/issues.src index 0b77b0e854..ce27ac3cfb 100644 --- a/src/go/types/testdata/check/issues.src +++ b/src/go/types/testdata/check/issues.src @@ -132,12 +132,12 @@ func issue10260() { var x I1 x = T1 /* ERROR cannot use .*: missing method foo \(foo has pointer receiver\) */ {} - _ = x /* ERROR .* cannot have dynamic type T1 \(missing method foo \(foo has pointer receiver\)\) */ .(T1) + _ = x /* ERROR impossible type assertion: x\.\(T1\)\n\tT1 does not implement I1 \(method foo has pointer receiver\) */ .(T1) T1{}.foo /* ERROR cannot call pointer method foo on T1 */ () x.Foo /* ERROR "x.Foo undefined \(type I1 has no field or method Foo, but does have foo\)" */ () - _ = i2 /* ERROR i2 .* cannot have dynamic type \*T1 \(wrong type for method foo \(have func\(\), want func\(x int\)\)\) */ .(*T1) + _ = i2 /* ERROR impossible type assertion: i2\.\(\*T1\)\n\t\*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ .(*T1) i1 = i0 /* ERROR cannot use .* missing method foo */ i1 = t0 /* ERROR cannot use .* missing method foo */ diff --git a/src/go/types/testdata/fixedbugs/issue49005.go b/src/go/types/testdata/fixedbugs/issue49005.go new file mode 100644 index 0000000000..7083dc9eef --- /dev/null +++ b/src/go/types/testdata/fixedbugs/issue49005.go @@ -0,0 +1,31 @@ +// Copyright 2021 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 + +type T1 interface{ M() } + +func F1() T1 + +var _ = F1().(*X1 /* ERROR undeclared name: X1 */) + +func _() { + switch F1().(type) { + case *X1 /* ERROR undeclared name: X1 */ : + } +} + +type T2 interface{ M() } + +func F2() T2 + +var _ = F2 /* ERROR impossible type assertion: F2\(\)\.\(\*X2\)\n\t\*X2 does not implement T2 \(missing method M\) */ ().(*X2) + +type X2 struct{} + +func _() { + switch F2().(type) { + case * /* ERROR impossible type switch case: \*X2\n\tF2\(\) \(value of type T2\) cannot have dynamic type \*X2 \(missing method M\) */ X2: + } +} diff --git a/src/go/types/testdata/fixedbugs/issue50816.go2 b/src/go/types/testdata/fixedbugs/issue50816.go2 index a5eecc551b..025a338184 100644 --- a/src/go/types/testdata/fixedbugs/issue50816.go2 +++ b/src/go/types/testdata/fixedbugs/issue50816.go2 @@ -18,6 +18,6 @@ func (T2) foo() string { return "" } func _() { var i I - _ = i/* ERROR i \(variable of type I\) cannot have dynamic type T1 \(missing method Foo\) */.(T1) - _ = i/* ERROR i \(variable of type I\) cannot have dynamic type T2 \(missing method Foo\) */.(T2) + _ = i /* ERROR impossible type assertion: i\.\(T1\)\n\tT1 does not implement I \(missing method Foo\) */ .(T1) + _ = i /* ERROR impossible type assertion: i\.\(T2\)\n\tT2 does not implement I \(missing method Foo\) */ .(T2) } From aadbfc30afe7be3bc8d90e8267e7c9ca2dff95f4 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 3 Feb 2022 16:07:28 -0800 Subject: [PATCH 29/42] go/types, types2: always use missingMethodReason in checker.Implements Remove special case where we don't have a *Checker and always use Checker.missingMethodReason in Checker.implements. Look for zero methods rather than empty interface to exit early from Checker.missingMethod, and remove the extra test in Checker.implements. With this change we get consistent and more detailed error messages from all places where we do a form of the "implements" test. To make this possible, allow for the receiver to be nil in - Checker.sprintf - Checker.missingMethodReason - Checker.interfacePtrError - Checker.funcString Allowing Checker.sprintf with nil Checker permits further simplifying in a couple of places. Change-Id: I0ea7178c9efbcd4a25ded2a66e2b058db52dc4d4 Reviewed-on: https://go-review.googlesource.com/c/go/+/383054 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/errors.go | 7 ++- .../compile/internal/types2/instantiate.go | 29 +++--------- src/cmd/compile/internal/types2/lookup.go | 13 ++++-- .../internal/types2/testdata/check/issues.src | 44 +++++++++---------- .../types2/testdata/fixedbugs/issue49579.go2 | 2 +- src/go/types/errors.go | 9 +++- src/go/types/instantiate.go | 33 +++----------- src/go/types/lookup.go | 14 ++++-- src/go/types/testdata/check/issues.src | 44 +++++++++---------- .../types/testdata/fixedbugs/issue49579.go2 | 2 +- 10 files changed, 90 insertions(+), 107 deletions(-) diff --git a/src/cmd/compile/internal/types2/errors.go b/src/cmd/compile/internal/types2/errors.go index 2318b95f3d..77ae75a0a2 100644 --- a/src/cmd/compile/internal/types2/errors.go +++ b/src/cmd/compile/internal/types2/errors.go @@ -167,8 +167,13 @@ func (check *Checker) markImports(pkg *Package) { } } +// check may be nil. func (check *Checker) sprintf(format string, args ...interface{}) string { - return sprintf(check.qualifier, false, format, args...) + var qf Qualifier + if check != nil { + qf = check.qualifier + } + return sprintf(qf, false, format, args...) } func (check *Checker) report(err *error_) { diff --git a/src/cmd/compile/internal/types2/instantiate.go b/src/cmd/compile/internal/types2/instantiate.go index e0f2d8abe1..90a669f754 100644 --- a/src/cmd/compile/internal/types2/instantiate.go +++ b/src/cmd/compile/internal/types2/instantiate.go @@ -160,21 +160,17 @@ func (check *Checker) implements(V, T Type) error { return nil // avoid follow-on errors (see issue #49541 for an example) } - var qf Qualifier - if check != nil { - qf = check.qualifier - } errorf := func(format string, args ...interface{}) error { - return errors.New(sprintf(qf, false, format, args...)) + return errors.New(check.sprintf(format, args...)) } Ti, _ := Tu.(*Interface) if Ti == nil { var cause string if isInterfacePtr(Tu) { - cause = sprintf(qf, false, "type %s is pointer to interface, not interface", T) + cause = check.sprintf("type %s is pointer to interface, not interface", T) } else { - cause = sprintf(qf, false, "%s is not an interface", T) + cause = check.sprintf("%s is not an interface", T) } return errorf("%s does not implement %s (%s)", V, T, cause) } @@ -199,23 +195,8 @@ func (check *Checker) implements(V, T Type) error { } // V must implement T's methods, if any. - if Ti.NumMethods() > 0 { - if m, wrong := check.missingMethod(V, Ti, true); m != nil /* !Implements(V, Ti) */ { - if check != nil && check.conf.CompilerErrorMessages { - return errorf("%s does not implement %s %s", V, T, check.missingMethodReason(V, T, m, wrong)) - } - var cause string - if wrong != nil { - if Identical(m.typ, wrong.typ) { - cause = fmt.Sprintf("missing method %s (%s has pointer receiver)", m.name, m.name) - } else { - cause = fmt.Sprintf("wrong type for method %s (have %s, want %s)", m.Name(), wrong.typ, m.typ) - } - } else { - cause = "missing method " + m.Name() - } - return errorf("%s does not implement %s: %s", V, T, cause) - } + if m, wrong := check.missingMethod(V, Ti, true); m != nil /* !Implements(V, Ti) */ { + return errorf("%s does not implement %s %s", V, T, check.missingMethodReason(V, T, m, wrong)) } // If T is comparable, V must be comparable. diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index 1aeb2beaa0..7e528fb1aa 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -297,7 +297,7 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b // as the second result. func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, alt *Func) { // fast path for common case - if T.Empty() { + if T.NumMethods() == 0 { return } @@ -368,9 +368,10 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, // and may include more have/want info after that. If non-nil, alt is a relevant // method that matches in some way. It may have the correct name, but wrong type, or // it may have a pointer receiver, or it may have the correct name except wrong case. +// check may be nil. func (check *Checker) missingMethodReason(V, T Type, m, alt *Func) string { var mname string - if check.conf.CompilerErrorMessages { + if check != nil && check.conf.CompilerErrorMessages { mname = m.Name() + " method" } else { mname = "method " + m.Name() @@ -406,6 +407,7 @@ func isInterfacePtr(T Type) bool { return p != nil && IsInterface(p.base) } +// check may be nil. func (check *Checker) interfacePtrError(T Type) string { assert(isInterfacePtr(T)) if p, _ := under(T).(*Pointer); isTypeParam(p.base) { @@ -415,9 +417,14 @@ func (check *Checker) interfacePtrError(T Type) string { } // funcString returns a string of the form name + signature for f. +// check may be nil. func (check *Checker) funcString(f *Func) string { buf := bytes.NewBufferString(f.name) - WriteSignature(buf, f.typ.(*Signature), check.qualifier) + var qf Qualifier + if check != nil { + qf = check.qualifier + } + WriteSignature(buf, f.typ.(*Signature), qf) return buf.String() } diff --git a/src/cmd/compile/internal/types2/testdata/check/issues.src b/src/cmd/compile/internal/types2/testdata/check/issues.src index 3b27e03585..42c5bc8f12 100644 --- a/src/cmd/compile/internal/types2/testdata/check/issues.src +++ b/src/cmd/compile/internal/types2/testdata/check/issues.src @@ -131,7 +131,7 @@ func issue10260() { ) var x I1 - x = T1 /* ERROR cannot use .*: missing method foo \(foo has pointer receiver\) */ {} + x = T1 /* ERROR cannot use T1{} .* as I1 value in assignment: T1 does not implement I1 \(method foo has pointer receiver\) */ {} _ = x /* ERROR impossible type assertion: x\.\(T1\)\n\tT1 does not implement I1 \(method foo has pointer receiver\) */ .(T1) T1{}.foo /* ERROR cannot call pointer method foo on T1 */ () @@ -139,34 +139,34 @@ func issue10260() { _ = i2 /* ERROR impossible type assertion: i2\.\(\*T1\)\n\t\*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ .(*T1) - i1 = i0 /* ERROR cannot use .* missing method foo */ - i1 = t0 /* ERROR cannot use .* missing method foo */ - i1 = i2 /* ERROR cannot use .* wrong type for method foo */ - i1 = t2 /* ERROR cannot use .* wrong type for method foo */ - i2 = i1 /* ERROR cannot use .* wrong type for method foo */ - i2 = t1 /* ERROR cannot use .* wrong type for method foo */ + i1 = i0 /* ERROR cannot use i0 .* as I1 value in assignment: I0 does not implement I1 \(missing method foo\) */ + i1 = t0 /* ERROR .* t0 .* as I1 .*: \*T0 does not implement I1 \(missing method foo\) */ + i1 = i2 /* ERROR .* i2 .* as I1 .*: I2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ + i1 = t2 /* ERROR .* t2 .* as I1 .*: \*T2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ + i2 = i1 /* ERROR .* i1 .* as I2 .*: I1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ + i2 = t1 /* ERROR .* t1 .* as I2 .*: \*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ - _ = func() I1 { return i0 /* ERROR cannot use .* missing method foo */ } - _ = func() I1 { return t0 /* ERROR cannot use .* missing method foo */ } - _ = func() I1 { return i2 /* ERROR cannot use .* wrong type for method foo */ } - _ = func() I1 { return t2 /* ERROR cannot use .* wrong type for method foo */ } - _ = func() I2 { return i1 /* ERROR cannot use .* wrong type for method foo */ } - _ = func() I2 { return t1 /* ERROR cannot use .* wrong type for method foo */ } + _ = func() I1 { return i0 /* ERROR cannot use i0 .* as I1 value in return statement: I0 does not implement I1 \(missing method foo\) */ } + _ = func() I1 { return t0 /* ERROR .* t0 .* as I1 .*: \*T0 does not implement I1 \(missing method foo\) */ } + _ = func() I1 { return i2 /* ERROR .* i2 .* as I1 .*: I2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ } + _ = func() I1 { return t2 /* ERROR .* t2 .* as I1 .*: \*T2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ } + _ = func() I2 { return i1 /* ERROR .* i1 .* as I2 .*: I1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ } + _ = func() I2 { return t1 /* ERROR .* t1 .* as I2 .*: \*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ } // a few more - less exhaustive now f := func(I1, I2){} - f(i0 /* ERROR cannot use .* missing method foo */ , i1 /* ERROR cannot use .* wrong type for method foo \(have func\(\), want func\(x int\)\) */ ) + f(i0 /* ERROR missing method foo */ , i1 /* ERROR wrong type for method foo */ ) - _ = [...]I1{i0 /* ERROR cannot use .* missing method foo */ } - _ = [...]I1{i2 /* ERROR cannot use .* wrong type for method foo */ } - _ = []I1{i0 /* ERROR cannot use .* missing method foo */ } - _ = []I1{i2 /* ERROR cannot use .* wrong type for method foo */ } - _ = map[int]I1{0: i0 /* ERROR cannot use .* missing method foo */ } - _ = map[int]I1{0: i2 /* ERROR cannot use .* wrong type for method foo */ } + _ = [...]I1{i0 /* ERROR cannot use i0 .* as I1 value in array or slice literal: I0 does not implement I1 \(missing method foo\) */ } + _ = [...]I1{i2 /* ERROR cannot use i2 .* as I1 value in array or slice literal: I2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ } + _ = []I1{i0 /* ERROR missing method foo */ } + _ = []I1{i2 /* ERROR wrong type for method foo */ } + _ = map[int]I1{0: i0 /* ERROR missing method foo */ } + _ = map[int]I1{0: i2 /* ERROR wrong type for method foo */ } - make(chan I1) <- i0 /* ERROR I0 does not implement I1: missing method foo */ - make(chan I1) <- i2 /* ERROR wrong type for method foo \(have func\(x int\), want func\(\)\) */ + make(chan I1) <- i0 /* ERROR missing method foo */ + make(chan I1) <- i2 /* ERROR wrong type for method foo */ } // Check that constants representable as integers are in integer form diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49579.go2 b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49579.go2 index 9e20ae5468..ee2d94ab89 100644 --- a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49579.go2 +++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue49579.go2 @@ -9,7 +9,7 @@ type I[F any] interface { } func G[F any]() I[any] { - return g /* ERROR "missing method Q \(Q has pointer receiver\)" */ [F]{} + return g /* ERROR cannot use g\[F\]{} .* as I\[any\] value in return statement: g\[F\] does not implement I\[any\] \(method Q has pointer receiver\) */ [F]{} } type g[F any] struct{} diff --git a/src/go/types/errors.go b/src/go/types/errors.go index ce62a8cbdd..a1786ec0ff 100644 --- a/src/go/types/errors.go +++ b/src/go/types/errors.go @@ -63,8 +63,15 @@ func (check *Checker) markImports(pkg *Package) { } } +// check may be nil. func (check *Checker) sprintf(format string, args ...any) string { - return sprintf(check.fset, check.qualifier, false, format, args...) + var fset *token.FileSet + var qf Qualifier + if check != nil { + fset = check.fset + qf = check.qualifier + } + return sprintf(fset, qf, false, format, args...) } func sprintf(fset *token.FileSet, qf Qualifier, debug bool, format string, args ...any) string { diff --git a/src/go/types/instantiate.go b/src/go/types/instantiate.go index 347815f9dd..aeb30fa412 100644 --- a/src/go/types/instantiate.go +++ b/src/go/types/instantiate.go @@ -160,25 +160,17 @@ func (check *Checker) implements(V, T Type) error { return nil // avoid follow-on errors (see issue #49541 for an example) } - var qf Qualifier - if check != nil { - qf = check.qualifier - } errorf := func(format string, args ...any) error { - return errors.New(sprintf(nil, qf, false, format, args...)) + return errors.New(check.sprintf(format, args...)) } Ti, _ := Tu.(*Interface) if Ti == nil { - var fset *token.FileSet - if check != nil { - fset = check.fset - } var cause string if isInterfacePtr(Tu) { - cause = sprintf(fset, qf, false, "type %s is pointer to interface, not interface", T) + cause = check.sprintf("type %s is pointer to interface, not interface", T) } else { - cause = sprintf(fset, qf, false, "%s is not an interface", T) + cause = check.sprintf("%s is not an interface", T) } return errorf("%s does not implement %s (%s)", V, T, cause) } @@ -203,23 +195,8 @@ func (check *Checker) implements(V, T Type) error { } // V must implement T's methods, if any. - if Ti.NumMethods() > 0 { - if m, wrong := check.missingMethod(V, Ti, true); m != nil /* !Implements(V, Ti) */ { - if check != nil && compilerErrorMessages { - return errorf("%s does not implement %s %s", V, T, check.missingMethodReason(V, T, m, wrong)) - } - var cause string - if wrong != nil { - if Identical(m.typ, wrong.typ) { - cause = fmt.Sprintf("missing method %s (%s has pointer receiver)", m.name, m.name) - } else { - cause = fmt.Sprintf("wrong type for method %s (have %s, want %s)", m.Name(), wrong.typ, m.typ) - } - } else { - cause = "missing method " + m.Name() - } - return errorf("%s does not implement %s: %s", V, T, cause) - } + if m, wrong := check.missingMethod(V, Ti, true); m != nil /* !Implements(V, Ti) */ { + return errorf("%s does not implement %s %s", V, T, check.missingMethodReason(V, T, m, wrong)) } // If T is comparable, V must be comparable. diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index 1b4f953803..ad5438aefb 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -298,7 +298,7 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b // Note: case-folding lookup is currently disabled func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, alt *Func) { // fast path for common case - if T.Empty() { + if T.NumMethods() == 0 { return } @@ -370,9 +370,10 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, // and may include more have/want info after that. If non-nil, alt is a relevant // method that matches in some way. It may have the correct name, but wrong type, or // it may have a pointer receiver, or it may have the correct name except wrong case. +// check may be nil. func (check *Checker) missingMethodReason(V, T Type, m, alt *Func) string { var mname string - if compilerErrorMessages { + if check != nil && compilerErrorMessages { mname = m.Name() + " method" } else { mname = "method " + m.Name() @@ -408,6 +409,7 @@ func isInterfacePtr(T Type) bool { return p != nil && IsInterface(p.base) } +// check may be nil. func (check *Checker) interfacePtrError(T Type) string { assert(isInterfacePtr(T)) if p, _ := under(T).(*Pointer); isTypeParam(p.base) { @@ -416,10 +418,14 @@ func (check *Checker) interfacePtrError(T Type) string { return check.sprintf("type %s is pointer to interface, not interface", T) } -// funcString returns a string of the form name + signature for f. +// check may be nil. func (check *Checker) funcString(f *Func) string { buf := bytes.NewBufferString(f.name) - WriteSignature(buf, f.typ.(*Signature), check.qualifier) + var qf Qualifier + if check != nil { + qf = check.qualifier + } + WriteSignature(buf, f.typ.(*Signature), qf) return buf.String() } diff --git a/src/go/types/testdata/check/issues.src b/src/go/types/testdata/check/issues.src index ce27ac3cfb..8bb4c8c5ca 100644 --- a/src/go/types/testdata/check/issues.src +++ b/src/go/types/testdata/check/issues.src @@ -131,7 +131,7 @@ func issue10260() { ) var x I1 - x = T1 /* ERROR cannot use .*: missing method foo \(foo has pointer receiver\) */ {} + x = T1 /* ERROR cannot use \(T1 literal\) .* as I1 value in assignment: T1 does not implement I1 \(method foo has pointer receiver\) */ {} _ = x /* ERROR impossible type assertion: x\.\(T1\)\n\tT1 does not implement I1 \(method foo has pointer receiver\) */ .(T1) T1{}.foo /* ERROR cannot call pointer method foo on T1 */ () @@ -139,34 +139,34 @@ func issue10260() { _ = i2 /* ERROR impossible type assertion: i2\.\(\*T1\)\n\t\*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ .(*T1) - i1 = i0 /* ERROR cannot use .* missing method foo */ - i1 = t0 /* ERROR cannot use .* missing method foo */ - i1 = i2 /* ERROR cannot use .* wrong type for method foo */ - i1 = t2 /* ERROR cannot use .* wrong type for method foo */ - i2 = i1 /* ERROR cannot use .* wrong type for method foo */ - i2 = t1 /* ERROR cannot use .* wrong type for method foo */ + i1 = i0 /* ERROR cannot use i0 .* as I1 value in assignment: I0 does not implement I1 \(missing method foo\) */ + i1 = t0 /* ERROR .* t0 .* as I1 .*: \*T0 does not implement I1 \(missing method foo\) */ + i1 = i2 /* ERROR .* i2 .* as I1 .*: I2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ + i1 = t2 /* ERROR .* t2 .* as I1 .*: \*T2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ + i2 = i1 /* ERROR .* i1 .* as I2 .*: I1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ + i2 = t1 /* ERROR .* t1 .* as I2 .*: \*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ - _ = func() I1 { return i0 /* ERROR cannot use .* missing method foo */ } - _ = func() I1 { return t0 /* ERROR cannot use .* missing method foo */ } - _ = func() I1 { return i2 /* ERROR cannot use .* wrong type for method foo */ } - _ = func() I1 { return t2 /* ERROR cannot use .* wrong type for method foo */ } - _ = func() I2 { return i1 /* ERROR cannot use .* wrong type for method foo */ } - _ = func() I2 { return t1 /* ERROR cannot use .* wrong type for method foo */ } + _ = func() I1 { return i0 /* ERROR cannot use i0 .* as I1 value in return statement: I0 does not implement I1 \(missing method foo\) */ } + _ = func() I1 { return t0 /* ERROR .* t0 .* as I1 .*: \*T0 does not implement I1 \(missing method foo\) */ } + _ = func() I1 { return i2 /* ERROR .* i2 .* as I1 .*: I2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ } + _ = func() I1 { return t2 /* ERROR .* t2 .* as I1 .*: \*T2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ } + _ = func() I2 { return i1 /* ERROR .* i1 .* as I2 .*: I1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ } + _ = func() I2 { return t1 /* ERROR .* t1 .* as I2 .*: \*T1 does not implement I2 \(wrong type for method foo\)\n\t\thave foo\(\)\n\t\twant foo\(x int\) */ } // a few more - less exhaustive now f := func(I1, I2){} - f(i0 /* ERROR cannot use .* missing method foo */ , i1 /* ERROR cannot use .* wrong type for method foo \(have func\(\), want func\(x int\)\) */ ) + f(i0 /* ERROR missing method foo */ , i1 /* ERROR wrong type for method foo */ ) - _ = [...]I1{i0 /* ERROR cannot use .* missing method foo */ } - _ = [...]I1{i2 /* ERROR cannot use .* wrong type for method foo */ } - _ = []I1{i0 /* ERROR cannot use .* missing method foo */ } - _ = []I1{i2 /* ERROR cannot use .* wrong type for method foo */ } - _ = map[int]I1{0: i0 /* ERROR cannot use .* missing method foo */ } - _ = map[int]I1{0: i2 /* ERROR cannot use .* wrong type for method foo */ } + _ = [...]I1{i0 /* ERROR cannot use i0 .* as I1 value in array or slice literal: I0 does not implement I1 \(missing method foo\) */ } + _ = [...]I1{i2 /* ERROR cannot use i2 .* as I1 value in array or slice literal: I2 does not implement I1 \(wrong type for method foo\)\n\t\thave foo\(x int\)\n\t\twant foo\(\) */ } + _ = []I1{i0 /* ERROR missing method foo */ } + _ = []I1{i2 /* ERROR wrong type for method foo */ } + _ = map[int]I1{0: i0 /* ERROR missing method foo */ } + _ = map[int]I1{0: i2 /* ERROR wrong type for method foo */ } - make(chan I1) <- i0 /* ERROR I0 does not implement I1: missing method foo */ - make(chan I1) <- i2 /* ERROR wrong type for method foo \(have func\(x int\), want func\(\)\) */ + make(chan I1) <- i0 /* ERROR missing method foo */ + make(chan I1) <- i2 /* ERROR wrong type for method foo */ } // Check that constants representable as integers are in integer form diff --git a/src/go/types/testdata/fixedbugs/issue49579.go2 b/src/go/types/testdata/fixedbugs/issue49579.go2 index 9e20ae5468..07748bd0dc 100644 --- a/src/go/types/testdata/fixedbugs/issue49579.go2 +++ b/src/go/types/testdata/fixedbugs/issue49579.go2 @@ -9,7 +9,7 @@ type I[F any] interface { } func G[F any]() I[any] { - return g /* ERROR "missing method Q \(Q has pointer receiver\)" */ [F]{} + return g /* ERROR cannot use \(g\[F\] literal\) .* as I\[any\] value in return statement: g\[F\] does not implement I\[any\] \(method Q has pointer receiver\) */ [F]{} } type g[F any] struct{} From 7c9885def52a408532085a566eea107f31ad1556 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 3 Feb 2022 17:34:43 -0800 Subject: [PATCH 30/42] go/types, types2: use identical missingMethod in both type checkers Further simplify and regularize Checker.missingMethod and use the same code in both type checkers. This enables case-folding lookup for go/types. Adjusted test case that looks for alternative methods. Change-Id: I5b8cc598c295c329ff93b1c65787cc6140f0900e Reviewed-on: https://go-review.googlesource.com/c/go/+/382858 Trust: Robert Griesemer Reviewed-by: Robert Findley --- src/cmd/compile/internal/types2/lookup.go | 36 +++++++----------- src/go/types/lookup.go | 38 +++++++------------ .../types/testdata/fixedbugs/issue50816.go2 | 4 +- 3 files changed, 30 insertions(+), 48 deletions(-) diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index 7e528fb1aa..b8ddd94cd7 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -296,21 +296,21 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b // on T when looked up with case-folding, this alternative method is returned // as the second result. func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, alt *Func) { - // fast path for common case if T.NumMethods() == 0 { return } - if ityp, _ := under(V).(*Interface); ityp != nil { + // V is an interface + if u, _ := under(V).(*Interface); u != nil { + tset := u.typeSet() for _, m := range T.typeSet().methods { - _, f := ityp.typeSet().LookupMethod(m.pkg, m.name, false) + _, f := tset.LookupMethod(m.pkg, m.name, false) if f == nil { if !static { continue } - // We don't do any case-fold check if V is an interface. - return m, f + return m, nil } if !Identical(f.typ, m.typ) { @@ -321,30 +321,22 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, return } - // A concrete type implements T if it implements all methods of T. + // V is not an interface for _, m := range T.typeSet().methods { // TODO(gri) should this be calling LookupFieldOrMethod instead (and why not)? obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name, false) - // Check if *V implements this method of T. - if obj == nil { - ptr := NewPointer(V) - obj, _, _ = lookupFieldOrMethod(ptr, false, m.pkg, m.name, false) + // check if m is on *V, or on V with case-folding + found := obj != nil + if !found { + // TODO(gri) Instead of NewPointer(V) below, can we just set the "addressable" argument? + obj, _, _ = lookupFieldOrMethod(NewPointer(V), false, m.pkg, m.name, false) if obj == nil { - // If we didn't find the exact method (even with pointer receiver), - // check if there is a matching method using case-folding. - obj, _, _ = lookupFieldOrMethod(V, false, m.pkg, m.name, true) - } - if obj != nil { - // methods may not have a fully set up signature yet - if check != nil { - check.objDecl(obj, nil) - } - return m, obj.(*Func) + obj, _, _ = lookupFieldOrMethod(V, false, m.pkg, m.name, true /* fold case */) } } - // we must have a method (not a field of matching function type) + // we must have a method (not a struct field) f, _ := obj.(*Func) if f == nil { return m, nil @@ -355,7 +347,7 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, check.objDecl(f, nil) } - if !Identical(f.typ, m.typ) { + if !found || !Identical(f.typ, m.typ) { return m, f } } diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index ad5438aefb..f2f38be266 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -295,23 +295,22 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b // If a method is missing on T but is found on *T, or if a method is found // on T when looked up with case-folding, this alternative method is returned // as the second result. -// Note: case-folding lookup is currently disabled func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, alt *Func) { - // fast path for common case if T.NumMethods() == 0 { return } - if ityp, _ := under(V).(*Interface); ityp != nil { + // V is an interface + if u, _ := under(V).(*Interface); u != nil { + tset := u.typeSet() for _, m := range T.typeSet().methods { - _, f := ityp.typeSet().LookupMethod(m.pkg, m.name, false) + _, f := tset.LookupMethod(m.pkg, m.name, false) if f == nil { if !static { continue } - // We don't do any case-fold check if V is an interface. - return m, f + return m, nil } if !Identical(f.typ, m.typ) { @@ -322,31 +321,22 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, return } - // A concrete type implements T if it implements all methods of T. + // V is not an interface for _, m := range T.typeSet().methods { // TODO(gri) should this be calling LookupFieldOrMethod instead (and why not)? obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name, false) - // Check if *V implements this method of T. - if obj == nil { - ptr := NewPointer(V) - obj, _, _ = lookupFieldOrMethod(ptr, false, m.pkg, m.name, false) + // check if m is on *V, or on V with case-folding + found := obj != nil + if !found { + // TODO(gri) Instead of NewPointer(V) below, can we just set the "addressable" argument? + obj, _, _ = lookupFieldOrMethod(NewPointer(V), false, m.pkg, m.name, false) if obj == nil { - // TODO(gri) enable this code - // If we didn't find the exact method (even with pointer receiver), - // check if there is a matching method using case-folding. - // obj, _, _ = lookupFieldOrMethod(V, false, m.pkg, m.name, true) - } - if obj != nil { - // methods may not have a fully set up signature yet - if check != nil { - check.objDecl(obj, nil) - } - return m, obj.(*Func) + obj, _, _ = lookupFieldOrMethod(V, false, m.pkg, m.name, true /* fold case */) } } - // we must have a method (not a field of matching function type) + // we must have a method (not a struct field) f, _ := obj.(*Func) if f == nil { return m, nil @@ -357,7 +347,7 @@ func (check *Checker) missingMethod(V Type, T *Interface, static bool) (method, check.objDecl(f, nil) } - if !Identical(f.typ, m.typ) { + if !found || !Identical(f.typ, m.typ) { return m, f } } diff --git a/src/go/types/testdata/fixedbugs/issue50816.go2 b/src/go/types/testdata/fixedbugs/issue50816.go2 index 025a338184..e7e31d9192 100644 --- a/src/go/types/testdata/fixedbugs/issue50816.go2 +++ b/src/go/types/testdata/fixedbugs/issue50816.go2 @@ -18,6 +18,6 @@ func (T2) foo() string { return "" } func _() { var i I - _ = i /* ERROR impossible type assertion: i\.\(T1\)\n\tT1 does not implement I \(missing method Foo\) */ .(T1) - _ = i /* ERROR impossible type assertion: i\.\(T2\)\n\tT2 does not implement I \(missing method Foo\) */ .(T2) + _ = i /* ERROR impossible type assertion: i\.\(T1\)\n\tT1 does not implement I \(missing method Foo\)\n\t\thave foo\(\)\n\t\twant Foo\(\) */ .(T1) + _ = i /* ERROR impossible type assertion: i\.\(T2\)\n\tT2 does not implement I \(missing method Foo\)\n\t\thave foo\(\) string\n\t\twant Foo\(\) */ .(T2) } From d588f487703e773ba4a2f0a04f2d4141610bff6b Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Fri, 4 Feb 2022 21:14:13 -0800 Subject: [PATCH 31/42] runtime: change sys.PtrSize to goarch.PtrSize in comments The code was updated, the comments were not. Change-Id: If387779f3abd5e8a1b487fe34c33dcf9ce5fa7ff Reviewed-on: https://go-review.googlesource.com/c/go/+/383495 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor Reviewed-by: Michael Knyszek TryBot-Result: Gopher Robot --- src/runtime/mbarrier.go | 2 +- src/runtime/mbitmap.go | 12 ++++++------ src/runtime/slice.go | 2 +- src/runtime/symtab.go | 2 +- src/runtime/syscall_windows.go | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/runtime/mbarrier.go b/src/runtime/mbarrier.go index 0f8b2af5fa..465c21f83f 100644 --- a/src/runtime/mbarrier.go +++ b/src/runtime/mbarrier.go @@ -198,7 +198,7 @@ func reflectlite_typedmemmove(typ *_type, dst, src unsafe.Pointer) { // typedmemmovepartial is like typedmemmove but assumes that // dst and src point off bytes into the value and only copies size bytes. -// off must be a multiple of sys.PtrSize. +// off must be a multiple of goarch.PtrSize. //go:linkname reflect_typedmemmovepartial reflect.typedmemmovepartial func reflect_typedmemmovepartial(typ *_type, dst, src unsafe.Pointer, off, size uintptr) { if writeBarrier.needed && typ.ptrdata > off && size >= goarch.PtrSize { diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go index 1c6f3f959f..937968807b 100644 --- a/src/runtime/mbitmap.go +++ b/src/runtime/mbitmap.go @@ -843,7 +843,7 @@ func heapBitsSetType(x, size, dataSize uintptr, typ *_type) { // size is sizeof(_defer{}) (at least 6 words) and dataSize may be // arbitrarily larger. // - // The checks for size == sys.PtrSize and size == 2*sys.PtrSize can therefore + // The checks for size == goarch.PtrSize and size == 2*goarch.PtrSize can therefore // assume that dataSize == size without checking it explicitly. if goarch.PtrSize == 8 && size == goarch.PtrSize { @@ -893,7 +893,7 @@ func heapBitsSetType(x, size, dataSize uintptr, typ *_type) { } return } - // Otherwise typ.size must be 2*sys.PtrSize, + // Otherwise typ.size must be 2*goarch.PtrSize, // and typ.kind&kindGCProg == 0. if doubleCheck { if typ.size != 2*goarch.PtrSize || typ.kind&kindGCProg != 0 { @@ -1095,8 +1095,8 @@ func heapBitsSetType(x, size, dataSize uintptr, typ *_type) { // Replicate ptrmask to fill entire pbits uintptr. // Doubling and truncating is fewer steps than // iterating by nb each time. (nb could be 1.) - // Since we loaded typ.ptrdata/sys.PtrSize bits - // but are pretending to have typ.size/sys.PtrSize, + // Since we loaded typ.ptrdata/goarch.PtrSize bits + // but are pretending to have typ.size/goarch.PtrSize, // there might be no replication necessary/possible. pbits = b endnb = nb @@ -1564,7 +1564,7 @@ func heapBitsSetTypeGCProg(h heapBits, progSize, elemSize, dataSize, allocSize u // progToPointerMask returns the 1-bit pointer mask output by the GC program prog. // size the size of the region described by prog, in bytes. -// The resulting bitvector will have no more than size/sys.PtrSize bits. +// The resulting bitvector will have no more than size/goarch.PtrSize bits. func progToPointerMask(prog *byte, size uintptr) bitvector { n := (size/goarch.PtrSize + 7) / 8 x := (*[1 << 30]byte)(persistentalloc(n+1, 1, &memstats.buckhash_sys))[:n+1] @@ -1697,7 +1697,7 @@ Run: // into a register and use that register for the entire loop // instead of repeatedly reading from memory. // Handling fewer than 8 bits here makes the general loop simpler. - // The cutoff is sys.PtrSize*8 - 7 to guarantee that when we add + // The cutoff is goarch.PtrSize*8 - 7 to guarantee that when we add // the pattern to a bit buffer holding at most 7 bits (a partial byte) // it will not overflow. src := dst diff --git a/src/runtime/slice.go b/src/runtime/slice.go index ac0b7d5fef..e0aeba604f 100644 --- a/src/runtime/slice.go +++ b/src/runtime/slice.go @@ -214,7 +214,7 @@ func growslice(et *_type, old slice, cap int) slice { var lenmem, newlenmem, capmem uintptr // Specialize for common values of et.size. // For 1 we don't need any division/multiplication. - // For sys.PtrSize, compiler will optimize division/multiplication into a shift by a constant. + // For goarch.PtrSize, compiler will optimize division/multiplication into a shift by a constant. // For powers of 2, use a variable shift. switch { case et.size == 1: diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index 21dd95a397..017b0a0749 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -863,7 +863,7 @@ type pcvalueCacheEnt struct { // pcvalueCacheKey returns the outermost index in a pcvalueCache to use for targetpc. // It must be very cheap to calculate. -// For now, align to sys.PtrSize and reduce mod the number of entries. +// For now, align to goarch.PtrSize and reduce mod the number of entries. // In practice, this appears to be fairly randomly and evenly distributed. func pcvalueCacheKey(targetpc uintptr) uintptr { return (targetpc / goarch.PtrSize) % uintptr(len(pcvalueCache{}.entries)) diff --git a/src/runtime/syscall_windows.go b/src/runtime/syscall_windows.go index e76b403ade..9c38facf08 100644 --- a/src/runtime/syscall_windows.go +++ b/src/runtime/syscall_windows.go @@ -152,7 +152,7 @@ func (p *abiDesc) assignArg(t *_type) { // tryRegAssignArg tries to register-assign a value of type t. // If this type is nested in an aggregate type, then offset is the // offset of this type within its parent type. -// Assumes t.size <= sys.PtrSize and t.size != 0. +// Assumes t.size <= goarch.PtrSize and t.size != 0. // // Returns whether the assignment succeeded. func (p *abiDesc) tryRegAssignArg(t *_type, offset uintptr) bool { From 1d6051380c1faa3e515db73c4cfe14f807e2c686 Mon Sep 17 00:00:00 2001 From: Benny Siegert Date: Sat, 5 Feb 2022 17:23:26 +0100 Subject: [PATCH 32/42] cmd/dist: skip internal linking tests on arm64 The previous workaround for issue #39466 only disabled this test for Linux. However, the issue manifests for all arm64 systems with gcc 9.4 and above. The new netbsd-arm64 builder uses NetBSD-current with gcc 10.3, so it fails in the same way. Updates #39466 Change-Id: I276a99a5e60914e5c22f74a680e461bea17cfe92 Reviewed-on: https://go-review.googlesource.com/c/go/+/383554 Trust: Benny Siegert Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Ian Lance Taylor --- src/cmd/dist/test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go index 50a2e5936c..4b67565430 100644 --- a/src/cmd/dist/test.go +++ b/src/cmd/dist/test.go @@ -1117,9 +1117,9 @@ func (t *tester) cgoTest(dt *distTest) error { cmd := t.addCmd(dt, "misc/cgo/test", t.goTest()) setEnv(cmd, "GOFLAGS", "-ldflags=-linkmode=auto") - // Skip internal linking cases on linux/arm64 to support GCC-9.4 and above. + // Skip internal linking cases on arm64 to support GCC-9.4 and above. // See issue #39466. - skipInternalLink := goarch == "arm64" && goos == "linux" + skipInternalLink := goarch == "arm64" && goos != "windows" if t.internalLink() && !skipInternalLink { cmd := t.addCmd(dt, "misc/cgo/test", t.goTest(), "-tags=internal") From 334a591a3f4d868368913328b3e81ddf5b0f46fa Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Sun, 6 Feb 2022 19:34:32 -0800 Subject: [PATCH 33/42] os: don't repeat dir argument in CreateTemp error The dir argument is already in prefix, we shouldn't add it again. Change-Id: I42a158bec3a43950fce24f57b808da3ad8c5ef5b Reviewed-on: https://go-review.googlesource.com/c/go/+/383636 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Brad Fitzpatrick --- src/os/tempfile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/os/tempfile.go b/src/os/tempfile.go index 5b681fcebf..3be3d13dfb 100644 --- a/src/os/tempfile.go +++ b/src/os/tempfile.go @@ -46,7 +46,7 @@ func CreateTemp(dir, pattern string) (*File, error) { if try++; try < 10000 { continue } - return nil, &PathError{Op: "createtemp", Path: dir + string(PathSeparator) + prefix + "*" + suffix, Err: ErrExist} + return nil, &PathError{Op: "createtemp", Path: prefix + "*" + suffix, Err: ErrExist} } return f, err } From 867a3d55024b654347fcbc0782a39ecd57d94a27 Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Wed, 2 Feb 2022 14:09:26 -0500 Subject: [PATCH 34/42] test: apply GO_TEST_TIMEOUT_SCALE scaling to test timeouts Change run.go to apply the GO_TEST_TIMEOUT_SCALE scaling factor to test timeouts (mentioned in "-t" clause in test header). Also with this patch, bump up the timeout for fixedbugs/issue46234.go from 30 to 45 seconds, to avoid flakes on very slow builders. Updates #50973. Change-Id: Icbafa482860e24cc1e72fee53511bcc764d06bf1 Reviewed-on: https://go-review.googlesource.com/c/go/+/382774 Reviewed-by: Ian Lance Taylor Trust: Than McIntosh Run-TryBot: Than McIntosh TryBot-Result: Gopher Robot --- test/fixedbugs/issue46234.go | 3 ++- test/run.go | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/test/fixedbugs/issue46234.go b/test/fixedbugs/issue46234.go index 8e7eb8bf8d..ed1c05cfbf 100644 --- a/test/fixedbugs/issue46234.go +++ b/test/fixedbugs/issue46234.go @@ -1,5 +1,6 @@ -// buildrun -t 30 +// buildrun -t 45 +//go:build !js // +build !js // Copyright 2021 The Go Authors. All rights reserved. diff --git a/test/run.go b/test/run.go index 9ba421510c..ae5afc751d 100644 --- a/test/run.go +++ b/test/run.go @@ -710,6 +710,13 @@ func (t *test) run() { if err != nil { t.err = fmt.Errorf("need number of seconds for -t timeout, got %s instead", args[0]) } + if s := os.Getenv("GO_TEST_TIMEOUT_SCALE"); s != "" { + timeoutScale, err := strconv.Atoi(s) + if err != nil { + log.Fatalf("failed to parse $GO_TEST_TIMEOUT_SCALE = %q as integer: %v", s, err) + } + tim *= timeoutScale + } case "-goexperiment": // set GOEXPERIMENT environment args = args[1:] if goexp != "" { @@ -834,6 +841,13 @@ func (t *test) run() { if tim != 0 { err = cmd.Start() // This command-timeout code adapted from cmd/go/test.go + // Note: the Go command uses a more sophisticated timeout + // strategy, first sending SIGQUIT (if appropriate for the + // OS in question) to try to trigger a stack trace, then + // finally much later SIGKILL. If timeouts prove to be a + // common problem here, it would be worth porting over + // that code as well. See https://do.dev/issue/50973 + // for more discussion. if err == nil { tick := time.NewTimer(time.Duration(tim) * time.Second) done := make(chan error) From 3c4c10ea8c87836a75c4065660c72c0929309dd7 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Mon, 7 Feb 2022 11:33:30 -0500 Subject: [PATCH 35/42] misc/cgo: fix aliasing bugs in parallel tests that append to shared slices These tests use a slice to represent the base C compiler command (with flags). Appending to that slice can cause subtle aliasing bugs, such as commands that silently corrupt the arguments of other concurrent commands in parallel tests. In this change, we explicitly reduce the capacity of the command slice to force appends to it to always allocate unique new slices. Fixes #49693 Change-Id: Ide466bf65f12cb6cead3dcba69f513cccb60a160 Reviewed-on: https://go-review.googlesource.com/c/go/+/383754 Trust: Bryan Mills Run-TryBot: Bryan Mills Reviewed-by: Ian Lance Taylor TryBot-Result: Gopher Robot --- misc/cgo/errors/badsym_test.go | 4 ++++ misc/cgo/testcarchive/carchive_test.go | 30 +++----------------------- misc/cgo/testcshared/cshared_test.go | 3 +++ 3 files changed, 10 insertions(+), 27 deletions(-) diff --git a/misc/cgo/errors/badsym_test.go b/misc/cgo/errors/badsym_test.go index fc687567bf..bc3ba2b489 100644 --- a/misc/cgo/errors/badsym_test.go +++ b/misc/cgo/errors/badsym_test.go @@ -201,6 +201,10 @@ func cCompilerCmd(t *testing.T) []string { if !lastSpace { cc = append(cc, s[start:]) } + + // Force reallocation (and avoid aliasing bugs) for tests that append to cc. + cc = cc[:len(cc):len(cc)] + return cc } diff --git a/misc/cgo/testcarchive/carchive_test.go b/misc/cgo/testcarchive/carchive_test.go index a821396c77..d36b97b70e 100644 --- a/misc/cgo/testcarchive/carchive_test.go +++ b/misc/cgo/testcarchive/carchive_test.go @@ -11,7 +11,6 @@ import ( "flag" "fmt" "io" - "io/fs" "log" "os" "os/exec" @@ -141,6 +140,9 @@ func testMain(m *testing.M) int { libgodir = filepath.Join(GOPATH, "pkg", libbase, "testcarchive") cc = append(cc, "-I", libgodir) + // Force reallocation (and avoid aliasing bugs) for parallel tests that append to cc. + cc = cc[:len(cc):len(cc)] + if GOOS == "windows" { exeSuffix = ".exe" } @@ -248,29 +250,6 @@ func testInstall(t *testing.T, exe, libgoa, libgoh string, buildcmd ...string) { var badLineRegexp = regexp.MustCompile(`(?m)^#line [0-9]+ "/.*$`) -// checkIsExecutable verifies that exe exists and has execute permission. -// -// (https://golang.org/issue/49693 notes failures with "no such file or -// directory", so we want to double-check that the executable actually exists -// immediately after we build it in order to better understand that failure -// mode.) -func checkIsExecutable(t *testing.T, exe string) { - t.Helper() - fi, err := os.Stat(exe) - if err != nil { - t.Fatal(err) - } - if runtime.GOOS == "windows" { - // os.File doesn't check the "execute" permission on Windows files - // and as a result doesn't set that bit in a file's permissions. - // Assume that if the file exists it is “executable enough”. - return - } - if fi.Mode()&0111 == 0 { - t.Fatalf("%s is not executable: %0o", exe, fi.Mode()&fs.ModePerm) - } -} - // checkLineComments checks that the export header generated by // -buildmode=c-archive doesn't have any absolute paths in the #line // comments. We don't want those paths because they are unhelpful for @@ -964,7 +943,6 @@ func TestSIGPROF(t *testing.T) { if err != nil { t.Fatal(err) } - checkIsExecutable(t, "./testp6"+exeSuffix) argv := cmdToRun("./testp6") cmd = exec.Command(argv[0], argv[1:]...) @@ -1113,7 +1091,6 @@ func TestManyCalls(t *testing.T) { if err != nil { t.Fatal(err) } - checkIsExecutable(t, "./testp7"+exeSuffix) argv := cmdToRun("./testp7") cmd = exec.Command(argv[0], argv[1:]...) @@ -1170,7 +1147,6 @@ func TestPreemption(t *testing.T) { if err != nil { t.Fatal(err) } - checkIsExecutable(t, "./testp8"+exeSuffix) argv := cmdToRun("./testp8") cmd = exec.Command(argv[0], argv[1:]...) diff --git a/misc/cgo/testcshared/cshared_test.go b/misc/cgo/testcshared/cshared_test.go index 13ec8761e8..c9e9e5fe63 100644 --- a/misc/cgo/testcshared/cshared_test.go +++ b/misc/cgo/testcshared/cshared_test.go @@ -117,6 +117,9 @@ func testMain(m *testing.M) int { } cc = append(cc, "-I", filepath.Join("pkg", libgodir)) + // Force reallocation (and avoid aliasing bugs) for parallel tests that append to cc. + cc = cc[:len(cc):len(cc)] + if GOOS == "windows" { exeSuffix = ".exe" } From fa0484640479a26687608706c9f6628eac1174d2 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Mon, 7 Feb 2022 10:02:30 -0800 Subject: [PATCH 36/42] doc/go1.18: mention new x/exp generic packages Change-Id: I119cfb1a0da9af89ced78935b8fcdfdb8d9b4ab6 Reviewed-on: https://go-review.googlesource.com/c/go/+/383794 Trust: Ian Lance Taylor Reviewed-by: Robert Griesemer --- doc/go1.18.html | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/doc/go1.18.html b/doc/go1.18.html index 4d77f14d53..7e11f73820 100644 --- a/doc/go1.18.html +++ b/doc/go1.18.html @@ -90,6 +90,39 @@ Do not send CLs removing the interior tags from such phrases. +

+ There are three experimental packages using generics that may be + useful. + These packages are in x/exp repository; their API is not covered by + the Go 1 guarantee and may change as we gain more experience with + generics. +

+
golang.org/x/exp/constraints
+
+

+ Constraints that are useful for generic code, such as + constraints.Ordered. +

+
+ +
golang.org/x/exp/slices
+
+

+ A collection of generic functions that operate on slices of + any element type. +

+
+ +
golang.org/x/exp/maps
+
+

+ A collection of generic functions that operate on maps of + any key or element type. +

+
+
+

+

The current generics implementation has the following limitations: