Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: add imports for Named Type its Instance Types #199

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

TimVosch
Copy link
Contributor

@TimVosch TimVosch commented Jun 20, 2023

Fixes missing imports and package prefixing to generic types. See #177

In the following example a method must be mocked which has a return parameter that has a generic type: otherpackage.Foo.

package genericreturn

import "github.com/matryer/moq/pkg/moq/testpackages/genericreturn/otherpackage"

type GenericBar[T any] struct {
	Bar T
}

type IFooBar interface {
	Foobar() GenericBar[otherpackage.Foo]
}
package otherpackage

type Foo struct {
	A int
	B string
}

Currently mocking IFooBar will result in a missing otherpackage import and the package prefix to Foo. Note: FoobarFunc: func() GenericBar[Foo] instead of FoobarFunc: func() GenericBar[otherpackage.Foo].

// IFooBarMock is a mock implementation of IFooBar.
//
//	func TestSomethingThatUsesIFooBar(t *testing.T) {
//
//		// make and configure a mocked IFooBar
//		mockedIFooBar := &IFooBarMock{
//			FoobarFunc: func() GenericBar[Foo] {
//				panic("mock out the Foobar method")
//			},
//		}
//
//		// use mockedIFooBar in code that requires IFooBar
//		// and then make assertions.
//
//	}

This happens because of an invalid key lookup here, which returns an empty string "": internal/registry/var.go

This is fixed by checking if a Named Type has type arguments, and populating imports for those types.
internal/registry/method_scope.go

func (m MethodScope) populateImports(t types.Type, imports map[string]*Package) {
	switch t := t.(type) {
	case *types.Named:
		...
		// The imports of a Type with a TypeList must be added to the imports list
		// For example: Foo[otherpackage.Bar] , must have otherpackage imported
		if targs := t.TypeArgs(); targs != nil {
			for i := 0; i < targs.Len(); i++ {
				m.populateImports(targs.At(i), imports)
			}
		}

A new golden test was added for this specific case.

Closes #177
Closes #190

@sudo-suhas
Copy link
Collaborator

Expected error message in the TestParseError test is couldn't load source package: ~/moq/pkg/moq/testpackages/_parseerror/service/service.go:3:8: could not import github.com/matryer/notexist (invalid package name: "").

Not sure why the error is couldn't load source package: err: exit status 1: stderr: go build github.com/matryer/notexist: service.go:3:8: cannot find package "." in: ~/moq/pkg/moq/testpackages/vendor/github.com/matryer/notexist only on Go 1.19.

@TimVosch
Copy link
Contributor Author

TimVosch commented Jun 21, 2023

Hmm, this also appears to happen on the main branch. I expect it's an issue unrelated to this PR.
I'll see if I can find something.

``` ╭─timvosch@XPS15-ARCH ~/src/moq ‹373b8f9› ╰─➤ docker run --rm -it -v `pwd`:/project --workdir /project golang:1.19 go test ./... go: downloading github.com/pmezard/go-difflib v1.0.0 go: downloading golang.org/x/tools v0.3.0 go: downloading golang.org/x/mod v0.7.0 go: downloading golang.org/x/sys v0.2.0 ? github.com/matryer/moq [no test files] ok github.com/matryer/moq/example 0.001s [no tests to run] ? github.com/matryer/moq/generate [no test files] ? github.com/matryer/moq/internal/registry [no test files] ok github.com/matryer/moq/internal/template 0.001s --- FAIL: TestGoGenerateVendoredPackages (0.00s) moq_test.go:629: Wait: exit status 1 --- FAIL: TestParseError (0.01s) moq_test.go:663: unexpected error: couldn't load source package: err: exit status 1: stderr: go build github.com/matryer/notexist: service.go:3:8: cannot find package "." in: /project/pkg/moq/testpackages/vendor/github.com/matryer/notexist FAIL FAIL github.com/matryer/moq/pkg/moq 6.336s FAIL ```

Edit: Actually installing the moq package in the docker container runs the tests on main succesfully now, but only for <= 1.19.9

docker run --rm -it -v `pwd`:/project --workdir /project golang:1.19.0 sh -c 'go version && git --version && go install -buildvcs=false . && go test ./...'

Specifically version 1.19.10 does not work. All minor revisions below that work. Since it was released just two weeks ago, that is why it probably wasn't picked up earlier. I am going through the changelog to see why. I expected the error message just changed and the test needs an update.

docker run --rm -it -v `pwd`:/project --workdir /project golang:1.19.10 sh -c 'git -v && go install -buildvcs=false . && go test ./...'

Edit 2: Between go 1.19.9 and 1.19.10 the following change happened. I expect that the test should expect a build error instead of the import error, but only for go 1.19.10.

diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go
index c24d210711..567f045922 100644
--- a/src/cmd/go/internal/work/exec.go
+++ b/src/cmd/go/internal/work/exec.go
@@ -528,6 +528,12 @@ func (b *Builder) build(ctx context.Context, a *Action) (err error) {
                b.Print(a.Package.ImportPath + "\n")
        }
 
+       if p.Error != nil {
+               // Don't try to build anything for packages with errors. There may be a
+               // problem with the inputs that makes the package unsafe to build.
+               return p.Error
+       }
+
        if a.Package.BinaryOnly {
                p.Stale = true
                p.StaleReason = "binary-only packages are no longer supported"

@sudo-suhas I am unsure what to do with this, do you have an idea?

@sudo-suhas
Copy link
Collaborator

We can add an additional clause for the error string check to cover errors from both Go 1.18 and Go 1.19.

@TimVosch
Copy link
Contributor Author

I updated the test to also check for stderr: go build github.com/matryer/notexist

@sudo-suhas sudo-suhas merged commit 3795b74 into matryer:main Jun 21, 2023
4 checks passed
@sudo-suhas
Copy link
Collaborator

This change has been released in v0.3.2 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moq ignores package names of arguments of generic types
2 participants