From b01c9e097dd2ee0e8504a9fd34fb142428181483 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Thu, 30 Nov 2017 19:05:02 -0500 Subject: [PATCH] Gazelle: split merging into two stages We need to merge some attributes on rules before indexing them. In particular, we need to merge "importpath" and "srcs", since these become keys in the index. It's important to account for '# keep' comments and '# gazelle:ignore' directives so we need to use the regular merging algorithm instead of something ad hoc in the indexer. However, we also need to merge deps after they're resolved. This means that we need to merge rules in two separate phases. Right after rules are generated, we merge everything except "deps". Then we resolve dependencies. Then we merge "deps" only. This should also prevent us from indexing rules that are deleted. Related #859 --- go/tools/gazelle/gazelle/main.go | 98 ++++++++------------------ go/tools/gazelle/merger/merger.go | 58 ++++++++------- go/tools/gazelle/merger/merger_test.go | 15 +--- go/tools/gazelle/resolve/resolve.go | 38 +++++----- 4 files changed, 89 insertions(+), 120 deletions(-) diff --git a/go/tools/gazelle/gazelle/main.go b/go/tools/gazelle/gazelle/main.go index cef2ac921f..27664937df 100644 --- a/go/tools/gazelle/gazelle/main.go +++ b/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) { diff --git a/go/tools/gazelle/merger/merger.go b/go/tools/gazelle/merger/merger.go index 8bd5ebadfe..99319a5eed 100644 --- a/go/tools/gazelle/merger/merger.go +++ b/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 diff --git a/go/tools/gazelle/merger/merger_test.go b/go/tools/gazelle/merger/merger_test.go index 428b3f9aa3..0011c88d19 100644 --- a/go/tools/gazelle/merger/merger_test.go +++ b/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) - } -} diff --git a/go/tools/gazelle/resolve/resolve.go b/go/tools/gazelle/resolve/resolve.go index 87dab359a4..4a64eb5ec5 100644 --- a/go/tools/gazelle/resolve/resolve.go +++ b/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)