Go codegenerators, unpredictable direct dependencies and the strict dependencies check

[robbertvanginkel]

While attempting to develop gazelle extensions for go codegenerators that take go code as input and generate go code as output, we're running into trouble determining the dependencies to add to a rule. Specifically in the presence of a typealias, some codegenerators generate code with imports from the source's transitive dependencies. With rules_go's "strict dependencies" check, this is currently hard to model in bazel/gazelle.

For example, consider the following toy project for which we want to generate mocks with golang/mock:

-- ./foo/foo.go --
package foo

import "github.com/example/project/bar"

type Foo interface {
	DoFoo(bar.Bar)
}
-- ./bar/bar.go --
package bar

import "github.com/example/project/baz"

type Bar = baz.Baz
-- ./baz/baz.go --
package baz

type Baz struct{}

Using https://github.com/jmhodges/bazel_gomock, its straightforward to add a rule that would generate go file with a mock:

gomock(
    name = "foo_mock",
    out = "foo_mock.go",
    interfaces = ["Foo"],
    library = "//foo:go_default_library",
)

Our goal was to have a gazelle extension that could detect that the foo_mock.go file was generated based off of //foo:go_default_library, so that we could generate a go_library (or a similar go_mock_library) rule for foo_mock.go that would include all deps of //foo:go_default_library in its deps. Something like:

go_library(
	name = "foo_mock_library",
	srcs = ["foo_mock.go"],
	deps = ["//bar:go_default_library"], # filled by the deps from //foo:go_default_library
)

However, with that you'll run into the following compile error:

compilepkg: missing strict dependencies:
	foo_mock.go: import of "github.com/example/project/baz"

Looking at the generated file, this is because mockgen "follows the typealias" (golang/mock#244) and generates code with possible imports from the set of transitive dependencies of the library being mocked, not just the direct dependencies:

$ bazel build //:foo_mock.go && cat bazel-bin/foo_mock.go
// Code generated by MockGen. DO NOT EDIT.
// Source: github.com/example/project/foo (interfaces: Foo)

// Package mock_foo is a generated GoMock package.
package mock_foo

import (
	baz "github.com/example/project/baz"
	gomock "github.com/golang/mock/gomock"
	reflect "reflect"
)

...

// DoFoo mocks base method
func (m *MockFoo) DoFoo(arg0 baz.Baz) {

Gazelle doesn't seem to be a very good fit for creating rules where the dependencies can't directly be inferred by reading a local source file. We'd like to find a way to help auto generate some of these rules as writing them manually can be tedious and error prone.

We've thought of a few ways to tackle this:

• Don't (only) use gazelle. We can have an external tool that: creates a mock rule, builds mock rule, reads generated imports, writes a gazelle directive with generated importpaths, runs gazelle to resolve those imports to labels. This works but is slow, fragile and the resulting directives are very duplicative to the rules they generate.

• Use an aspect to collect all transitive GoSource providers from dependencies of the library to mock in a custom go_mock_library rule and supply them as directs when using go.new_library(). A custom rule like this could avoid the need for specifying deps, but it seems like this could lead to overspecifying dependencies and result in unnecessary rebuilds.

• Investigate if the "missing strict dependencies" check can be relaxed. The value of this check for handwritten code is clear, but it came to mind that this might be less necessary for generated code in which the generated dependencies are always a subset of the original library's transitive dependencies: the problematic case of breakage when an implicit dependency on something transitive that gets refactored away wouldn't be possible, as when the refactoring removes the implicit dependency it could not be part of the codegenerator's result. Not sure if this is possible at all.

• Avoid / fix the codegenerators that generate code for which deps can't be easily inferred. Unfortunately, it seems like codegen that is not solely based on go/ast exhibits the typealias problem. We've observed it in a few internal ones that do codegen based on go/types and x/tools/go/packages too.

Hoping there might be some advice/insight on how this could be best handled.

cc @linzhp

[jayconrod]

Not sure I can offer much advice beyond what you've already written. I see a few solutions:

Ideally, gomock would generate files with a set of imports easily predicted from the original files. That may not be realistic though for code generators in general.

If a Gazelle extension generates the gomock rule, perhaps it could attach some information that the language/go extension could use for imports? I don't think there's a mechanism for that now, but we could discuss that further. The extension would need to understand the imports that gomock would generate though.

If there were a go_mock_library rule that generates files and compiles them without exposing them to other rules (except for embedding through GoSource, GoArchive), that rule would have more control over what gets passed to the compiler. I don't think an aspect would be necessary; GoArchive carries information about all transitive dependencies. go_library doesn't pass all that information to the action because analysis cost would be proportional to the number of transitive dependencies instead of direct dependencies (basically O(n^2) instead of O(n)). Or alternatively, go_mock_library targets could be generated by a Gazelle extension with precise understanding of dependencies (same problem as above).

[robbertvanginkel]

Thanks! Was hoping that there might be a better solution we were missing out on but unfortunately maybe not so.

I think even for gomock its hard tho predict exactly which dependencies will be neecessary from just the file. Besides the typealias issue, its also hard to do with embedded interfaces:

-- ./foo/foo.go --
package foo

import "github.com/example/project/bar"

type Foo interface {
	bar.Bar
	DoFoo(int)
}
-- ./bar/bar.go --
package bar

import "github.com/example/project/baz"

type Bar interface {
	DoBar(baz.Baz)
}
-- ./baz/baz.go --
package baz

type Baz struct{}

In this case, the mocked code would also need a direct import of github.com/example/project/baz, and that's not easily determined just by looking at foo.go. With the imports being hard to predict, the gazelle route seems hard to get right for the corner cases.

For a go_mock_library I was indeed thinking of something that didn't expose the source file but just allowed for embedding or depending on. Altough GoArchive does carry transitive dependenecy information, I've not been able to use this successfully to pass all the transitive dependencies to the compiler for a new GoSource/GoArchive for the generated mock source. As I understand it, when using go.new_library to construct this new lib the full list of transitive deps would need to be specified as direct deps so that they would get passed to to the compile step that verifies the strict dependencies. For this you'd need a list of Target to add to the attr (either through go.new_library's resolver, or modify the attr in go.library_to_source). The GoArchive aspect however only contains transitive GoArchiveData, from which I could not find an attribute to fill the deps correctly. This is why I was thinking an aspect might be an option to collect the transitive Targets as found in the GoSource provider's deps attribute. Is there another way I had missed you had in mind to do this without aspect?

[jayconrod]

For a go_mock_library I was indeed thinking of something that didn't expose the source file but just allowed for embedding or depending on. Altough GoArchive does carry transitive dependenecy information, I've not been able to use this successfully to pass all the transitive dependencies to the compiler for a new GoSource/GoArchive for the generated mock source. As I understand it, when using go.new_library to construct this new lib the full list of transitive deps would need to be specified as direct deps so that they would get passed to to the compile step that verifies the strict dependencies. For this you'd need a list of Target to add to the attr (either through go.new_library's resolver, or modify the attr in go.library_to_source). The GoArchive aspect however only contains transitive GoArchiveData, from which I could not find an attribute to fill the deps correctly. This is why I was thinking an aspect might be an option to collect the transitive Targets as found in the GoSource provider's deps attribute. Is there another way I had missed you had in mind to do this without aspect?

Hmm, you're right. GoArchive has all the information you'd need to pass to the compiler or linker, but go_library won't actually pass that information for transitive dependencies. So you'd effectively need to turn transitive dependencies into direct dependencies, and there's not enough information to do that without an aspect.

[robbertvanginkel]

go_library doesn't pass all that information to the action because analysis cost would be proportional to the number of transitive dependencies instead of direct dependencies (basically O(n^2) instead of O(n)).

I was under the impression that with the depset() structure, most of these concerns would be avoided. Going through the documentation though I saw https://github.com/bazelbuild/rules_go/blob/master/go/providers.rst#goarchivedata mention that:

GoArchiveData contains information about a compiled Go package. GoArchiveData only contains immutable information about a package itself. It does not contain any information about dependencies or references to other providers. This makes it suitable to include in depsets.

Are there any specific do or don't for what can be put in a depset to avoid the O(n^2) issue?

Hmm, you're right. GoArchive has all the information you'd need to pass to the compiler or linker, but go_library won't actually pass that information for transitive dependencies.

Is there a reasonable change here that could be made so that that info would be available? Or is this more a conscious design decision that rules_go doesn't want to support?

So you'd effectively need to turn transitive dependencies into direct dependencies, and there's not enough information to do that without an aspect.

I'd experimented a bit with this and come up with something like:

TransitiveGoSource = provider()

def _transitive_source_impl(target, ctx):
    return [TransitiveGoSource(
        deps = depset(target[GoSource].deps, transitive = [d[TransitiveGoSource].deps for d in ctx.rule.attr.deps]),
    )]

_transitive_source = aspect(
    implementation = _transitive_source_impl,
    attr_aspects = ["deps", "embed"],
)

Which seems like it would do the trick for accessing transitive Target's of a go_library and add them as direct dependencies on a new go library in a custom rule. I haven't examined if the analysis costs of that would be prohibitive, but other than that this direction would over specify deps do you see any problems with further investigating this direction?

[jayconrod]

I was under the impression that with the depset() structure, most of these concerns would be avoided. ...
Are there any specific do or don't for what can be put in a depset to avoid the O(n^2) issue?

depsets are meant to solve this problem, but to get that benefit, we can only iterate the elements of a depset once at link time (at the root of the dependency tree). We can't iterate depsets at every vertex in the graph or we'll fall back to O(n^2) overall performance.

Is there a reasonable change here that could be made so that that info would be available? Or is this more a conscious design decision that rules_go doesn't want to support?

For go_binary and go_library, it's definitely a conscious decision to use strict dependencies.

But I think it probably makes sense to carry enough information in providers that we can implement this kind of thing in other rules if it's really necessary. I ran into this recently in #1877. Ideally, go_test should be able to recompile the external test package's dependencies, replacing the library under test with the internal test package. There's not enough information in GoArchive or GoSource to do that, so the only way to make it work would be an aspect.

Not sure a simple change would be the right step to take though. I need to think about it more. I've been thinking for a while that the providers should be consolidated, and some obsolete stuff should be removed. Maybe this should be part of a larger redesign.

[robbertvanginkel]

Makes sense that depsets don't help much of they get expanded everywhere.

Thank you for the explanations! I don't think I have any further questions on this then, feel free to close or keep open if you want to hold on to the thought for some redesign.