Skip to content

Commit 33ccc99

Browse files
authored
Avoid stringer-related deadlocks without adding ISGOMOCK (#204)
A deadlock related to controller calling Stringer on mocks themselves was revealed in #116. #144 solved this deadlock by adding a generated `ISGOMOCK()` method to all generated mocks, and then checking for it before calling `.String()` on arguments. This reveals an exported method on each generated mock that: * Bloats the generated code * Can be taken dependency on in strange ways via Hyrum's Law * Technically opens up another route for naming collision. This PR attempts to clean up this type of check by instead generating an unexported field in generated mock structs instead, and checks for it using reflect. This hides this implementation detail of gomock/mockgen better, and produces less generated bloat. Note that, importantly, the regression test added in #144 still passes with this PR. Ref: #193
1 parent 60372e3 commit 33ccc99

File tree

41 files changed

+78
-274
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+78
-274
lines changed

gomock/internal/mock_gomock/mock_matcher.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

gomock/mock_test.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

gomock/string.go

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,36 @@
11
package gomock
22

3-
import "fmt"
4-
5-
type mockInstance interface {
6-
ISGOMOCK() struct{}
7-
}
8-
type mockedStringer interface {
9-
fmt.Stringer
10-
mockInstance
11-
}
3+
import (
4+
"fmt"
5+
"reflect"
6+
)
127

138
// getString is a safe way to convert a value to a string for printing results
149
// If the value is a a mock, getString avoids calling the mocked String() method,
1510
// which avoids potential deadlocks
1611
func getString(x any) string {
17-
switch v := x.(type) {
18-
case mockedStringer:
19-
return fmt.Sprintf("%T", v)
20-
case fmt.Stringer:
21-
return v.String()
22-
default:
23-
return fmt.Sprintf("%v", v)
12+
if isGeneratedMock(x) {
13+
return fmt.Sprintf("%T", x)
14+
}
15+
if s, ok := x.(fmt.Stringer); ok {
16+
return s.String()
17+
}
18+
return fmt.Sprintf("%v", x)
19+
}
20+
21+
// isGeneratedMock checks if the given type has a "isgomock" field,
22+
// indicating it is a generated mock.
23+
func isGeneratedMock(x any) bool {
24+
typ := reflect.TypeOf(x)
25+
if typ == nil {
26+
return false
27+
}
28+
if typ.Kind() == reflect.Ptr {
29+
typ = typ.Elem()
30+
}
31+
if typ.Kind() != reflect.Struct {
32+
return false
2433
}
34+
_, isgomock := typ.FieldByName("isgomock")
35+
return isgomock
2536
}

mockgen/internal/tests/add_generate_directive/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/const_array_length/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/copyright_file/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/custom_package_name/greeter/greeter_mock_test.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/defined_import_local_name/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/dot_imports/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/empty_interface/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/exclude/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/extra_import/mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/generated_identifier_conflict/bugreport_mock.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

mockgen/internal/tests/generics/source/mock_external_mock.go

Lines changed: 4 additions & 20 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)