Gazelle: split merging into two stages

go/tools/gazelle/gazelle/main.go

@@ -70,8 +70,8 @@ type visitRecord struct {
 	// empty is a list of empty Go rules that may be deleted.
 	empty []bf.Expr
 
-	// oldFile is an existing build file in the directory. May be nil.
-	oldFile *bf.File
+	// file is the build file being processed.
+	file *bf.File
 }
 
 type byPkgRel []visitRecord
@@ -89,15 +89,16 @@ func run(c *config.Config, cmd command, emit emitFunc) {
 	// Visit directories to modify.
 	// TODO: visit all directories in the repository in order to index rules.
 	for _, dir := range c.Dirs {
-		packages.Walk(c, dir, func(rel string, c *config.Config, pkg *packages.Package, oldFile *bf.File, isUpdateDir bool) {
+		packages.Walk(c, dir, func(rel string, c *config.Config, pkg *packages.Package, file *bf.File, isUpdateDir bool) {
 			// Fix existing files.
-			if oldFile != nil {
+			if file != nil {
+				file = merger.FixFileMinor(c, file)
 				if shouldFix {
-					oldFile = merger.FixFile(c, oldFile)
+					file = merger.FixFile(c, file)
 				} else {
-					fixedFile := merger.FixFile(c, oldFile)
-					if fixedFile != oldFile {
-						log.Printf("%s: warning: file contains rules whose structure is out of date. Consider running 'gazelle fix'.", oldFile.Path)
+					fixedFile := merger.FixFile(c, file)
+					if fixedFile != file {
+						log.Printf("%s: warning: file contains rules whose structure is out of date. Consider running 'gazelle fix'.", file.Path)
 					}
 				}
 			}
@@ -108,77 +109,40 @@ func run(c *config.Config, cmd command, emit emitFunc) {
 
 			// Generate rules.
 			if pkg != nil {
-				g := rules.NewGenerator(c, l, oldFile)
+				g := rules.NewGenerator(c, l, file)
 				rules, empty := g.GenerateRules(pkg)
+				file, rules = merger.MergeFile(rules, empty, file, merger.MergeableGeneratedAttrs)
+				if file.Path == "" {
+					file.Path = filepath.Join(c.RepoRoot, filepath.FromSlash(rel), c.DefaultBuildFileName())
+				}
 				visits = append(visits, visitRecord{
-					pkgRel:  rel,
-					rules:   rules,
-					empty:   empty,
-					oldFile: oldFile,
+					pkgRel: rel,
+					rules:  rules,
+					file:   file,
 				})
 			}
 		})
-
-		// TODO: resolve dependencies using the index.
-		resolver := resolve.NewResolver(c, l)
-		for _, v := range visits {
-			for _, r := range v.rules {
-				resolver.ResolveRule(r, v.pkgRel)
-			}
-		}
-
-		// Merge old files and generated files. Emit merged files.
-		for _, v := range visits {
-			genFile := &bf.File{
-				Path: filepath.Join(c.RepoRoot, filepath.FromSlash(v.pkgRel), c.DefaultBuildFileName()),
-				Stmt: v.rules,
-			}
-			mergeAndEmit(c, genFile, v.oldFile, v.empty, emit)
-		}
 	}
-}
 
-// mergeAndEmit merges "genFile" with "oldFile". "oldFile" may be nil if
-// no file exists. If v.c.ShouldFix is true, deprecated usage of old rules in
-// "oldFile" will be fixed. The resulting merged file will be emitted using
-// the "v.emit" function.
-func mergeAndEmit(c *config.Config, genFile, oldFile *bf.File, empty []bf.Expr, emit emitFunc) {
-	if oldFile == nil {
-		// No existing file, so no merge required.
-		rules.SortLabels(genFile)
-		genFile = merger.FixLoads(genFile)
-		bf.Rewrite(genFile, nil) // have buildifier 'format' our rules.
-		if err := emit(c, genFile); err != nil {
-			log.Print(err)
+	// Resolve dependencies.
+	// TODO: resolve dependencies using the index.
+	resolver := resolve.NewResolver(c, l)
+	for i := range visits {
+		for j := range visits[i].rules {
+			visits[i].rules[j] = resolver.ResolveRule(visits[i].rules[j], visits[i].pkgRel)
 		}
-		return
+		visits[i].file, _ = merger.MergeFile(visits[i].rules, nil, visits[i].file, merger.MergeableResolvedAttrs)
 	}
 
-	// Existing file. Fix it or see if it needs fixing before merging.
-	oldFile = merger.FixFileMinor(c, oldFile)
-	if c.ShouldFix {
-		oldFile = merger.FixFile(c, oldFile)
-	} else {
-		fixedFile := merger.FixFile(c, oldFile)
-		if fixedFile != oldFile {
-			log.Printf("%s: warning: file contains rules whose structure is out of date. Consider running 'gazelle fix'.", oldFile.Path)
+	// Emit merged files.
+	for _, v := range visits {
+		rules.SortLabels(v.file)
+		v.file = merger.FixLoads(v.file)
+		bf.Rewrite(v.file, nil) // have buildifier 'format' our rules.
+		if err := emit(c, v.file); err != nil {
+			log.Print(err)
 		}
 	}
-
-	// Existing file, so merge and replace the old one.
-	mergedFile := merger.MergeWithExisting(genFile, oldFile, empty)
-	if mergedFile == nil {
-		// Ignored file. Don't emit.
-		return
-	}
-
-	rules.SortLabels(mergedFile)
-	mergedFile = merger.FixLoads(mergedFile)
-	bf.Rewrite(mergedFile, nil) // have buildifier 'format' our rules.
-	if err := emit(c, mergedFile); err != nil {
-		log.Print(err)
-		return
-	}
 }
 
 func usage(fs *flag.FlagSet) {

go/tools/gazelle/merger/merger.go

@@ -29,42 +29,46 @@ import (
 const keep = "keep" // marker in srcs or deps to tell gazelle to preserve.
 
 var (
-	mergeableFields = map[string]bool{
+	// MergeableGeneratedAttrs is the set of attributes that should be merged
+	// before dependencies are resolved.
+	MergeableGeneratedAttrs = map[string]bool{
 		"cgo":        true,
 		"clinkopts":  true,
 		"copts":      true,
-		"deps":       true,
 		"embed":      true,
 		"importpath": true,
 		"proto":      true,
 		"srcs":       true,
 	}
+
+	// MergeableResolvedAttrs is the set of attributes that should be merged
+	// after dependencies are resolved.
+	MergeableResolvedAttrs = map[string]bool{
+		"deps": true,
+		config.GazelleImportsKey: true,
+	}
 )
 
-// MergeWithExisting merges "genFile" with "oldFile" and returns the
-// merged file.
-//
-// "genFile" is a file generated by Gazelle. It must not be nil.
-// "oldFile" is the existing file. It may be nil if no file was found.
-// "empty" is a list of rules that may be deleted.
-//
-// If "oldFile" is nil, "genFile" will be returned. If "oldFile" contains
-// a "# gazelle:ignore" comment, nil will be returned. If an error occurs,
-// it will be logged, and nil will be returned.
-func MergeWithExisting(genFile, oldFile *bf.File, empty []bf.Expr) *bf.File {
+// MergeFile merges the rules in genRules with matching rules in oldFile and
+// adds unmatched rules to the end of the merged file. MergeFile also merges
+// rules in empty with matching rules in oldFile and deletes rules that
+// are empty after merging. attrs is the set of attributes to merge. Attributes
+// not in this set will be left alone if they already exist.
+func MergeFile(genRules []bf.Expr, empty []bf.Expr, oldFile *bf.File, attrs map[string]bool) (mergedFile *bf.File, mergedRules []bf.Expr) {
 	if oldFile == nil {
-		return genFile
+		return &bf.File{Stmt: genRules}, genRules
 	}
 	if shouldIgnore(oldFile) {
-		return nil
+		return nil, nil
 	}
 
-	mergedFile := *oldFile
+	mergedFile = new(bf.File)
+	*mergedFile = *oldFile
 	mergedFile.Stmt = make([]bf.Expr, 0, len(oldFile.Stmt))
 	for _, s := range oldFile.Stmt {
 		if oldRule, ok := s.(*bf.CallExpr); ok {
 			if _, genRule := match(empty, oldRule); genRule != nil {
-				s = mergeRule(genRule, oldRule)
+				s = mergeRule(genRule, oldRule, attrs)
 				if s == nil {
 					// Deleted empty rule
 					continue
@@ -75,26 +79,30 @@ func MergeWithExisting(genFile, oldFile *bf.File, empty []bf.Expr) *bf.File {
 	}
 	oldStmtCount := len(mergedFile.Stmt)
 
-	for _, s := range genFile.Stmt {
+	for _, s := range genRules {
 		genRule, ok := s.(*bf.CallExpr)
 		if !ok {
-			log.Panicf("got %v expected only CallExpr in %q", s, genFile.Path)
+			log.Panicf("got %v expected only CallExpr in genRules", s)
 		}
 		i, oldRule := match(mergedFile.Stmt[:oldStmtCount], genRule)
 		if oldRule == nil {
 			mergedFile.Stmt = append(mergedFile.Stmt, genRule)
+			mergedRules = append(mergedRules, genRule)
 			continue
 		}
-		mergedFile.Stmt[i] = mergeRule(genRule, oldRule)
+		merged := mergeRule(genRule, oldRule, attrs)
+		mergedFile.Stmt[i] = merged
+		mergedRules = append(mergedRules, mergedFile.Stmt[i])
 	}
 
-	return &mergedFile
+	return mergedFile, mergedRules
 }
 
-// merge combines information from gen and old and returns an updated rule.
+// mergeRule combines information from gen and old and returns an updated rule.
 // Both rules must be non-nil and must have the same kind and same name.
+// attrs is the set of attributes which may be merged.
 // If nil is returned, the rule should be deleted.
-func mergeRule(gen, old *bf.CallExpr) bf.Expr {
+func mergeRule(gen, old *bf.CallExpr, attrs map[string]bool) bf.Expr {
 	if old != nil && shouldKeep(old) {
 		return old
 	}
@@ -120,7 +128,7 @@ func mergeRule(gen, old *bf.CallExpr) bf.Expr {
 	// Assume generated attributes have no comments.
 	for _, k := range oldRule.AttrKeys() {
 		oldAttr := oldRule.AttrDefn(k)
-		if !mergeableFields[k] || shouldKeep(oldAttr) {
+		if !attrs[k] || shouldKeep(oldAttr) {
 			merged.List = append(merged.List, oldAttr)
 			continue
 		}
@@ -425,7 +433,7 @@ func shouldKeep(e bf.Expr) bool {
 
 // match looks for the matching CallExpr in stmts using X and name
 // i.e. two 'go_library(name = "foo", ...)' are considered matches
-// despite the values of the other fields.
+// despite the values of the other attributes.
 // exception: if c is a 'load' statement, the match is done on the first value.
 func match(stmts []bf.Expr, c *bf.CallExpr) (int, *bf.CallExpr) {
 	var m matcher

go/tools/gazelle/merger/merger_test.go

@@ -385,7 +385,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
 go_library(
     name = "go_default_library",
     srcs = ["foo.go"],
-    deps = ["deleted"],
+    embed = ["deleted"],
 )
 `,
 		current: `
@@ -747,7 +747,7 @@ go_library(
 	},
 }
 
-func TestMergeWithExisting(t *testing.T) {
+func TestMergeFile(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			genFile, err := bf.Parse("current", []byte(tc.current))
@@ -762,7 +762,7 @@ func TestMergeWithExisting(t *testing.T) {
 			if err != nil {
 				t.Fatalf("%s: %v", tc.desc, err)
 			}
-			mergedFile := MergeWithExisting(genFile, oldFile, emptyFile.Stmt)
+			mergedFile, _ := MergeFile(genFile.Stmt, emptyFile.Stmt, oldFile, MergeableGeneratedAttrs)
 			if mergedFile == nil {
 				if !tc.ignore {
 					t.Errorf("%s: got nil; want file", tc.desc)
@@ -785,12 +785,3 @@ func TestMergeWithExisting(t *testing.T) {
 		})
 	}
 }
-
-func TestMergeWithExistingDifferentName(t *testing.T) {
-	oldFile := &bf.File{Path: "BUILD"}
-	genFile := &bf.File{Path: "BUILD.bazel"}
-	mergedFile := MergeWithExisting(genFile, oldFile, nil)
-	if got, want := mergedFile.Path, oldFile.Path; got != want {
-		t.Errorf("got %q; want %q", got, want)
-	}
-}

go/tools/gazelle/resolve/resolve.go

@@ -60,13 +60,15 @@ func NewResolver(c *config.Config, l *Labeler) *Resolver {
 	}
 }
 
-// ResolveRule modifies a generated rule e by replacing the import paths in the
-// "_gazelle_imports" attribute with labels in a "deps" attribute. This may
-// may safely called on expressions that aren't Go rules (nothing will happen).
-func (r *Resolver) ResolveRule(e bf.Expr, pkgRel string) {
+// ResolveRule copies and modifies a generated rule e by replacing the import
+// paths in the "_gazelle_imports" attribute with labels in a "deps"
+// attribute. This may be safely called on expressions that aren't Go rules
+// (the original expression will be returned). Any existing "deps" attribute
+// is deleted, so it may be necessary to merge the result.
+func (r *Resolver) ResolveRule(e bf.Expr, pkgRel string) bf.Expr {
 	call, ok := e.(*bf.CallExpr)
 	if !ok {
-		return
+		return e
 	}
 	rule := bf.Rule{Call: call}
 
@@ -79,15 +81,17 @@ func (r *Resolver) ResolveRule(e bf.Expr, pkgRel string) {
 	case "go_proto_library", "go_grpc_library":
 		resolve = r.resolveGoProto
 	default:
-		return
+		return e
 	}
 
-	imports := rule.AttrDefn(config.GazelleImportsKey)
-	if imports == nil {
-		return
-	}
+	resolved := *call
+	resolved.List = append([]bf.Expr{}, call.List...)
+	rule.Call = &resolved
 
-	deps := mapExprStrings(imports.Y, func(imp string) string {
+	imports := rule.Attr(config.GazelleImportsKey)
+	rule.DelAttr(config.GazelleImportsKey)
+	rule.DelAttr("deps")
+	deps := mapExprStrings(imports, func(imp string) string {
 		label, err := resolve(imp, pkgRel)
 		if err != nil {
 			if _, ok := err.(standardImportError); !ok {
@@ -98,12 +102,11 @@ func (r *Resolver) ResolveRule(e bf.Expr, pkgRel string) {
 		label.Relative = label.Repo == "" && label.Pkg == pkgRel
 		return label.String()
 	})
-	if deps == nil {
-		rule.DelAttr(config.GazelleImportsKey)
-	} else {
-		imports.X.(*bf.LiteralExpr).Token = "deps"
-		imports.Y = deps
+	if deps != nil {
+		rule.SetAttr("deps", deps)
 	}
+
+	return &resolved
 }
 
 type standardImportError struct {
@@ -118,6 +121,9 @@ func (e standardImportError) Error() string {
 // expression with the results. Scalar strings, lists, dicts, selects, and
 // concatenations are supported.
 func mapExprStrings(e bf.Expr, f func(string) string) bf.Expr {
+	if e == nil {
+		return nil
+	}
 	switch expr := e.(type) {
 	case *bf.StringExpr:
 		s := f(expr.Value)