diff --git a/WARNINGS.md b/WARNINGS.md index 2514b8813..f3297692d 100644 --- a/WARNINGS.md +++ b/WARNINGS.md @@ -10,6 +10,7 @@ Warning categories supported by buildifier's linter: * [`attr-non-empty`](#attr-non-empty) * [`attr-output-default`](#attr-output-default) * [`attr-package-metadata`](#attr-package-metadata) + * [`attr-policy`](#attr-policy) * [`attr-single-file`](#attr-single-file) * [`build-args-kwargs`](#build-args-kwargs) * [`bzl-visibility`](#bzl-visibility) @@ -231,6 +232,58 @@ Using `package_metadata` as an attribute name may cause unexpected behavior. Its -------------------------------------------------------------------------------- +## Attribute value violates a configured policy rule + + * Category name: `attr-policy` + * Automatic fix: no + * [Suppress the warning](#suppress): `# buildifier: disable=attr-policy` + +Enforces declarative attribute constraints configured in `.buildifier.json` under +`attrPolicy`. Each rule names an attribute and a constraint family (scalar, +list, dict, numeric bounds, or presence). List rules support `allowListItems` for +closed-world lexicons; entries ending in `*` match by prefix. Targets matching an +`allowlist` pattern are exempt. + +When no `attrPolicy` block is configured, the warning applies these built-in rules: + +* `shard_count` must be ≤ 50 on `*_test` rules (matching Bazel's default constraint) +* `licenses` must not be set on any rule (deprecated; see [bazel#188](https://github.com/bazelbuild/bazel/issues/188)) +* `output_licenses` must not be set on `genrule`, `cc_binary`, `cc_toolchain`, `java_binary`, or `java_plugin` (deprecated; see [bazel#7444](https://github.com/bazelbuild/bazel/issues/7444)) + +Example: + +```json +{ + "attrPolicy": { + "rules": [ + { + "name": "no-eternal-timeout", + "ruleKinds": ["*_test"], + "attr": "timeout", + "forbidValues": ["eternal"], + "allowlist": ["//slow/..."] + }, + { + "name": "max-shard-count", + "ruleKinds": ["*_test"], + "attr": "shard_count", + "maxValue": 50 + }, + { + "name": "allowed-tags", + "attr": "tags", + "allowListItems": ["manual", "assistant-ds*", "exclusive"] + } + ] + } +} +``` + +A JSON Schema for `.buildifier.json` (including `attrPolicy`) lives at +`buildifier/config/buildifier.schema.json`. + +-------------------------------------------------------------------------------- + ## `single_file` is deprecated * Category name: `attr-single-file` diff --git a/buildifier/README.md b/buildifier/README.md index f183c334b..a5aa14193 100644 --- a/buildifier/README.md +++ b/buildifier/README.md @@ -93,6 +93,22 @@ type). See also the [full list](../WARNINGS.md) or the supported warnings. +## Configuration (`.buildifier.json`) + +Buildifier reads optional settings from `.buildifier.json` in the workspace (or +from the path in `BUILDIFIER_CONFIG` / `--config`). The file supports lint +options, custom tables, and config-driven policy rules under `attrPolicy` (see +the [`attr-policy`](../WARNINGS.md#attr-policy) warning). + +A [JSON Schema](config/buildifier.schema.json) is available for editor +validation and agent tooling. Reference it from your config file: + +```json +{ + "$schema": "https://raw.githubusercontent.com/bazelbuild/buildtools/main/buildifier/config/buildifier.schema.json" +} +``` + ## Setup and usage via Bazel You can also invoke buildifier via the Bazel rule. diff --git a/buildifier/config/BUILD.bazel b/buildifier/config/BUILD.bazel index 070d75605..4cffd85d1 100644 --- a/buildifier/config/BUILD.bazel +++ b/buildifier/config/BUILD.bazel @@ -1,14 +1,18 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") +exports_files(["buildifier.schema.json"]) + go_library( name = "config", srcs = [ + "attrpolicy.go", "config.go", "validation.go", ], importpath = "github.com/bazelbuild/buildtools/buildifier/config", visibility = ["//buildifier:__pkg__"], deps = [ + "//labels", "//tables", "//warn", "//wspace", @@ -17,7 +21,10 @@ go_library( go_test( name = "config_test", - srcs = ["config_test.go"], + srcs = [ + "attrpolicy_test.go", + "config_test.go", + ], embed = [":config"], ) diff --git a/buildifier/config/attrpolicy.go b/buildifier/config/attrpolicy.go new file mode 100644 index 000000000..01663cc7e --- /dev/null +++ b/buildifier/config/attrpolicy.go @@ -0,0 +1,229 @@ +package config + +import ( + "fmt" + "path" + "strings" + + "github.com/bazelbuild/buildtools/labels" + "github.com/bazelbuild/buildtools/warn" +) + +// AttrPolicy is the attrPolicy block in .buildifier.json. +type AttrPolicy struct { + Rules []AttrPolicyRule `json:"rules,omitempty"` +} + +// AttrPolicyRule is a single attribute policy rule. +type AttrPolicyRule struct { + Name string `json:"name"` + RuleKinds []string `json:"ruleKinds,omitempty"` + Attr string `json:"attr"` + ForbidValues []string `json:"forbidValues,omitempty"` + RequireValues []string `json:"requireValues,omitempty"` + ForbidListItems []string `json:"forbidListItems,omitempty"` + RequireListItems []string `json:"requireListItems,omitempty"` + AllowListItems []string `json:"allowListItems,omitempty"` + ForbidDictEntries map[string]string `json:"forbidDictEntries,omitempty"` + RequireDictEntries map[string]string `json:"requireDictEntries,omitempty"` + ForbidDictKeys []string `json:"forbidDictKeys,omitempty"` + MinValue *int `json:"minValue,omitempty"` + MaxValue *int `json:"maxValue,omitempty"` + Required bool `json:"required,omitempty"` + ForbidPresence bool `json:"forbidPresence,omitempty"` + Allowlist []string `json:"allowlist,omitempty"` + Suppressible *bool `json:"suppressible,omitempty"` + Message string `json:"message,omitempty"` +} + +func compileAttrPolicy(policy *AttrPolicy) ([]warn.AttrPolicyRuleCompiled, error) { + if policy == nil || len(policy.Rules) == 0 { + return warn.DefaultAttrPolicyRules(), nil + } + seen := make(map[string]bool) + var compiled []warn.AttrPolicyRuleCompiled + for i := range policy.Rules { + rule := &policy.Rules[i] + c, err := compileAttrPolicyRule(rule, seen) + if err != nil { + return nil, fmt.Errorf("attrPolicy.rules[%d]: %w", i, err) + } + compiled = append(compiled, c) + } + return compiled, nil +} + +func compileAttrPolicyRule(rule *AttrPolicyRule, seen map[string]bool) (warn.AttrPolicyRuleCompiled, error) { + name := strings.TrimSpace(rule.Name) + if name == "" { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: name is required", rule.Name) + } + if seen[name] { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: duplicate name", name) + } + seen[name] = true + + attr := strings.TrimSpace(rule.Attr) + if attr == "" { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: attr is required", name) + } + + family, families, err := attrPolicyConstraintFamily(rule) + if err != nil { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: %w", name, err) + } + if rule.ForbidPresence && rule.Required { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: forbidPresence and required cannot both be true", name) + } + if families == 0 && !rule.Required { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: at least one constraint, required=true, or forbidPresence=true is required", name) + } + + for _, kindGlob := range rule.RuleKinds { + if _, err := path.Match(kindGlob, "x"); err != nil { + return warn.AttrPolicyRuleCompiled{}, fmt.Errorf("attrPolicy rule %q: malformed ruleKinds glob %q: %w", name, kindGlob, err) + } + } + + allowlist, err := compileAllowlistPatterns(name, rule.Allowlist) + if err != nil { + return warn.AttrPolicyRuleCompiled{}, err + } + + suppressible := true + if rule.Suppressible != nil { + suppressible = *rule.Suppressible + } + + return warn.AttrPolicyRuleCompiled{ + Name: name, + RuleKinds: append([]string(nil), rule.RuleKinds...), + Attr: attr, + Family: family, + ForbidValues: append([]string(nil), rule.ForbidValues...), + RequireValues: append([]string(nil), rule.RequireValues...), + ForbidListItems: append([]string(nil), rule.ForbidListItems...), + RequireListItems: append([]string(nil), rule.RequireListItems...), + AllowListItems: append([]string(nil), rule.AllowListItems...), + ForbidDictEntries: copyStringMap(rule.ForbidDictEntries), + RequireDictEntries: copyStringMap(rule.RequireDictEntries), + ForbidDictKeys: append([]string(nil), rule.ForbidDictKeys...), + MinValue: cloneIntPtr(rule.MinValue), + MaxValue: cloneIntPtr(rule.MaxValue), + Required: rule.Required, + Allowlist: allowlist, + Suppressible: suppressible, + Message: rule.Message, + }, nil +} + +func attrPolicyConstraintFamily(rule *AttrPolicyRule) (warn.AttrPolicyConstraintFamily, int, error) { + scalar := len(rule.ForbidValues) > 0 || len(rule.RequireValues) > 0 + list := len(rule.ForbidListItems) > 0 || len(rule.RequireListItems) > 0 || len(rule.AllowListItems) > 0 + dict := len(rule.ForbidDictEntries) > 0 || len(rule.RequireDictEntries) > 0 || len(rule.ForbidDictKeys) > 0 + numeric := rule.MinValue != nil || rule.MaxValue != nil + presence := rule.ForbidPresence + + families := 0 + var family warn.AttrPolicyConstraintFamily + if scalar { + families++ + family = warn.AttrPolicyScalarFamily + } + if list { + families++ + family = warn.AttrPolicyListFamily + } + if dict { + families++ + family = warn.AttrPolicyDictFamily + } + if numeric { + families++ + family = warn.AttrPolicyNumericFamily + } + if presence { + families++ + family = warn.AttrPolicyForbidPresenceFamily + } + if families > 1 { + return 0, families, fmt.Errorf("cannot mix scalar, list, dict, numeric, and presence constraint families") + } + if numeric && rule.MinValue != nil && rule.MaxValue != nil && *rule.MinValue > *rule.MaxValue { + return 0, families, fmt.Errorf("minValue must be <= maxValue") + } + return family, families, nil +} + +func compileAllowlistPatterns(ruleName string, entries []string) ([]warn.AttrPolicyAllowlistPattern, error) { + var patterns []warn.AttrPolicyAllowlistPattern + for _, entry := range entries { + p, err := parseAllowlistPattern(entry) + if err != nil { + return nil, fmt.Errorf("attrPolicy rule %q: allowlist entry %q: %w", ruleName, entry, err) + } + patterns = append(patterns, p) + } + return patterns, nil +} + +func parseAllowlistPattern(entry string) (warn.AttrPolicyAllowlistPattern, error) { + if entry == "" { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("empty pattern") + } + if strings.HasPrefix(entry, "@") { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("repository-qualified entries are not supported") + } + if entry == "..." { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("bare ... is not supported") + } + if entry == "//..." { + return warn.AttrPolicyAllowlistPattern{Kind: warn.AttrPolicyAllowAll}, nil + } + if !strings.HasPrefix(entry, "//") { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("pattern must start with //") + } + if strings.HasSuffix(entry, "/...") { + pkg := strings.TrimPrefix(entry, "//") + pkg = strings.TrimSuffix(pkg, "/...") + if pkg == "" { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("invalid recursive pattern") + } + if strings.Contains(pkg, ":") { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("invalid recursive pattern") + } + return warn.AttrPolicyAllowlistPattern{Kind: warn.AttrPolicyAllowRecursive, Pkg: pkg}, nil + } + + label := labels.Parse(entry) + if label.Target == "all" || label.Target == "*" { + if label.Package == "" { + return warn.AttrPolicyAllowlistPattern{}, fmt.Errorf("invalid package pattern") + } + return warn.AttrPolicyAllowlistPattern{Kind: warn.AttrPolicyAllowPackageAll, Pkg: label.Package}, nil + } + return warn.AttrPolicyAllowlistPattern{ + Kind: warn.AttrPolicyAllowExact, + Pkg: label.Package, + Target: label.Target, + }, nil +} + +func copyStringMap(in map[string]string) map[string]string { + if len(in) == 0 { + return nil + } + out := make(map[string]string, len(in)) + for k, v := range in { + out[k] = v + } + return out +} + +func cloneIntPtr(v *int) *int { + if v == nil { + return nil + } + c := *v + return &c +} diff --git a/buildifier/config/attrpolicy_test.go b/buildifier/config/attrpolicy_test.go new file mode 100644 index 000000000..70657fdd4 --- /dev/null +++ b/buildifier/config/attrpolicy_test.go @@ -0,0 +1,225 @@ +/* +Copyright 2026 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "strings" + "testing" + + "github.com/bazelbuild/buildtools/warn" +) + +func TestCompileAttrPolicy(t *testing.T) { + for name, tc := range map[string]struct { + policy *AttrPolicy + wantLen int + wantName string + }{ + "nil uses default shard_count rule": { + policy: nil, + wantLen: 3, + wantName: "max-shard-count", + }, + "empty rules uses default shard_count rule": { + policy: &AttrPolicy{}, + wantLen: 3, + wantName: "max-shard-count", + }, + } { + t.Run(name, func(t *testing.T) { + compiled, err := compileAttrPolicy(tc.policy) + if err != nil { + t.Fatalf("compileAttrPolicy() error = %v", err) + } + if len(compiled) != tc.wantLen { + t.Fatalf("len(compiled) = %d, want %d", len(compiled), tc.wantLen) + } + if compiled[0].Name != tc.wantName { + t.Fatalf("compiled[0].Name = %q, want %q", compiled[0].Name, tc.wantName) + } + if compiled[0].MaxValue == nil || *compiled[0].MaxValue != 50 { + t.Fatalf("compiled[0].MaxValue = %+v", compiled[0].MaxValue) + } + }) + } + + policy := &AttrPolicy{ + Rules: []AttrPolicyRule{ + { + Name: "no-eternal-timeout", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + ForbidValues: []string{"eternal"}, + Allowlist: []string{"//slow/...", "//foo:big_test"}, + }, + { + Name: "max-shard-count", + RuleKinds: []string{"*_test"}, + Attr: "shard_count", + MaxValue: intPtr(50), + }, + }, + } + compiled, err := compileAttrPolicy(policy) + if err != nil { + t.Fatalf("compileAttrPolicy() error = %v", err) + } + if len(compiled) != 2 { + t.Fatalf("len(compiled) = %d, want 2", len(compiled)) + } + if compiled[0].Name != "no-eternal-timeout" || compiled[0].Family != warn.AttrPolicyScalarFamily { + t.Fatalf("compiled[0] = %+v", compiled[0]) + } + if compiled[1].MaxValue == nil || *compiled[1].MaxValue != 50 { + t.Fatalf("compiled[1].MaxValue = %+v", compiled[1].MaxValue) + } +} + +func TestCompileAttrPolicyValidation(t *testing.T) { + for name, tc := range map[string]struct { + policy *AttrPolicy + wantErr string + }{ + "duplicate name": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}}, + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}}, + }}, + wantErr: `duplicate name`, + }, + "missing name": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Attr: "timeout", ForbidValues: []string{"eternal"}}, + }}, + wantErr: `name is required`, + }, + "missing attr": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", ForbidValues: []string{"eternal"}}, + }}, + wantErr: `attr is required`, + }, + "no constraints": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout"}, + }}, + wantErr: `at least one constraint`, + }, + "mixed families": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}, ForbidListItems: []string{"exclusive"}}, + }}, + wantErr: `cannot mix scalar, list, dict, numeric, and presence`, + }, + "numeric min greater than max": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "shard_count", MinValue: intPtr(10), MaxValue: intPtr(5)}, + }}, + wantErr: `minValue must be <= maxValue`, + }, + "bad ruleKinds glob": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}, RuleKinds: []string{"["}}, + }}, + wantErr: `malformed ruleKinds`, + }, + "bad allowlist": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidValues: []string{"eternal"}, Allowlist: []string{"@repo//foo:bar"}}, + }}, + wantErr: `repository-qualified`, + }, + "required only": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", Required: true}, + }}, + }, + "forbid presence only": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "licenses", ForbidPresence: true}, + }}, + }, + "allow list items only": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "tags", AllowListItems: []string{"manual", "assistant-ds*"}}, + }}, + }, + "forbid presence and required": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "licenses", ForbidPresence: true, Required: true}, + }}, + wantErr: `forbidPresence and required cannot both be true`, + }, + "forbid presence mixed with scalar": { + policy: &AttrPolicy{Rules: []AttrPolicyRule{ + {Name: "x", Attr: "timeout", ForbidPresence: true, ForbidValues: []string{"eternal"}}, + }}, + wantErr: `cannot mix scalar, list, dict, numeric, and presence`, + }, + } { + t.Run(name, func(t *testing.T) { + _, err := compileAttrPolicy(tc.policy) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %v, want substring %q", err, tc.wantErr) + } + }) + } +} + +func TestParseAllowlistPattern(t *testing.T) { + for name, tc := range map[string]struct { + entry string + wantErr string + kind warn.AttrPolicyAllowlistKind + pkg string + target string + }{ + "all": {entry: "//...", kind: warn.AttrPolicyAllowAll}, + "exact": {entry: "//foo:bar", kind: warn.AttrPolicyAllowExact, pkg: "foo", target: "bar"}, + "package": {entry: "//pkg", kind: warn.AttrPolicyAllowExact, pkg: "pkg", target: "pkg"}, + "package all": {entry: "//pkg:all", kind: warn.AttrPolicyAllowPackageAll, pkg: "pkg"}, + "recursive": {entry: "//slow/...", kind: warn.AttrPolicyAllowRecursive, pkg: "slow"}, + "repo qualified": {entry: "@repo//foo:bar", wantErr: "repository-qualified"}, + "bare ellipsis": {entry: "...", wantErr: "bare ..."}, + } { + t.Run(name, func(t *testing.T) { + got, err := parseAllowlistPattern(tc.entry) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %v, want substring %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.Kind != tc.kind || got.Pkg != tc.pkg || got.Target != tc.target { + t.Fatalf("parseAllowlistPattern(%q) = %+v", tc.entry, got) + } + }) + } +} + +func intPtr(v int) *int { + return &v +} diff --git a/buildifier/config/buildifier.schema.json b/buildifier/config/buildifier.schema.json new file mode 100644 index 000000000..0acbdf2bb --- /dev/null +++ b/buildifier/config/buildifier.schema.json @@ -0,0 +1,313 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/bazelbuild/buildtools/blob/main/buildifier/config/buildifier.schema.json", + "title": "Buildifier configuration", + "description": "Schema for .buildifier.json. Use with editor JSON Schema support or agent tooling for safer config edits.", + "type": "object", + "additionalProperties": false, + "properties": { + "type": { + "type": "string", + "enum": ["auto", "build", "bzl", "workspace", "default", "module"], + "description": "Input file type. auto selects based on filename." + }, + "format": { + "type": "string", + "enum": ["text", "json"], + "description": "Diagnostics format when mode is check." + }, + "mode": { + "type": "string", + "enum": ["check", "diff", "fix", "print_if_changed"], + "description": "Formatting mode." + }, + "diffMode": { + "type": "boolean", + "description": "Alias for mode=diff." + }, + "lint": { + "type": "string", + "enum": ["off", "warn", "fix"], + "description": "Lint mode." + }, + "warnings": { + "type": "string", + "description": "Comma-separated warning identifiers, or all/default, optionally with +/- modifiers." + }, + "warningsList": { + "type": "array", + "items": { "type": "string" }, + "description": "List form of warnings." + }, + "recursive": { + "type": "boolean", + "description": "Find Starlark files recursively." + }, + "verbose": { + "type": "boolean" + }, + "diffCommand": { + "type": "string" + }, + "multiDiff": { + "type": "boolean" + }, + "tables": { + "type": "string", + "description": "Path to JSON tables file replacing built-in tables." + }, + "addTables": { + "type": "string", + "description": "Path to JSON tables file merged with built-in tables." + }, + "path": { + "type": "string", + "description": "Workspace-relative path assumed for a single formatted BUILD file." + }, + "buildifier_disable": { + "type": "array", + "items": { "type": "string" } + }, + "allowsort": { + "type": "array", + "items": { "type": "string" } + }, + "attrPolicy": { + "type": "object", + "description": "Config-driven attribute policy rules enforced by the attr-policy lint warning.", + "additionalProperties": false, + "properties": { + "rules": { + "type": "array", + "items": { "$ref": "#/$defs/attrPolicyRule" } + } + }, + "required": ["rules"] + } + }, + "$defs": { + "attrPolicyRule": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { + "type": "string", + "minLength": 1, + "description": "Stable rule identifier shown in findings. Must be unique." + }, + "ruleKinds": { + "type": "array", + "items": { "type": "string", "minLength": 1 }, + "description": "Globs matched against rule kind, e.g. *_test. Empty/absent matches all kinds." + }, + "attr": { + "type": "string", + "minLength": 1, + "description": "Attribute name to inspect." + }, + "forbidValues": { + "type": "array", + "items": { "type": "string" }, + "description": "Scalar attribute must not equal any listed value (string or boolean literal)." + }, + "requireValues": { + "type": "array", + "items": { "type": "string" }, + "description": "If the attribute is present, its value must be one of these." + }, + "forbidListItems": { + "type": "array", + "items": { "type": "string" }, + "description": "List attribute must not contain any listed item." + }, + "requireListItems": { + "type": "array", + "items": { "type": "string" }, + "description": "List attribute must contain all listed items." + }, + "allowListItems": { + "type": "array", + "items": { "type": "string", "minLength": 1 }, + "description": "Every list item must match an allowed value. Entries ending in * are prefix patterns; all others are exact matches." + }, + "forbidDictEntries": { + "type": "object", + "additionalProperties": { "type": "string" }, + "description": "Dict attribute must not contain these key/value pairs." + }, + "requireDictEntries": { + "type": "object", + "additionalProperties": { "type": "string" }, + "description": "Dict attribute must contain all listed key/value pairs." + }, + "forbidDictKeys": { + "type": "array", + "items": { "type": "string" }, + "description": "Dict attribute must not contain any listed key." + }, + "minValue": { + "type": "integer", + "description": "Numeric attribute must be greater than or equal to this value." + }, + "maxValue": { + "type": "integer", + "description": "Numeric attribute must be less than or equal to this value." + }, + "required": { + "type": "boolean", + "default": false, + "description": "Attribute must be present." + }, + "forbidPresence": { + "type": "boolean", + "default": false, + "description": "Attribute must not be present." + }, + "allowlist": { + "type": "array", + "items": { + "type": "string", + "pattern": "^//" + }, + "description": "Target patterns exempt from this rule, e.g. //pkg:name, //pkg:all, //pkg/..., //..." + }, + "suppressible": { + "type": "boolean", + "default": true, + "description": "Whether # buildifier: disable=attr-policy silences this rule." + }, + "message": { + "type": "string", + "description": "Custom finding message. Rule name is prefixed automatically." + } + }, + "required": ["name", "attr"], + "oneOf": [ + { + "description": "Scalar constraint family", + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } + ] + } + }, + { + "description": "List constraint family", + "anyOf": [ + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["allowListItems"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } + ] + } + }, + { + "description": "Dict constraint family", + "anyOf": [ + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } + ] + } + }, + { + "description": "Numeric constraint family", + "anyOf": [ + { "required": ["minValue"] }, + { "required": ["maxValue"] } + ], + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["forbidPresence"] } + ] + } + }, + { + "description": "Presence-only rule", + "required": ["required"], + "properties": { + "required": { "const": true } + }, + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] }, + { "required": ["forbidPresence"] } + ] + } + }, + { + "description": "Forbid-presence rule", + "required": ["forbidPresence"], + "properties": { + "forbidPresence": { "const": true } + }, + "not": { + "anyOf": [ + { "required": ["forbidValues"] }, + { "required": ["requireValues"] }, + { "required": ["forbidListItems"] }, + { "required": ["requireListItems"] }, + { "required": ["allowListItems"] }, + { "required": ["forbidDictEntries"] }, + { "required": ["requireDictEntries"] }, + { "required": ["forbidDictKeys"] }, + { "required": ["minValue"] }, + { "required": ["maxValue"] }, + { "required": ["required"] } + ] + } + } + ] + } + } +} diff --git a/buildifier/config/config.go b/buildifier/config/config.go index e9783b949..57f6705da 100644 --- a/buildifier/config/config.go +++ b/buildifier/config/config.go @@ -130,6 +130,8 @@ type Config struct { DisableRewrites ArrayFlags `json:"buildifier_disable,omitempty"` // AllowSort specifies additional sort contexts to treat as safe AllowSort ArrayFlags `json:"allowsort,omitempty"` + // AttrPolicy configures config-driven attribute policy lint rules. + AttrPolicy *AttrPolicy `json:"attrPolicy,omitempty"` // Help is true if the -h flag is set Help bool `json:"-"` @@ -231,6 +233,12 @@ func (c *Config) Validate(args []string) error { } } + compiledAttrPolicy, err := compileAttrPolicy(c.AttrPolicy) + if err != nil { + return err + } + warn.SetAttrPolicy(compiledAttrPolicy) + warningsList := c.WarningsList if c.Warnings != "" { warningsList = append(warningsList, c.Warnings) @@ -276,5 +284,17 @@ func Example() *Config { c.Mode = "fix" c.Lint = "fix" c.WarningsList = warn.AllWarnings + c.AttrPolicy = &AttrPolicy{ + Rules: []AttrPolicyRule{ + { + Name: "no-eternal-timeout", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + ForbidValues: []string{"eternal"}, + Allowlist: []string{"//slow/..."}, + Message: "'eternal' timeout requires approval; add the target to the attrPolicy allowlist.", + }, + }, + } return c } diff --git a/buildifier/config/config_test.go b/buildifier/config/config_test.go index c9291769e..57c6fe066 100644 --- a/buildifier/config/config_test.go +++ b/buildifier/config/config_test.go @@ -50,6 +50,7 @@ func ExampleExample() { // "attr-licenses", // "attr-non-empty", // "attr-output-default", + // "attr-policy", // "attr-single-file", // "build-args-kwargs", // "bzl-visibility", @@ -143,7 +144,25 @@ func ExampleExample() { // "unreachable", // "unsorted-dict-items", // "unused-variable" - // ] + // ], + // "attrPolicy": { + // "rules": [ + // { + // "name": "no-eternal-timeout", + // "ruleKinds": [ + // "*_test" + // ], + // "attr": "timeout", + // "forbidValues": [ + // "eternal" + // ], + // "allowlist": [ + // "//slow/..." + // ], + // "message": "'eternal' timeout requires approval; add the target to the attrPolicy allowlist." + // } + // ] + // } // } } @@ -271,6 +290,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", @@ -373,6 +393,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", @@ -475,6 +496,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", @@ -577,6 +599,7 @@ func TestValidate(t *testing.T) { "attr-licenses", "attr-non-empty", "attr-output-default", + "attr-policy", "attr-single-file", "build-args-kwargs", "bzl-visibility", diff --git a/buildifier/integration_test.sh b/buildifier/integration_test.sh index b9a6389c5..3df3633fa 100755 --- a/buildifier/integration_test.sh +++ b/buildifier/integration_test.sh @@ -265,6 +265,7 @@ cat > golden/.buildifier.example.json < golden/.buildifier.example.json <` comments already work for + every registered warning; the new warning gets this for free. +- **Label utilities** (`labels/labels.go`): `labels.Parse(target) Label`, + `labels.Equal(l1, l2, pkg) bool`; `Label{Repository, Package, Target}`. +- **Tests**: `warn/*_test.go` use `checkFindingsAndFix(t, categories, input, output, + expected, scope)` and scope constants (`scopeBuild`, `scopeBzl`, ...) from + `warn/warn_test.go`. + +--- + +## 3. Workstream A — `attr-policy` warning (buildifier) + +### 3.1 Goal + +A **single generalized** warning, `attr-policy`, driven entirely by config. It covers +the initial asks (eternal-timeout allow-list, no `exclusive` on tests) and any future +"attribute constrained on rule-kind unless allow-listed" rule **without new Go code** — +including boolean literals (`local = True`), dict attributes +(`execution_requirements = {"no-cache": "1"}`), and numeric bounds (`shard_count` ≤ 50). + +Rationale for one generalized warning vs. many hardcoded ones: buildifier warnings are +individually toggleable by name, but the *policy content* here is site-specific and +belongs in config, not in the binary. One warning + rich config keeps the binary +generic and lets each repo express its own policy. + +### 3.2 Config schema (extends `.buildifier.json`) + +`.buildifier.json` is parsed with standard `encoding/json` (no comments). Examples +below are valid JSON you can copy into config. + +```json +{ + "attrPolicy": { + "rules": [ + { + "name": "no-eternal-timeout", + "ruleKinds": ["*_test"], + "attr": "timeout", + "forbidValues": ["eternal"], + "allowlist": ["//slow/...", "//foo:big_test"], + "message": "'eternal' timeout requires approval; add the target to the attrPolicy allowlist." + }, + { + "name": "no-exclusive-tests", + "ruleKinds": ["*_test"], + "attr": "tags", + "forbidListItems": ["exclusive"], + "allowlist": [] + }, + { + "name": "no-local-tests", + "ruleKinds": ["*_test"], + "attr": "local", + "forbidValues": ["True"], + "message": "Tests must not set local = True; use a hermetic test instead." + }, + { + "name": "no-no-cache", + "attr": "execution_requirements", + "forbidDictEntries": { + "no-cache": "1" + }, + "message": "Do not set execution_requirements['no-cache'] = '1'." + }, + { + "name": "max-shard-count", + "ruleKinds": ["*_test"], + "attr": "shard_count", + "maxValue": 50, + "allowlist": ["//huge_suite:..."], + "message": "shard_count must not exceed 50; add the target to the allowlist for larger suites." + } + ] + } +} +``` + +**Per-rule fields:** + +| Field | Type | Meaning | +|---|---|---| +| `name` | string (required) | Stable identifier, included in the finding message. Must be unique. | +| `ruleKinds` | []string | Globs matched against `rule.Kind()`. Empty/absent ⇒ matches all kinds. | +| `attr` | string (required) | Attribute to inspect. | +| `forbidValues` | []string | Scalar attr must not equal any of these (see scalar matching below). | +| `requireValues` | []string | If attr present, must equal one of these. (If also want "must be present", see `required`.) | +| `forbidListItems` | []string | List attr must not contain any of these items. | +| `requireListItems` | []string | List attr must contain all of these items. | +| `allowListItems` | []string | Every list item must match an allowed value. Entries ending in `*` are prefix patterns; all others are exact matches. | +| `forbidDictEntries` | object (string→string) | Dict attr must not contain any of these key→value pairs. | +| `requireDictEntries` | object (string→string) | Dict attr must contain all of these key→value pairs. | +| `forbidDictKeys` | []string | Dict attr must not contain any of these keys (value ignored). | +| `minValue` | int | Numeric attr must be ≥ this (inclusive). At least one of `minValue` / `maxValue` required for the numeric family. | +| `maxValue` | int | Numeric attr must be ≤ this (inclusive). | +| `required` | bool | Attr must be present at all. | +| `allowlist` | []string | Target patterns exempt from this rule (see grammar below). | +| `suppressible` | bool (default `true`) | Whether `# buildifier: disable=attr-policy` silences this rule. `false` = a "hard" rule enforced authoritatively by CI regardless of in-file comments (see §6). | +| `message` | string | Custom message. If absent, a default is synthesized from the constraint. | + +**Scalar matching (`forbidValues` / `requireValues`).** Covers string attributes +(`timeout = "eternal"`) and boolean/identifier literals (`local = True`, +`flaky = False`). At check time, read the attribute with `rule.AttrString` first, +then fall back to `rule.AttrLiteral` (which returns `True`/`False` for boolean +literals and identifier names for unquoted tokens). Compare the normalized string +form. An absent attribute does not match `forbidValues`; it only fails +`requireValues` when the attribute is present but wrong. + +**Dict matching (`forbidDictEntries` / `requireDictEntries` / `forbidDictKeys`).** +Covers `string_dict` / `label_dict` attributes such as `execution_requirements`. +The attribute must be a `*build.DictExpr`; look up keys with `edit.DictionaryGet`. +Dict values are normalized the same way as scalars (string literal or +`AttrLiteral` on the value node). Example: `execution_requirements = {"no-cache": +"1"}` is flagged by the `no-no-cache` rule above because the dict contains key +`no-cache` with value `"1"`. `forbidDictKeys` is a weaker variant that flags the +presence of a key regardless of its value (useful when any non-default value is +undesirable but the exact value varies). + +**Numeric matching (`minValue` / `maxValue`).** Covers integer attributes such as +`shard_count`. Read the attribute with `rule.AttrLiteral` (a `*build.LiteralExpr` +whose `Token` is the decimal representation) and parse with `strconv.Atoi`. An absent +attribute does not violate `maxValue` / `minValue` — only an explicitly set value +outside the range is flagged. This is the mechanism repos use to re-impose limits +Bazel no longer enforces globally (e.g. the former 50-shard cap removed in +[figma/bazel#12](https://github.com/figma/bazel/pull/12)). + +Exactly one *constraint family* (`forbidValues`/`requireValues` **or** +`forbidListItems`/`requireListItems` **or** +`forbidDictEntries`/`requireDictEntries`/`forbidDictKeys` **or** +`minValue`/`maxValue`) should be set per rule; `required` is orthogonal. Validation +enforces this (see 3.5). + +**Allow-list pattern grammar.** Entries are Bazel-style target patterns (a subset of +what users already know from `bazel build`): + +| Pattern | Matches | +|---|---| +| `//pkg:name` or `//pkg` | exactly the target `//pkg:name` (`//pkg` ⇒ `//pkg:pkg`) | +| `//pkg:all` (also `//pkg:*`) | any target *directly* in package `pkg` | +| `//pkg/...` | any target in package `pkg` or any package beneath it (recursive) | +| `//...` | every target | + +There is **no off-the-shelf matcher** for this in the repo — `labels.Equal` is +exact-only, and buildozer's `/...` handling is for filesystem BUILD discovery, not +in-memory label matching. So the warning ships a small matcher (`allowlistMatch`) that +parses each entry once at config-compile time and, for a given target +`//{f.Pkg}:{rule.Name()}`, tests: exact via `labels.Equal`; `:all`/`:*` via package +equality; `/...` via `pkg == P || strings.HasPrefix(pkg, P+"/")`. + +**Semantics:** +- Empty/absent `attrPolicy` ⇒ warning is a no-op. Safe to enable in `--warnings=all`. +- A target matches a policy rule if its `Kind()` matches any `ruleKinds` glob **and** + its label matches **no** entry in `allowlist`. +- Target label computed as `//{f.Pkg}:{rule.Name()}` and tested with `allowlistMatch` + per the grammar above (not a bare `labels.Equal`, which can't express patterns). +- Finding is anchored on the offending node. Default: the attribute expression + (`rule.Attr(attr)`). Exceptions for precise IDE highlighting: + - **List items:** anchor on the matching string literal inside the list (e.g. the + `"exclusive"` entry in `tags`), not the whole list. + - **Dict entries:** anchor on the offending value node when possible. + - **Missing required attr:** anchor on `rule.Call`. + +### 3.3 Go types + +New file `buildifier/config/attrpolicy.go` (or inline in `config.go`) — keep JSON tags: + +```go +type AttrPolicy struct { + Rules []AttrPolicyRule `json:"rules,omitempty"` +} + +type AttrPolicyRule struct { + Name string `json:"name"` + RuleKinds []string `json:"ruleKinds,omitempty"` + Attr string `json:"attr"` + ForbidValues []string `json:"forbidValues,omitempty"` + RequireValues []string `json:"requireValues,omitempty"` + ForbidListItems []string `json:"forbidListItems,omitempty"` + RequireListItems []string `json:"requireListItems,omitempty"` + ForbidDictEntries map[string]string `json:"forbidDictEntries,omitempty"` + RequireDictEntries map[string]string `json:"requireDictEntries,omitempty"` + ForbidDictKeys []string `json:"forbidDictKeys,omitempty"` + MinValue *int `json:"minValue,omitempty"` // pointer: absent vs set-to-zero + MaxValue *int `json:"maxValue,omitempty"` + Required bool `json:"required,omitempty"` + Allowlist []string `json:"allowlist,omitempty"` + Suppressible *bool `json:"suppressible,omitempty"` // pointer so absent ⇒ default true + Message string `json:"message,omitempty"` +} +``` + +Add to `Config` struct (`buildifier/config/config.go:93`): + +```go +AttrPolicy *AttrPolicy `json:"attrPolicy,omitempty"` +``` + +### 3.4 Wiring config → warning (mirror the `tables` pattern) + +The warning function has signature `func(f *build.File) []*LinterFinding` and cannot +take config directly (would churn 100+ warning signatures). Instead use a package-level +global in `warn`, set once during config application — exactly how `tables` works. + +- In `warn/warn_attr_policy.go`, define: + ```go + // AttrPolicyConfig is process-global policy, set from buildifier config before linting. + var AttrPolicyConfig []AttrPolicyRuleCompiled // compiled form: kind globs + allow-list patterns precompiled + + func SetAttrPolicy(rules []AttrPolicyRuleCompiled) { AttrPolicyConfig = rules } + ``` + To avoid an import cycle (`warn` must not import `buildifier/config`), define the + *compiled* policy type in the `warn` package and have the config layer translate + `config.AttrPolicy` → `[]warn.AttrPolicyRuleCompiled` and call `warn.SetAttrPolicy`. +- Apply in `buildifier/config/config.go` `Validate()` (next to the tables block, + ~line 231): if `c.AttrPolicy != nil`, compile and call `warn.SetAttrPolicy(...)`. + Return a validation error on malformed rules. + +> **Import-cycle note for agents:** confirm direction. `buildifier/config` may import +> `warn`; `warn` must not import `buildifier/config`. If `warn` already imports config +> anywhere, keep the compiled type in `warn` regardless. This is the one wiring risk — +> verify before writing code. + +### 3.5 Validation (`buildifier/config/validation.go`) + +On load, reject: +- duplicate `name`s, +- empty `name` or empty `attr`, +- a rule with no constraint set, +- a rule mixing scalar, list, dict, and numeric constraint families, +- a numeric rule with neither `minValue` nor `maxValue` set, or with `minValue` > + `maxValue` when both are set, +- malformed globs in `ruleKinds` / malformed target patterns in `allowlist` (must + match the §3.2 grammar; reject a bare `...`, repository-qualified entries, etc.). + +Emit clear errors (`fmt.Errorf("attrPolicy rule %q: ...", name)`). + +### 3.6 Warning implementation sketch (`warn/warn_attr_policy.go`) + +```go +func attrPolicyWarning(f *build.File) []*LinterFinding { + if f.Type != build.TypeBuild || len(AttrPolicyConfig) == 0 { + return nil + } + var findings []*LinterFinding + for _, rule := range f.Rules("") { + kind := rule.Kind() + label := labels.Label{Package: f.Pkg, Target: rule.Name()}.Format() + for _, p := range AttrPolicyConfig { + if !p.matchesKind(kind) || p.allowed(label, f.Pkg) { + continue + } + if fnd := p.check(rule, f); fnd != nil { + findings = append(findings, fnd) + } + } + } + return findings +} +``` + +- `p.allowed` runs the precompiled `allowlistMatch` (§3.2 grammar) over the target + label — exact / `:all` / `/...` — not a bare `labels.Equal`. +- `p.check` handles the constraint families: + - **Scalars:** `attrScalarString(rule, attr)` → `rule.AttrString`, else + `rule.AttrLiteral`; compare against `forbidValues` / `requireValues`. + - **Lists:** `rule.AttrStrings`; for `forbidListItems`, anchor on the matching + list element node (not the whole list). + - **Dicts:** `rule.Attr(attr)` as `*build.DictExpr`; `edit.DictionaryGet` per + key; normalize values like scalars. `forbidDictKeys` flags any listed key + that is present. + - **Numerics:** `strconv.Atoi(rule.AttrLiteral(attr))`; flag if value `< minValue` + or `> maxValue` (bounds inclusive; absent bound ignored). + Returns `makeLinterFinding(node, msg)` anchored on the offending attr node (or a + specific dict entry's value node when possible). +- Scope: BUILD files only (targets live there). Return `nil` for other file types. +- **Autofix: none initially.** We cannot infer a correct scalar value. The one + mechanically-safe fix (remove a forbidden list item / forbidden tag) is a good + fast-follow via `LinterReplacement`; leave a `// TODO(attr-policy): autofix remove-item`. + +### 3.7 Registration, docs, tests + +- Register in `warn/warn.go`: `FileWarningMap["attr-policy"] = attrPolicyWarning`. +- Decide default-on vs. opt-in: **default-on** (included in `DefaultWarnings`). When no + `attrPolicy` config is present, the warning enforces Bazel's default `shard_count` + constraint (`maxValue: 50` on `*_test` rules). Custom rules replace the default + entirely once `attrPolicy.rules` is non-empty. +- Docs: add an entry to `WARNINGS.md` and `warn/docs/warnings.textproto` describing the + warning **and** the `attrPolicy` config block (with the example rules, including + boolean, dict, and numeric constraints). +- Add a `-config=example` sample entry so `buildifier -config=example` prints an + `attrPolicy` stub (see `config.Example()`). +- Tests `warn/warn_attr_policy_test.go`: + - Set `warn.SetAttrPolicy(...)` in the test, then use `checkFindings`/ + `checkFindingsAndFix` with `scopeBuild`. + - Cases: forbidden scalar value flagged; allow-listed target exempt; recursive + `//pkg/...` allow-list; forbidden list item in `tags`; `ruleKinds` glob match & + non-match; missing-when-`required`; `# buildifier: disable=attr-policy` suppression; + forbidden boolean literal (`local = True`); forbidden dict entry + (`execution_requirements = {"no-cache": "1"}`); `forbidDictKeys` on a dict key; + `shard_count` above `maxValue` flagged, within range OK, absent OK; allow-listed + high-shard target exempt; empty config applies built-in defaults (`shard_count` ≤ 50, + deprecated `licenses`, deprecated `output_licenses` on binary-producing rules). + - Config tests in `buildifier/config/config_test.go`: parse the sample JSON; + validation rejects malformed rules. + +### 3.8 Acceptance criteria (Workstream A) + +- `buildifier --lint=warn --warnings=attr-policy BUILD` flags eternal timeout on + non-allow-listed test targets, `exclusive` tags on tests, and `shard_count > 50`, + given the sample config. +- Zero findings when `attrPolicy` is absent. +- All new + existing `warn` and `config` tests pass; `WARNINGS.md` regeneration (if + applicable) is consistent. + +--- + +## 4. Workstream B — `testpolicy` tool (empirical tuning) + +A **new binary** beside `buildifier`/`buildozer`. It never runs inside buildifier and +never blocks pre-commit. + +### 4.1 Layout + +``` +testpolicy/ + main.go // CLI: window, filters, report output + source/ // warehouse adapter + source.go // interface + data model + bigquery.go // concrete impl (behind a build tag / flag) + analyze/ + timeout.go // timeout recommender + flaky.go // flakiness scoring + flaky recommender + shard.go // (future) shard_count recommender — needs setup vs assertion timing + emit/ + buildozer.go // emit buildozer command script (terminal deliverable) + report/ // human-readable + machine (JSON) report +``` + +### 4.2 Warehouse adapter interface (data-source-agnostic) + +```go +// source/source.go +type TargetStats struct { + Label string + Runs int + DurationP50 time.Duration + DurationP95 time.Duration + DurationMax time.Duration + DeclaredTimeout string // bucket keyword (short|moderate|long|eternal) as observed, if available; may be empty (then derived from `size`, see §4.3) + TimeoutFailures int // failures attributed to hitting the timeout + // Per-attempt outcomes for flakiness math: + Attempts int // total attempts observed + PassByAttempt []float64 // PassByAttempt[k] = P(pass by attempt k+1), empirical + // Per-shard timing (optional; needed for shard_count recommender): + ShardCount int // declared shard_count, if any + ShardDurationP95 []time.Duration // per-shard P95, when available + SetupDurationP95 time.Duration // fixture/SUT setup time P95, when separable from assertions +} + +type Source interface { + // Query returns stats for targets matching the filter over [since, until]. + Query(ctx context.Context, since, until time.Time, filter Filter) ([]TargetStats, error) +} +``` + +Keeping this interface is what makes the "Metrics DB / warehouse" decision concrete +while letting the query backend be swapped/tested with a fake. + +### 4.3 Timeout recommender (`analyze/timeout.go`) + +**Bucket → seconds is a configured input, not a constant.** The `timeout` attribute is a +*bucket keyword* (`short`/`moderate`/`long`/`eternal`); the number of seconds each +bucket allows defaults to `60 / 300 / 900 / 3600` but a repo can override it with +`--test_timeout=short,moderate,long,eternal` (usually in `.bazelrc`). The recommender +cannot resolve a bucket to seconds — nor pick a bucket for an observed duration — +without the repo's actual mapping. So: + +- The tool takes a **`timeoutBuckets` config** (`map[string]int` keyword→seconds), + defaulting to Bazel's `60/300/900/3600`. Populate it either by parsing the repo's + `.bazelrc` for `--test_timeout`, or via an explicit flag/config value. Surface the + effective mapping in the report so recommendations are auditable. +- **Timeout may be implicit.** If a target sets no `timeout`, Bazel derives the bucket + from `size` (`small→short`, `medium→moderate`, `large→long`, `enormous→eternal`), then + resolves seconds via the same `timeoutBuckets` map. The recommender must apply this + fallback when `DeclaredTimeout` is empty, and decide whether to write a `timeout` attr + or adjust `size` (recommend setting `timeout` explicitly to avoid perturbing other + `size`-driven behavior like resource reservations). +- Recommend the **smallest bucket** whose configured seconds ≥ `DurationP95 * + safetyFactor` (default `safetyFactor = 1.5`, configurable). +- **Bump up** if either: observed runtime is within `X%` (default 20%) of the current + bucket's configured seconds, **or** `TimeoutFailures > 0`. +- **Never recommend `eternal`** unless the target is on the eternal allow-list (shared + with Workstream A's config so policy stays single-sourced). If data says a target + needs > `long` and isn't allow-listed, emit a **report finding for a human**, not an + auto-edit. +- Output per target: `{current, recommended, reason}`. + +### 4.4 Flakiness scoring (`analyze/flaky.go`) + +Answers "does a single retry likely pass, or does it need multiple?". + +- Let `a = P(a single attempt passes | not chronically broken)`, estimated from history + (prefer empirical `PassByAttempt` when present; else independence model). +- Under independence: `P(pass within N) = 1 - (1-a)^N`. To reach target pass-rate `T`: + + ``` + N ≥ ceil( log(1 - T) / log(1 - a) ) + ``` + +**Boundary cases** (handle before applying the formula): + +| Condition | Handling | +|---|---| +| `a = 1` (never fails) | `N = 1`; recommend `flaky = 0` / leave unset | +| `a = 0` or `a` below a minimum threshold (e.g. 0.05) | Classify as chronically broken; do **not** recommend retries | +| `a` very close to 1 (e.g. `a ≥ 0.999`) | Treat as `a = 1` to avoid `log(0)` | + +- `N` is used internally to distinguish "a single retry almost always recovers it" + from "retries rarely help", which drives the `flaky` recommendation: + | `N` | Meaning | Recommendation | + |---|---|---| + | ≤ 1 | not flaky | `flaky = 0` (or leave unset) | + | 2–3 | retries reliably recover it | `flaky = N - 1` (extra retries beyond the first attempt) | + | > 3 | retries rarely recover it | report for owner; more retries won't reliably help | + | very low `a` | chronically broken | **do not** mask with retries; report for owner | + +- **Semantics (with integer `flaky`):** When `--flaky_test_attempts=default`, Bazel runs + `1 + flaky` total attempts for flaky targets (`flaky = 0` ⇒ one attempt). The tool + writes the integer directly: `buildozer 'set flaky 2' //pkg:target` for three total + attempts. This is implemented in [figma/bazel#13](https://github.com/figma/bazel/pull/13); + upstream tracking in [bazelbuild/bazel#30108](https://github.com/bazelbuild/bazel/issues/30108). + When CI sets `--flaky_test_attempts=N` (bare integer), that flag overrides the attribute + for all targets — the tool should surface that in its report so owners know BUILD-file + edits may have no effect. + +### 4.5 Shard-count recommender (`analyze/shard.go`) — future + +`shard_count` is the third execution-reflecting test attribute alongside `timeout` and +`flaky`: all three describe **how the test currently executes**, not how it was designed. + +Tuning `shard_count` is feasible **only if** telemetry can separate fixture/SUT setup time +from assertion (test-case) time. Without that split, the tool cannot tell whether more +shards would shorten the critical path or just duplicate expensive setup across shards. + +When per-shard data is available: + +- Recommend raising `shard_count` when per-shard assertion time is high and setup is + amortizable (e.g. many cases per shard, setup ≪ case time). +- Recommend lowering `shard_count` when shards are mostly idle or setup dominates. +- Respect `attr-policy` caps (e.g. `shard_count` ≤ 50) from Workstream A. + +Output per target: `{current, recommended, reason}`; emit `buildozer 'set shard_count N'`. + +### 4.6 Emit (`emit/`) + +The tool's **terminal output** is a `buildozer` command script that would repair the +repo, printed to stdout/file, e.g.: +``` +buildozer 'set timeout "short"' //pkg:target +buildozer 'set flaky 2' //pkg:other +buildozer 'set shard_count 4' //pkg:sharded +``` +- Also emit a machine-readable JSON report (recommendations + reasons + effective + `timeoutBuckets`) for auditability. +- The tool **does not run buildozer, commit, or open PRs.** Executing the script, + batching edits, opening PRs, and routing to reviewers are downstream responsibilities + of whatever CI/automation calls `testpolicy`. Keeping the boundary here makes the tool + trivially testable (assert on emitted commands) and reusable by any apply/review flow. +- **Skip stale targets:** warehouse rows may refer to deleted or renamed targets. + Before emitting a `buildozer` line, verify the label still exists in the workspace + (e.g. index targets while walking BUILD files, or a fast `bazel query` check). Report + skipped labels in the JSON output instead of emitting commands that would fail. + +### 4.7 Safety / guardrails + +- Require a minimum `Runs` sample size before recommending (default e.g. 20); otherwise + report "insufficient data". +- **Lowering timeouts is a normal, first-class recommendation** — not gated behind a + flag. Bazel's default `size` is `medium` ⇒ `moderate` (300s), so many genuinely fast + tests sit at `moderate` unnecessarily; dropping them to `short` speeds failure + detection and scheduling. Safety comes from the recommender itself, not from + suppressing downgrades: the `safetyFactor` headroom and the bump-up-on-`TimeoutFailures` + / near-limit rules (§4.3) already keep a downgrade from cutting it too close, and the + minimum-sample-size guard avoids acting on noise. +- Respect the eternal allow-list from `.buildifier.json` so the two systems agree. +- Dry-run is the default mode. +- Do not emit `buildozer` commands for labels absent from the workspace (see §4.6). + +### 4.8 Acceptance criteria (Workstream B) + +- With a fake `Source`, `testpolicy` produces correct `timeout`, `flaky`, and (when + fixture data includes per-shard timing) `shard_count` recommendations and a valid + buildozer command script for a fixture dataset. +- No warehouse credentials required for tests (fake source). +- Output is deterministic (stable ordering) so the emitted script can be asserted on. + +--- + +## 5. Interaction between the two workstreams + +- The **eternal-timeout allow-list lives once** in `.buildifier.json` (`attrPolicy`). + Both `buildifier` (enforce) and `testpolicy` (never recommend eternal off-list) read + it. Consider a tiny shared loader package so the schema isn't duplicated. +- `buildifier` enforces the *invariants*; `testpolicy` proposes the *values*. A + `testpolicy`-emitted edit must itself satisfy `attr-policy` — i.e. the tool won't emit + a buildozer command that buildifier would then reject. + +--- + +## 6. Suppression & enforcement + +Every buildifier warning is silenceable with `# buildifier: disable=` (also +`# buildozer: disable=`). Suppression is centralized: `runWarningsFunction` drops any +finding for which `DisabledWarning(f, line, category)` is true (`warn/warn.go:281`), +keyed only on the category string. There is **no per-warning "cannot be suppressed" +flag** today — so out of the box a user can write: + +```python +# buildifier: disable=attr-policy +my_test(name = "x", timeout = "eternal") +``` + +…and the policy evaporates locally. We handle this deliberately rather than fighting it. + +**Reframe:** a `disable=` comment is not a silent back door — it lives in the BUILD +file, shows up in the diff, in `git blame`, and in code review. The governance question +is not "can it be bypassed?" (it can) but "is the bypass visible and attributable?" +(yes). That points to a two-tier model. + +### 6.1 Two-tier enforcement + +1. **buildifier `attr-policy` — fast, local, suppressible.** Normal buildifier UX: + dev-time feedback, silenceable with a visible comment. Advisory; not a gate. +2. **Authoritative CI gate — non-bypassable.** A CI job (naturally hosted in the + `testpolicy` tool, which already loads the policy config) re-evaluates the *same* + `.buildifier.json` policy against the tree and, for rules with `suppressible: false`, + **ignores `disable=` comments entirely**. In-file suppression can't defeat it because + the gate doesn't consult the file's comments for hard rules. This is where + "eternal timeout requires the allow-list, full stop" actually lives. + +Result: the **sanctioned** escape hatch is the allow-list (a reviewed edit to +`.buildifier.json`, ideally CODEOWNER-guarded); the **unsanctioned** one (a `disable=` +comment on a hard rule) is ignored by the gate and surfaced as debt. + +### 6.2 `suppressible` config field + +Per-rule `suppressible` (§3.2, default `true`) tunes this: +- `true` — soft rule; local `disable=attr-policy` silences it (advisory policy). +- `false` — hard rule; the CI gate enforces it regardless of comments. buildifier + locally still shows (and, unless we take the core change in §6.4, still lets users + *locally* suppress) it, but the merge gate is authoritative. + +### 6.3 Suppression audit + +A cheap job/report that inventories every `disable=attr-policy` (and, if we add +sub-scoping, `attr-policy=`) comment across the repo, so escape hatches are +**visible and burn-down-able** instead of accumulating silently. Pairs well with +CODEOWNERS on `.buildifier.json` and on BUILD files so both allow-list edits and +suppressions land on a policy owner. + +### 6.4 Optional core change (open question) + +If we want the *local* linter — not just CI — to refuse suppression for hard rules, +that's a small but real change to buildifier's contract: add a `NonSuppressible bool` +to `LinterFinding` and have `runWarningsFunction` skip the `DisabledWarning` check when +it's set; `attr-policy` sets it per-finding from the rule's `suppressible` flag. Clean +and gives per-rule granularity, **but** it breaks the long-standing invariant that every +warning is silenceable, so it needs buy-in before upstreaming. Recommendation: keep the +CI gate as the real enforcement regardless, and treat this as a nice-to-have. See §8. + +--- + +## 7. Phasing & work breakdown (for agent hand-off) + +Each task is independently ownable; dependencies noted. "AC" = acceptance criteria above. + +### Phase 1 — buildifier `attr-policy` (Workstream A) +- **A1. Config types + parsing** — add `AttrPolicy`/`AttrPolicyRule` to + `buildifier/config`, JSON round-trip test. *(no deps)* +- **A2. Validation** — `buildifier/config/validation.go` rules from §3.5. *(dep: A1)* +- **A3. Warning + compiled policy type + global** — `warn/warn_attr_policy.go`, + glob & allow-list matching, register in `warn/warn.go`. *(no deps; can stub config)* +- **A4. Config→warn wiring** — compile in `Validate()`, resolve import-cycle question + (§3.4 note). *(dep: A1, A3)* +- **A5. Tests** — `warn/warn_attr_policy_test.go` cases from §3.7. *(dep: A3/A4)* +- **A6. Docs** — `WARNINGS.md`, `warn/docs/warnings.textproto`, `-config=example` + stub. *(dep: A1, A3)* + +### Phase 2 — `testpolicy` skeleton + timeout recommender (Workstream B) +- **B1. Tool scaffold + CLI + fake source** — `testpolicy/main.go`, `source` interface, + in-memory fake. *(no deps)* +- **B2. Timeout recommender** — §4.3 + tests on fixtures. *(dep: B1)* +- **B3. buildozer emitter + report** — §4.6 dry-run path. *(dep: B1)* + +### Phase 3 — flakiness + real data source +- **B4. Flakiness scoring** — §4.4 + tests. *(dep: B1)* +- **B5. Warehouse (BigQuery/DB) source impl** — behind flag. *(dep: B1)* + +### Phase 4 — shard_count tuning (future) +- **B6. Shard-count recommender** — §4.5; requires per-shard telemetry with setup vs + assertion split. *(dep: B1, B5)* + +### Phase 1b — enforcement (Workstream A, in parallel with Phase 2) +- **A7. Authoritative CI gate** — evaluate policy ignoring `disable=` for + `suppressible: false` rules (§6.1/§6.2); non-zero exit on violation. *(dep: A1–A4)* +- **A8. Suppression audit** — inventory `disable=attr-policy` comments across the + repo as a report (§6.3). *(dep: A3)* + +--- + +## 8. Open questions + +1. **Import direction** between `warn` and `buildifier/config` — confirm no cycle + (§3.4). If one exists, the compiled-policy-type-in-`warn` approach resolves it. +2. ~~Should `attr-policy` be **default-on** (config-gated no-op) or opt-in via + `--warnings`?~~ **Resolved:** default-on; with no `attrPolicy` config, enforces + Bazel's `shard_count` ≤ 50 constraint on test rules. +3. **`NonSuppressible` core change (§6.4)** — do we extend buildifier so hard rules + can't be silenced by `disable=` locally, accepting a break to the "every warning is + suppressible" contract? Or rely solely on the CI gate? (Leaning CI-gate-only for the + first cut.) +4. Warehouse **schema/columns** available for `TargetStats` — especially whether + per-attempt outcomes (`PassByAttempt`) exist, or only aggregate pass/fail. This + changes flakiness estimation fidelity. +5. **`timeoutBuckets` sourcing (§4.3)** — parse `.bazelrc` for `--test_timeout`, or + require it as explicit tool config? Note `--test_timeout` can differ per bazelrc + `--config`/platform, so "the" mapping may be ambiguous; do we pin one config, or + analyze per-config? +6. Timeout **safety factor** and bucket cutoffs — defaults proposed; confirm with SRE/CI. +7. Do we want the shared eternal-allow-list loader as its own small package now, or + duplicate-read for Phase 1 and refactor later? diff --git a/warn/BUILD.bazel b/warn/BUILD.bazel index 377a38975..6a350dcd5 100644 --- a/warn/BUILD.bazel +++ b/warn/BUILD.bazel @@ -3,9 +3,11 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "warn", srcs = [ + "attr_policy.go", "multifile.go", "types.go", "warn.go", + "warn_attr_policy.go", "warn_bazel.go", "warn_bazel_api.go", "warn_bazel_operation.go", @@ -36,6 +38,7 @@ go_test( size = "small", srcs = [ "types_test.go", + "warn_attr_policy_test.go", "warn_bazel_api_test.go", "warn_bazel_operation_test.go", "warn_bazel_test.go", diff --git a/warn/attr_policy.go b/warn/attr_policy.go new file mode 100644 index 000000000..b45b85caa --- /dev/null +++ b/warn/attr_policy.go @@ -0,0 +1,155 @@ +package warn + +import ( + "path" + "strings" + + "github.com/bazelbuild/buildtools/labels" +) + +// AttrPolicyConstraintFamily identifies which constraint fields apply to a policy rule. +type AttrPolicyConstraintFamily int + +const ( + AttrPolicyScalarFamily AttrPolicyConstraintFamily = iota + AttrPolicyListFamily + AttrPolicyDictFamily + AttrPolicyNumericFamily + AttrPolicyForbidPresenceFamily +) + +// AttrPolicyAllowlistKind is a compiled allow-list pattern kind. +type AttrPolicyAllowlistKind int + +const ( + AttrPolicyAllowAll AttrPolicyAllowlistKind = iota + AttrPolicyAllowExact + AttrPolicyAllowPackageAll + AttrPolicyAllowRecursive +) + +// AttrPolicyAllowlistPattern is a compiled allow-list entry. +type AttrPolicyAllowlistPattern struct { + Kind AttrPolicyAllowlistKind + Pkg string + Target string +} + +// AttrPolicyRuleCompiled is a compiled attribute policy rule. +type AttrPolicyRuleCompiled struct { + Name string + RuleKinds []string + Attr string + Family AttrPolicyConstraintFamily + + ForbidValues []string + RequireValues []string + ForbidListItems []string + RequireListItems []string + AllowListItems []string + ForbidDictEntries map[string]string + RequireDictEntries map[string]string + ForbidDictKeys []string + MinValue *int + MaxValue *int + + Required bool + Allowlist []AttrPolicyAllowlistPattern + Suppressible bool + Message string +} + +const defaultShardCountMax = 50 + +var defaultOutputLicensesRuleKinds = []string{ + "genrule", + "cc_binary", + "cc_toolchain", + "java_binary", + "java_plugin", +} + +// DefaultAttrPolicyRules returns the built-in attribute policy rules applied when +// no attrPolicy configuration is present. +func DefaultAttrPolicyRules() []AttrPolicyRuleCompiled { + maxValue := defaultShardCountMax + return []AttrPolicyRuleCompiled{ + { + Name: "max-shard-count", + RuleKinds: []string{"*_test"}, + Attr: "shard_count", + Family: AttrPolicyNumericFamily, + MaxValue: &maxValue, + Message: "Having more than 50 shards is indicative of poor test organization. Please reduce the number of shards.", + Suppressible: true, + }, + { + Name: "no-licenses", + Attr: "licenses", + Family: AttrPolicyForbidPresenceFamily, + Message: "The licenses attribute is deprecated; use package(default_applicable_licenses = ...) and applicable_licenses on targets instead (https://github.com/bazelbuild/bazel/issues/188).", + Suppressible: true, + }, + { + Name: "no-output-licenses", + RuleKinds: append([]string(nil), defaultOutputLicensesRuleKinds...), + Attr: "output_licenses", + Family: AttrPolicyForbidPresenceFamily, + Message: "The output_licenses attribute is deprecated; use applicable_licenses instead (https://github.com/bazelbuild/bazel/issues/7444).", + Suppressible: true, + }, + } +} + +// AttrPolicyConfig is process-global policy, set from buildifier config before linting. +var AttrPolicyConfig []AttrPolicyRuleCompiled + +// SetAttrPolicy replaces the active attribute policy rules. +func SetAttrPolicy(rules []AttrPolicyRuleCompiled) { + AttrPolicyConfig = rules +} + +func effectiveAttrPolicyConfig() []AttrPolicyRuleCompiled { + if len(AttrPolicyConfig) > 0 { + return AttrPolicyConfig + } + return DefaultAttrPolicyRules() +} + +func matchesRuleKind(globs []string, kind string) bool { + if len(globs) == 0 { + return true + } + for _, g := range globs { + if matched, err := path.Match(g, kind); err == nil && matched { + return true + } + } + return false +} + +func allowlistMatches(patterns []AttrPolicyAllowlistPattern, label, pkg string) bool { + for _, p := range patterns { + if allowlistPatternMatches(p, label, pkg) { + return true + } + } + return false +} + +func allowlistPatternMatches(p AttrPolicyAllowlistPattern, label, pkg string) bool { + switch p.Kind { + case AttrPolicyAllowAll: + return true + case AttrPolicyAllowRecursive: + l := labels.Parse(label) + return l.Package == p.Pkg || strings.HasPrefix(l.Package, p.Pkg+"/") + case AttrPolicyAllowPackageAll: + return labels.Parse(label).Package == p.Pkg + case AttrPolicyAllowExact: + want := labels.Label{Package: p.Pkg, Target: p.Target}.Format() + return labels.Equal(label, want, pkg) + default: + return false + } +} diff --git a/warn/docs/warnings.textproto b/warn/docs/warnings.textproto index 8be362ea0..bd9d2276b 100644 --- a/warn/docs/warnings.textproto +++ b/warn/docs/warnings.textproto @@ -83,6 +83,52 @@ warnings: { autofix: false } +warnings: { + name: "attr-policy" + header: "Attribute value violates a configured policy rule" + description: + "Enforces declarative attribute constraints configured in `.buildifier.json` under\n" + "`attrPolicy`. Each rule names an attribute and a constraint family (scalar,\n" + "list, dict, numeric bounds, or presence). List rules support `allowListItems` for\n" + "closed-world lexicons; entries ending in `*` match by prefix. Targets matching an\n" + "`allowlist` pattern are exempt.\n\n" + "When no `attrPolicy` block is configured, the warning applies these built-in rules:\n\n" + "* `shard_count` must be ≤ 50 on `*_test` rules (matching Bazel's default constraint)\n" + "* `licenses` must not be set on any rule (deprecated; see [bazel#188](https://github.com/bazelbuild/bazel/issues/188))\n" + "* `output_licenses` must not be set on `genrule`, `cc_binary`, `cc_toolchain`, " + "`java_binary`, or `java_plugin` (deprecated; see [bazel#7444](https://github.com/bazelbuild/bazel/issues/7444))\n\n" + "Example:\n\n" + "```json\n" + "{\n" + " \"attrPolicy\": {\n" + " \"rules\": [\n" + " {\n" + " \"name\": \"no-eternal-timeout\",\n" + " \"ruleKinds\": [\"*_test\"],\n" + " \"attr\": \"timeout\",\n" + " \"forbidValues\": [\"eternal\"],\n" + " \"allowlist\": [\"//slow/...\"]\n" + " },\n" + " {\n" + " \"name\": \"max-shard-count\",\n" + " \"ruleKinds\": [\"*_test\"],\n" + " \"attr\": \"shard_count\",\n" + " \"maxValue\": 50\n" + " },\n" + " {\n" + " \"name\": \"allowed-tags\",\n" + " \"attr\": \"tags\",\n" + " \"allowListItems\": [\"manual\", \"assistant-ds*\", \"exclusive\"]\n" + " }\n" + " ]\n" + " }\n" + "}\n" + "```\n\n" + "A JSON Schema for `.buildifier.json` (including `attrPolicy`) lives at\n" + "`buildifier/config/buildifier.schema.json`." + autofix: false +} + warnings: { name: "attr-single-file" header: "`single_file` is deprecated" diff --git a/warn/warn.go b/warn/warn.go index 2d5d71b44..f09b04dc6 100644 --- a/warn/warn.go +++ b/warn/warn.go @@ -117,6 +117,7 @@ var RuleWarningMap = map[string]func(call *build.CallExpr, pkg string) *LinterFi // FileWarningMap lists the warnings that run on the whole file. var FileWarningMap = map[string]func(f *build.File) []*LinterFinding{ "allowed-symbol-load-locations": symbolLoadLocationWarning, + "attr-policy": attrPolicyWarning, "attr-applicable_licenses": attrApplicableLicensesWarning, "attr-cfg": attrConfigurationWarning, "attr-license": attrLicenseWarning, diff --git a/warn/warn_attr_policy.go b/warn/warn_attr_policy.go new file mode 100644 index 000000000..ce2aa9cc4 --- /dev/null +++ b/warn/warn_attr_policy.go @@ -0,0 +1,215 @@ +package warn + +import ( + "fmt" + "slices" + "strconv" + "strings" + + "github.com/bazelbuild/buildtools/build" + "github.com/bazelbuild/buildtools/edit" + "github.com/bazelbuild/buildtools/labels" +) + +func attrPolicyWarning(f *build.File) []*LinterFinding { + if f.Type != build.TypeBuild { + return nil + } + config := effectiveAttrPolicyConfig() + var findings []*LinterFinding + for _, rule := range f.Rules("") { + kind := rule.Kind() + label := labels.Label{Package: f.Pkg, Target: rule.Name()}.Format() + for _, p := range config { + if !matchesRuleKind(p.RuleKinds, kind) || allowlistMatches(p.Allowlist, label, f.Pkg) { + continue + } + findings = append(findings, attrPolicyCheckRule(rule, p)...) + } + } + return findings +} + +func attrPolicyCheckRule(rule *build.Rule, p AttrPolicyRuleCompiled) []*LinterFinding { + var findings []*LinterFinding + attrExpr := rule.Attr(p.Attr) + + if p.Required && attrExpr == nil { + findings = append(findings, makeLinterFinding(rule.Call, attrPolicyMessage(p, + fmt.Sprintf("attribute %q is required", p.Attr)))) + return findings + } + + switch p.Family { + case AttrPolicyScalarFamily: + if attrExpr == nil { + return findings + } + value := attrScalarString(rule, p.Attr) + for _, forbidden := range p.ForbidValues { + if value == forbidden { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not be %q", p.Attr, forbidden)))) + break + } + } + if len(p.RequireValues) > 0 && !slices.Contains(p.RequireValues, value) { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must be one of %s", p.Attr, quoteList(p.RequireValues))))) + } + case AttrPolicyListFamily: + if attrExpr == nil { + return findings + } + items := rule.AttrStrings(p.Attr) + if items == nil { + return findings + } + for _, forbidden := range p.ForbidListItems { + if slices.Contains(items, forbidden) { + node := attrExpr + if itemExpr := listItemExpr(attrExpr, forbidden); itemExpr != nil { + node = itemExpr + } + findings = append(findings, makeLinterFinding(node, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain %q", p.Attr, forbidden)))) + } + } + for _, required := range p.RequireListItems { + if !slices.Contains(items, required) { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must contain %q", p.Attr, required)))) + } + } + for _, item := range items { + if len(p.AllowListItems) > 0 && !allowListItemMatches(p.AllowListItems, item) { + node := attrExpr + if itemExpr := listItemExpr(attrExpr, item); itemExpr != nil { + node = itemExpr + } + findings = append(findings, makeLinterFinding(node, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain %q; allowed values are %s", + p.Attr, item, quoteList(p.AllowListItems))))) + } + } + case AttrPolicyDictFamily: + if attrExpr == nil { + return findings + } + dict, ok := attrExpr.(*build.DictExpr) + if !ok { + return findings + } + for key, forbiddenValue := range p.ForbidDictEntries { + if valueExpr := edit.DictionaryGet(dict, key); valueExpr != nil { + if exprScalarString(valueExpr) == forbiddenValue { + findings = append(findings, makeLinterFinding(valueExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain %q: %q", p.Attr, key, forbiddenValue)))) + } + } + } + for key, requiredValue := range p.RequireDictEntries { + valueExpr := edit.DictionaryGet(dict, key) + if valueExpr == nil || exprScalarString(valueExpr) != requiredValue { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must contain %q: %q", p.Attr, key, requiredValue)))) + } + } + for _, key := range p.ForbidDictKeys { + if valueExpr := edit.DictionaryGet(dict, key); valueExpr != nil { + findings = append(findings, makeLinterFinding(valueExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not contain key %q", p.Attr, key)))) + } + } + case AttrPolicyNumericFamily: + if attrExpr == nil { + return findings + } + value, err := strconv.Atoi(rule.AttrLiteral(p.Attr)) + if err != nil { + return findings + } + if p.MinValue != nil && value < *p.MinValue { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must be >= %d", p.Attr, *p.MinValue)))) + } + if p.MaxValue != nil && value > *p.MaxValue { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must be <= %d", p.Attr, *p.MaxValue)))) + } + case AttrPolicyForbidPresenceFamily: + if attrExpr != nil { + findings = append(findings, makeLinterFinding(attrExpr, attrPolicyMessage(p, + fmt.Sprintf("attribute %q must not be set", p.Attr)))) + } + } + return findings +} + +func attrPolicyMessage(p AttrPolicyRuleCompiled, defaultMsg string) string { + if p.Message != "" { + return fmt.Sprintf("[%s] %s", p.Name, p.Message) + } + return fmt.Sprintf("[%s] %s", p.Name, defaultMsg) +} + +func attrScalarString(rule *build.Rule, key string) string { + if s := rule.AttrString(key); s != "" { + return s + } + return rule.AttrLiteral(key) +} + +func exprScalarString(expr build.Expr) string { + if expr == nil { + return "" + } + if s, ok := expr.(*build.StringExpr); ok { + return s.Value + } + if i, ok := expr.(*build.Ident); ok { + return i.Name + } + if l, ok := expr.(*build.LiteralExpr); ok { + return l.Token + } + return "" +} + +func quoteList(values []string) string { + quoted := make([]string, len(values)) + for i, v := range values { + quoted[i] = fmt.Sprintf("%q", v) + } + return strings.Join(quoted, ", ") +} + +func listItemExpr(attrExpr build.Expr, item string) build.Expr { + list, ok := attrExpr.(*build.ListExpr) + if !ok { + return nil + } + for _, elem := range list.List { + if str, ok := elem.(*build.StringExpr); ok && str.Value == item { + return elem + } + } + return nil +} + +// allowListItemMatches reports whether item satisfies any allowListItems entry. +// Entries ending in * are prefix patterns; all other entries are exact matches. +func allowListItemMatches(allowed []string, item string) bool { + for _, pattern := range allowed { + if strings.HasSuffix(pattern, "*") { + if strings.HasPrefix(item, strings.TrimSuffix(pattern, "*")) { + return true + } + continue + } + if item == pattern { + return true + } + } + return false +} diff --git a/warn/warn_attr_policy_test.go b/warn/warn_attr_policy_test.go new file mode 100644 index 000000000..552fff080 --- /dev/null +++ b/warn/warn_attr_policy_test.go @@ -0,0 +1,267 @@ +/* +Copyright 2026 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package warn + +import ( + "testing" +) + +func attrPolicyTestRules() []AttrPolicyRuleCompiled { + return []AttrPolicyRuleCompiled{ + { + Name: "no-eternal-timeout", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"eternal"}, + Allowlist: []AttrPolicyAllowlistPattern{ + {Kind: AttrPolicyAllowExact, Pkg: "test/package", Target: "big_test"}, + }, + }, + { + Name: "no-exclusive-tests", + RuleKinds: []string{"*_test"}, + Attr: "tags", + Family: AttrPolicyListFamily, + ForbidListItems: []string{"exclusive"}, + }, + { + Name: "no-local-tests", + RuleKinds: []string{"*_test"}, + Attr: "local", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"True"}, + }, + { + Name: "no-no-cache", + Attr: "execution_requirements", + Family: AttrPolicyDictFamily, + ForbidDictEntries: map[string]string{ + "no-cache": "1", + }, + }, + { + Name: "max-shard-count", + RuleKinds: []string{"*_test"}, + Attr: "shard_count", + Family: AttrPolicyNumericFamily, + MaxValue: intPtr(50), + Allowlist: []AttrPolicyAllowlistPattern{ + {Kind: AttrPolicyAllowExact, Pkg: "test/package", Target: "massive_test"}, + }, + }, + } +} + +func intPtr(v int) *int { + return &v +} + +func TestAttrPolicyWarning(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy(attrPolicyTestRules()) + + checkFindings(t, "attr-policy", ` +cc_test(name = "ok", timeout = "short") +cc_test(name = "bad", timeout = "eternal") +cc_test(name = "big_test", timeout = "eternal") +cc_test(name = "exclusive", tags = ["exclusive"]) +cc_test(name = "local", local = True) +cc_test(name = "cached", execution_requirements = {"no-cache": "1"}) +cc_test(name = "sharded", shard_count = 100) +cc_test(name = "massive_test", shard_count = 100) +cc_library(name = "lib", timeout = "eternal") +`, []string{ + `:2: [no-eternal-timeout] attribute "timeout" must not be "eternal"`, + `:4: [no-exclusive-tests] attribute "tags" must not contain "exclusive"`, + `:5: [no-local-tests] attribute "local" must not be "True"`, + `:6: [no-no-cache] attribute "execution_requirements" must not contain "no-cache": "1"`, + `:7: [max-shard-count] attribute "shard_count" must be <= 50`, + }, scopeBuild) + + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "recursive-allow", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"eternal"}, + Allowlist: []AttrPolicyAllowlistPattern{ + {Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, + }, + }, + }) + cleanup := setUpTestPackage("slow/nested") + defer cleanup() + checkFindings(t, "attr-policy", ` +cc_test(name = "allowed", timeout = "eternal") +`, nil, scopeBuild) + + cleanup2 := setUpTestPackage("fast") + defer cleanup2() + checkFindings(t, "attr-policy", ` +cc_test(name = "bad", timeout = "eternal") +`, []string{ + `:1: [recursive-allow] attribute "timeout" must not be "eternal"`, + }, scopeBuild) + + SetAttrPolicy(nil) + checkFindings(t, "attr-policy", ` +cc_test(name = "bad", timeout = "eternal") +cc_test(name = "sharded", shard_count = 100) +cc_test(name = "ok", shard_count = 4) +cc_library(name = "lib", licenses = ["notice"]) +cc_binary(name = "bin", licenses = ["notice"], output_licenses = ["notice"]) +cc_library(name = "lib2", output_licenses = ["notice"]) +`, []string{ + `:2: [max-shard-count] Having more than 50 shards is indicative of poor test organization. Please reduce the number of shards.`, + `:4: [no-licenses] The licenses attribute is deprecated; use package(default_applicable_licenses = ...) and applicable_licenses on targets instead (https://github.com/bazelbuild/bazel/issues/188).`, + `:5: [no-licenses] The licenses attribute is deprecated; use package(default_applicable_licenses = ...) and applicable_licenses on targets instead (https://github.com/bazelbuild/bazel/issues/188).`, + `:5: [no-output-licenses] The output_licenses attribute is deprecated; use applicable_licenses instead (https://github.com/bazelbuild/bazel/issues/7444).`, + }, scopeBuild) +} + +func TestAttrPolicyForbidPresence(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "no-licenses", + Attr: "licenses", + Family: AttrPolicyForbidPresenceFamily, + }, + { + Name: "no-output-licenses", + RuleKinds: append([]string(nil), defaultOutputLicensesRuleKinds...), + Attr: "output_licenses", + Family: AttrPolicyForbidPresenceFamily, + }, + }) + + checkFindings(t, "attr-policy", ` +cc_library(name = "lib", licenses = ["notice"]) +cc_binary(name = "bin", output_licenses = ["notice"]) +cc_library(name = "lib2", output_licenses = ["notice"]) +cc_library(name = "clean") +`, []string{ + `:1: [no-licenses] attribute "licenses" must not be set`, + `:2: [no-output-licenses] attribute "output_licenses" must not be set`, + }, scopeBuild) +} + +func TestAttrPolicyRequired(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "needs-timeout", + Attr: "timeout", + Family: AttrPolicyScalarFamily, + Required: true, + }, + }) + checkFindings(t, "attr-policy", ` +cc_test(name = "missing") +cc_test(name = "present", timeout = "short") +`, []string{ + `:1: [needs-timeout] attribute "timeout" is required`, + }, scopeBuild) +} + +func TestAttrPolicyRuleKindGlob(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "tests-only", + RuleKinds: []string{"*_test"}, + Attr: "timeout", + Family: AttrPolicyScalarFamily, + ForbidValues: []string{"eternal"}, + }, + }) + checkFindings(t, "attr-policy", ` +cc_test(name = "bad", timeout = "eternal") +cc_library(name = "lib", timeout = "eternal") +`, []string{ + `:1: [tests-only] attribute "timeout" must not be "eternal"`, + }, scopeBuild) +} + +func TestAttrPolicyAllowListItems(t *testing.T) { + old := AttrPolicyConfig + defer func() { SetAttrPolicy(old) }() + SetAttrPolicy([]AttrPolicyRuleCompiled{ + { + Name: "allowed-tags", + Attr: "tags", + Family: AttrPolicyListFamily, + AllowListItems: []string{"manual", "assistant-ds*", "exclusive"}, + }, + }) + + checkFindings(t, "attr-policy", ` +cc_test(name = "ok", tags = ["manual", "assistant-ds-community", "exclusive"]) +cc_test(name = "bad", tags = ["my-new-tag"]) +`, []string{ + `:2: [allowed-tags] attribute "tags" must not contain "my-new-tag"; allowed values are "manual", "assistant-ds*", "exclusive"`, + }, scopeBuild) +} + +func TestAllowListItemMatches(t *testing.T) { + tests := []struct { + allowed []string + item string + want bool + }{ + {[]string{"manual", "exclusive"}, "manual", true}, + {[]string{"manual", "exclusive"}, "exclusive", true}, + {[]string{"manual", "exclusive"}, "other", false}, + {[]string{"assistant-ds*"}, "assistant-ds", true}, + {[]string{"assistant-ds*"}, "assistant-ds-community", true}, + {[]string{"assistant-ds*"}, "assistant-hero", false}, + } + for _, tc := range tests { + if got := allowListItemMatches(tc.allowed, tc.item); got != tc.want { + t.Errorf("allowListItemMatches(%v, %q) = %v, want %v", tc.allowed, tc.item, got, tc.want) + } + } +} + +func TestAllowlistPatternMatches(t *testing.T) { + tests := []struct { + pattern AttrPolicyAllowlistPattern + label string + pkg string + want bool + }{ + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowAll}, "//any:target", "any", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowExact, Pkg: "foo", Target: "bar"}, "//foo:bar", "foo", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowExact, Pkg: "foo", Target: "bar"}, "//foo:baz", "foo", false}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowPackageAll, Pkg: "foo"}, "//foo:bar", "foo", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowPackageAll, Pkg: "foo"}, "//bar:baz", "bar", false}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, "//slow:big", "slow", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, "//slow/nested:big", "slow/nested", true}, + {AttrPolicyAllowlistPattern{Kind: AttrPolicyAllowRecursive, Pkg: "slow"}, "//fast:big", "fast", false}, + } + for _, tc := range tests { + if got := allowlistPatternMatches(tc.pattern, tc.label, tc.pkg); got != tc.want { + t.Errorf("allowlistPatternMatches(%+v, %q, %q) = %v, want %v", tc.pattern, tc.label, tc.pkg, got, tc.want) + } + } +}