Refactor the label-related methods (bazelbuild#903)

BUILD.bazel

@@ -27,6 +27,7 @@ test_suite(
         "//deps_proto:deps.gen.pb.go_checkshtest",
         "//edit:go_default_test",
         "//extra_actions_base_proto:extra_actions_base.gen.pb.go_checkshtest",
+        "//labels:go_default_test",
         "//lang:tables.gen.go_checkshtest",
         "//tables:go_default_test",
         "//warn:go_default_test",

edit/BUILD.bazel

@@ -15,6 +15,7 @@ go_library(
         "//build:go_default_library",
         "//build_proto:go_default_library",
         "//file:go_default_library",
+        "//labels:go_default_library",
         "//lang:go_default_library",
         "//tables:go_default_library",
         "//wspace:go_default_library",

edit/buildozer.go

@@ -37,6 +37,7 @@ import (
 	apipb "github.com/bazelbuild/buildtools/api_proto"
 	"github.com/bazelbuild/buildtools/build"
 	"github.com/bazelbuild/buildtools/file"
+	"github.com/bazelbuild/buildtools/labels"
 	"github.com/golang/protobuf/proto"
 )
 
@@ -437,7 +438,7 @@ func cmdReplace(opts *Options, env CmdEnvironment) (*build.File, error) {
 	for _, key := range attrKeysForPattern(env.Rule, env.Args[0]) {
 		attr := env.Rule.Attr(key)
 		if e, ok := attr.(*build.StringExpr); ok {
-			if LabelsEqual(e.Value, oldV, env.Pkg) {
+			if labels.Equal(e.Value, oldV, env.Pkg) {
 				env.Rule.SetAttr(key, getAttrValueExpr(key, []string{newV}, env))
 			}
 		} else {
@@ -524,13 +525,13 @@ func getStringValue(value string) string {
 	return value
 }
 
-// getStringExpr creates a StringExpr from an input argument, which can be either quoter or not,
+// getStringExpr creates a StringExpr from an input argument, which can be either quoted or not,
 // and shortens the label value if possible.
 func getStringExpr(value, pkg string) build.Expr {
 	if unquoted, triple, err := build.Unquote(value); err == nil {
-		return &build.StringExpr{Value: ShortenLabel(unquoted, pkg), TripleQuote: triple}
+		return &build.StringExpr{Value: labels.ShortenLabel(unquoted, pkg), TripleQuote: triple}
 	}
-	return &build.StringExpr{Value: ShortenLabel(value, pkg)}
+	return &build.StringExpr{Value: labels.ShortenLabel(value, pkg)}
 }
 
 func cmdCopy(opts *Options, env CmdEnvironment) (*build.File, error) {
@@ -924,8 +925,8 @@ func rewrite(opts *Options, commandsForFile commandsForFile) *rewriteResult {
 		target := commands.target
 		commands := commands.commands
 		_, absPkg, rule := InterpretLabelForWorkspaceLocation(opts.RootDir, target)
-		_, pkg, _ := ParseLabel(target)
-		if pkg == stdinPackageName { // Special-case: This is already absolute
+		if label := labels.ParseLabel(target); label.Package == stdinPackageName {
+			// Special-case: This is already absolute
 			absPkg = stdinPackageName
 		}
 
@@ -1101,8 +1102,7 @@ func appendCommands(opts *Options, commandMap map[string][]commandsForTarget, ar
 			}
 		}
 		var buildFiles []string
-		_, pkg, _ := ParseLabel(target)
-		if pkg == stdinPackageName {
+		if label := labels.ParseLabel(target); label.Package == stdinPackageName {
 			buildFiles = []string{stdinPackageName}
 		} else {
 			buildFiles = targetExpressionToBuildFiles(opts.RootDir, target)

edit/edit.go

@@ -21,14 +21,13 @@ package edit
 import (
 	"fmt"
 	"os"
-	"path"
 	"path/filepath"
 	"regexp"
 	"strconv"
 	"strings"
 
 	"github.com/bazelbuild/buildtools/build"
-	"github.com/bazelbuild/buildtools/tables"
+	"github.com/bazelbuild/buildtools/labels"
 	"github.com/bazelbuild/buildtools/wspace"
 )
 
@@ -39,74 +38,6 @@ var (
 	DeleteWithComments = true
 )
 
-// ParseLabel parses a Blaze label (eg. //devtools/buildozer:rule), and returns
-// the repo name ("" for the main repo), package (with leading slashes trimmed)
-// and rule name (e.g. ["", "devtools/buildozer", "rule"]).
-func ParseLabel(target string) (string, string, string) {
-	repo := ""
-	if strings.HasPrefix(target, "@") {
-		target = strings.TrimLeft(target, "@")
-		parts := strings.SplitN(target, "/", 2)
-		if len(parts) == 1 {
-			// "@foo" -> "foo", "", "foo" (ie @foo//:foo)
-			return target, "", target
-		}
-		repo = parts[0]
-		target = "/" + parts[1]
-	}
-	// TODO(bazel-team): check if the next line can now be deleted
-	target = strings.TrimRight(target, ":") // labels can end with ':'
-	parts := strings.SplitN(target, ":", 2)
-	parts[0] = strings.TrimPrefix(parts[0], "//")
-	if len(parts) == 1 {
-		if strings.HasPrefix(target, "//") || tables.StripLabelLeadingSlashes {
-			// "//absolute/pkg" -> "absolute/pkg", "pkg"
-			return repo, parts[0], path.Base(parts[0])
-		}
-		// "relative/label" -> "", "relative/label"
-		return repo, "", parts[0]
-	}
-	return repo, parts[0], parts[1]
-}
-
-// ShortenLabel rewrites labels to use the canonical form (the form
-// recommended by build-style).  This behavior can be disabled using the
-// --noshorten_labels flag for projects that consistently use long-form labels.
-// "//foo/bar:bar" => "//foo/bar", or ":bar" when possible.
-func ShortenLabel(label string, pkg string) string {
-	if !ShortenLabelsFlag {
-		return label
-	}
-	if !strings.Contains(label, "//") {
-		// It doesn't look like a long label, so we preserve it.
-		return label
-	}
-	repo, labelPkg, rule := ParseLabel(label)
-	if repo == "" && labelPkg == pkg { // local label
-		return ":" + rule
-	}
-	slash := strings.LastIndex(labelPkg, "/")
-	if (slash >= 0 && labelPkg[slash+1:] == rule) || labelPkg == rule {
-		if repo == "" {
-			return "//" + labelPkg
-		}
-		return "@" + repo + "//" + labelPkg
-	}
-	if strings.HasPrefix(label, "@") && repo == rule && labelPkg == "" {
-		return "@" + repo
-	}
-	return label
-}
-
-// LabelsEqual returns true if label1 and label2 are equal. The function
-// takes care of the optional ":" prefix and differences between long-form
-// labels and local labels.
-func LabelsEqual(label1, label2, pkg string) bool {
-	str1 := strings.TrimPrefix(ShortenLabel(label1, pkg), ":")
-	str2 := strings.TrimPrefix(ShortenLabel(label2, pkg), ":")
-	return str1 == str2
-}
-
 // isFile returns true if the path refers to a regular file after following
 // symlinks.
 func isFile(path string) bool {
@@ -125,7 +56,10 @@ func isFile(path string) bool {
 // edit, the full package name, and the rule. It takes a workspace-rooted
 // directory to use.
 func InterpretLabelForWorkspaceLocation(root string, target string) (buildFile string, pkg string, rule string) {
-	repo, pkg, rule := ParseLabel(target)
+	label := labels.ParseLabel(target)
+	repo := label.Repository
+	pkg = label.Package
+	rule = label.Target
 	rootDir, relativePath := wspace.FindWorkspaceRoot(root)
 	if repo != "" {
 		files, err := wspace.FindRepoBuildFiles(rootDir)
@@ -492,11 +426,11 @@ func AllStrings(e build.Expr) []*build.StringExpr {
 
 // listsFind looks for a string in list expressions
 func listsFind(lists []*build.ListExpr, item string, pkg string) *build.StringExpr {
-	item = ShortenLabel(item, pkg)
+	item = labels.ShortenLabel(item, pkg)
 	for _, list := range lists {
 		for _, elem := range list.List {
 			str, ok := elem.(*build.StringExpr)
-			if ok && LabelsEqual(str.Value, item, pkg) {
+			if ok && labels.Equal(str.Value, item, pkg) {
 				return str
 			}
 		}
@@ -508,15 +442,15 @@ func listsFind(lists []*build.ListExpr, item string, pkg string) *build.StringEx
 // concatenation of lists). It returns the element if it is found. nil
 // otherwise.
 func ListFind(e build.Expr, item string, pkg string) *build.StringExpr {
-	item = ShortenLabel(item, pkg)
+	item = labels.ShortenLabel(item, pkg)
 	return listsFind(AllLists(e), item, pkg)
 }
 
 // listOrSelectFind looks for a string in the list expression (which may be a
 // concatenation of lists and select statements). It returns the element
 // if it is found. nil otherwise.
 func listOrSelectFind(e build.Expr, item string, pkg string) *build.StringExpr {
-	item = ShortenLabel(item, pkg)
+	item = labels.ShortenLabel(item, pkg)
 	return listsFind(allListsIncludingSelects(e), item, pkg)
 }
 
@@ -702,7 +636,7 @@ func RemoveFromList(li *build.ListExpr, item, pkg string, deleted **build.String
 	var all []build.Expr
 	for _, elem := range li.List {
 		if str, ok := elem.(*build.StringExpr); ok {
-			if LabelsEqual(str.Value, item, pkg) && (DeleteWithComments || !hasComments(str)) {
+			if labels.Equal(str.Value, item, pkg) && (DeleteWithComments || !hasComments(str)) {
 				if deleted != nil {
 					*deleted = str
 				}
@@ -722,7 +656,7 @@ func ListDelete(e build.Expr, item, pkg string) (deleted *build.StringExpr) {
 		item = unquoted
 	}
 	deleted = nil
-	item = ShortenLabel(item, pkg)
+	item = labels.ShortenLabel(item, pkg)
 	for _, li := range AllLists(e) {
 		RemoveFromList(li, item, pkg, &deleted)
 	}
@@ -748,14 +682,14 @@ func ListAttributeDelete(rule *build.Rule, attr, item, pkg string) *build.String
 // to indicate whether the replacement was successful.
 func ListReplace(e build.Expr, old, value, pkg string) bool {
 	replaced := false
-	old = ShortenLabel(old, pkg)
+	old = labels.ShortenLabel(old, pkg)
 	for _, li := range allListsIncludingSelects(e) {
 		for k, elem := range li.List {
 			str, ok := elem.(*build.StringExpr)
-			if !ok || !LabelsEqual(str.Value, old, pkg) {
+			if !ok || !labels.Equal(str.Value, old, pkg) {
 				continue
 			}
-			li.List[k] = &build.StringExpr{Value: ShortenLabel(value, pkg), Comments: *elem.Comment()}
+			li.List[k] = &build.StringExpr{Value: labels.ShortenLabel(value, pkg), Comments: *elem.Comment()}
 			replaced = true
 		}
 	}

edit/edit_test.go

@@ -29,92 +29,6 @@ import (
 	"github.com/bazelbuild/buildtools/build"
 )
 
-var parseLabelTests = []struct {
-	in   string
-	repo string
-	pkg  string
-	rule string
-}{
-	{"//devtools/buildozer:rule", "", "devtools/buildozer", "rule"},
-	{"devtools/buildozer:rule", "", "devtools/buildozer", "rule"},
-	{"//devtools/buildozer", "", "devtools/buildozer", "buildozer"},
-	{"//base", "", "base", "base"},
-	{"//base:", "", "base", "base"},
-	{"@r//devtools/buildozer:rule", "r", "devtools/buildozer", "rule"},
-	{"@r//devtools/buildozer", "r", "devtools/buildozer", "buildozer"},
-	{"@r//base", "r", "base", "base"},
-	{"@r//base:", "r", "base", "base"},
-	{"@foo", "foo", "", "foo"},
-	{":label", "", "", "label"},
-	{"label", "", "", "label"},
-	{"/abs/path/to/WORKSPACE:rule", "", "/abs/path/to/WORKSPACE", "rule"},
-}
-
-func TestParseLabel(t *testing.T) {
-	for i, tt := range parseLabelTests {
-		repo, pkg, rule := ParseLabel(tt.in)
-		if repo != tt.repo || pkg != tt.pkg || rule != tt.rule {
-			t.Errorf("%d. ParseLabel(%q) => (%q, %q, %q), want (%q, %q, %q)",
-				i, tt.in, repo, pkg, rule, tt.repo, tt.pkg, tt.rule)
-		}
-	}
-}
-
-var shortenLabelTests = []struct {
-	in     string
-	pkg    string
-	result string
-}{
-	{"//devtools/buildozer:rule", "devtools/buildozer", ":rule"},
-	{"@//devtools/buildozer:rule", "devtools/buildozer", ":rule"},
-	{"//devtools/buildozer:rule", "devtools", "//devtools/buildozer:rule"},
-	{"//base:rule", "devtools", "//base:rule"},
-	{"//base:base", "devtools", "//base"},
-	{"//base", "base", ":base"},
-	{"//devtools/buildozer:buildozer", "", "//devtools/buildozer"},
-	{"@r//devtools/buildozer:buildozer", "devtools/buildozer", "@r//devtools/buildozer"},
-	{"@r//devtools/buildozer", "devtools/buildozer", "@r//devtools/buildozer"},
-	{"@r//devtools", "devtools", "@r//devtools"},
-	{"@r:rule", "", "@r:rule"},
-	{"@r", "", "@r"},
-	{"@foo//:foo", "", "@foo"},
-	{"@foo//devtools:foo", "", "@foo//devtools:foo"},
-	{"@foo//devtools:foo", "devtools", "@foo//devtools:foo"},
-	{"@foo//foo:foo", "", "@foo//foo"},
-	{":local", "", ":local"},
-	{"something else", "", "something else"},
-	{"/path/to/file", "path/to", "/path/to/file"},
-}
-
-func TestShortenLabel(t *testing.T) {
-	for i, tt := range shortenLabelTests {
-		result := ShortenLabel(tt.in, tt.pkg)
-		if result != tt.result {
-			t.Errorf("%d. ShortenLabel(%q, %q) => %q, want %q",
-				i, tt.in, tt.pkg, result, tt.result)
-		}
-	}
-}
-
-var labelsEqualTests = []struct {
-	label1   string
-	label2   string
-	pkg      string
-	expected bool
-}{
-	{"//devtools/buildozer:rule", "rule", "devtools/buildozer", true},
-	{"//devtools/buildozer:rule", "rule:jar", "devtools", false},
-}
-
-func TestLabelsEqual(t *testing.T) {
-	for i, tt := range labelsEqualTests {
-		if got := LabelsEqual(tt.label1, tt.label2, tt.pkg); got != tt.expected {
-			t.Errorf("%d. LabelsEqual(%q, %q, %q) => %v, want %v",
-				i, tt.label1, tt.label2, tt.pkg, got, tt.expected)
-		}
-	}
-}
-
 var splitOnSpacesTests = []struct {
 	in  string
 	out []string

edit/fix.go

@@ -24,6 +24,7 @@ import (
 	"strings"
 
 	"github.com/bazelbuild/buildtools/build"
+	"github.com/bazelbuild/buildtools/labels"
 )
 
 // splitOptionsWithSpaces is a cleanup function.
@@ -88,8 +89,8 @@ func shortenLabels(_ *build.File, r *build.Rule, pkg string) bool {
 		for _, li := range AllLists(e) {
 			for _, elem := range li.List {
 				str, ok := elem.(*build.StringExpr)
-				if ok && str.Value != ShortenLabel(str.Value, pkg) {
-					str.Value = ShortenLabel(str.Value, pkg)
+				if ok && str.Value != labels.ShortenLabel(str.Value, pkg) {
+					str.Value = labels.ShortenLabel(str.Value, pkg)
 					fixed = true
 				}
 			}

labels/BUILD.bazel

@@ -0,0 +1,18 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "go_default_library",
+    srcs = [
+        "labels.go",
+    ],
+    importpath = "github.com/bazelbuild/buildtools/labels",
+    visibility = ["//visibility:public"],
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = [
+        "labels_test.go",
+    ],
+    embed = [":go_default_library"],
+)