gazelle: Generate exports for non-private return types (#158)

Return values which are used by calling code require the return type to
be on the classpath in order to compile uses of that value. Accordingly,
we should export those dependencies, to make sure the depender has them
on the classpath while compiling, to avoid strict deps issues.

We don't do this for private functions, but all other visibilities may
be called from other targets, so may need exporting.

BUILD.bazel

@@ -15,6 +15,7 @@ buildifier(
 # gazelle:resolve java com.beust.jcommander.converters @contrib_rules_jvm_deps//:com_beust_jcommander
 # gazelle:resolve java com.gazelle.java.javaparser.v0 //java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0:gazelle_java_build_v0_java_library
 # gazelle:resolve java com.google.common.annotations @contrib_rules_jvm_deps//:com_google_guava_guava
+# gazelle:resolve java com.google.common.base @contrib_rules_jvm_deps//:com_google_guava_guava
 # gazelle:resolve java com.google.common.collect @contrib_rules_jvm_deps//:com_google_guava_guava
 # gazelle:resolve java com.google.errorprone.annotations.concurrent @contrib_rules_jvm_deps//:com_google_errorprone_error_prone_annotations
 # gazelle:resolve java com.google.gson @contrib_rules_jvm_deps//:com_google_code_gson_gson

java/gazelle/README.md

@@ -81,6 +81,11 @@ Currently, the gazelle plugin makes the following assumptions about the code it'
 1. Non-test code doesn't depend on test code.
 1. Non-test code used by one package of tests either lives in the same directory as those tests, or lives in a non-test-code directory. We also detect non-test code used from another test package, if that other package doesn't have a corresponding non-test code directory, but require you to manually set the visibility on the depended-on target, because this is an unexpected set-up.
 1. Package names and class/interface names follow standard java conventions; that is: package names are all lower-case, and class and interface names start with Upper Case letters.
+1. Code doesn't use types which it doesn't name _only_ through unnamed means, across multiple calls. For example, if some code calls `x.foo().bar()` where the return type of `foo` is defined in another target, and the calling code explicitly uses a type from that target somewhere else. In the case of `x.foo()`, we add exports so that the caller will have access to the return type of `foo()`, but do not track dependencies on the return types across _multiple_ calls.
+
+   This limitation could be lifted, but would require us to export all _transitively_ used symbols from every function. This would serve to add direct dependencies between lots of targets, which can slow down compilation and reduce cache hits.
+
+   In our experience, this kind of code is rare in Java - most code tends to either introduce intermediate variables (at which point the type gets used and we detect that a dependency needs to be added), or tends to already use the targets containing the intermediate types somewhere else (at which point the dependency will already exist), but we're open to discussion about this heuristic if it poses problems for a real-world codebase.
 
 If these assumptions are violated, the rest of the generation should still function properly, but the specific files which violate the assumptions (or depend on files which violate the assumptions) will not get complete results. We strive to emit warnings when this happens.
 

java/gazelle/generate.go

@@ -155,6 +155,7 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes
 	// Files and imports for code which isn't tests, and isn't used as helpers in tests.
 	productionJavaFiles := sorted_set.NewSortedSet([]string{})
 	productionJavaImports := sorted_set.NewSortedSetFn([]types.PackageName{}, types.PackageNameLess)
+	nonLocalJavaExports := sorted_set.NewSortedSetFn([]types.PackageName{}, types.PackageNameLess)
 
 	// Files and imports for actual test classes.
 	testJavaFiles := sorted_set.NewSortedSetFn([]javaFile{}, javaFileLess)
@@ -177,13 +178,14 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes
 			allPackageNames.Add(mJavaPkg.Name)
 
 			if !mJavaPkg.TestPackage {
-				addNonLocalImports(productionJavaImports, mJavaPkg.ImportedClasses, mJavaPkg.ImportedPackagesWithoutSpecificClasses, mJavaPkg.Name, javaClassNamesFromFileNames)
+				addNonLocalImportsAndExports(productionJavaImports, nonLocalJavaExports, mJavaPkg.ImportedClasses, mJavaPkg.ImportedPackagesWithoutSpecificClasses, mJavaPkg.ExportedClasses, mJavaPkg.Name, javaClassNamesFromFileNames)
 				for _, f := range mJavaPkg.Files.SortedSlice() {
 					productionJavaFiles.Add(filepath.Join(mRel, f))
 				}
 				allMains.AddAll(mJavaPkg.Mains)
 			} else {
-				addNonLocalImports(testJavaImports, mJavaPkg.ImportedClasses, mJavaPkg.ImportedPackagesWithoutSpecificClasses, mJavaPkg.Name, javaClassNamesFromFileNames)
+				// Tests don't get to export things, as things shouldn't depend on them.
+				addNonLocalImportsAndExports(testJavaImports, nil, mJavaPkg.ImportedClasses, mJavaPkg.ImportedPackagesWithoutSpecificClasses, mJavaPkg.ExportedClasses, mJavaPkg.Name, javaClassNamesFromFileNames)
 				for _, f := range mJavaPkg.Files.SortedSlice() {
 					path := filepath.Join(mRel, f)
 					file := javaFile{
@@ -197,9 +199,10 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes
 	} else {
 		allPackageNames.Add(javaPkg.Name)
 		if javaPkg.TestPackage {
-			addNonLocalImports(testJavaImports, javaPkg.ImportedClasses, javaPkg.ImportedPackagesWithoutSpecificClasses, javaPkg.Name, javaClassNamesFromFileNames)
+			// Tests don't get to export things, as things shouldn't depend on them.
+			addNonLocalImportsAndExports(testJavaImports, nil, javaPkg.ImportedClasses, javaPkg.ImportedPackagesWithoutSpecificClasses, javaPkg.ExportedClasses, javaPkg.Name, javaClassNamesFromFileNames)
 		} else {
-			addNonLocalImports(productionJavaImports, javaPkg.ImportedClasses, javaPkg.ImportedPackagesWithoutSpecificClasses, javaPkg.Name, javaClassNamesFromFileNames)
+			addNonLocalImportsAndExports(productionJavaImports, nonLocalJavaExports, javaPkg.ImportedClasses, javaPkg.ImportedPackagesWithoutSpecificClasses, javaPkg.ExportedClasses, javaPkg.Name, javaClassNamesFromFileNames)
 		}
 		allMains.AddAll(javaPkg.Mains)
 		for _, f := range javaFilenamesRelativeToPackage {
@@ -227,7 +230,7 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes
 	})
 
 	if productionJavaFiles.Len() > 0 {
-		l.generateJavaLibrary(args.File, args.Rel, filepath.Base(args.Rel), productionJavaFiles.SortedSlice(), allPackageNames, nonLocalProductionJavaImports, false, &res)
+		l.generateJavaLibrary(args.File, args.Rel, filepath.Base(args.Rel), productionJavaFiles.SortedSlice(), allPackageNames, nonLocalProductionJavaImports, nonLocalJavaExports, false, &res)
 	}
 
 	for _, m := range allMains.SortedSlice() {
@@ -249,7 +252,7 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes
 				testJavaImportsWithHelpers.Add(tf.pkg)
 				srcs = append(srcs, tf.pathRelativeToBazelWorkspaceRoot)
 			}
-			l.generateJavaLibrary(args.File, args.Rel, filepath.Base(args.Rel), srcs, packages, testJavaImports, true, &res)
+			l.generateJavaLibrary(args.File, args.Rel, filepath.Base(args.Rel), srcs, packages, testJavaImports, nonLocalJavaExports, true, &res)
 		}
 	}
 
@@ -349,21 +352,29 @@ func (l javaLang) collectRuntimeDeps(kind, name string, file *rule.File) *sorted
 }
 
 // We exclude intra-target imports because otherwise we'd get self-dependencies come resolve time.
-func addNonLocalImports(to *sorted_set.SortedSet[types.PackageName], fromClasses *sorted_set.SortedSet[types.ClassName], fromPackages *sorted_set.SortedSet[types.PackageName], pkg types.PackageName, localClasses *sorted_set.SortedSet[string]) {
-	for _, imp := range fromClasses.SortedSlice() {
-		if pkg == imp.PackageName() {
-			if localClasses.Contains(imp.BareOuterClassName()) {
+// toExports is optional and may be nil. All other parameters are required and must be non-nil.
+func addNonLocalImportsAndExports(toImports *sorted_set.SortedSet[types.PackageName], toExports *sorted_set.SortedSet[types.PackageName], fromImportedClasses *sorted_set.SortedSet[types.ClassName], fromPackages *sorted_set.SortedSet[types.PackageName], fromExportedClasses *sorted_set.SortedSet[types.ClassName], pkg types.PackageName, localClasses *sorted_set.SortedSet[string]) {
+	toImports.AddAll(fromPackages)
+	addFilteringOutOwnPackage(toImports, fromImportedClasses, pkg, localClasses)
+	if toExports != nil {
+		addFilteringOutOwnPackage(toExports, fromExportedClasses, pkg, localClasses)
+	}
+}
+
+func addFilteringOutOwnPackage(to *sorted_set.SortedSet[types.PackageName], from *sorted_set.SortedSet[types.ClassName], ownPackage types.PackageName, localOuterClassNames *sorted_set.SortedSet[string]) {
+	for _, fromPackage := range from.SortedSlice() {
+		if ownPackage == fromPackage.PackageName() {
+			if localOuterClassNames.Contains(fromPackage.BareOuterClassName()) {
 				continue
 			}
 		}
 
-		if imp.PackageName().Name == "" {
+		if fromPackage.PackageName().Name == "" {
 			continue
 		}
 
-		to.Add(imp.PackageName())
+		to.Add(fromPackage.PackageName())
 	}
-	to.AddAll(fromPackages)
 }
 
 func accumulateJavaFile(cfg *javaconfig.Config, testJavaFiles, testHelperJavaFiles *sorted_set.SortedSet[javaFile], separateTestJavaFiles map[javaFile]map[string]bzl.Expr, file javaFile, perClassMetadata map[string]java.PerClassMetadata, log zerolog.Logger) {
@@ -389,7 +400,7 @@ func accumulateJavaFile(cfg *javaconfig.Config, testJavaFiles, testHelperJavaFil
 	}
 }
 
-func (l javaLang) generateJavaLibrary(file *rule.File, pathToPackageRelativeToBazelWorkspace string, name string, srcsRelativeToBazelWorkspace []string, packages, imports *sorted_set.SortedSet[types.PackageName], testonly bool, res *language.GenerateResult) {
+func (l javaLang) generateJavaLibrary(file *rule.File, pathToPackageRelativeToBazelWorkspace string, name string, srcsRelativeToBazelWorkspace []string, packages, imports *sorted_set.SortedSet[types.PackageName], exports *sorted_set.SortedSet[types.PackageName], testonly bool, res *language.GenerateResult) {
 	const ruleKind = "java_library"
 	r := rule.NewRule(ruleKind, name)
 
@@ -421,6 +432,7 @@ func (l javaLang) generateJavaLibrary(file *rule.File, pathToPackageRelativeToBa
 	resolveInput := types.ResolveInput{
 		PackageNames:         packages,
 		ImportedPackageNames: imports,
+		ExportedPackageNames: exports,
 	}
 	res.Imports = append(res.Imports, resolveInput)
 }

java/gazelle/generate_test.go

@@ -282,8 +282,9 @@ func TestAddNonLocalImports(t *testing.T) {
 		src.Add(*name)
 	}
 
-	dst := sorted_set.NewSortedSetFn([]types.PackageName{}, types.PackageNameLess)
-	addNonLocalImports(dst, src, sorted_set.NewSortedSetFn[types.PackageName]([]types.PackageName{}, types.PackageNameLess), types.NewPackageName("com.example.a.b"), sorted_set.NewSortedSet([]string{"Foo", "Bar"}))
+	depsDst := sorted_set.NewSortedSetFn([]types.PackageName{}, types.PackageNameLess)
+	exportsDst := sorted_set.NewSortedSetFn([]types.PackageName{}, types.PackageNameLess)
+	addNonLocalImportsAndExports(depsDst, exportsDst, src, sorted_set.NewSortedSetFn[types.PackageName]([]types.PackageName{}, types.PackageNameLess), sorted_set.NewSortedSetFn([]types.ClassName{}, types.ClassNameLess), types.NewPackageName("com.example.a.b"), sorted_set.NewSortedSet([]string{"Foo", "Bar"}))
 
 	want := stringsToPackageNames([]string{
 		"com.another.a.b",
@@ -292,7 +293,7 @@ func TestAddNonLocalImports(t *testing.T) {
 		"com.example.a.b.c",
 	}).SortedSlice()
 
-	if diff := cmp.Diff(want, dst.SortedSlice()); diff != "" {
+	if diff := cmp.Diff(want, depsDst.SortedSlice()); diff != "" {
 		t.Errorf("filterImports() mismatch (-want +got):\n%s", diff)
 	}
 }

java/gazelle/lang.go

@@ -74,11 +74,25 @@ var kindWithoutRuntimeDeps = rule.KindInfo{
 	},
 }
 
+var javaLibraryKind = rule.KindInfo{
+	NonEmptyAttrs: map[string]bool{
+		"deps":    true,
+		"exports": true,
+		"srcs":    true,
+	},
+	MergeableAttrs: map[string]bool{"srcs": true},
+	ResolveAttrs: map[string]bool{
+		"deps":         true,
+		"exports":      true,
+		"runtime_deps": true,
+	},
+}
+
 func (l javaLang) Kinds() map[string]rule.KindInfo {
 	return map[string]rule.KindInfo{
 		"java_binary":        kindWithRuntimeDeps,
 		"java_junit5_test":   kindWithRuntimeDeps,
-		"java_library":       kindWithRuntimeDeps,
+		"java_library":       javaLibraryKind,
 		"java_test":          kindWithRuntimeDeps,
 		"java_test_suite":    kindWithRuntimeDeps,
 		"java_proto_library": kindWithoutRuntimeDeps,

java/gazelle/private/java/package.go

@@ -9,6 +9,7 @@ type Package struct {
 	Name types.PackageName
 
 	ImportedClasses                        *sorted_set.SortedSet[types.ClassName]
+	ExportedClasses                        *sorted_set.SortedSet[types.ClassName]
 	ImportedPackagesWithoutSpecificClasses *sorted_set.SortedSet[types.PackageName]
 	Mains                                  *sorted_set.SortedSet[types.ClassName]
 

java/gazelle/private/javaparser/javaparser.go

@@ -73,6 +73,14 @@ func (r Runner) ParsePackage(ctx context.Context, in *ParsePackageRequest) (*jav
 		}
 		importedClasses.Add(*className)
 	}
+	exportedClasses := sorted_set.NewSortedSetFn([]types.ClassName{}, types.ClassNameLess)
+	for _, export := range resp.GetExportedClasses() {
+		className, err := types.ParseClassName(export)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse exports: %w", err)
+		}
+		exportedClasses.Add(*className)
+	}
 	importedPackages := sorted_set.NewSortedSetFn([]types.PackageName{}, types.PackageNameLess)
 	for _, pkg := range resp.GetImportedPackagesWithoutSpecificClasses() {
 		importedPackages.Add(types.NewPackageName(pkg))
@@ -85,6 +93,7 @@ func (r Runner) ParsePackage(ctx context.Context, in *ParsePackageRequest) (*jav
 	return &java.Package{
 		Name:                                   packageName,
 		ImportedClasses:                        importedClasses,
+		ExportedClasses:                        exportedClasses,
 		ImportedPackagesWithoutSpecificClasses: importedPackages,
 		Mains:                                  mains,
 		Files:                                  sorted_set.NewSortedSet(in.Files),

java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0/javaparser.proto

@@ -45,6 +45,9 @@ message Package {
   repeated string mains = 3;
 
   map<string, PerClassMetadata> per_class_metadata = 4;
+
+  // The list of classes which dependers may need to have exported to them because they are part of the public interface of the request.
+  repeated string exported_classes = 6;
 }
 
 message PerClassMetadata {

java/gazelle/private/types/types.go

@@ -96,6 +96,7 @@ func ClassNameLess(l, r ClassName) bool {
 type ResolveInput struct {
 	PackageNames         *sorted_set.SortedSet[PackageName]
 	ImportedPackageNames *sorted_set.SortedSet[PackageName]
+	ExportedPackageNames *sorted_set.SortedSet[PackageName]
 }
 
 type ResolvableJavaPackage struct {

java/gazelle/resolve.go

@@ -80,25 +80,26 @@ func (Resolver) Embeds(r *rule.Rule, from label.Label) []label.Label {
 
 func (jr Resolver) Resolve(c *config.Config, ix *resolve.RuleIndex, rc *repo.RemoteCache, r *rule.Rule, imports interface{}, from label.Label) {
 	resolveInput := imports.(types.ResolveInput)
-	javaImports := resolveInput.ImportedPackageNames
-	if javaImports.Len() == 0 {
-		return
-	}
 
-	deps := sorted_set.NewSortedSetFn[label.Label]([]label.Label{}, labelLess)
+	isTestRule := isTestRule(r.Kind())
+
+	jr.populateAttr(c, r, "deps", resolveInput.ImportedPackageNames, ix, isTestRule, from, resolveInput.PackageNames)
+	jr.populateAttr(c, r, "exports", resolveInput.ExportedPackageNames, ix, isTestRule, from, resolveInput.PackageNames)
+}
+
+func (jr Resolver) populateAttr(c *config.Config, r *rule.Rule, attrName string, requiredPackageNames *sorted_set.SortedSet[types.PackageName], ix *resolve.RuleIndex, isTestRule bool, from label.Label, ownPackageNames *sorted_set.SortedSet[types.PackageName]) {
+	labels := sorted_set.NewSortedSetFn[label.Label]([]label.Label{}, labelLess)
 
-	for _, implicitDep := range r.AttrStrings("deps") {
+	for _, implicitDep := range r.AttrStrings(attrName) {
 		l, err := label.Parse(implicitDep)
 		if err != nil {
-			panic(fmt.Sprintf("error converting implicit dep %q to label: %v", implicitDep, err))
+			panic(fmt.Sprintf("error converting implicit %s %q to label: %v", attrName, implicitDep, err))
 		}
-		deps.Add(l)
+		labels.Add(l)
 	}
 
-	isTestRule := isTestRule(r.Kind())
-
-	for _, imp := range javaImports.SortedSlice() {
-		dep, err := jr.convertImport(c, imp, ix, from, isTestRule, resolveInput.PackageNames)
+	for _, imp := range requiredPackageNames.SortedSlice() {
+		dep, err := jr.resolveSinglePackage(c, imp, ix, from, isTestRule, ownPackageNames)
 		if err != nil {
 			jr.lang.logger.Error().Str("import", dep.String()).Err(err).Msg("error converting import")
 			panic(fmt.Sprintf("error converting import: %s", err))
@@ -107,18 +108,20 @@ func (jr Resolver) Resolve(c *config.Config, ix *resolve.RuleIndex, rc *repo.Rem
 			continue
 		}
 
-		deps.Add(simplifyLabel(c.RepoName, dep, from))
+		labels.Add(simplifyLabel(c.RepoName, dep, from))
 	}
 
-	if deps.Len() > 0 {
-		var exprs []build.Expr
-		for _, l := range deps.SortedSlice() {
+	var exprs []build.Expr
+	if labels.Len() > 0 {
+		for _, l := range labels.SortedSlice() {
 			if l.Relative && l.Name == from.Name {
 				continue
 			}
 			exprs = append(exprs, &build.StringExpr{Value: l.String()})
 		}
-		r.SetAttr("deps", exprs)
+	}
+	if len(exprs) > 0 {
+		r.SetAttr(attrName, exprs)
 	}
 }
 
@@ -148,7 +151,7 @@ func simplifyLabel(repoName string, l label.Label, from label.Label) label.Label
 	return l
 }
 
-func (jr *Resolver) convertImport(c *config.Config, imp types.PackageName, ix *resolve.RuleIndex, from label.Label, isTestRule bool, ownPackageNames *sorted_set.SortedSet[types.PackageName]) (out label.Label, err error) {
+func (jr *Resolver) resolveSinglePackage(c *config.Config, imp types.PackageName, ix *resolve.RuleIndex, from label.Label, isTestRule bool, ownPackageNames *sorted_set.SortedSet[types.PackageName]) (out label.Label, err error) {
 	cacheKey := types.NewResolvableJavaPackage(imp, false, false)
 	importSpec := resolve.ImportSpec{Lang: languageName, Imp: cacheKey.String()}
 	if ol, found := resolve.FindRuleWithOverride(c, importSpec, languageName); found {
@@ -170,7 +173,7 @@ func (jr *Resolver) convertImport(c *config.Config, imp types.PackageName, ix *r
 		jr.lang.logger.Error().
 			Str("pkg", imp.Name).
 			Strs("targets", labels).
-			Msg("convertImport found MULTIPLE results in rule index")
+			Msg("resolveSinglePackage found MULTIPLE results in rule index")
 	}
 
 	if v, ok := jr.internalCache.Get(cacheKey); ok {

java/gazelle/testdata/interface_exports/.bazelrc

@@ -0,0 +1,4 @@
+build --java_runtime_version=remotejdk_11
+build --java_language_version=11
+build --tool_java_runtime_version=remotejdk_11
+build --tool_java_language_version=11

java/gazelle/testdata/interface_exports/expectedStderr.txt

@@ -0,0 +1 @@
+12:00AM WRN java/gazelle/private/maven/resolver.go:XXX > not loading maven dependencies error="open %WORKSPACEPATH%/maven_install.json: no such file or directory" _c=maven-resolver

java/gazelle/testdata/interface_exports/src/main/java/com/example/consumer/BUILD.out

@@ -0,0 +1,8 @@
+load("@rules_java//java:defs.bzl", "java_library")
+
+java_library(
+    name = "consumer",
+    srcs = ["Consumer.java"],
+    visibility = ["//:__subpackages__"],
+    deps = ["//src/main/java/com/example/iface"],
+)

java/gazelle/testdata/interface_exports/src/main/java/com/example/consumer/Consumer.java

@@ -0,0 +1,10 @@
+package com.example.consumer;
+
+import com.example.iface.Interface;
+
+public class Consumer {
+  public void useInterface(Interface i) {
+    var t = i.get();
+    System.out.println(t.stringify() + "hello");
+  }
+}

java/gazelle/testdata/interface_exports/src/main/java/com/example/iface/BUILD.out

@@ -0,0 +1,9 @@
+load("@rules_java//java:defs.bzl", "java_library")
+
+java_library(
+    name = "iface",
+    srcs = ["Interface.java"],
+    visibility = ["//:__subpackages__"],
+    exports = ["//src/main/java/com/example/types"],
+    deps = ["//src/main/java/com/example/types"],
+)

java/gazelle/testdata/interface_exports/src/main/java/com/example/iface/Interface.java

@@ -0,0 +1,7 @@
+package com.example.iface;
+
+import com.example.types.Type;
+
+public interface Interface {
+  public Type get();
+}

java/gazelle/testdata/interface_exports/src/main/java/com/example/types/BUILD.out

@@ -0,0 +1,7 @@
+load("@rules_java//java:defs.bzl", "java_library")
+
+java_library(
+    name = "types",
+    srcs = ["Type.java"],
+    visibility = ["//:__subpackages__"],
+)

java/gazelle/testdata/interface_exports/src/main/java/com/example/types/Type.java

@@ -0,0 +1,7 @@
+package com.example.types;
+
+public class Type {
+  public String stringify() {
+    return "I am Type";
+  }
+}