fixup! fix: overwrite attribute values when not mergeable or resolveable

merger/merger.go

@@ -107,23 +107,23 @@ 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) {
-	isMergeable := func(r *rule.Rule, attr string) bool {
-		// Attributes merged in this phase.
-		if phase == PreResolve {
-			return kinds[r.Kind()].MergeableAttrs[attr]
-		} else {
-			return kinds[r.Kind()].ResolveAttrs[attr]
-		}
-	}
+	getMergeAttrs := func(r *rule.Rule) map[string]bool {
+		kind := kinds[r.Kind()]
 
-	isManaged := func(r *rule.Rule, attr string) bool {
-		// Name and visibility are uniquely managed by gazelle.
-		if attr == "name" || attr == "visibility" {
-			return true
+		mergeableAttrs := map[string]bool{
+			// Name and visibility are uniquely managed by gazelle.
+			"name":       false,
+			"visibility": false,
 		}
-		k := kinds[r.Kind()]
-		// All known attributes of this rule kind.
-		return k.NonEmptyAttrs[attr] || k.SubstituteAttrs[attr] || k.ResolveAttrs[attr] || k.MergeableAttrs[attr]
+
+		// All attributes managed by gazelle, marking only the current merge attributes
+		// with a truthy value.
+		combineMergeAttrs(mergeableAttrs, kind.MergeableAttrs, phase == PreResolve)
+		combineMergeAttrs(mergeableAttrs, kind.ResolveAttrs, phase == PostResolve)
+		combineMergeAttrs(mergeableAttrs, kind.NonEmptyAttrs, false)
+		combineMergeAttrs(mergeableAttrs, kind.SubstituteAttrs, false)
+
+		return mergeableAttrs
 	}
 
 	// Merge empty rules into the file and delete any rules which become empty.
@@ -132,7 +132,7 @@ func MergeFile(oldFile *rule.File, emptyRules, genRules []*rule.Rule, phase Phas
 			if oldRule.ShouldKeep() {
 				continue
 			}
-			rule.MergeRules(emptyRule, oldRule, isMergeable, isManaged, oldFile.Path)
+			rule.MergeRules(emptyRule, oldRule, getMergeAttrs(emptyRule), oldFile.Path)
 			if oldRule.IsEmpty(kinds[oldRule.Kind()]) {
 				oldRule.Delete()
 			}
@@ -180,11 +180,17 @@ func MergeFile(oldFile *rule.File, emptyRules, genRules []*rule.Rule, phase Phas
 				genRule.Insert(oldFile)
 			}
 		} else {
-			rule.MergeRules(genRule, matchRules[i], isMergeable, isManaged, oldFile.Path)
+			rule.MergeRules(genRule, matchRules[i], getMergeAttrs(genRule), oldFile.Path)
 		}
 	}
 }
 
+func combineMergeAttrs(dst map[string]bool, src map[string]bool, value bool) {
+	for k, _ := range src {
+		dst[k] = value || dst[k]
+	}
+}
+
 // substituteRule replaces local labels (those beginning with ":", referring to
 // targets in the same package) according to a substitution map. This is used
 // to update generated rules before merging when the corresponding existing

rule/merge.go

@@ -24,8 +24,6 @@ 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.
 //
@@ -43,14 +41,17 @@ type IsMergeable = func(r *Rule, attr string) bool
 // 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, isMergeable, isManaged IsMergeable, filename string) {
+//
+// If src has an attribute not present in 'mergeable' and not marked with a
+// "# keep" comment, values in the dist attribute will be overwritten.
+func MergeRules(src, dst *Rule, mergeable map[string]bool, 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 || !isMergeable(src, key) || ShouldKeep(dstAttr.expr) {
+		if _, ok := src.attrs[key]; ok || !mergeable[key] || ShouldKeep(dstAttr.expr) {
 			continue
 		}
 		if mergedValue, err := mergeAttrValues(nil, &dstAttr); err != nil {
@@ -72,12 +73,12 @@ func MergeRules(src, dst *Rule, isMergeable, isManaged IsMergeable, filename str
 			continue
 		}
 
-		// Do not override attributes marked with "# keep".
+		// Do not overwrite attributes marked with "# keep".
 		if ShouldKeep(dstAttr.expr) {
 			continue
 		}
 
-		if isMergeable(src, key) {
+		if attrMergeable, attrKnown := mergeable[key]; attrMergeable {
 			// Merge mergeable attributes.
 			if mergedValue, err := mergeAttrValues(&srcAttr, &dstAttr); err != nil {
 				start, end := dstAttr.expr.RHS.Span()
@@ -87,7 +88,7 @@ func MergeRules(src, dst *Rule, isMergeable, isManaged IsMergeable, filename str
 			} else {
 				dst.SetAttr(key, mergedValue)
 			}
-		} else if !isManaged(src, key) {
+		} else if !attrKnown {
 			// Overwrite unknown attributes.
 			dst.SetAttr(key, srcAttr.expr.RHS)
 		}

rule/merge_test.go

@@ -23,21 +23,14 @@ import (
 	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")
 		privateAttrKey := "_my_private_attr"
 		privateAttrVal := "private_value"
 		src.SetPrivateAttr(privateAttrKey, privateAttrVal)
 		dst := rule.NewRule("go_library", "go_default_library")
-		rule.MergeRules(src, dst, notMergeable, notMergeable, "")
+		rule.MergeRules(src, dst, map[string]bool{}, "")
 		if dst.PrivateAttr(privateAttrKey).(string) != privateAttrVal {
 			t.Fatalf("private attributes are merged: got %v; want %s",
 				dst.PrivateAttr(privateAttrKey), privateAttrVal)
@@ -52,7 +45,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, depsMergeable, notMergeable, "")
+		rule.MergeRules(src, dst, map[string]bool{"deps": true}, "")
 
 		valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr)
 		if !ok {
@@ -76,7 +69,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, depsMergeable, notMergeable, "")
+		rule.MergeRules(src, dst, map[string]bool{"deps": true}, "")
 
 		valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr)
 		if !ok {
@@ -98,7 +91,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, depsMergeable, notMergeable, "")
+		rule.MergeRules(src, dst, map[string]bool{"deps": true}, "")
 
 		if dst.Attr(sortedStringAttrKey) != nil {
 			t.Fatalf("delete existing sorted strings: got %v; want nil",
@@ -114,7 +107,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, depsMergeable, notMergeable, "")
+		rule.MergeRules(src, dst, map[string]bool{"deps": true}, "")
 
 		valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr)
 		if !ok {
@@ -138,7 +131,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, depsMergeable, notMergeable, "")
+		rule.MergeRules(src, dst, map[string]bool{"deps": true}, "")
 
 		valExpr, ok := dst.Attr(sortedStringAttrKey).(*bzl.ListExpr)
 		if !ok {
@@ -164,18 +157,54 @@ func TestMergeRules_WithUnmanagedAttr(t *testing.T) {
 		src.SetAttr(unknownStringAttrKey, "foobar")
 		dst := rule.NewRule("go_library", "go_default_library")
 		dst.SetAttr(unknownStringAttrKey, overwrittenAttrVal)
-		rule.MergeRules(src, dst, notMergeable, notMergeable, "")
+		rule.MergeRules(src, dst, map[string]bool{}, "")
 
 		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)))
+			t.Fatalf("unknown attributes (%q) are overwritten: got %v; want *bzl.StringExpr",
+				unknownStringAttrKey, reflect.TypeOf(dst.Attr(unknownStringAttrKey)))
 		}
 
 		expected := "foobar"
 		if valExpr.Value != expected {
-			t.Fatalf("unknown attributes are overwritten: got %v; want %v",
-				valExpr.Value, expected)
+			t.Fatalf("unknown attributes (%q) are overwritten: got %v; want %v",
+				unknownStringAttrKey, valExpr.Value, expected)
+		}
+	})
+
+	t.Run("known string attributes do NOT overwrite attributes in non-empty rule", func(t *testing.T) {
+		src := rule.NewRule("go_library", "go_default_library")
+		unknownStringAttrKey := "unknown_attr"
+		knownStringAttrKey := "knownAttrName"
+		overwrittenAttrVal := "original"
+		src.SetAttr(unknownStringAttrKey, "foobar")
+		src.SetAttr(knownStringAttrKey, "foobar")
+		dst := rule.NewRule("go_library", "go_default_library")
+		dst.SetAttr(unknownStringAttrKey, overwrittenAttrVal)
+		dst.SetAttr(knownStringAttrKey, overwrittenAttrVal)
+		rule.MergeRules(src, dst, map[string]bool{knownStringAttrKey: false}, "")
+
+		// The unknown + overwritten attribute
+		unknownValExpr, ok := dst.Attr(unknownStringAttrKey).(*bzl.StringExpr)
+		if !ok {
+			t.Fatalf("unknown attributes (%q) are overwritten: got %v; want *bzl.StringExpr",
+				unknownStringAttrKey, reflect.TypeOf(dst.Attr(unknownStringAttrKey)))
+		}
+		expected := "foobar"
+		if unknownValExpr.Value != expected {
+			t.Fatalf("unknown attributes (%q) are overwritten: got %v; want %v",
+				unknownStringAttrKey, unknownValExpr.Value, expected)
+		}
+
+		// The known but non-mergeable attributes
+		knownValExpr, ok := dst.Attr(knownStringAttrKey).(*bzl.StringExpr)
+		if !ok {
+			t.Fatalf("known attributes (%q) are NOT overwritten: got %v; want *bzl.StringExpr",
+				knownStringAttrKey, reflect.TypeOf(dst.Attr(knownStringAttrKey)))
+		}
+		if knownValExpr.Value != overwrittenAttrVal {
+			t.Fatalf("known attributes (%q) are NOT overwritten: got %v; want %v",
+				knownStringAttrKey, knownValExpr.Value, overwrittenAttrVal)
 		}
 	})
 }