fix: overwrite attribute values when not mergeable or resolveable

cmd/gazelle/BUILD.bazel

@@ -22,7 +22,7 @@ go_library(
     ],
     importpath = "github.com/bazelbuild/bazel-gazelle/cmd/gazelle",
     tags = ["manual"],
-    visibility = ["//visibility:public"],
+    visibility = ["//visibility:public"],  #keep
     deps = [
         "//config",
         "//flag",

cmd/gazelle/integration_test.go

@@ -272,7 +272,7 @@ go_library(
     ],
     cgo = True,
     importpath = "example.com/foo",
-    visibility = ["//visibility:default"],
+    visibility = ["//visibility:public"],
 )
 
 cgo_library(
@@ -294,7 +294,7 @@ go_library(
     ],
     cgo = True,
     importpath = "example.com/foo",
-    visibility = ["//visibility:default"],
+    visibility = ["//visibility:public"],
 )
 `,
 		},

internal/language/test_filegroup/BUILD.bazel

@@ -4,7 +4,7 @@ go_library(
     name = "test_filegroup",
     srcs = ["lang.go"],
     importpath = "github.com/bazelbuild/bazel-gazelle/internal/language/test_filegroup",
-    visibility = ["//visibility:public"],
+    visibility = ["//visibility:public"],  #keep
     deps = [
         "//config",
         "//label",

internal/language/test_load_for_packed_rules/BUILD.bazel

@@ -4,7 +4,7 @@ go_library(
     name = "test_load_for_packed_rules",
     srcs = ["lang.go"],
     importpath = "github.com/bazelbuild/bazel-gazelle/internal/language/test_load_for_packed_rules",
-    visibility = ["//visibility:public"],
+    visibility = ["//:__subpackages__"],
     deps = [
         "//config",
         "//label",

internal/language/test_loads_from_flag/BUILD.bazel

@@ -4,7 +4,7 @@ go_library(
     name = "test_loads_from_flag",
     srcs = ["lang.go"],
     importpath = "github.com/bazelbuild/bazel-gazelle/internal/language/test_loads_from_flag",
-    visibility = ["//visibility:public"],
+    visibility = ["//:__subpackages__"],
     deps = [
         "//config",
         "//language",

internal/wspace/BUILD.bazel

@@ -4,7 +4,7 @@ go_library(
     name = "wspace",
     srcs = ["finder.go"],
     importpath = "github.com/bazelbuild/bazel-gazelle/internal/wspace",
-    visibility = ["//visibility:public"],
+    visibility = ["//:__subpackages__"],
 )
 
 go_test(

language/proto/gen/BUILD.bazel

@@ -12,7 +12,7 @@ go_library(
 go_binary(
     name = "gen_known_imports",
     embed = [":gen_lib"],
-    visibility = ["//:__subpackages__"],
+    visibility = ["//visibility:public"],
 )
 
 filegroup(

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 is uniquely managed by gazelle.
+		if attr == "name" {
+			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)
 		}
 	}
 }

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)
 		}
 	}
 

rule/merge_test.go

@@ -22,14 +22,21 @@ 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, 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 +51,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 +75,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 +97,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 +113,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 +137,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 {