Skip to content

Commit 9b9871d

Browse files
committed
go/analysis/passes/copylock: test for noCopy for sync Map, Mutex, Once
Right now the copylock tests check for details on these types that are implementation details and/or have some other problem (a less useful error message, or a false negative altogether). CL 627777 modifies the sync package to attach explicit anonymous noCopy fields on each of these types. This change updates the tests to match that, which also helps with and/or resolves a couple issues with copylock captured in the test suite. Note: this change also temporarily disables a couple of the problematic tests. In the next CL we'll re-enable them, once CL 627777 lands. Change-Id: I100c71ea05b4f08595e37d0c8e81f9543abe7d74 Reviewed-on: https://go-review.googlesource.com/c/tools/+/628435 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com> Reviewed-by: Tim King <taking@google.com>
1 parent a54bd37 commit 9b9871d

File tree

4 files changed

+55
-18
lines changed

4 files changed

+55
-18
lines changed

go/analysis/passes/copylock/copylock_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ import (
1616

1717
func Test(t *testing.T) {
1818
testdata := analysistest.TestData()
19+
// TODO(mknyszek): Add "unfortunate" package once CL 627777 lands. That CL changes
20+
// the internals of the sync package structures to carry an explicit noCopy to prevent
21+
// problems from changes to the implementations of those structures, such as these
22+
// tests failing, or a bad user experience.
1923
analysistest.Run(t, testdata, copylock.Analyzer, "a", "typeparams", "issue67787")
2024
}
2125

go/analysis/passes/copylock/testdata/src/a/copylock_func.go

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,25 @@
55
// This file contains tests for the copylock checker's
66
// function declaration analysis.
77

8+
// There are two cases missing from this file which
9+
// are located in the "unfortunate" package in the
10+
// testdata directory. Once the go.mod >= 1.26 for this
11+
// repository, merge local_go124.go back into this file.
12+
813
package a
914

1015
import "sync"
1116

1217
func OkFunc(*sync.Mutex) {}
1318
func BadFunc(sync.Mutex) {} // want "BadFunc passes lock by value: sync.Mutex"
14-
func BadFunc2(sync.Map) {} // want "BadFunc2 passes lock by value: sync.Map contains sync.Mutex"
19+
func BadFunc2(sync.Map) {} // want "BadFunc2 passes lock by value: sync.Map contains sync.(Mutex|noCopy)"
1520
func OkRet() *sync.Mutex {}
1621
func BadRet() sync.Mutex {} // Don't warn about results
1722

1823
var (
1924
OkClosure = func(*sync.Mutex) {}
2025
BadClosure = func(sync.Mutex) {} // want "func passes lock by value: sync.Mutex"
21-
BadClosure2 = func(sync.Map) {} // want "func passes lock by value: sync.Map contains sync.Mutex"
26+
BadClosure2 = func(sync.Map) {} // want "func passes lock by value: sync.Map contains sync.(Mutex|noCopy)"
2227
)
2328

2429
type EmbeddedRWMutex struct {
@@ -118,19 +123,3 @@ func AcceptedCases() {
118123
x = BadRet() // function call on RHS is OK (#16227)
119124
x = *OKRet() // indirection of function call on RHS is OK (#16227)
120125
}
121-
122-
// TODO: Unfortunate cases
123-
124-
// Non-ideal error message:
125-
// Since we're looking for Lock methods, sync.Once's underlying
126-
// sync.Mutex gets called out, but without any reference to the sync.Once.
127-
type LocalOnce sync.Once
128-
129-
func (LocalOnce) Bad() {} // want `Bad passes lock by value: a.LocalOnce contains sync.\b.*`
130-
131-
// False negative:
132-
// LocalMutex doesn't have a Lock method.
133-
// Nevertheless, it is probably a bad idea to pass it by value.
134-
type LocalMutex sync.Mutex
135-
136-
func (LocalMutex) Bad() {} // WANTED: An error here :(
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Copyright 2024 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
//go:build !go1.24
6+
7+
package unfortunate
8+
9+
import "sync"
10+
11+
// TODO: Unfortunate cases
12+
13+
// Non-ideal error message:
14+
// Since we're looking for Lock methods, sync.Once's underlying
15+
// sync.Mutex gets called out, but without any reference to the sync.Once.
16+
type LocalOnce sync.Once
17+
18+
func (LocalOnce) Bad() {} // want `Bad passes lock by value: a.LocalOnce contains sync.\b.*`
19+
20+
// False negative:
21+
// LocalMutex doesn't have a Lock method.
22+
// Nevertheless, it is probably a bad idea to pass it by value.
23+
type LocalMutex sync.Mutex
24+
25+
func (LocalMutex) Bad() {} // WANTED: An error here :(
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright 2024 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
//go:build go1.24
6+
7+
package unfortunate
8+
9+
import "sync"
10+
11+
// Cases where the interior sync.noCopy shows through.
12+
13+
type LocalOnce sync.Once
14+
15+
func (LocalOnce) Bad() {} // want "Bad passes lock by value: a.LocalOnce contains sync.noCopy"
16+
17+
type LocalMutex sync.Mutex
18+
19+
func (LocalMutex) Bad() {} // want "Bad passes lock by value: a.LocalMutex contains sync.noCopy"

0 commit comments

Comments
 (0)