diff --git a/merger/merger.go b/merger/merger.go index b1827d18c..6047c79e5 100644 --- a/merger/merger.go +++ b/merger/merger.go @@ -107,21 +107,32 @@ const UnstableInsertIndexKey = "_gazelle_insert_index" // If a rule is marked with a "# keep" comment, the whole rule will not // be modified. func MergeFile(oldFile *rule.File, emptyRules, genRules []*rule.Rule, phase Phase, kinds map[string]rule.KindInfo) { - getMergeAttrs := func(r *rule.Rule) map[string]bool { + isMergeable := func(r *rule.Rule, attr string) bool { + // Attributes merged in this phase. if phase == PreResolve { - return kinds[r.Kind()].MergeableAttrs + return kinds[r.Kind()].MergeableAttrs[attr] } else { - return kinds[r.Kind()].ResolveAttrs + return kinds[r.Kind()].ResolveAttrs[attr] } } + isManaged := func(r *rule.Rule, attr string) bool { + // Name and visibility are uniquely managed by gazelle. + if attr == "name" || attr == "visibility" { + return true + } + k := kinds[r.Kind()] + // All known attributes of this rule kind. + return k.NonEmptyAttrs[attr] || k.SubstituteAttrs[attr] || k.ResolveAttrs[attr] || k.MergeableAttrs[attr] + } + // Merge empty rules into the file and delete any rules which become empty. for _, emptyRule := range emptyRules { if oldRule, _ := match(oldFile.Rules, emptyRule, kinds[emptyRule.Kind()], false); oldRule != nil { if oldRule.ShouldKeep() { continue } - rule.MergeRules(emptyRule, oldRule, getMergeAttrs(emptyRule), oldFile.Path) + rule.MergeRules(emptyRule, oldRule, isMergeable, isManaged, oldFile.Path) if oldRule.IsEmpty(kinds[oldRule.Kind()]) { oldRule.Delete() } @@ -169,7 +180,7 @@ func MergeFile(oldFile *rule.File, emptyRules, genRules []*rule.Rule, phase Phas genRule.Insert(oldFile) } } else { - rule.MergeRules(genRule, matchRules[i], getMergeAttrs(genRule), oldFile.Path) + rule.MergeRules(genRule, matchRules[i], isMergeable, isManaged, oldFile.Path) } } } diff --git a/rule/merge.go b/rule/merge.go index ae2acdda4..6f5aebb33 100644 --- a/rule/merge.go +++ b/rule/merge.go @@ -24,6 +24,8 @@ import ( bzl "github.com/bazelbuild/buildtools/build" ) +type IsMergeable = func(r *Rule, attr string) bool + // MergeRules copies information from src into dst, usually discarding // information in dst when they have the same attributes. // @@ -41,14 +43,14 @@ import ( // marked with a "# keep" comment, values in the attribute not marked with // a "# keep" comment will be dropped. If the attribute is empty afterward, // it will be deleted. -func MergeRules(src, dst *Rule, mergeable map[string]bool, filename string) { +func MergeRules(src, dst *Rule, isMergeable, isManaged IsMergeable, filename string) { if dst.ShouldKeep() { return } // Process attributes that are in dst but not in src. for key, dstAttr := range dst.attrs { - if _, ok := src.attrs[key]; ok || !mergeable[key] || ShouldKeep(dstAttr.expr) { + if _, ok := src.attrs[key]; ok || !isMergeable(src, key) || ShouldKeep(dstAttr.expr) { continue } if mergedValue, err := mergeAttrValues(nil, &dstAttr); err != nil { @@ -63,9 +65,20 @@ func MergeRules(src, dst *Rule, mergeable map[string]bool, filename string) { // Merge attributes from src into dst. for key, srcAttr := range src.attrs { - if dstAttr, ok := dst.attrs[key]; !ok { + // Always set properties that are not yet set. + dstAttr, dstAttrExists := dst.attrs[key] + if !dstAttrExists { dst.SetAttr(key, srcAttr.expr.RHS) - } else if mergeable[key] && !ShouldKeep(dstAttr.expr) { + continue + } + + // Do not override attributes marked with "# keep". + if ShouldKeep(dstAttr.expr) { + continue + } + + if isMergeable(src, key) { + // Merge mergeable attributes. if mergedValue, err := mergeAttrValues(&srcAttr, &dstAttr); err != nil { start, end := dstAttr.expr.RHS.Span() log.Printf("%s:%d.%d-%d.%d: could not merge expression", filename, start.Line, start.LineRune, end.Line, end.LineRune) @@ -74,6 +87,9 @@ func MergeRules(src, dst *Rule, mergeable map[string]bool, filename string) { } else { dst.SetAttr(key, mergedValue) } + } else if !isManaged(src, key) { + // Overwrite unknown attributes. + dst.SetAttr(key, srcAttr.expr.RHS) } } diff --git a/rule/merge_test.go b/rule/merge_test.go index 6718d90b6..35d863295 100644 --- a/rule/merge_test.go +++ b/rule/merge_test.go @@ -16,12 +16,20 @@ limitations under the License. package rule_test import ( + "reflect" "testing" "github.com/bazelbuild/bazel-gazelle/rule" bzl "github.com/bazelbuild/buildtools/build" ) +func depsMergeable(r *rule.Rule, attr string) bool { + return attr == "deps" +} +func notMergeable(r *rule.Rule, attr string) bool { + return false +} + func TestMergeRules(t *testing.T) { t.Run("private attributes are merged", func(t *testing.T) { src := rule.NewRule("go_library", "go_default_library") @@ -29,7 +37,7 @@ func TestMergeRules(t *testing.T) { privateAttrVal := "private_value" src.SetPrivateAttr(privateAttrKey, privateAttrVal) dst := rule.NewRule("go_library", "go_default_library") - rule.MergeRules(src, dst, map[string]bool{}, "") + rule.MergeRules(src, dst, notMergeable, notMergeable, "") if dst.PrivateAttr(privateAttrKey).(string) != privateAttrVal { t.Fatalf("private attributes are merged: got %v; want %s", dst.PrivateAttr(privateAttrKey), privateAttrVal) @@ -44,7 +52,7 @@ func TestMergeRules_WithSortedStringAttr(t *testing.T) { sortedStringAttrVal := rule.SortedStrings{"@qux", "//foo:bar", "//foo:baz"} src.SetAttr(sortedStringAttrKey, sortedStringAttrVal) dst := rule.NewRule("go_library", "go_default_library") - rule.MergeRules(src, dst, map[string]bool{}, "") + rule.MergeRules(src, dst, depsMergeable, notMergeable, "") valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr) if !ok { @@ -68,7 +76,7 @@ func TestMergeRules_WithSortedStringAttr(t *testing.T) { src.SetAttr(sortedStringAttrKey, sortedStringAttrVal) dst := rule.NewRule("go_library", "go_default_library") dst.SetAttr(sortedStringAttrKey, rule.SortedStrings{"@qux", "//foo:bar", "//bacon:eggs"}) - rule.MergeRules(src, dst, map[string]bool{"deps": true}, "") + rule.MergeRules(src, dst, depsMergeable, notMergeable, "") valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr) if !ok { @@ -90,7 +98,7 @@ func TestMergeRules_WithSortedStringAttr(t *testing.T) { dst := rule.NewRule("go_library", "go_default_library") sortedStringAttrVal := rule.SortedStrings{"@qux", "//foo:bar", "//foo:baz"} dst.SetAttr(sortedStringAttrKey, sortedStringAttrVal) - rule.MergeRules(src, dst, map[string]bool{"deps": true}, "") + rule.MergeRules(src, dst, depsMergeable, notMergeable, "") if dst.Attr(sortedStringAttrKey) != nil { t.Fatalf("delete existing sorted strings: got %v; want nil", @@ -106,7 +114,7 @@ func TestMergeRules_WithUnsortedStringAttr(t *testing.T) { sortedStringAttrVal := rule.UnsortedStrings{"@qux", "//foo:bar", "//foo:baz"} src.SetAttr(sortedStringAttrKey, sortedStringAttrVal) dst := rule.NewRule("go_library", "go_default_library") - rule.MergeRules(src, dst, map[string]bool{}, "") + rule.MergeRules(src, dst, depsMergeable, notMergeable, "") valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr) if !ok { @@ -130,7 +138,7 @@ func TestMergeRules_WithUnsortedStringAttr(t *testing.T) { src.SetAttr(sortedStringAttrKey, sortedStringAttrVal) dst := rule.NewRule("go_library", "go_default_library") dst.SetAttr(sortedStringAttrKey, rule.UnsortedStrings{"@qux", "//foo:bar", "//bacon:eggs"}) - rule.MergeRules(src, dst, map[string]bool{"deps": true}, "") + rule.MergeRules(src, dst, depsMergeable, notMergeable, "") valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr) if !ok { @@ -147,3 +155,27 @@ func TestMergeRules_WithUnsortedStringAttr(t *testing.T) { } }) } + +func TestMergeRules_WithUnmanagedAttr(t *testing.T) { + t.Run("unknown string attributes overwrite attributes in non-empty rule", func(t *testing.T) { + src := rule.NewRule("go_library", "go_default_library") + unknownStringAttrKey := "unknown_attr" + overwrittenAttrVal := rule.UnsortedStrings{"@qux", "//foo:bar", "//foo:baz"} + src.SetAttr(unknownStringAttrKey, "foobar") + dst := rule.NewRule("go_library", "go_default_library") + dst.SetAttr(unknownStringAttrKey, overwrittenAttrVal) + rule.MergeRules(src, dst, notMergeable, notMergeable, "") + + valExpr, ok := dst.Attr(unknownStringAttrKey).(*bzl.StringExpr) + if !ok { + t.Fatalf("unknown attributes are overwritten: got %v; want *bzl.StringExpr", + reflect.TypeOf(dst.Attr(unknownStringAttrKey))) + } + + expected := "foobar" + if valExpr.Value != expected { + t.Fatalf("unknown attributes are overwritten: got %v; want %v", + valExpr.Value, expected) + } + }) +}