mockgen: handle more cases of "duplicate" imports #405
Conversation
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 <steve@chef.io>
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 <steve@chef.io>
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Do you think it would be possible to address this use case as well in this PR? Before your changes this would have manifested as: type Foo interface {
http.ResponseWriter
} |
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 <steve@chef.io>
Signed-off-by: Steven Danna <steve@chef.io>
This is a common case of an imported interface that only depends on the standard library. Signed-off-by: Steven Danna <steve@chef.io>
Signed-off-by: Steven Danna <steve@chef.io>
|
@codyoss That's a great test case, thanks! It caught a bug for which I've pushed a fix. I've added that example to the mockgen integration tests. There is still a problem with interfaces from dot imports, but since this is also a limitation on master, I think I'd like to defer addressing it for now. |
|
Heya, no rush here, just wondering if there is anything else y'all need from me on this one. |
|
Yes, I plan on getting a release out sometime next week. Just need to do a little cleanup before then 😄 |
|
Could you please release this PR? |
|
@codyoss I don't see this PR included in the latest release, any updates? |
|
Sorry, about sluggishness here. Once #478 has landed I plan to cut a formal release. In the meantime if this is a blocker for you can always pin to HEAD temporarily: |
|
Thanks! That's what we've been doing but was just checking if we can get back to an official release soon. |
|
I will be sure to ping this thread when I have drafted a release. |
|
This too far too long, but here it is: https://github.com/golang/mock/releases/tag/v1.5.0 |
Description
This partially addresses two issues related to how the source-mode parser handles embedded interfaces from other packages. Each is addressed in their own commit, but I've submitted this as a single PR since it is modifying very similar code. The problems this addresses are:
Incorrect generation caused by packages with the same name
When an interface embeds other interfaces that happen to refer to packages with the same name, we would previously generate incorrect mocks. For example, given following code:
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.
We solve this by creating a tree of
fileParserobjects rather than merging all imports and interfaces into a single set of maps. This helps ensure that transitive imports from the source file's dependencies don't erroneously conflict with any top-level imports.Fatal Error on duplicate imports
Previously, it was possible to get the following error:
From a multi-file package that happens to import two packages named "log." While the package itself is valid, the parser can't currently handle this case because it merges all imports into a single map. Fully fixing this issue will likely require larger changes. Here, we cover what I think will be many of the common cases of hitting this error by deferring the error until the ambiguous package name is actually referenced. This is enough to handle the following common case which currently results in this error:
Partially fixes #156
_
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
mockgen/internal/tests/import_embedded_interface/Reviewer Notes
Release Notes