merger: merge os, arch, and platform selects separately (#64)

Gazelle can now merge expressions that contain multiple select
sub-expressions. Selects are identified as os-specific, arch-specific,
or platform-specific, based on the labels used in the select
dict. The dicts are merged independently.

Fixes #55

merger/BUILD.bazel

@@ -10,6 +10,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//config:go_default_library",
+        "//resolve:go_default_library",
         "@com_github_bazelbuild_buildtools//build:go_default_library",
     ],
 )

merger/fix.go

@@ -138,48 +138,38 @@ func squashCgoLibrary(oldFile *bf.File) *bf.File {
 
 // squashExpr combines two expressions. Unlike mergeExpr, squashExpr does not
 // discard information from an "old" expression. It does not sort or de-duplicate
-// elements. The following kinds of expressions are recognized:
-//
-//   * nil
-//   * lists
-//   * calls to select with a dict argument. The dict keys must be strings,
-//     and the values must be lists.
-//   * lists combined with select using +. The list must be the left operand.
+// elements. Any non-scalar expressions that mergeExpr understands can be
+// squashed.
 func squashExpr(x, y bf.Expr) (bf.Expr, error) {
-	xList, xDict, err := exprListAndDict(x)
+	xExprs, err := extractPlatformStringsExprs(x)
 	if err != nil {
 		return nil, err
 	}
-	yList, yDict, err := exprListAndDict(y)
+	yExprs, err := extractPlatformStringsExprs(y)
 	if err != nil {
 		return nil, err
 	}
-
-	squashedList := squashList(xList, yList)
-	squashedDict, err := squashDict(xDict, yDict)
+	squashedExprs, err := squashPlatformStringsExprs(xExprs, yExprs)
 	if err != nil {
 		return nil, err
 	}
+	return makePlatformStringsExpr(squashedExprs), nil
+}
 
-	var squashedSelect bf.Expr
-	if squashedDict != nil {
-		squashedSelect = &bf.CallExpr{
-			X:    &bf.LiteralExpr{Token: "select"},
-			List: []bf.Expr{squashedDict},
-		}
+func squashPlatformStringsExprs(x, y platformStringsExprs) (platformStringsExprs, error) {
+	var ps platformStringsExprs
+	var err error
+	ps.generic = squashList(x.generic, y.generic)
+	if ps.os, err = squashDict(x.os, y.os); err != nil {
+		return platformStringsExprs{}, err
 	}
-
-	if squashedList == nil {
-		return squashedDict, nil
+	if ps.arch, err = squashDict(x.arch, y.arch); err != nil {
+		return platformStringsExprs{}, err
 	}
-	if squashedSelect == nil {
-		return squashedList, nil
+	if ps.platform, err = squashDict(x.platform, y.platform); err != nil {
+		return platformStringsExprs{}, err
 	}
-	return &bf.BinaryExpr{
-		X:  squashedList,
-		Op: "+",
-		Y:  squashedSelect,
-	}, nil
+	return ps, nil
 }
 
 func squashList(x, y *bf.ListExpr) *bf.ListExpr {

merger/merger.go

@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	"github.com/bazelbuild/bazel-gazelle/config"
+	"github.com/bazelbuild/bazel-gazelle/resolve"
 	bf "github.com/bazelbuild/buildtools/build"
 )
 
@@ -140,7 +141,7 @@ func MergeFile(genRules []bf.Expr, empty []bf.Expr, oldFile *bf.File, attrs Merg
 	for _, s := range oldFile.Stmt {
 		if oldRule, ok := s.(*bf.CallExpr); ok {
 			if genRule, _, ok := match(empty, oldRule); ok && genRule != nil {
-				s = mergeRule(genRule, oldRule, attrs)
+				s = mergeRule(genRule, oldRule, attrs, oldFile.Path)
 				if s == nil {
 					// Deleted empty rule
 					continue
@@ -162,7 +163,7 @@ func MergeFile(genRules []bf.Expr, empty []bf.Expr, oldFile *bf.File, attrs Merg
 			mergedRules = append(mergedRules, genRule)
 			continue
 		} else if ok {
-			merged := mergeRule(genRule, oldRule, attrs)
+			merged := mergeRule(genRule, oldRule, attrs, oldFile.Path)
 			mergedFile.Stmt[i] = merged
 			mergedRules = append(mergedRules, mergedFile.Stmt[i])
 		}
@@ -175,7 +176,7 @@ func MergeFile(genRules []bf.Expr, empty []bf.Expr, oldFile *bf.File, attrs Merg
 // Both rules must be non-nil and must have the same kind and same name.
 // attrs is the set of attributes which may be merged.
 // If nil is returned, the rule should be deleted.
-func mergeRule(gen, old *bf.CallExpr, attrs MergeableAttrs) bf.Expr {
+func mergeRule(gen, old *bf.CallExpr, attrs MergeableAttrs, filename string) bf.Expr {
 	if old != nil && shouldKeep(old) {
 		return old
 	}
@@ -211,8 +212,9 @@ func mergeRule(gen, old *bf.CallExpr, attrs MergeableAttrs) bf.Expr {
 		genExpr := genRule.Attr(k)
 		mergedExpr, err := mergeExpr(genExpr, oldExpr)
 		if err != nil {
-			// TODO: add a verbose mode and log errors like this.
-			mergedExpr = genExpr
+			start, end := oldExpr.Span()
+			log.Printf("%s:%d.%d-%d.%d: could not merge expression", filename, start.Line, start.LineRune, end.Line, end.LineRune)
+			mergedExpr = oldExpr
 		}
 		if mergedExpr != nil {
 			mergedAttr := *oldAttr
@@ -258,82 +260,187 @@ func mergeExpr(gen, old bf.Expr) (bf.Expr, error) {
 		return gen, nil
 	}
 
-	genList, genDict, err := exprListAndDict(gen)
+	genExprs, err := extractPlatformStringsExprs(gen)
 	if err != nil {
 		return nil, err
 	}
-	oldList, oldDict, err := exprListAndDict(old)
+	oldExprs, err := extractPlatformStringsExprs(old)
 	if err != nil {
 		return nil, err
 	}
-
-	mergedList := mergeList(genList, oldList)
-	mergedDict, err := mergeDict(genDict, oldDict)
+	mergedExprs, err := mergePlatformStringsExprs(genExprs, oldExprs)
 	if err != nil {
 		return nil, err
 	}
+	return makePlatformStringsExpr(mergedExprs), nil
+}
 
-	var mergedSelect bf.Expr
-	if mergedDict != nil {
-		mergedSelect = &bf.CallExpr{
-			X:    &bf.LiteralExpr{Token: "select"},
-			List: []bf.Expr{mergedDict},
-		}
-	}
+// platformStringsExprs is a set of sub-expressions that match the structure
+// of package.PlatformStrings. rules.Generator produces expressions that
+// follow this structure for srcs, deps, and other attributes, so this matches
+// all non-scalar expressions generated by Gazelle.
+//
+// The matched expression has the form:
+//
+// [] + select({}) + select({}) + select({})
+//
+// The four collections may appear in any order, and some or all of them may
+// be omitted (all fields are nil for a nil expression).
+type platformStringsExprs struct {
+	generic            *bf.ListExpr
+	os, arch, platform *bf.DictExpr
+}
 
-	if mergedList == nil {
-		return mergedSelect, nil
+// extractPlatformStringsExprs matches an expression and attempts to extract
+// sub-expressions in platformStringsExprs. The sub-expressions can then be
+// merged with corresponding sub-expressions. Any field in the returned
+// structure may be nil. An error is returned if the given expression does
+// not follow the pattern described by platformStringsExprs.
+func extractPlatformStringsExprs(expr bf.Expr) (platformStringsExprs, error) {
+	var ps platformStringsExprs
+	if expr == nil {
+		return ps, nil
 	}
-	if mergedSelect == nil {
-		return mergedList, nil
+
+	// Break the expression into a sequence of expressions combined with +.
+	var parts []bf.Expr
+	for {
+		binop, ok := expr.(*bf.BinaryExpr)
+		if !ok {
+			parts = append(parts, expr)
+			break
+		}
+		parts = append(parts, binop.Y)
+		expr = binop.X
 	}
-	mergedList.ForceMultiLine = true
-	return &bf.BinaryExpr{
-		X:  mergedList,
-		Op: "+",
-		Y:  mergedSelect,
-	}, nil
-}
 
-// exprListAndDict matches an expression and attempts to extract either a list
-// of expressions, a call to select with a dictionary, or both.
-// An error is returned if the expression could not be matched.
-func exprListAndDict(expr bf.Expr) (*bf.ListExpr, *bf.DictExpr, error) {
-	if expr == nil {
-		return nil, nil, nil
-	}
-	switch expr := expr.(type) {
-	case *bf.ListExpr:
-		return expr, nil, nil
-	case *bf.CallExpr:
-		if x, ok := expr.X.(*bf.LiteralExpr); ok && x.Token == "select" && len(expr.List) == 1 {
-			if d, ok := expr.List[0].(*bf.DictExpr); ok {
-				return nil, d, nil
+	// Process each part. They may be in any order.
+	for _, part := range parts {
+		switch part := part.(type) {
+		case *bf.ListExpr:
+			if ps.generic != nil {
+				return platformStringsExprs{}, fmt.Errorf("expression could not be matched: multiple list expressions")
 			}
+			ps.generic = part
+
+		case *bf.CallExpr:
+			x, ok := part.X.(*bf.LiteralExpr)
+			if !ok || x.Token != "select" || len(part.List) != 1 {
+				return platformStringsExprs{}, fmt.Errorf("expression could not be matched: callee other than select or wrong number of args")
+			}
+			arg, ok := part.List[0].(*bf.DictExpr)
+			if !ok {
+				return platformStringsExprs{}, fmt.Errorf("expression could not be matched: select argument not dict")
+			}
+			var dict **bf.DictExpr
+			for _, item := range arg.List {
+				kv := item.(*bf.KeyValueExpr) // parser guarantees this
+				k, ok := kv.Key.(*bf.StringExpr)
+				if !ok {
+					return platformStringsExprs{}, fmt.Errorf("expression could not be matched: dict keys are not all strings")
+				}
+				if k.Value == "//conditions:default" {
+					continue
+				}
+				key, err := resolve.ParseLabel(k.Value)
+				if err != nil {
+					return platformStringsExprs{}, fmt.Errorf("expression could not be matched: dict key is not label: %q", k.Value)
+				}
+				if config.KnownOSSet[key.Name] {
+					dict = &ps.os
+					break
+				}
+				if config.KnownArchSet[key.Name] {
+					dict = &ps.arch
+					break
+				}
+				osArch := strings.Split(key.Name, "_")
+				if len(osArch) != 2 || !config.KnownOSSet[osArch[0]] || !config.KnownArchSet[osArch[1]] {
+					return platformStringsExprs{}, fmt.Errorf("expression could not be matched: dict key contains unknown platform: %q", k.Value)
+				}
+				dict = &ps.platform
+				break
+			}
+			if dict == nil {
+				// We could not identify the dict because it's empty or only contains
+				// //conditions:default. We'll call it the platform dict to avoid
+				// dropping it.
+				dict = &ps.platform
+			}
+			if *dict != nil {
+				return platformStringsExprs{}, fmt.Errorf("expression could not be matched: multiple selects that are either os-specific, arch-specific, or platform-specific")
+			}
+			*dict = arg
 		}
-	case *bf.BinaryExpr:
-		if expr.Op != "+" {
-			return nil, nil, fmt.Errorf("expression could not be matched: unknown operator: %s", expr.Op)
-		}
-		l, ok := expr.X.(*bf.ListExpr)
-		if !ok {
-			return nil, nil, fmt.Errorf("expression could not be matched: left operand not a list")
-		}
-		call, ok := expr.Y.(*bf.CallExpr)
-		if !ok || len(call.List) != 1 {
-			return nil, nil, fmt.Errorf("expression could not be matched: right operand not a call with one argument")
+	}
+	return ps, nil
+}
+
+// makePlatformStringsExpr constructs a single expression from the
+// sub-expressions in ps.
+func makePlatformStringsExpr(ps platformStringsExprs) bf.Expr {
+	makeSelect := func(dict *bf.DictExpr) bf.Expr {
+		return &bf.CallExpr{
+			X:    &bf.LiteralExpr{Token: "select"},
+			List: []bf.Expr{dict},
 		}
-		x, ok := call.X.(*bf.LiteralExpr)
-		if !ok || x.Token != "select" {
-			return nil, nil, fmt.Errorf("expression could not be matched: right operand not a call to select")
+	}
+	forceMultiline := func(e bf.Expr) {
+		switch e := e.(type) {
+		case *bf.ListExpr:
+			e.ForceMultiLine = true
+		case *bf.CallExpr:
+			e.List[0].(*bf.DictExpr).ForceMultiLine = true
 		}
-		d, ok := call.List[0].(*bf.DictExpr)
-		if !ok {
-			return nil, nil, fmt.Errorf("expression could not be matched: argument to right operand not a dict")
+	}
+
+	var parts []bf.Expr
+	if ps.generic != nil {
+		parts = append(parts, ps.generic)
+	}
+	if ps.os != nil {
+		parts = append(parts, makeSelect(ps.os))
+	}
+	if ps.arch != nil {
+		parts = append(parts, makeSelect(ps.arch))
+	}
+	if ps.platform != nil {
+		parts = append(parts, makeSelect(ps.platform))
+	}
+
+	if len(parts) == 0 {
+		return nil
+	}
+	if len(parts) == 1 {
+		return parts[0]
+	}
+	expr := parts[0]
+	forceMultiline(expr)
+	for _, part := range parts[1:] {
+		forceMultiline(part)
+		expr = &bf.BinaryExpr{
+			Op: "+",
+			X:  expr,
+			Y:  part,
 		}
-		return l, d, nil
 	}
-	return nil, nil, fmt.Errorf("expression could not be matched")
+	return expr
+}
+
+func mergePlatformStringsExprs(gen, old platformStringsExprs) (platformStringsExprs, error) {
+	var ps platformStringsExprs
+	var err error
+	ps.generic = mergeList(gen.generic, old.generic)
+	if ps.os, err = mergeDict(gen.os, old.os); err != nil {
+		return platformStringsExprs{}, err
+	}
+	if ps.arch, err = mergeDict(gen.arch, old.arch); err != nil {
+		return platformStringsExprs{}, err
+	}
+	if ps.platform, err = mergeDict(gen.platform, old.platform); err != nil {
+		return platformStringsExprs{}, err
+	}
+	return ps, nil
 }
 
 func mergeList(gen, old *bf.ListExpr) *bf.ListExpr {