Skip to content

Commit

Permalink
Avoid stringer-related deadlocks without adding ISGOMOCK (#204)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
JacobOaks authored Sep 18, 2024
1 parent 60372e3 commit 33ccc99
Show file tree
Hide file tree
Showing 41 changed files with 78 additions and 274 deletions.
6 changes: 1 addition & 5 deletions gomock/internal/mock_gomock/mock_matcher.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions gomock/mock_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 27 additions & 16 deletions gomock/string.go
Original file line number Diff line number Diff line change
@@ -1,25 +1,36 @@
package gomock

import "fmt"

type mockInstance interface {
ISGOMOCK() struct{}
}
type mockedStringer interface {
fmt.Stringer
mockInstance
}
import (
"fmt"
"reflect"
)

// getString is a safe way to convert a value to a string for printing results
// If the value is a a mock, getString avoids calling the mocked String() method,
// which avoids potential deadlocks
func getString(x any) string {
switch v := x.(type) {
case mockedStringer:
return fmt.Sprintf("%T", v)
case fmt.Stringer:
return v.String()
default:
return fmt.Sprintf("%v", v)
if isGeneratedMock(x) {
return fmt.Sprintf("%T", x)
}
if s, ok := x.(fmt.Stringer); ok {
return s.String()
}
return fmt.Sprintf("%v", x)
}

// isGeneratedMock checks if the given type has a "isgomock" field,
// indicating it is a generated mock.
func isGeneratedMock(x any) bool {
typ := reflect.TypeOf(x)
if typ == nil {
return false
}
if typ.Kind() == reflect.Ptr {
typ = typ.Elem()
}
if typ.Kind() != reflect.Struct {
return false
}
_, isgomock := typ.FieldByName("isgomock")
return isgomock
}
6 changes: 1 addition & 5 deletions mockgen/internal/tests/add_generate_directive/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions mockgen/internal/tests/const_array_length/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions mockgen/internal/tests/copyright_file/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions mockgen/internal/tests/defined_import_local_name/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions mockgen/internal/tests/dot_imports/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions mockgen/internal/tests/empty_interface/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions mockgen/internal/tests/exclude/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions mockgen/internal/tests/extra_import/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 4 additions & 20 deletions mockgen/internal/tests/generics/source/mock_external_mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 33ccc99

Please sign in to comment.