Skip to content

Allow callers to explicitly provide types for set and add commands #1448

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

Open
vladmos wants to merge 1 commit into
base: main
Choose a base branch
from
+169 −11
Open

Allow callers to explicitly provide types for set and add commands #1448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
94 changes: 94 additions & 0 deletions buildozer/buildozer_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,50 @@ function test_add_duplicate_label2() {
)'
}

function test_add_override_ident() {
in='go_library(
name = "edit",
deps = ["build"],
)'
run "$in" 'add deps:ident build' '//pkg:edit'
assert_equals 'go_library(
name = "edit",
deps = [
build,
"build",
],
)'
}

function test_add_override_ident2() {
in='go_library(
name = "edit",
deps = ["build"],
)'
run "$in" 'add deps:raw_list build test()' '//pkg:edit'
assert_equals 'go_library(
name = "edit",
deps = [
build,
test(),
"build",
],
)'
}

function test_add_override_unknown_type() {
in='go_library(
name = "edit",
deps = ["build"],
)'
ERROR=2 run "$in" 'add deps:foo build' '//pkg:edit'
assert_equals 'go_library(
name = "edit",
deps = ["build"],
)'
}


function test_remove_last_dep() {
run "$one_dep" 'remove deps //buildifier:build' '//pkg:edit'
assert_equals 'go_library(name = "edit")'
Expand Down Expand Up @@ -1085,6 +1129,16 @@ function test_set_if_absent_present() {
)'
}

function test_set_if_absent_override_string() {
in='soy_js(name = "a")'

run "$in" 'set_if_absent allowv1syntax:string 1' '//pkg:a'
assert_equals 'soy_js(
name = "a",
allowv1syntax = "1",
)'
}

function test_set_custom_code() {
in='cc_test(name = "a")'

Expand All @@ -1098,6 +1152,46 @@ function test_set_custom_code() {
)'
}

function test_set_override_string() {
in='cc_library(name = "a")'

run "$in" 'set copts:string foo' '//pkg:a'
assert_equals 'cc_library(
name = "a",
copts = "foo",
)'
}

function test_set_override_ident() {
in='cc_library(name = "a")'

run "$in" 'set copts:ident foo' '//pkg:a'
assert_equals 'cc_library(
name = "a",
copts = foo,
)'
}

function test_set_override_list_of_strings() {
in='cc_library(name = "a")'

run "$in" 'set name:list_of_strings b c' '//pkg:a'
assert_equals 'cc_library(name = [
"b",
"c",
])'
}

function test_set_override_list_of_idents() {
in='cc_library(name = "a")'

run "$in" 'set name:list b c' '//pkg:a'
assert_equals 'cc_library(name = [
b,
c,
])'
}

function assert_output() {
echo "$1" > "expected"
diff -u "expected" "$log" || fail "Output didn't match"
Expand Down
86 changes: 75 additions & 11 deletions edit/buildozer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,54 @@ type CmdEnvironment struct {
output *apipb.Output_Record // output proto, stores whatever a command wants to print
}

// AttrType is an enum representing a type of the attribute, e.g. int, string, list of strings, etc
type AttrType int

const (
// notProvidedTypeAttr represents an attr type that's not provided explicitly and
// needs to be inferred by buildozer from bazel's build language proto
notProvidedTypeAttr = iota
// rawAttr is a type for idents, ints or other types that buildozer should
// take as is, without quoting or wrapping
rawAttr
// stringAttr is a type for strings (or labels)
stringAttr
// rawListAttr is a type for lists of raw objects (same as rawAttr above)
rawListAttr
// stringListAttr is a type for lists of strings (or labels)
stringListAttr
)

// parseAttr parses the attr name and optional type, e.g. "foo" or "bar:string"
func parseAttr(attrName string) (attr string, attrType AttrType, err error) {
chunks := strings.SplitN(attrName, ":", 2)
if len(chunks) == 1 {
return attrName, notProvidedTypeAttr, nil
}
attr, typeName := chunks[0], chunks[1]
switch typeName {
case "int", "raw", "ident":
return attr, rawAttr, nil
case "string", "label":
return attr, stringAttr, nil
case "list", "raw_list":
return attr, rawListAttr, nil
case "list_of_strings", "list_of_labels":
return attr, stringListAttr, nil
default:
return attr, notProvidedTypeAttr, fmt.Errorf("unknown attr type: %q", typeName)
}
}

// The cmdXXX functions implement the various commands.

func cmdAdd(opts *Options, env CmdEnvironment) (*build.File, error) {
attr := env.Args[0]
attr, attrType, err := parseAttr(env.Args[0])
if err != nil {
return nil, err
}
for _, val := range env.Args[1:] {
if IsIntList(attr) {
if attrType == rawAttr || attrType == rawListAttr || (attrType == notProvidedTypeAttr && IsIntList(attr)) {
AddValueToListAttribute(env.Rule, attr, env.Pkg, &build.LiteralExpr{Token: val}, &env.Vars)
continue
}
Expand Down Expand Up @@ -528,7 +570,7 @@ func cmdReplace(opts *Options, env CmdEnvironment) (*build.File, error) {
attr := env.Rule.Attr(key)
if e, ok := attr.(*build.StringExpr); ok {
if labels.Equal(e.Value, oldV, env.Pkg) {
env.Rule.SetAttr(key, getAttrValueExpr(key, []string{newV}, env))
env.Rule.SetAttr(key, getAttrValueExpr(key, notProvidedTypeAttr, []string{newV}, env))
}
} else {
ListReplace(attr, oldV, newV, env.Pkg)
Expand Down Expand Up @@ -558,48 +600,70 @@ func cmdSubstitute(opts *Options, env CmdEnvironment) (*build.File, error) {
}

func cmdSet(opts *Options, env CmdEnvironment) (*build.File, error) {
attr := env.Args[0]
attr, attrType, err := parseAttr(env.Args[0])
if err != nil {
return nil, err
}
args := env.Args[1:]
if attr == "kind" {
env.Rule.SetKind(args[0])
} else {
env.Rule.SetAttr(attr, getAttrValueExpr(attr, args, env))
env.Rule.SetAttr(attr, getAttrValueExpr(attr, attrType, args, env))
}
return env.File, nil
}

func cmdSetIfAbsent(opts *Options, env CmdEnvironment) (*build.File, error) {
attr := env.Args[0]
attr, attrType, err := parseAttr(env.Args[0])
if err != nil {
return nil, err
}
args := env.Args[1:]
if attr == "kind" {
return nil, fmt.Errorf("setting 'kind' is not allowed for set_if_absent. Got %s", env.Args)
}
if env.Rule.Attr(attr) == nil {
env.Rule.SetAttr(attr, getAttrValueExpr(attr, args, env))
env.Rule.SetAttr(attr, getAttrValueExpr(attr, attrType, args, env))
}
return env.File, nil
}

func getAttrValueExpr(attr string, args []string, env CmdEnvironment) build.Expr {
func getAttrValueExpr(attr string, attrType AttrType, args []string, env CmdEnvironment) build.Expr {
switch {
case attr == "kind":
return nil
case IsIntList(attr):
case attrType == rawAttr:
// treat as an ident (it could be an arbitrary expression though, e.g. an
// already quoted string or a function call, will be re-parsed and
// formatted properly by the subsequent call of buildifier on the
//resulting file)
return &build.Ident{Name: args[0]}
case attrType == rawListAttr || (attrType == notProvidedTypeAttr && IsIntList(attr)):
// list of raw objects (e.g. ints or idents)
var list []build.Expr
for _, i := range args {
list = append(list, &build.LiteralExpr{Token: i})
}
return &build.ListExpr{List: list}
case IsList(attr) && !(len(args) == 1 && strings.HasPrefix(args[0], "glob(")):
case attrType == stringListAttr || (attrType == notProvidedTypeAttr && IsList(attr) && !(len(args) == 1 && strings.HasPrefix(args[0], "glob("))):
// list of strings/labels
var list []build.Expr
for _, arg := range args {
list = append(list, getStringExpr(arg, env.Pkg))
}
return &build.ListExpr{List: list}
case attrType == rawListAttr:
// list of arbitrary objects (idents, ints, already quoted strings, etc.)
var list []build.Expr
for _, arg := range args {
list = append(list, &build.Ident{Name: arg})
}
return &build.ListExpr{List: list}
Comment on lines +655 to +661
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

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

medium

The case attrType == rawListAttr: at line 655 is unreachable. The previous case statement at line 641 already handles attrType == rawListAttr || (attrType == notProvidedTypeAttr && IsIntList(attr)). Since the switch statement executes the first matching case, the block at lines 655-661 will never be reached. This block should be removed to avoid dead code and potential confusion, or the logic should be refactored to ensure correct and distinct handling if different behaviors are intended for rawListAttr under different conditions.

case len(args) == 0:
// Expected a non-list argument, nothing provided
return &build.Ident{Name: "None"}
case IsString(attr):
case attrType == stringAttr || (attrType == notProvidedTypeAttr && IsString(attr)):
// single string
return getStringExpr(args[0], env.Pkg)
default:
return &build.Ident{Name: args[0]}
Expand Down