From db56572e7fe424c6b3e8e48b69e59efca1a07cf9 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Sun, 23 Feb 2020 22:52:40 +0000 Subject: [PATCH 1/6] mockgen: correctly generate mocks with overlapping package names Previous, when generating a mock with the following structure: ```go // source import ( "some.org/package/foo" "some.org/package/bar" ) type Source interface { foo.Foo bar.Bar } ``` ```go // some.org/package/foo package foo import "some.org/package/foo/internal/bar" type Foreign interface { bar.InternalBar } ``` ```go // some.org/package/bar type Bar interface { TopLevelBarMethod() string } ``` ```go // some.org/package/foo/internal/bar type Bar interface { InternalBarMethod() string } ``` The resulting mock would have two generated implementations of TopLevelBarMethod() and no copies of InternalBarMethod(). This was because all interfaces were merged into a single map keyed by the package name. This commit fixes the problem by generating a new map of interfaces for each dependency we recurse into. Signed-off-by: Steven Danna --- .../import_embedded_interface/bugreport.go | 21 +++++ .../bugreport_mock.go | 63 +++++++++++++++ .../bugreport_test.go | 18 +++++ .../ersatz/ersatz.go | 7 ++ .../import_embedded_interface/faux/faux.go | 7 ++ .../other/ersatz/ersatz.go | 7 ++ mockgen/parse.go | 81 ++++++++++++------- mockgen/parse_test.go | 28 ++++--- 8 files changed, 190 insertions(+), 42 deletions(-) create mode 100644 mockgen/internal/tests/import_embedded_interface/bugreport.go create mode 100644 mockgen/internal/tests/import_embedded_interface/bugreport_mock.go create mode 100644 mockgen/internal/tests/import_embedded_interface/bugreport_test.go create mode 100644 mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go create mode 100644 mockgen/internal/tests/import_embedded_interface/faux/faux.go create mode 100644 mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go diff --git a/mockgen/internal/tests/import_embedded_interface/bugreport.go b/mockgen/internal/tests/import_embedded_interface/bugreport.go new file mode 100644 index 00000000..d09cd55e --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/bugreport.go @@ -0,0 +1,21 @@ +//go:generate mockgen -destination bugreport_mock.go -package bugreport -source=bugreport.go + +package bugreport + +import ( + "log" + + "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/ersatz" + "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/faux" +) + +// Source is an interface w/ an embedded foreign interface +type Source interface { + ersatz.Embedded + faux.Foreign +} + +func CallForeignMethod(s Source) { + log.Println(s.Ersatz()) + log.Println(s.OtherErsatz()) +} diff --git a/mockgen/internal/tests/import_embedded_interface/bugreport_mock.go b/mockgen/internal/tests/import_embedded_interface/bugreport_mock.go new file mode 100644 index 00000000..c618497a --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/bugreport_mock.go @@ -0,0 +1,63 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: bugreport.go + +// Package bugreport is a generated GoMock package. +package bugreport + +import ( + gomock "github.com/golang/mock/gomock" + ersatz "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/ersatz" + ersatz0 "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/other/ersatz" + reflect "reflect" +) + +// MockSource is a mock of Source interface +type MockSource struct { + ctrl *gomock.Controller + recorder *MockSourceMockRecorder +} + +// MockSourceMockRecorder is the mock recorder for MockSource +type MockSourceMockRecorder struct { + mock *MockSource +} + +// NewMockSource creates a new mock instance +func NewMockSource(ctrl *gomock.Controller) *MockSource { + mock := &MockSource{ctrl: ctrl} + mock.recorder = &MockSourceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockSource) EXPECT() *MockSourceMockRecorder { + return m.recorder +} + +// Ersatz mocks base method +func (m *MockSource) Ersatz() ersatz.Return { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Ersatz") + ret0, _ := ret[0].(ersatz.Return) + return ret0 +} + +// Ersatz indicates an expected call of Ersatz +func (mr *MockSourceMockRecorder) Ersatz() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Ersatz", reflect.TypeOf((*MockSource)(nil).Ersatz)) +} + +// OtherErsatz mocks base method +func (m *MockSource) OtherErsatz() ersatz0.Return { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "OtherErsatz") + ret0, _ := ret[0].(ersatz0.Return) + return ret0 +} + +// OtherErsatz indicates an expected call of OtherErsatz +func (mr *MockSourceMockRecorder) OtherErsatz() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OtherErsatz", reflect.TypeOf((*MockSource)(nil).OtherErsatz)) +} diff --git a/mockgen/internal/tests/import_embedded_interface/bugreport_test.go b/mockgen/internal/tests/import_embedded_interface/bugreport_test.go new file mode 100644 index 00000000..99ed6390 --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/bugreport_test.go @@ -0,0 +1,18 @@ +package bugreport + +import ( + "testing" + + "github.com/golang/mock/gomock" +) + +// TestValidInterface assesses whether or not the generated mock is valid +func TestValidInterface(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + s := NewMockSource(ctrl) + s.EXPECT().Ersatz().Return("") + s.EXPECT().OtherErsatz().Return("") + CallForeignMethod(s) +} diff --git a/mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go b/mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go new file mode 100644 index 00000000..4dbdd41d --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go @@ -0,0 +1,7 @@ +package ersatz + +type Embedded interface { + Ersatz() Return +} + +type Return interface{} diff --git a/mockgen/internal/tests/import_embedded_interface/faux/faux.go b/mockgen/internal/tests/import_embedded_interface/faux/faux.go new file mode 100644 index 00000000..4f52cfe7 --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/faux/faux.go @@ -0,0 +1,7 @@ +package faux + +import "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/other/ersatz" + +type Foreign interface { + ersatz.Embedded +} diff --git a/mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go b/mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go new file mode 100644 index 00000000..0d724c4a --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go @@ -0,0 +1,7 @@ +package ersatz + +type Embedded interface { + OtherErsatz() Return +} + +type Return interface{} diff --git a/mockgen/parse.go b/mockgen/parse.go index a8edde80..2bb94b2e 100644 --- a/mockgen/parse.go +++ b/mockgen/parse.go @@ -62,7 +62,7 @@ func sourceMode(source string) (*model.Package, error) { p := &fileParser{ fileSet: fs, - imports: make(map[string]string), + imports: make(map[string]importedPkg), importedInterfaces: make(map[string]map[string]*ast.InterfaceType), auxInterfaces: make(map[string]map[string]*ast.InterfaceType), srcDir: srcDir, @@ -79,7 +79,7 @@ func sourceMode(source string) (*model.Package, error) { dotImports[v] = true } else { // TODO: Catch dupes? - p.imports[k] = v + p.imports[k] = importedPkg{path: v} } } } @@ -100,9 +100,14 @@ func sourceMode(source string) (*model.Package, error) { return pkg, nil } +type importedPkg struct { + path string + parser *fileParser +} + type fileParser struct { fileSet *token.FileSet - imports map[string]string // package name => import path + imports map[string]importedPkg // package name => import path importedInterfaces map[string]map[string]*ast.InterfaceType // package (or "") => name => interface auxFiles []*ast.File @@ -156,7 +161,7 @@ func (p *fileParser) parseFile(importPath string, file *ast.File) (*model.Packag // Don't stomp imports provided by -imports. Those should take precedence. for pkg, pkgPath := range allImports { if _, ok := p.imports[pkg]; !ok { - p.imports[pkg] = pkgPath + p.imports[pkg] = importedPkg{path: pkgPath} } } // Add imports from auxiliary files, which might be needed for embedded interfaces. @@ -165,7 +170,7 @@ func (p *fileParser) parseFile(importPath string, file *ast.File) (*model.Packag auxImports, _ := importsOfFile(f) for pkg, pkgPath := range auxImports { if _, ok := p.imports[pkg]; !ok { - p.imports[pkg] = pkgPath + p.imports[pkg] = importedPkg{path: pkgPath} } } } @@ -186,31 +191,38 @@ func (p *fileParser) parseFile(importPath string, file *ast.File) (*model.Packag }, nil } -// parsePackage loads package specified by path, parses it and populates -// corresponding imports and importedInterfaces into the fileParser. -func (p *fileParser) parsePackage(path string) error { +// parsePackage loads package specified by path, parses it and returns +// a new fileParser with the parsed imports and interfaces. +func (p *fileParser) parsePackage(path string) (*fileParser, error) { + newP := &fileParser{ + fileSet: token.NewFileSet(), + imports: make(map[string]importedPkg), + importedInterfaces: make(map[string]map[string]*ast.InterfaceType), + auxInterfaces: make(map[string]map[string]*ast.InterfaceType), + srcDir: p.srcDir, + } + var pkgs map[string]*ast.Package - if imp, err := build.Import(path, p.srcDir, build.FindOnly); err != nil { - return err - } else if pkgs, err = parser.ParseDir(p.fileSet, imp.Dir, nil, 0); err != nil { - return err + if imp, err := build.Import(path, newP.srcDir, build.FindOnly); err != nil { + return nil, err + } else if pkgs, err = parser.ParseDir(newP.fileSet, imp.Dir, nil, 0); err != nil { + return nil, err } + for _, pkg := range pkgs { file := ast.MergePackageFiles(pkg, ast.FilterFuncDuplicates|ast.FilterUnassociatedComments|ast.FilterImportDuplicates) if _, ok := p.importedInterfaces[path]; !ok { - p.importedInterfaces[path] = make(map[string]*ast.InterfaceType) + newP.importedInterfaces[path] = make(map[string]*ast.InterfaceType) } for ni := range iterInterfaces(file) { - p.importedInterfaces[path][ni.name.Name] = ni.it + newP.importedInterfaces[path][ni.name.Name] = ni.it } imports, _ := importsOfFile(file) for pkgName, pkgPath := range imports { - if _, ok := p.imports[pkgName]; !ok { - p.imports[pkgName] = pkgPath - } + newP.imports[pkgName] = importedPkg{path: pkgPath} } } - return nil + return newP, nil } func (p *fileParser) parseInterface(name, pkg string, it *ast.InterfaceType) (*model.Interface, error) { @@ -252,21 +264,31 @@ func (p *fileParser) parseInterface(name, pkg string, it *ast.InterfaceType) (*m if !ok { return nil, p.errorf(v.X.Pos(), "unknown package %s", fpkg) } + + var eintf *model.Interface + var err error ei := p.auxInterfaces[fpkg][sel] - if ei == nil { - fpkg = epkg - if _, ok = p.importedInterfaces[epkg]; !ok { - if err := p.parsePackage(epkg); err != nil { + if ei != nil { + eintf, err = p.parseInterface(sel, fpkg, ei) + if err != nil { + return nil, err + } + } else { + fpkg = epkg.path + if epkg.parser == nil { + ip, err := p.parsePackage(fpkg) + if err != nil { return nil, p.errorf(v.Pos(), "could not parse package %s: %v", fpkg, err) } + epkg.parser = ip } - if ei = p.importedInterfaces[epkg][sel]; ei == nil { + if ei = epkg.parser.importedInterfaces[fpkg][sel]; ei == nil { return nil, p.errorf(v.Pos(), "unknown embedded interface %s.%s", fpkg, sel) } - } - eintf, err := p.parseInterface(sel, fpkg, ei) - if err != nil { - return nil, err + eintf, err = epkg.parser.parseInterface(sel, fpkg, ei) + if err != nil { + return nil, err + } } // Copy the methods. // TODO: apply shadowing rules. @@ -383,7 +405,7 @@ func (p *fileParser) parseType(pkg string, typ ast.Expr) (model.Type, error) { // if so, patch the import w/ the fully qualified import maybeImportedPkg, ok := p.imports[pkg] if ok { - pkg = maybeImportedPkg + pkg = maybeImportedPkg.path } // assume type in this package return &model.NamedType{Package: pkg, Type: v.Name}, nil @@ -412,7 +434,7 @@ func (p *fileParser) parseType(pkg string, typ ast.Expr) (model.Type, error) { if !ok { return nil, p.errorf(v.Pos(), "unknown package %q", pkgName) } - return &model.NamedType{Package: pkg, Type: v.Sel.String()}, nil + return &model.NamedType{Package: pkg.path, Type: v.Sel.String()}, nil case *ast.StarExpr: t, err := p.parseType(pkg, v.X) if err != nil { @@ -469,7 +491,6 @@ func importsOfFile(file *ast.File) (normalImports map[string]string, dotImports if pkgName == "." { dotImports = append(dotImports, importPath) } else { - if _, ok := normalImports[pkgName]; ok { log.Fatalf("imported package collision: %q imported twice", pkgName) } diff --git a/mockgen/parse_test.go b/mockgen/parse_test.go index dab22fa5..26852368 100644 --- a/mockgen/parse_test.go +++ b/mockgen/parse_test.go @@ -16,7 +16,7 @@ func TestFileParser_ParseFile(t *testing.T) { p := fileParser{ fileSet: fs, - imports: make(map[string]string), + imports: make(map[string]importedPkg), importedInterfaces: make(map[string]map[string]*ast.InterfaceType), } @@ -47,16 +47,16 @@ func TestFileParser_ParsePackage(t *testing.T) { p := fileParser{ fileSet: fs, - imports: make(map[string]string), + imports: make(map[string]importedPkg), importedInterfaces: make(map[string]map[string]*ast.InterfaceType), } - err = p.parsePackage("github.com/golang/mock/mockgen/internal/tests/custom_package_name/greeter") + newP, err := p.parsePackage("github.com/golang/mock/mockgen/internal/tests/custom_package_name/greeter") if err != nil { t.Fatalf("Unexpected error: %v", err) } - checkGreeterImports(t, p.imports) + checkGreeterImports(t, newP.imports) } func TestImportsOfFile(t *testing.T) { @@ -67,17 +67,21 @@ func TestImportsOfFile(t *testing.T) { } imports, _ := importsOfFile(file) - checkGreeterImports(t, imports) + importedPkgs := make(map[string]importedPkg, len(imports)) + for pkg, path := range imports { + importedPkgs[pkg] = importedPkg{path: path} + } + checkGreeterImports(t, importedPkgs) } -func checkGreeterImports(t *testing.T, imports map[string]string) { +func checkGreeterImports(t *testing.T, imports map[string]importedPkg) { // check that imports have stdlib package "fmt" if fmtPackage, ok := imports["fmt"]; !ok { t.Errorf("Expected imports to have key \"fmt\"") } else { expectedFmtPackage := "fmt" - if fmtPackage != expectedFmtPackage { - t.Errorf("Expected fmt key to have value %s but got %s", expectedFmtPackage, fmtPackage) + if fmtPackage.path != expectedFmtPackage { + t.Errorf("Expected fmt key to have value %s but got %s", expectedFmtPackage, fmtPackage.path) } } @@ -86,8 +90,8 @@ func checkGreeterImports(t *testing.T, imports map[string]string) { t.Errorf("Expected imports to have key \"fmt\"") } else { expectedValidatorPackage := "github.com/golang/mock/mockgen/internal/tests/custom_package_name/validator" - if validatorPackage != expectedValidatorPackage { - t.Errorf("Expected validator key to have value %s but got %s", expectedValidatorPackage, validatorPackage) + if validatorPackage.path != expectedValidatorPackage { + t.Errorf("Expected validator key to have value %s but got %s", expectedValidatorPackage, validatorPackage.path) } } @@ -96,8 +100,8 @@ func checkGreeterImports(t *testing.T, imports map[string]string) { t.Errorf("Expected imports to have key \"client\"") } else { expectedClientPackage := "github.com/golang/mock/mockgen/internal/tests/custom_package_name/client/v1" - if clientPackage != expectedClientPackage { - t.Errorf("Expected client key to have value %s but got %s", expectedClientPackage, clientPackage) + if clientPackage.path != expectedClientPackage { + t.Errorf("Expected client key to have value %s but got %s", expectedClientPackage, clientPackage.path) } } From 13f59c046c784f3953e62617cfa831b7139ffb9a Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Mon, 24 Feb 2020 01:46:51 +0000 Subject: [PATCH 2/6] mockgen: defer failing on conflicting imports In source mode, all package files are merged before parsing. This causes a problem for imports since imports are file scoped. Thus, some valid packages fail to parse with an error about duplicate imports. I think that fixing this completely will take some large restructuring of the parser. However, here I expand the number of packages we can parse by only raising the duplicate package error if conflicting package is actually accessed. For example, we would previously fail to generate a mock for the following file, but now it succeeds: package service import "google.golang.org/grpc" type Test interface { grpc.ClientStream } Signed-off-by: Steven Danna --- .../faux/conflict.go | 7 ++ .../import_embedded_interface/faux/faux.go | 10 ++- .../other/log/log.go | 3 + mockgen/parse.go | 90 ++++++++++++++----- mockgen/parse_test.go | 24 +++-- 5 files changed, 95 insertions(+), 39 deletions(-) create mode 100644 mockgen/internal/tests/import_embedded_interface/faux/conflict.go create mode 100644 mockgen/internal/tests/import_embedded_interface/other/log/log.go diff --git a/mockgen/internal/tests/import_embedded_interface/faux/conflict.go b/mockgen/internal/tests/import_embedded_interface/faux/conflict.go new file mode 100644 index 00000000..b7413d7c --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/faux/conflict.go @@ -0,0 +1,7 @@ +package faux + +import "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/other/log" + +func Conflict1() { + log.Foo() +} diff --git a/mockgen/internal/tests/import_embedded_interface/faux/faux.go b/mockgen/internal/tests/import_embedded_interface/faux/faux.go index 4f52cfe7..9f138c21 100644 --- a/mockgen/internal/tests/import_embedded_interface/faux/faux.go +++ b/mockgen/internal/tests/import_embedded_interface/faux/faux.go @@ -1,7 +1,15 @@ package faux -import "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/other/ersatz" +import ( + "log" + + "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/other/ersatz" +) type Foreign interface { ersatz.Embedded } + +func Conflict0() { + log.Println() +} diff --git a/mockgen/internal/tests/import_embedded_interface/other/log/log.go b/mockgen/internal/tests/import_embedded_interface/other/log/log.go new file mode 100644 index 00000000..0f25a63d --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/other/log/log.go @@ -0,0 +1,3 @@ +package log + +func Foo() {} diff --git a/mockgen/parse.go b/mockgen/parse.go index 2bb94b2e..e373923f 100644 --- a/mockgen/parse.go +++ b/mockgen/parse.go @@ -62,7 +62,7 @@ func sourceMode(source string) (*model.Package, error) { p := &fileParser{ fileSet: fs, - imports: make(map[string]importedPkg), + imports: make(map[string]importedPackage), importedInterfaces: make(map[string]map[string]*ast.InterfaceType), auxInterfaces: make(map[string]map[string]*ast.InterfaceType), srcDir: srcDir, @@ -100,14 +100,39 @@ func sourceMode(source string) (*model.Package, error) { return pkg, nil } +type importedPackage interface { + Path() string + Parser() *fileParser +} + type importedPkg struct { path string parser *fileParser } +func (i importedPkg) Path() string { return i.path } +func (i importedPkg) Parser() *fileParser { return i.parser } + +// duplicateImport is a bit of a misnomer. Currently the parser can't +// handle cases of multi-file packages importing different packages +// under the same name. Often these imports would not be problematic, +// so this type lets us defer raising an error unless the package name +// is actually used. +type duplicateImport struct { + name string + duplicates []string +} + +func (d duplicateImport) Error() string { + return fmt.Sprintf("%q is ambigous because of duplicate imports: %v", d.name, d.duplicates) +} + +func (d duplicateImport) Path() string { log.Fatal(d.Error()); return "" } +func (d duplicateImport) Parser() *fileParser { log.Fatal(d.Error()); return nil } + type fileParser struct { fileSet *token.FileSet - imports map[string]importedPkg // package name => import path + imports map[string]importedPackage // package name => imported package importedInterfaces map[string]map[string]*ast.InterfaceType // package (or "") => name => interface auxFiles []*ast.File @@ -159,18 +184,18 @@ func (p *fileParser) addAuxInterfacesFromFile(pkg string, file *ast.File) { func (p *fileParser) parseFile(importPath string, file *ast.File) (*model.Package, error) { allImports, dotImports := importsOfFile(file) // Don't stomp imports provided by -imports. Those should take precedence. - for pkg, pkgPath := range allImports { + for pkg, pkgI := range allImports { if _, ok := p.imports[pkg]; !ok { - p.imports[pkg] = importedPkg{path: pkgPath} + p.imports[pkg] = pkgI } } // Add imports from auxiliary files, which might be needed for embedded interfaces. // Don't stomp any other imports. for _, f := range p.auxFiles { auxImports, _ := importsOfFile(f) - for pkg, pkgPath := range auxImports { + for pkg, pkgI := range auxImports { if _, ok := p.imports[pkg]; !ok { - p.imports[pkg] = importedPkg{path: pkgPath} + p.imports[pkg] = pkgI } } } @@ -196,7 +221,7 @@ func (p *fileParser) parseFile(importPath string, file *ast.File) (*model.Packag func (p *fileParser) parsePackage(path string) (*fileParser, error) { newP := &fileParser{ fileSet: token.NewFileSet(), - imports: make(map[string]importedPkg), + imports: make(map[string]importedPackage), importedInterfaces: make(map[string]map[string]*ast.InterfaceType), auxInterfaces: make(map[string]map[string]*ast.InterfaceType), srcDir: p.srcDir, @@ -218,8 +243,8 @@ func (p *fileParser) parsePackage(path string) (*fileParser, error) { newP.importedInterfaces[path][ni.name.Name] = ni.it } imports, _ := importsOfFile(file) - for pkgName, pkgPath := range imports { - newP.imports[pkgName] = importedPkg{path: pkgPath} + for pkgName, pkgI := range imports { + newP.imports[pkgName] = pkgI } } return newP, nil @@ -274,18 +299,23 @@ func (p *fileParser) parseInterface(name, pkg string, it *ast.InterfaceType) (*m return nil, err } } else { - fpkg = epkg.path - if epkg.parser == nil { - ip, err := p.parsePackage(fpkg) + path := epkg.Path() + parser := epkg.Parser() + if parser == nil { + ip, err := p.parsePackage(path) if err != nil { - return nil, p.errorf(v.Pos(), "could not parse package %s: %v", fpkg, err) + return nil, p.errorf(v.Pos(), "could not parse package %s: %v", path, err) + } + parser = ip + p.imports[fpkg] = importedPkg{ + path: epkg.Path(), + parser: parser, } - epkg.parser = ip } - if ei = epkg.parser.importedInterfaces[fpkg][sel]; ei == nil { - return nil, p.errorf(v.Pos(), "unknown embedded interface %s.%s", fpkg, sel) + if ei = parser.importedInterfaces[path][sel]; ei == nil { + return nil, p.errorf(v.Pos(), "unknown embedded interface %s.%s", path, sel) } - eintf, err = epkg.parser.parseInterface(sel, fpkg, ei) + eintf, err = parser.parseInterface(sel, path, ei) if err != nil { return nil, err } @@ -405,7 +435,7 @@ func (p *fileParser) parseType(pkg string, typ ast.Expr) (model.Type, error) { // if so, patch the import w/ the fully qualified import maybeImportedPkg, ok := p.imports[pkg] if ok { - pkg = maybeImportedPkg.path + pkg = maybeImportedPkg.Path() } // assume type in this package return &model.NamedType{Package: pkg, Type: v.Name}, nil @@ -434,7 +464,7 @@ func (p *fileParser) parseType(pkg string, typ ast.Expr) (model.Type, error) { if !ok { return nil, p.errorf(v.Pos(), "unknown package %q", pkgName) } - return &model.NamedType{Package: pkg.path, Type: v.Sel.String()}, nil + return &model.NamedType{Package: pkg.Path(), Type: v.Sel.String()}, nil case *ast.StarExpr: t, err := p.parseType(pkg, v.X) if err != nil { @@ -453,7 +483,7 @@ func (p *fileParser) parseType(pkg string, typ ast.Expr) (model.Type, error) { // importsOfFile returns a map of package name to import path // of the imports in file. -func importsOfFile(file *ast.File) (normalImports map[string]string, dotImports []string) { +func importsOfFile(file *ast.File) (normalImports map[string]importedPackage, dotImports []string) { var importPaths []string for _, is := range file.Imports { if is.Name != nil { @@ -463,7 +493,7 @@ func importsOfFile(file *ast.File) (normalImports map[string]string, dotImports importPaths = append(importPaths, importPath) } packagesName := createPackageMap(importPaths) - normalImports = make(map[string]string) + normalImports = make(map[string]importedPackage) dotImports = make([]string, 0) for _, is := range file.Imports { var pkgName string @@ -491,10 +521,22 @@ func importsOfFile(file *ast.File) (normalImports map[string]string, dotImports if pkgName == "." { dotImports = append(dotImports, importPath) } else { - if _, ok := normalImports[pkgName]; ok { - log.Fatalf("imported package collision: %q imported twice", pkgName) + if pkg, ok := normalImports[pkgName]; ok { + switch p := pkg.(type) { + case duplicateImport: + normalImports[pkgName] = duplicateImport{ + name: p.name, + duplicates: append([]string{importPath}, p.duplicates...), + } + case importedPkg: + normalImports[pkgName] = duplicateImport{ + name: pkgName, + duplicates: []string{p.path, importPath}, + } + } + } else { + normalImports[pkgName] = importedPkg{path: importPath} } - normalImports[pkgName] = importPath } } return diff --git a/mockgen/parse_test.go b/mockgen/parse_test.go index 26852368..e555b45e 100644 --- a/mockgen/parse_test.go +++ b/mockgen/parse_test.go @@ -16,7 +16,7 @@ func TestFileParser_ParseFile(t *testing.T) { p := fileParser{ fileSet: fs, - imports: make(map[string]importedPkg), + imports: make(map[string]importedPackage), importedInterfaces: make(map[string]map[string]*ast.InterfaceType), } @@ -47,7 +47,7 @@ func TestFileParser_ParsePackage(t *testing.T) { p := fileParser{ fileSet: fs, - imports: make(map[string]importedPkg), + imports: make(map[string]importedPackage), importedInterfaces: make(map[string]map[string]*ast.InterfaceType), } @@ -67,21 +67,17 @@ func TestImportsOfFile(t *testing.T) { } imports, _ := importsOfFile(file) - importedPkgs := make(map[string]importedPkg, len(imports)) - for pkg, path := range imports { - importedPkgs[pkg] = importedPkg{path: path} - } - checkGreeterImports(t, importedPkgs) + checkGreeterImports(t, imports) } -func checkGreeterImports(t *testing.T, imports map[string]importedPkg) { +func checkGreeterImports(t *testing.T, imports map[string]importedPackage) { // check that imports have stdlib package "fmt" if fmtPackage, ok := imports["fmt"]; !ok { t.Errorf("Expected imports to have key \"fmt\"") } else { expectedFmtPackage := "fmt" - if fmtPackage.path != expectedFmtPackage { - t.Errorf("Expected fmt key to have value %s but got %s", expectedFmtPackage, fmtPackage.path) + if fmtPackage.Path() != expectedFmtPackage { + t.Errorf("Expected fmt key to have value %s but got %s", expectedFmtPackage, fmtPackage.Path()) } } @@ -90,8 +86,8 @@ func checkGreeterImports(t *testing.T, imports map[string]importedPkg) { t.Errorf("Expected imports to have key \"fmt\"") } else { expectedValidatorPackage := "github.com/golang/mock/mockgen/internal/tests/custom_package_name/validator" - if validatorPackage.path != expectedValidatorPackage { - t.Errorf("Expected validator key to have value %s but got %s", expectedValidatorPackage, validatorPackage.path) + if validatorPackage.Path() != expectedValidatorPackage { + t.Errorf("Expected validator key to have value %s but got %s", expectedValidatorPackage, validatorPackage.Path()) } } @@ -100,8 +96,8 @@ func checkGreeterImports(t *testing.T, imports map[string]importedPkg) { t.Errorf("Expected imports to have key \"client\"") } else { expectedClientPackage := "github.com/golang/mock/mockgen/internal/tests/custom_package_name/client/v1" - if clientPackage.path != expectedClientPackage { - t.Errorf("Expected client key to have value %s but got %s", expectedClientPackage, clientPackage.path) + if clientPackage.Path() != expectedClientPackage { + t.Errorf("Expected client key to have value %s but got %s", expectedClientPackage, clientPackage.Path()) } } From ab7489301d0f75e26d8d4b9c671f51f4b5ea69e0 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Sat, 29 Feb 2020 11:12:33 +0000 Subject: [PATCH 3/6] mockgen/parser: check correct imported interfaces map We were checking the parent fileParser rather than the new fileParser we were creating, leading to some non-deterministic failures. Signed-off-by: Steven Danna --- mockgen/parse.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mockgen/parse.go b/mockgen/parse.go index e373923f..7840eed6 100644 --- a/mockgen/parse.go +++ b/mockgen/parse.go @@ -236,7 +236,7 @@ func (p *fileParser) parsePackage(path string) (*fileParser, error) { for _, pkg := range pkgs { file := ast.MergePackageFiles(pkg, ast.FilterFuncDuplicates|ast.FilterUnassociatedComments|ast.FilterImportDuplicates) - if _, ok := p.importedInterfaces[path]; !ok { + if _, ok := newP.importedInterfaces[path]; !ok { newP.importedInterfaces[path] = make(map[string]*ast.InterfaceType) } for ni := range iterInterfaces(file) { From df8aaa28bc72a049628461360109fc323f16cb63 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Sat, 29 Feb 2020 11:17:25 +0000 Subject: [PATCH 4/6] add copyright notices to test files Signed-off-by: Steven Danna --- .../tests/import_embedded_interface/bugreport.go | 14 ++++++++++++++ .../import_embedded_interface/bugreport_test.go | 13 +++++++++++++ .../import_embedded_interface/ersatz/ersatz.go | 13 +++++++++++++ .../import_embedded_interface/faux/conflict.go | 13 +++++++++++++ .../tests/import_embedded_interface/faux/faux.go | 13 +++++++++++++ .../other/ersatz/ersatz.go | 13 +++++++++++++ .../import_embedded_interface/other/log/log.go | 13 +++++++++++++ 7 files changed, 92 insertions(+) diff --git a/mockgen/internal/tests/import_embedded_interface/bugreport.go b/mockgen/internal/tests/import_embedded_interface/bugreport.go index d09cd55e..c6d73f0f 100644 --- a/mockgen/internal/tests/import_embedded_interface/bugreport.go +++ b/mockgen/internal/tests/import_embedded_interface/bugreport.go @@ -1,3 +1,17 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// //go:generate mockgen -destination bugreport_mock.go -package bugreport -source=bugreport.go package bugreport diff --git a/mockgen/internal/tests/import_embedded_interface/bugreport_test.go b/mockgen/internal/tests/import_embedded_interface/bugreport_test.go index 99ed6390..5915cbe1 100644 --- a/mockgen/internal/tests/import_embedded_interface/bugreport_test.go +++ b/mockgen/internal/tests/import_embedded_interface/bugreport_test.go @@ -1,3 +1,16 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package bugreport import ( diff --git a/mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go b/mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go index 4dbdd41d..0d2c7815 100644 --- a/mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go +++ b/mockgen/internal/tests/import_embedded_interface/ersatz/ersatz.go @@ -1,3 +1,16 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package ersatz type Embedded interface { diff --git a/mockgen/internal/tests/import_embedded_interface/faux/conflict.go b/mockgen/internal/tests/import_embedded_interface/faux/conflict.go index b7413d7c..27803dd6 100644 --- a/mockgen/internal/tests/import_embedded_interface/faux/conflict.go +++ b/mockgen/internal/tests/import_embedded_interface/faux/conflict.go @@ -1,3 +1,16 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package faux import "github.com/golang/mock/mockgen/internal/tests/import_embedded_interface/other/log" diff --git a/mockgen/internal/tests/import_embedded_interface/faux/faux.go b/mockgen/internal/tests/import_embedded_interface/faux/faux.go index 9f138c21..e1d66988 100644 --- a/mockgen/internal/tests/import_embedded_interface/faux/faux.go +++ b/mockgen/internal/tests/import_embedded_interface/faux/faux.go @@ -1,3 +1,16 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package faux import ( diff --git a/mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go b/mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go index 0d724c4a..9ca63914 100644 --- a/mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go +++ b/mockgen/internal/tests/import_embedded_interface/other/ersatz/ersatz.go @@ -1,3 +1,16 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package ersatz type Embedded interface { diff --git a/mockgen/internal/tests/import_embedded_interface/other/log/log.go b/mockgen/internal/tests/import_embedded_interface/other/log/log.go index 0f25a63d..c0d032bf 100644 --- a/mockgen/internal/tests/import_embedded_interface/other/log/log.go +++ b/mockgen/internal/tests/import_embedded_interface/other/log/log.go @@ -1,3 +1,16 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package log func Foo() {} From e3dc0d5b75a23c41f62df2270326ca5f57a3dfff Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Sat, 29 Feb 2020 11:35:46 +0000 Subject: [PATCH 5/6] add test case for http.ResponseWriter This is a common case of an imported interface that only depends on the standard library. Signed-off-by: Steven Danna --- .../tests/import_embedded_interface/net.go | 25 +++++++ .../import_embedded_interface/net_mock.go | 75 +++++++++++++++++++ .../import_embedded_interface/net_test.go | 30 ++++++++ 3 files changed, 130 insertions(+) create mode 100644 mockgen/internal/tests/import_embedded_interface/net.go create mode 100644 mockgen/internal/tests/import_embedded_interface/net_mock.go create mode 100644 mockgen/internal/tests/import_embedded_interface/net_test.go diff --git a/mockgen/internal/tests/import_embedded_interface/net.go b/mockgen/internal/tests/import_embedded_interface/net.go new file mode 100644 index 00000000..38fbd039 --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/net.go @@ -0,0 +1,25 @@ +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +//go:generate mockgen -destination net_mock.go -package bugreport -source=net.go +package bugreport + +import "net/http" + +type Net interface { + http.ResponseWriter +} + +func CallResponseWriterMethods(n Net) { + n.WriteHeader(10) +} diff --git a/mockgen/internal/tests/import_embedded_interface/net_mock.go b/mockgen/internal/tests/import_embedded_interface/net_mock.go new file mode 100644 index 00000000..50ec11ec --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/net_mock.go @@ -0,0 +1,75 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: net.go + +// Package bugreport is a generated GoMock package. +package bugreport + +import ( + gomock "github.com/golang/mock/gomock" + http "net/http" + reflect "reflect" +) + +// MockNet is a mock of Net interface +type MockNet struct { + ctrl *gomock.Controller + recorder *MockNetMockRecorder +} + +// MockNetMockRecorder is the mock recorder for MockNet +type MockNetMockRecorder struct { + mock *MockNet +} + +// NewMockNet creates a new mock instance +func NewMockNet(ctrl *gomock.Controller) *MockNet { + mock := &MockNet{ctrl: ctrl} + mock.recorder = &MockNetMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockNet) EXPECT() *MockNetMockRecorder { + return m.recorder +} + +// Header mocks base method +func (m *MockNet) Header() http.Header { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Header") + ret0, _ := ret[0].(http.Header) + return ret0 +} + +// Header indicates an expected call of Header +func (mr *MockNetMockRecorder) Header() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Header", reflect.TypeOf((*MockNet)(nil).Header)) +} + +// Write mocks base method +func (m *MockNet) Write(arg0 []byte) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Write", arg0) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Write indicates an expected call of Write +func (mr *MockNetMockRecorder) Write(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Write", reflect.TypeOf((*MockNet)(nil).Write), arg0) +} + +// WriteHeader mocks base method +func (m *MockNet) WriteHeader(statusCode int) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "WriteHeader", statusCode) +} + +// WriteHeader indicates an expected call of WriteHeader +func (mr *MockNetMockRecorder) WriteHeader(statusCode interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WriteHeader", reflect.TypeOf((*MockNet)(nil).WriteHeader), statusCode) +} diff --git a/mockgen/internal/tests/import_embedded_interface/net_test.go b/mockgen/internal/tests/import_embedded_interface/net_test.go new file mode 100644 index 00000000..381f7de3 --- /dev/null +++ b/mockgen/internal/tests/import_embedded_interface/net_test.go @@ -0,0 +1,30 @@ +// Copyright 2020 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package bugreport + +import ( + "testing" + + "github.com/golang/mock/gomock" +) + +// TestValidInterface assesses whether or not the generated mock is valid +func TestValidNetInterface(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + s := NewMockNet(ctrl) + s.EXPECT().WriteHeader(10) + CallResponseWriterMethods(s) +} From c36ba0e56797ff92b8ad881d197bc38b23369d4a Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Sat, 29 Feb 2020 11:40:19 +0000 Subject: [PATCH 6/6] update go.sum Signed-off-by: Steven Danna --- go.sum | 1 + 1 file changed, 1 insertion(+) diff --git a/go.sum b/go.sum index 21dbce53..0549f947 100644 --- a/go.sum +++ b/go.sum @@ -1,4 +1,5 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/net v0.0.0-20190311183353-d8887717615a h1:oWX7TPOiFAMXLq8o0ikBYfCJVlRHBcsciT5bXOrH628= golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=