From 5cbddef39a3e17d54596e010d607959658c70fe5 Mon Sep 17 00:00:00 2001
From: Roger Peppe <rogpeppe@gmail.com>
Date: Wed, 10 Jul 2024 12:03:36 +0100
Subject: [PATCH] internal/buildattr: stop at package clause and support
 parentheses

The existing logic was not behaving as expected: it did not stop when it
encountered a `package` clause.

Also, the expressions did not support parentheses, which seems like an
oversight: it's much less useful to have `&&` and `!` operators if it's
not possible to group subexpressions.

Fixes #3228.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ifed4419f4c89b6d52389c06aa960563e3f045a87
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1197530
Reviewed-by: Paul Jolly <paul@myitcv.io>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
---
 cmd/cue/cmd/testdata/script/issue3228.txtar | 31 +++++++++++++++++++++
 internal/buildattr/buildattr.go             | 13 +++++----
 internal/buildattr/buildattr_test.go        | 20 ++++---------
 3 files changed, 45 insertions(+), 19 deletions(-)
 create mode 100644 cmd/cue/cmd/testdata/script/issue3228.txtar

diff --git a/cmd/cue/cmd/testdata/script/issue3228.txtar b/cmd/cue/cmd/testdata/script/issue3228.txtar
new file mode 100644
index 00000000000..599e0f28ef9
--- /dev/null
+++ b/cmd/cue/cmd/testdata/script/issue3228.txtar
@@ -0,0 +1,31 @@
+# This verifies that evaluation of build attributes is not order-dependent.
+
+! exec cue export
+cmp stderr error
+
+exec cue export -t C
+cmp stdout out
+
+exec cue export -t A -t B
+cmp stdout out
+
+exec cue export -t A -t C
+cmp stdout out
+
+exec cue export -t B -t C
+cmp stdout out
+
+exec cue export -t A -t B -t C
+cmp stdout out
+-- example.cue --
+@if( A && B || C )
+package example
+
+x: 1
+-- out --
+{
+    "x": 1
+}
+-- error --
+build constraints exclude all CUE files in .:
+    script-issue3228/example.cue: @if( A && B || C ) did not match
diff --git a/internal/buildattr/buildattr.go b/internal/buildattr/buildattr.go
index 86c90e9c1fc..5df67f28c83 100644
--- a/internal/buildattr/buildattr.go
+++ b/internal/buildattr/buildattr.go
@@ -17,7 +17,7 @@ import (
 func ShouldBuildFile(f *ast.File, tagIsSet func(key string) bool) (bool, *ast.Attribute, errors.Error) {
 	a, err := getBuildAttr(f)
 	if err != nil {
-		return false, nil, err
+		return false, a, err
 	}
 	if a == nil {
 		return true, nil, nil
@@ -50,12 +50,12 @@ func getBuildAttr(f *ast.File) (*ast.Attribute, errors.Error) {
 				err := errors.Newf(d.Pos(), "multiple @if attributes")
 				err = errors.Append(err,
 					errors.Newf(a.Pos(), "previous declaration here"))
-				return nil, err
+				return a, err
 			}
 			a = x
 
 		case *ast.Package:
-			break
+			return a, nil
 		}
 	}
 	return a, nil
@@ -66,6 +66,9 @@ func shouldInclude(expr ast.Expr, tagIsSet func(key string) bool) (bool, errors.
 	case *ast.Ident:
 		return tagIsSet(x.Name), nil
 
+	case *ast.ParenExpr:
+		return shouldInclude(x.X, tagIsSet)
+
 	case *ast.BinaryExpr:
 		switch x.Op {
 		case token.LAND, token.LOR:
@@ -83,12 +86,12 @@ func shouldInclude(expr ast.Expr, tagIsSet func(key string) bool) (bool, errors.
 			return a || b, nil
 
 		default:
-			return false, errors.Newf(token.NoPos, "invalid operator %v", x.Op)
+			return false, errors.Newf(token.NoPos, "invalid operator %v in build attribute", x.Op)
 		}
 
 	case *ast.UnaryExpr:
 		if x.Op != token.NOT {
-			return false, errors.Newf(token.NoPos, "invalid operator %v", x.Op)
+			return false, errors.Newf(token.NoPos, "invalid operator %v in build attribute", x.Op)
 		}
 		v, err := shouldInclude(x.X, tagIsSet)
 		if err != nil {
diff --git a/internal/buildattr/buildattr_test.go b/internal/buildattr/buildattr_test.go
index 43bafb86c3b..f234e75457d 100644
--- a/internal/buildattr/buildattr_test.go
+++ b/internal/buildattr/buildattr_test.go
@@ -50,20 +50,17 @@ package something
 
 @if(foo)
 `,
-	wantOK:       false,
-	wantTagCalls: map[string]bool{"foo": true},
-	wantAttr:     "@if(foo)",
+	wantOK: true,
 }, {
 	testName: "InvalidExpr",
 	syntax: `
-
 @if(foo + bar)
 
 package something
 `,
 	wantOK:   false,
 	wantAttr: "@if(foo + bar)",
-	wantError: `invalid operator \+
+	wantError: `invalid operator \+ in build attribute
 `,
 }, {
 	testName: "MultipleIfAttributes",
@@ -74,7 +71,8 @@ package something
 
 package something
 `,
-	wantOK: false,
+	wantOK:   false,
+	wantAttr: "@if(foo)",
 	wantError: `previous declaration here:
     testfile.cue:3:1
 multiple @if attributes:
@@ -91,12 +89,8 @@ package something
 @if(bar)
 `,
 	wantOK:       false,
+	wantAttr:     "@if(foo)",
 	wantTagCalls: map[string]bool{"foo": true},
-	wantError: `previous declaration here:
-    testfile.cue:3:1
-multiple @if attributes:
-    testfile.cue:7:1
-`,
 }, {
 	testName: "And#0",
 	syntax: `
@@ -242,9 +236,7 @@ package something
 	tags: map[string]bool{
 		"baz": true,
 	},
-	wantOK: false,
-	wantError: `invalid type \*ast.ParenExpr in build attribute
-`,
+	wantOK: true,
 	wantTagCalls: map[string]bool{
 		"foo": true,
 		"bar": true,