Skip to content

Commit

Permalink
Gazelle: split merging into two stages (#1099)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jayconrod
jayconrod authored Dec 4, 2017
1 parent 7a686cc commit 3930b2c
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 120 deletions.
98 changes: 31 additions & 67 deletions go/tools/gazelle/gazelle/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
}
}
Expand All @@ -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) {
Expand Down
58 changes: 33 additions & 25 deletions go/tools/gazelle/merger/merger.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
15 changes: 3 additions & 12 deletions go/tools/gazelle/merger/merger_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: `
Expand Down Expand Up @@ -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))
Expand All @@ -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)
Expand All @@ -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)
}
}
38 changes: 22 additions & 16 deletions go/tools/gazelle/resolve/resolve.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}

Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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)
Expand Down

0 comments on commit 3930b2c

Please sign in to comment.