Repository: bazel-contrib/bazel-gazelle

Issue:
resolver.Imports method needs RegularFiles list to evaluate glob
This is an issue we encountered with gazelle_cc. I don't think there's an easy way to solve it yet, so I'm curious to hear what other folks think.

When Gazelle indexes the repo, it calls [`resolver.Resolver.Imports`](https://pkg.go.dev/github.com/bazelbuild/bazel-gazelle/resolve#Resolver.Imports) on each rule in each indexed directory. Suppose we have a `cc_library` like this:

```
cc_library(
    name = "foo",
    hdrs = glob(["**/*.h"]),
)
```

We'd like `Imports` to be able to return an entry for every file matched by the `glob`. That should include files in the current directory, as well as subdirectories within the same Bazel package. It should not include files excluded by the `.bazelignore` file or `# gazelle:exclude` directives.

The [`language.Language.GenerateRules`](https://pkg.go.dev/github.com/bazelbuild/bazel-gazelle/language#Language) method also considers this set of file from [`GenerateArgs.RegularFiles`](https://pkg.go.dev/github.com/bazelbuild/bazel-gazelle/language#GenerateArgs). We'd like to be able to access that set of files from `Imports`.

We can't add a parameter to `Imports` unfortunately since that would be an incompatible change to the interface. However, the `walk` package has all the information we need internally. I wonder if we should export a function like:

```
func GetDirInfo(rel string) (DirInfo, error)

type DirInfo struct {
  Subdirs, RegularFiles []string
  File *rule.File
}
```


Discussion:
@jayconrod: cc @WojciechMazur @jayconrod: Reopening due to feedback from @WojciechMazur and @jbedard in [this thread](https://github.com/bazel-contrib/bazel-gazelle/pull/2141/files#r2224125478). `GetDirInfo` needs to be able to return info for subdirectories that haven't been visited yet.

Solution patch:
diff --git a/language/lang.go b/language/lang.go
index 740d34f..da8d81e 100644
--- a/language/lang.go
+++ b/language/lang.go
@@ -161,8 +161,9 @@ type GenerateArgs struct {
 	// symbolic links to directories that Gazelle will follow.
 	// RegularFiles is a list of regular files including other symbolic
 	// links.
-	// GeneratedFiles is a list of generated files in the directory
+	// GenFiles is a list of generated files in the directory
 	// (usually these are mentioned as "out" or "outs" attributes in rules).
+	// These slices must not be modified.
 	Subdirs, RegularFiles, GenFiles []string
 
 	// OtherEmpty is a list of empty rules generated by other languages.
diff --git a/walk/cache.go b/walk/cache.go
index 7bb08ee..4dbed79 100644
--- a/walk/cache.go
+++ b/walk/cache.go
@@ -15,7 +15,7 @@ type cache struct {
 
 type cacheEntry struct {
 	doneC chan struct{}
-	info  dirInfo
+	info  DirInfo
 	err   error
 }
 
@@ -28,7 +28,7 @@ type cacheEntry struct {
 //
 // get may be called by multiple threads concurrently. Later calls block
 // until the result from the first call is ready.
-func (c *cache) get(key string, load func(rel string) (dirInfo, error)) (dirInfo, error) {
+func (c *cache) get(key string, load func(rel string) (DirInfo, error)) (DirInfo, error) {
 	// Optimistically load the entry. This is technically unnecessary, but it
 	// avoids allocating a new entry in the case where one already exists.
 	raw, ok := c.entryMap.Load(key)
@@ -56,7 +56,7 @@ func (c *cache) get(key string, load func(rel string) (dirInfo, error)) (dirInfo
 
 // getLoaded returns the result of a previous call to get with the same key.
 // getLoaded panics if get was not called or has not returned yet.
-func (c *cache) getLoaded(rel string) (dirInfo, error) {
+func (c *cache) getLoaded(rel string) (DirInfo, error) {
 	e, ok := c.entryMap.Load(rel)
 	if ok {
 		select {
@@ -71,3 +71,35 @@ func (c *cache) getLoaded(rel string) (dirInfo, error) {
 	ce := e.(*cacheEntry)
 	return ce.info, ce.err
 }
+
+var globalCache *cache
+
+func setGlobalCache(c *cache) func() {
+	if globalCache != nil {
+		panic("globalCache already set")
+	}
+	globalCache = c
+	return func() { globalCache = nil }
+}
+
+// GetDirInfo returns the list of files and subdirectories contained in a
+// directory named by rel. It also returns the parsed build file or nil if
+// none was present. rel is a slash-separated path, relative to the repository
+// root directory or "" for the root directory itself. The returned values
+// must not be modified.
+//
+// GetDirInfo does not directly perform any I/O. Instead, it provides access
+// to an internal cache to an internal cache that Walk and Walk2 use to speed
+// up directory operations. For this reason, GetDirInfo must not be called
+// on a directory Gazelle has not visited.
+//
+// In general, language extensions should prefer to use the RegularFiles,
+// Subdirs, and File fields of language.GenerateArgs. This function returns
+// the same information and may be used by methods like Resolver.Imports
+// that get called earlier without the same information.
+func GetDirInfo(rel string) (DirInfo, error) {
+	if globalCache == nil {
+		panic("global cache is not set")
+	}
+	return globalCache.getLoaded(rel)
+}
diff --git a/walk/dirinfo.go b/walk/dirinfo.go
index c690f52..9f7387c 100644
--- a/walk/dirinfo.go
+++ b/walk/dirinfo.go
@@ -11,15 +11,18 @@ import (
 	"github.com/bazelbuild/bazel-gazelle/rule"
 )
 
-// dirInfo holds all the information about a directory that Walk2 needs.
-type dirInfo struct {
-	// subdirs and regularFiles hold the names of subdirectories and regular files
+// DirInfo holds all the information about a directory that Walk2 needs.
+type DirInfo struct {
+	// Subdirs and RegularFiles hold the names of subdirectories and regular files
 	// that are not ignored or excluded.
-	subdirs, regularFiles []string
+	// GenFiles is a list of generated files, named in "out" or "outs" attributes
+	// of targets in the directory's build file.
+	// The content of these slices must not be modified.
+	Subdirs, RegularFiles, GenFiles []string
 
-	// file is the directory's build file. May be nil if the build file doesn't
+	// File is the directory's build File. May be nil if the build File doesn't
 	// exist or contains errors.
-	file *rule.File
+	File *rule.File
 
 	// config is the configuration used by Configurer. We may precompute this
 	// before Configure is called to parallelize directory traversal without
@@ -39,8 +42,8 @@ type dirInfo struct {
 // This method may return partial results with an error. For example, if the
 // directory's build file contains a syntax error, the contents of the
 // directory are still returned.
-func (w *walker) loadDirInfo(rel string) (dirInfo, error) {
-	var info dirInfo
+func (w *walker) loadDirInfo(rel string) (DirInfo, error) {
+	var info DirInfo
 	var errs []error
 	var err error
 	dir := filepath.Join(w.rootConfig.RepoRoot, rel)
@@ -61,12 +64,12 @@ func (w *walker) loadDirInfo(rel string) (dirInfo, error) {
 		parentConfig = parentInfo.config
 	}
 
-	info.file, err = loadBuildFile(parentConfig, w.rootConfig.ReadBuildFilesDir, rel, dir, entries)
+	info.File, err = loadBuildFile(parentConfig, w.rootConfig.ReadBuildFilesDir, rel, dir, entries)
 	if err != nil {
 		errs = append(errs, err)
 	}
 
-	info.config = configureForWalk(parentConfig, rel, info.file)
+	info.config = configureForWalk(parentConfig, rel, info.File)
 	if info.config.isExcludedDir(rel) {
 		// Build file excludes the current directory. Ignore contents.
 		entries = nil
@@ -76,12 +79,21 @@ func (w *walker) loadDirInfo(rel string) (dirInfo, error) {
 		entryRel := path.Join(rel, e.Name())
 		e = maybeResolveSymlink(info.config, dir, entryRel, e)
 		if e.IsDir() && !info.config.isExcludedDir(entryRel) {
-			info.subdirs = append(info.subdirs, e.Name())
+			info.Subdirs = append(info.Subdirs, e.Name())
 		} else if !e.IsDir() && !info.config.isExcludedFile(entryRel) {
-			info.regularFiles = append(info.regularFiles, e.Name())
+			info.RegularFiles = append(info.RegularFiles, e.Name())
 		}
 	}
 
+	info.GenFiles = findGenFiles(info.config, info.File)
+
+	// Reduce cap of each slice to len, so that if the caller appends, they'll
+	// need to copy to a new backing array. This is defensive: it prevents
+	// multiple callers from overwriting the same backing array.
+	info.RegularFiles = info.RegularFiles[:len(info.RegularFiles):len(info.RegularFiles)]
+	info.Subdirs = info.Subdirs[:len(info.Subdirs):len(info.Subdirs)]
+	info.GenFiles = info.GenFiles[:len(info.GenFiles):len(info.GenFiles)]
+
 	return info, errors.Join(errs...)
 }
 
@@ -110,7 +122,7 @@ func (w *walker) populateCache(rels []string) {
 			return
 		}
 
-		for _, subdir := range info.subdirs {
+		for _, subdir := range info.Subdirs {
 			subdirRel := path.Join(rel, subdir)
 			sem <- struct{}{} // acquire semaphore for child
 			wg.Add(1)
diff --git a/walk/walk.go b/walk/walk.go
index 72cac22..4224103 100644
--- a/walk/walk.go
+++ b/walk/walk.go
@@ -210,6 +210,8 @@ func Walk2(c *config.Config, cexts []config.Configurer, dirs []string, mode Mode
 	if err != nil {
 		return err
 	}
+	cleanup := setGlobalCache(w.cache)
+	defer cleanup()
 
 	// Do the main tree walk, visiting directories the user requested.
 	w.visit(c, "", false)
@@ -444,16 +446,16 @@ func (w *walker) visit(c *config.Config, rel string, updateParent bool) {
 		return
 	}
 
-	containedByParent := info.file == nil && wc.updateOnly
+	containedByParent := info.File == nil && wc.updateOnly
 
 	// Configure the directory, if we haven't done so already.
 	_, alreadyConfigured := w.visits[rel]
 	if !containedByParent && !alreadyConfigured {
-		configure(w.cexts, w.knownDirectives, c, rel, info.file, info.config)
+		configure(w.cexts, w.knownDirectives, c, rel, info.File, info.config)
 	}
 
-	regularFiles := info.regularFiles
-	subdirs := info.subdirs
+	regularFiles := info.RegularFiles
+	subdirs := info.Subdirs
 	shouldUpdate := w.shouldUpdate(rel, updateParent)
 	w.visits[rel] = visitInfo{
 		c:                 c,
@@ -498,16 +500,15 @@ func (w *walker) visit(c *config.Config, rel string, updateParent bool) {
 
 		// Call the callback to update this directory.
 		update := !wc.ignore && shouldUpdate && !hasBuildFileError
-		genFiles := findGenFiles(wc, info.file)
 		result := w.wf(Walk2FuncArgs{
 			Dir:          dir,
 			Rel:          rel,
 			Config:       c,
 			Update:       update,
-			File:         info.file,
+			File:         info.File,
 			Subdirs:      subdirs,
 			RegularFiles: regularFiles,
-			GenFiles:     genFiles,
+			GenFiles:     info.GenFiles,
 		})
 		if result.Err != nil {
 			w.errs = append(w.errs, result.Err)



Test patch:
diff --git a/walk/walk_test.go b/walk/walk_test.go
index d4ec2c8..786496f 100644
--- a/walk/walk_test.go
+++ b/walk/walk_test.go
@@ -230,7 +230,7 @@ func TestGenMode(t *testing.T) {
 	check := func(t *testing.T, visits []visitSpec) {
 		t.Helper()
 		if len(visits) != 7 {
-			t.Error(fmt.Sprintf("Expected 7 visits, got %v", len(visits)))
+			t.Errorf("Expected 7 visits, got %v", len(visits))
 		}
 
 		if !reflect.DeepEqual(visits[len(visits)-1].subdirs, []string{"mode-create", "mode-update"}) {
@@ -770,6 +770,56 @@ func TestRelsToVisit(t *testing.T) {
 	}
 }
 
+func TestGetDirInfo(t *testing.T) {
+	dir, cleanup := testtools.CreateFiles(t, []testtools.FileSpec{
+		{
+			Path: "BUILD.bazel",
+			Content: `
+# gazelle:exclude exclude
+genrule(
+    name = "gen",
+		outs = ["gen.txt"],
+)
+`,
+		},
+		{
+			Path: "exclude",
+		},
+		{
+			Path: "file",
+		},
+		{
+			Path: "subdir/",
+		},
+	})
+	defer cleanup()
+
+	wantRegularFiles := []string{"BUILD.bazel", "file"}
+	wantSubdirs := []string{"subdir"}
+	wantGenFiles := []string{"gen.txt"}
+
+	c, cexts := testConfig(t, dir)
+	err := Walk2(c, cexts, []string{dir}, VisitAllUpdateDirsMode, func(args Walk2FuncArgs) Walk2FuncResult {
+		di, err := GetDirInfo("")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if diff := cmp.Diff(wantRegularFiles, di.RegularFiles); diff != "" {
+			t.Errorf("regular files (-want, +got):\n%s", diff)
+		}
+		if diff := cmp.Diff(wantSubdirs, di.Subdirs); diff != "" {
+			t.Errorf("subdirectories (-want, +got):\n%s", diff)
+		}
+		if diff := cmp.Diff(wantGenFiles, di.GenFiles); diff != "" {
+			t.Errorf("gen files (-want, +got):\n%s", diff)
+		}
+		return Walk2FuncResult{}
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 func testConfig(t *testing.T, dir string) (*config.Config, []config.Configurer) {
 	args := []string{"-repo_root", dir}
 	cexts := []config.Configurer{&config.CommonConfigurer{}, &Configurer{}}