Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

mockgen: handle more cases of "duplicate" imports #405

Merged

Conversation

stevendanna
Copy link
Contributor

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:

// source
import (
    "some.org/package/foo"
    "some.org/package/bar"
)

type Source interface {
    foo.Foo
    bar.Bar
}
// some.org/package/foo
package foo

import "some.org/package/foo/internal/bar"

type Foreign interface {
    bar.InternalBar
}
// some.org/package/bar

type Bar interface {
    TopLevelBarMethod() string
}
// 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.

We solve this by creating a tree of fileParser objects 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:

2020/02/24 10:22:25 imported package collision: "log" imported twice

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:

package service

import "google.golang.org/grpc"

type Test interface {
        grpc.ClientStream
}

Partially fixes #156
_

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests: I've added a new integration-style test in mockgen/internal/tests/import_embedded_interface/

Reviewer Notes

  • The code flow looks good.
  • Tests added.

Release Notes

- Improved handling of embedded interfaces in source-mode parser.

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>
@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@stevendanna
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@codyoss codyoss self-requested a review February 24, 2020 15:17
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution, this is awesome. I am going to get one more set of eyes on this as well.

@codyoss codyoss requested a review from cvgw February 25, 2020 04:49
@codyoss
Copy link
Member

codyoss commented Feb 28, 2020

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: imported package collision: "rand" imported twice

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>
@stevendanna
Copy link
Contributor Author

@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.

@minicuts minicuts self-requested a review March 4, 2020 16:31
@stevendanna
Copy link
Contributor Author

Heya, no rush here, just wondering if there is anything else y'all need from me on this one.

Copy link
Collaborator

@cvgw cvgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the contribution

@codyoss codyoss merged commit 8a3d595 into golang:master Apr 6, 2020
@CecileRobertMichon
Copy link

Hi there 👋Thanks for the fix! Are there any plans to include this in a release?

@codyoss @cvgw

@codyoss
Copy link
Member

codyoss commented Jun 23, 2020

Yes, I plan on getting a release out sometime next week. Just need to do a little cleanup before then 😄

@ledmonster
Copy link

Could you please release this PR?

@CecileRobertMichon
Copy link

@codyoss I don't see this PR included in the latest release, any updates?

@codyoss
Copy link
Member

codyoss commented Sep 8, 2020

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: go get github.com/golang/mock@HEAD

@CecileRobertMichon
Copy link

Thanks! That's what we've been doing but was just checking if we can get back to an official release soon.

@codyoss
Copy link
Member

codyoss commented Sep 8, 2020

I will be sure to ping this thread when I have drafted a release.

@codyoss
Copy link
Member

codyoss commented Feb 20, 2021

This too far too long, but here it is: https://github.com/golang/mock/releases/tag/v1.5.0

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

Successfully merging this pull request may close these issues.

handle golang.org/x/net/context & context import collisions with source mode
6 participants