Skip to content

Commit

Permalink
fixup! fix: overwrite attribute values when not mergeable or resolveable
Browse files Browse the repository at this point in the history
  • Loading branch information
@jbedard
jbedard committed Oct 23, 2024
1 parent a01ac64 commit 23e8768
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 42 deletions.
40 changes: 23 additions & 17 deletions merger/merger.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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()
}
Expand Down Expand Up @@ -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
Expand Down
12 changes: 5 additions & 7 deletions rule/merge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -43,14 +41,14 @@ 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) {
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 {
Expand All @@ -72,12 +70,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()
Expand All @@ -87,7 +85,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)
}
Expand Down
65 changes: 47 additions & 18 deletions rule/merge_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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",
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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)
}
})
}

0 comments on commit 23e8768

Please sign in to comment.