separate go:generate logic from counterfeiter:generate logic

[rittneje]

Fixes #166.

@joefitzgerald According to the usage message from counterfeiter, the -generate flag is supposed to cause it to evaluate all and only the //counterfeiter:generate directives in the current package.

-generate
Identify all //counterfeiter:generate directives in .go file in the
current working directory and generate fakes for them. You can pass
arguments as usual.

  NOTE: This is not the same as //go:generate directives
  (used with the 'go generate' command), but it can be combined with
  go generate by adding the following to a .go file:

  # runs counterfeiter in generate mode
  //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate

Consequently, the existing behavior of trying to also include //go:generate counterfeiter directives in the same bulk request is counter to the documented behavior. Additionally, running counterfeiter without the -generate flag should not trigger any such bulk request logic (the legacy behavior).

As such, I have untangled the legacy logic from the -generate logic, which simplifies things greatly. In short:

1. If the -generate flag was passed, collect all //countefeiter:generate directives, and execute them.
2. Else, only generate what was specifically requested via the command line arguments.

[joefitzgerald]

@rittneje thanks so much for your contribution! 🙏🙏🙏

The current behavior was introduced to counteract the large regression (1-2 orders of magnitude) in speed associated with the use of go/packages (required for support of go modules). Because the tool is invoked for each go:generate directive in a package, this slowdown becomes very pronounced for invocations 2..n in the same package.

It's clear that there is an issue with the regular expression being used to identify go generate directives, which should be fixed regardless.

I am torn here on what to do:

• When there is a single go:generate directive in a package, our logic that scans all files for invocations slows down the initial invocation by a small number of milliseconds.
• When there is more than a singlego:generate directive in a package, our logic speeds up the total runtime of go generate by seconds per additional invocation.
• The logic that parses files for go:generate directives introduces a new source of bugs (as evidenced by this issue).

I am sympathetic to the argument that we should use the generate flag to opt into this behavior. But I also really don't want to slow down the large number of people who a) have more than one go:generate directive in a package (let alone in multiple packages) and b) are unaware of the flag.

[rittneje]

@joefitzgerald I don't understand how the existing logic helps people who are using the classic go:generate directive at all. Since go generate itself does not know that you are handling all generations in one invocation, it's still going to call counterfeiter once per go:generate directive. In fact, I would expect it to be significantly hurting performance, because if there are N directives in a package, you will handle all N of them N times, for a total of N^N generations.

[joefitzgerald]

The way it is implemented, only the first go:generate directive will result in invocations being returned. All subsequent invocations of counterfeiter by go generate are a no-op (

counterfeiter/command/runner.go

Line 80 in eb18930

if len(result) > 0 && result[0].File != os.Getenv("GOFILE") {

), precisely to avoid impacting performance.

Said another way: Using go/packages to load package type information n times (once per invocation of the tool by go:generate) is orders of magnitude more expensive than scanning the filesystem n times (once per invocation of the tool by `go generate).

The caching we can do when we intercept this delivers workflow enhancements that in some projects is measured in minutes per go generate ./... run (which may be something an individual or team does many times a day).

[rittneje]

@joefitzgerald I see. I was confused by that logic, but I understand now what it does. Basically, only the first go:generate directive in the alphabetically first file in the package does anything.

That being said, the current logic violates both its own documentation and the documentation of go generate. To demonstrate this point, consider the following:

bar.go

//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 os.FileInfo

foo.go

//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 io.Reader

If I run go generate bar.go it should only regenerate the fake for os.FileInfo. However, it incorrectly generates the fake for io.Reader as well.
Similarly, if I run go generate foo.go, it should regenerate the fake for io.Reader. However, it incorrectly does not generate anything at all.

So the only way to preserve the proper behavior of this tool is to generate all and only what was explicitly requested in the original command. The teams that need the performance enhancement need to use your counterfeiter:generate directives and opt-in to package-level optimizations. That will also simplify the logic within counterfeiter, since you no longer need to optimize against multiple invocations coming from go generate.

If you are concerned about people not knowing about the -generate flag, one simple option would be to emit a warning when it isn't used.

[joefitzgerald]

Yes, I see your point.

Perhaps I could switch the file scanning on invocation by go generate to emit a warning when multiple go:generate directives exist in a single package. The warning would advise them to use the counterfeiter:generate directive to speed up performance. We would probably also need a flag to allow suppression of the warning.

[rittneje]

Is it worth checking for multiple invocations? If the intent is to deprecate the classic go:generate directive, then it would seem best to always warn, even if there is only one. (Or at least, always warn if invoked by go generate without the -generate flag.) That will encourage/direct people to do it the better way in the first place.

To suppress the warning, I'm thinking an environment variable would be the best approach, so that the users in question don't have to go modify a ton of go:generate directives (unless they want the performance enhancement).

[rittneje]

@joefitzgerald I wanted to follow up on this. The version of counterfeiter we've been stuck on seems to be incompatible with Go 1.15, so we are between a rock and a hard place.

[joefitzgerald]

Thanks for your patience @rittneje this is now released: https://github.com/maxbrunsfeld/counterfeiter/releases/tag/v6.4.0 .