evaluating binary expressions correctly for srcs (#109)

Previously binary expressions were ignored by puku. This meant statements like glob(["*_test.go"]) + ["file.go"] would not be evaluated properly. causing unnecessary churn and issues when evaluating if all sources were assigned.

---------

Co-authored-by: rgodden <[email protected]>

ChangeLog

@@ -1,3 +1,7 @@
+Version 1.7.2
+------------
+    * evaluating binary expressions correctly for srcs. (#109)
+
 Version 1.7.1
 ------------
     * resolve deps for go_repo to subrepo targets rather than its install

eval/BUILD

@@ -13,6 +13,7 @@ go_library(
 go_test(
     name = "eval_test",
     srcs = ["eval_test.go"],
+    data = ["//:test_project"],
     deps = [
         ":eval",
         "///third_party/go/github.com_please-build_buildtools//build",

eval/eval.go

@@ -31,11 +31,53 @@ func LookLikeBuildLabel(l string) bool {
 }
 
 func (e *Eval) EvalGlobs(dir string, rule *build.Rule, attrName string) ([]string, error) {
-	globArgs := parseGlob(rule.Attr(attrName))
-	if globArgs != nil {
+	files, err := e.evalGlobs(dir, rule.Attr(attrName))
+	if err != nil {
+		return nil, err
+	}
+	ret := make([]string, 0, len(files))
+	for f := range files {
+		ret = append(ret, f)
+	}
+	return ret, nil
+}
+
+func (e *Eval) evalGlobs(dir string, val build.Expr) (map[string]struct{}, error) {
+	switch expr := val.(type) {
+	case *build.CallExpr:
+		globArgs := parseGlob(expr)
+		if globArgs == nil {
+			return nil, nil
+		}
 		return e.globber.Glob(dir, globArgs)
+	case *build.BinaryExpr:
+		ret, err := e.evalGlobs(dir, expr.X)
+		if err != nil {
+			return nil, err
+		}
+		y, err := e.evalGlobs(dir, expr.Y)
+		if err != nil {
+			return nil, err
+		}
+		switch expr.Op {
+		case "+":
+			for f := range y {
+				ret[f] = struct{}{}
+			}
+		case "-":
+			for f := range y {
+				delete(ret, f)
+			}
+		}
+		return ret, nil
+	default:
+		str := build.Strings(expr)
+		ret := make(map[string]struct{}, len(str))
+		for _, s := range str {
+			ret[s] = struct{}{}
+		}
+		return ret, nil
 	}
-	return rule.AttrStrings(attrName), nil
 }
 
 func (e *Eval) BuildSources(plzPath, dir string, rule *build.Rule, srcsArg string) ([]string, error) {

eval/eval_test.go

@@ -6,6 +6,8 @@ import (
 	"github.com/please-build/buildtools/build"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/please-build/puku/glob"
 )
 
 func TestParseGlob(t *testing.T) {
@@ -56,3 +58,51 @@ func TestParseGlob(t *testing.T) {
 		})
 	}
 }
+func TestEvalGlob(t *testing.T) {
+	e := New(glob.New())
+	testCases := []struct {
+		name     string
+		code     string
+		expected []string
+	}{
+		{
+			name:     "glob + glob",
+			code:     `glob(["mai*.go"]) + glob(["ba*.go"])`,
+			expected: []string{"main.go", "bar.go", "bar_test.go"},
+		},
+		{
+			name:     "glob + glob + strings",
+			code:     `glob(["mai*.go"]) + glob(["*_test.go"]) + ["bar.go"]`,
+			expected: []string{"main.go", "bar.go", "bar_test.go"},
+		},
+		{
+			name:     "strings + strings",
+			code:     `["main.go"] + ["bar.go"]`,
+			expected: []string{"main.go", "bar.go"},
+		},
+		{
+			name:     "glob + strings",
+			code:     `glob(["mai*.go"]) + ["bar.go"]`,
+			expected: []string{"main.go", "bar.go"},
+		},
+		{
+			name:     "glob - strings",
+			code:     `glob(["*.go"]) - ["bar.go"]`,
+			expected: []string{"main.go", "bar_test.go"},
+		},
+	}
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			file, err := build.ParseBuild(test.name, []byte(test.code))
+			require.NoError(t, err)
+			require.Len(t, file.Stmt, 1)
+			ret, err := e.evalGlobs("test_project", file.Stmt[0])
+			got := make([]string, 0, len(ret))
+			for f := range ret {
+				got = append(got, f)
+			}
+			require.NoError(t, err)
+			assert.ElementsMatch(t, test.expected, got)
+		})
+	}
+}

glob/glob.go

@@ -25,7 +25,7 @@ func New() *Globber {
 // 1) globs should only match .go files as they're being used in go rules
 // 2) go rules will never depend on files outside the package dir, so we don't need to support **
 // 3) we don't want symlinks, directories and other non-regular files
-func (g *Globber) Glob(dir string, args *Args) ([]string, error) {
+func (g *Globber) Glob(dir string, args *Args) (map[string]struct{}, error) {
 	inc := map[string]struct{}{}
 	for _, i := range args.Include {
 		fs, err := g.glob(dir, i)
@@ -49,11 +49,7 @@ func (g *Globber) Glob(dir string, args *Args) ([]string, error) {
 		}
 	}
 
-	ret := make([]string, 0, len(inc))
-	for i := range inc {
-		ret = append(ret, i)
-	}
-	return ret, nil
+	return inc, nil
 }
 
 // glob matches all regular files in a directory based on a glob pattern

glob/glob_test.go

@@ -15,7 +15,7 @@ func TestGlob(t *testing.T) {
 		})
 		require.NoError(t, err)
 
-		assert.ElementsMatch(t, []string{"bar_test.go"}, files)
+		assert.Equal(t, map[string]struct{}{"bar_test.go": {}}, files)
 	})
 
 	t.Run("excludes pattern", func(t *testing.T) {
@@ -25,6 +25,6 @@ func TestGlob(t *testing.T) {
 		})
 		require.NoError(t, err)
 
-		assert.ElementsMatch(t, []string{"main.go", "bar.go"}, files)
+		assert.Equal(t, map[string]struct{}{"main.go": {}, "bar.go": {}}, files)
 	})
 }