Skip to content

fix(itg): only propagate string-valued attributes through the optimiz… #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yushan8 merged 4 commits into from
Sep 22, 2026
Merged

fix(itg): only propagate string-valued attributes through the optimiz… #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4aa9c6f
fix(itg): only propagate string-valued attributes through the optimiz…
yushan8 Sep 22, 2026
db73f09
Merge branch 'main' into itg-attribute-string-type
yushan8 Sep 22, 2026
0a6c44f
fix(itg): guard nil attribute elements in optimized-graph filtering
yushan8 Sep 22, 2026
fd527e9
test(itg): verify tags order does not affect computeAvailableHashes
yushan8 Sep 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions core/itg/graph/graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,11 @@ func (g *OptimizedGraph) AddTarget(target *targethasher.Target) {

attributes := make(map[int]int, len(target.Attributes))
for _, attr := range target.Attributes {
attrNameID := getOrGenerateRecordReverse(attr.GetName(), g.AttrNameToID, g.AttrNameIDToString)
attrValueID := getOrGenerateRecordReverse(attr.GetStringValue(), g.AttrValueToID, g.AttrValueIDToString)
if attr == nil || attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil {
continue
}
attrNameID := getOrGenerateRecordReverse(*attr.Name, g.AttrNameToID, g.AttrNameIDToString)
attrValueID := getOrGenerateRecordReverse(*attr.StringValue, g.AttrValueToID, g.AttrValueIDToString)

@xytan0056 xytan0056 Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also btw does stringList order matter? such as tags = ["a", "b"] vs tags = ["b", "a"]

@yushan8 yushan8 Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good q, it does. Added a test to verify.

attributes[attrNameID] = attrValueID
}
optimizedTarget.HashWithoutDeps = target.HashWithoutDeps
Expand Down Expand Up @@ -271,9 +274,11 @@ func (g *OptimizedGraph) OptimizedTargetToTarget(targetID int) targethasher.Targ
for nameID, valID := range optimizedTarget.Attributes {
n := g.AttrNameIDToString[nameID]
v := g.AttrValueIDToString[valID]
t := buildpb.Attribute_STRING
target.Attributes = append(target.Attributes, &buildpb.Attribute{
Name: &n,
StringValue: &v,
Type: &t,
})
}

Expand Down
107 changes: 107 additions & 0 deletions core/itg/graph/graph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,16 @@ import (
"sort"
"testing"

buildpb "github.com/bazelbuild/buildtools/build_proto"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/uber/tango/core/targethasher"
)

func strPtr(s string) *string { return &s }

func attrTypePtr(t buildpb.Attribute_Discriminator) *buildpb.Attribute_Discriminator { return &t }

// --- IntSet ---

func TestIntSet(t *testing.T) {
Expand Down Expand Up @@ -184,6 +189,87 @@ func TestOptimizeGraph(t *testing.T) {
})
}

// --- AddTarget attribute filtering ---

func TestAddTargetAttributes(t *testing.T) {
t.Parallel()

t.Run("only string-typed attributes are recorded", func(t *testing.T) {
t.Parallel()
targets := map[string]*targethasher.Target{
"//pkg:a": {
Name: "//pkg:a",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
{Name: strPtr("importpath"), StringValue: strPtr("example.com/a"), Type: attrTypePtr(buildpb.Attribute_STRING)},
{Name: strPtr("deps"), StringValue: strPtr("ignored"), Type: attrTypePtr(buildpb.Attribute_LABEL_LIST)},
},
},
}
g := OptimizeGraph(targets)
aID := g.TargetNameToID["//pkg:a"]

assert.Len(t, g.OptimizedTargets[aID].Attributes, 1)
_, ok := g.AttrNameToID["importpath"]
assert.True(t, ok, "string attribute should be recorded")
_, ok = g.AttrNameToID["deps"]
assert.False(t, ok, "non-string attribute should be skipped")
})

t.Run("attribute with nil type is skipped", func(t *testing.T) {
t.Parallel()
targets := map[string]*targethasher.Target{
"//pkg:a": {
Name: "//pkg:a",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
{Name: strPtr("untyped"), StringValue: strPtr("value")},
},
},
}
g := OptimizeGraph(targets)
aID := g.TargetNameToID["//pkg:a"]

assert.Empty(t, g.OptimizedTargets[aID].Attributes)
})

t.Run("string attribute with nil name or value is skipped", func(t *testing.T) {
t.Parallel()
targets := map[string]*targethasher.Target{
"//pkg:a": {
Name: "//pkg:a",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
{Name: nil, StringValue: strPtr("value"), Type: attrTypePtr(buildpb.Attribute_STRING)},
{Name: strPtr("name"), StringValue: nil, Type: attrTypePtr(buildpb.Attribute_STRING)},
},
},
}
g := OptimizeGraph(targets)
aID := g.TargetNameToID["//pkg:a"]

assert.Empty(t, g.OptimizedTargets[aID].Attributes)
})

t.Run("nil attribute element does not panic", func(t *testing.T) {
t.Parallel()
targets := map[string]*targethasher.Target{
"//pkg:a": {
Name: "//pkg:a",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
nil,
{Name: strPtr("importpath"), StringValue: strPtr("example.com/a"), Type: attrTypePtr(buildpb.Attribute_STRING)},
},
},
}
g := OptimizeGraph(targets)
aID := g.TargetNameToID["//pkg:a"]

assert.Len(t, g.OptimizedTargets[aID].Attributes, 1)
})
}

// --- OptimizedTarget.Copy ---

func TestOptimizedTargetCopy(t *testing.T) {
Expand Down Expand Up @@ -288,4 +374,25 @@ func TestOptimizedTargetToTarget(t *testing.T) {
result := g.OptimizedTargetToTarget(libID)
assert.Contains(t, result.Deps, "//pkg:dep")
})

t.Run("reconstructed attributes are marked as string type", func(t *testing.T) {
t.Parallel()
targets := map[string]*targethasher.Target{
"//pkg:a": {
Name: "//pkg:a",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
{Name: strPtr("importpath"), StringValue: strPtr("example.com/a"), Type: attrTypePtr(buildpb.Attribute_STRING)},
},
},
}
g := OptimizeGraph(targets)
aID := g.TargetNameToID["//pkg:a"]

result := g.OptimizedTargetToTarget(aID)
require.Len(t, result.Attributes, 1)
assert.Equal(t, "importpath", result.Attributes[0].GetName())
assert.Equal(t, "example.com/a", result.Attributes[0].GetStringValue())
assert.Equal(t, buildpb.Attribute_STRING, result.Attributes[0].GetType())
})
}
8 changes: 6 additions & 2 deletions core/itg/graph/invalidate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"strings"

buildpb "github.com/bazelbuild/buildtools/build_proto"
"github.com/uber/tango/core/targethasher"
)

Expand Down Expand Up @@ -164,8 +165,11 @@ func (g *OptimizedGraph) upsertTarget(target *targethasher.Target, invalidated I

attributes := make(map[int]int, len(target.Attributes))
for _, attr := range target.Attributes {
attrNameID := getOrGenerateRecordReverse(attr.GetName(), g.AttrNameToID, g.AttrNameIDToString)
attrValueID := getOrGenerateRecordReverse(attr.GetStringValue(), g.AttrValueToID, g.AttrValueIDToString)
if attr == nil || attr.GetType() != buildpb.Attribute_STRING || attr.Name == nil || attr.StringValue == nil {
continue
}
attrNameID := getOrGenerateRecordReverse(*attr.Name, g.AttrNameToID, g.AttrNameIDToString)
attrValueID := getOrGenerateRecordReverse(*attr.StringValue, g.AttrValueToID, g.AttrValueIDToString)
attributes[attrNameID] = attrValueID
}

Expand Down
62 changes: 62 additions & 0 deletions core/itg/graph/invalidate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package graph
import (
"testing"

buildpb "github.com/bazelbuild/buildtools/build_proto"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/uber/tango/core/targethasher"
Expand Down Expand Up @@ -246,4 +247,65 @@ func TestUpsertTarget(t *testing.T) {
libID := g.TargetNameToID["//pkg:lib"]
assert.True(t, invalidated.Contains(libID), "target with nil hash should be in invalidated set")
})

t.Run("only string-typed attributes are recorded on upsert", func(t *testing.T) {
t.Parallel()
g := OptimizeGraph(nil)
newTarget := &targethasher.Target{
Name: "//pkg:lib",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
{Name: strPtr("importpath"), StringValue: strPtr("example.com/lib"), Type: attrTypePtr(buildpb.Attribute_STRING)},
{Name: strPtr("deps"), StringValue: strPtr("ignored"), Type: attrTypePtr(buildpb.Attribute_LABEL_LIST)},
},
}
require.NoError(t, g.upsertTarget(newTarget, NewIntSet()))

libID := g.TargetNameToID["//pkg:lib"]
assert.Len(t, g.OptimizedTargets[libID].Attributes, 1)
_, ok := g.AttrNameToID["deps"]
assert.False(t, ok, "non-string attribute should be skipped")
})

t.Run("nil attribute element does not panic on upsert", func(t *testing.T) {
t.Parallel()
g := OptimizeGraph(nil)
newTarget := &targethasher.Target{
Name: "//pkg:lib",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
nil,
{Name: strPtr("importpath"), StringValue: strPtr("example.com/lib"), Type: attrTypePtr(buildpb.Attribute_STRING)},
},
}
require.NoError(t, g.upsertTarget(newTarget, NewIntSet()))

libID := g.TargetNameToID["//pkg:lib"]
assert.Len(t, g.OptimizedTargets[libID].Attributes, 1)
})

t.Run("attribute updates replace prior attributes on upsert", func(t *testing.T) {
t.Parallel()
g := OptimizeGraph(map[string]*targethasher.Target{
"//pkg:lib": {
Name: "//pkg:lib",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
{Name: strPtr("importpath"), StringValue: strPtr("example.com/old"), Type: attrTypePtr(buildpb.Attribute_STRING)},
},
},
})
libID := g.TargetNameToID["//pkg:lib"]
require.Len(t, g.OptimizedTargets[libID].Attributes, 1)

updated := &targethasher.Target{
Name: "//pkg:lib",
RuleType: "go_library",
Attributes: []*buildpb.Attribute{
{Name: strPtr("nonstring"), StringValue: strPtr("value"), Type: attrTypePtr(buildpb.Attribute_INTEGER)},
},
}
require.NoError(t, g.upsertTarget(updated, NewIntSet()))
assert.Empty(t, g.OptimizedTargets[libID].Attributes, "non-string attribute should not replace prior one")
})
}
41 changes: 41 additions & 0 deletions core/itg/graph/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,47 @@ func TestComputeAvailableHashes(t *testing.T) {
assert.Nil(t, targets[name].Hash, "full hash is not computed here — deps are needed")
})

t.Run("tags order does not affect rule HashWithoutDeps", func(t *testing.T) {
t.Parallel()
name := "//pkg:lib"
ruleName := name
ruleClass := "go_library"
tagsAttr := func(tags []string) *buildpb.Attribute {
typ := buildpb.Attribute_STRING_LIST
n := "tags"
return &buildpb.Attribute{Name: &n, Type: &typ, StringListValue: tags}
}

forward := map[string]*targethasher.Target{
name: {
Name: name,
RuleType: "go_library",
Rule: &buildpb.Rule{
Name: &ruleName,
RuleClass: &ruleClass,
Attribute: []*buildpb.Attribute{tagsAttr([]string{"a", "b"})},
},
},
}
reversed := map[string]*targethasher.Target{
name: {
Name: name,
RuleType: "go_library",
Rule: &buildpb.Rule{
Name: &ruleName,
RuleClass: &ruleClass,
Attribute: []*buildpb.Attribute{tagsAttr([]string{"b", "a"})},
},
},
}

require.NoError(t, computeAvailableHashes(context.Background(), &fakeSourceHasher{}, forward))
require.NoError(t, computeAvailableHashes(context.Background(), &fakeSourceHasher{}, reversed))

assert.Equal(t, forward[name].HashWithoutDeps, reversed[name].HashWithoutDeps,
"tags=[a,b] and tags=[b,a] should hash identically")
})

t.Run("source hasher error is propagated", func(t *testing.T) {
t.Parallel()
hasher := &fakeSourceHasher{err: assert.AnError}
Expand Down
Loading