Fix for macro call tracking with optional list elements (#712)

* Fix for macro call tracking with optional list elements
* Additional coverage tests

interpreter/attributes_test.go

@@ -24,7 +24,6 @@ import (
 	"github.com/google/cel-go/checker/decls"
 	"github.com/google/cel-go/common"
 	"github.com/google/cel-go/common/containers"
-	"github.com/google/cel-go/common/operators"
 	"github.com/google/cel-go/common/types"
 	"github.com/google/cel-go/common/types/ref"
 	"github.com/google/cel-go/parser"
@@ -383,7 +382,6 @@ func TestAttributesOptional(t *testing.T) {
 		t.Fatalf("")
 	}
 	attrs := NewAttributeFactory(cont, reg, reg)
-
 	tests := []struct {
 		varName  string
 		quals    []any
@@ -860,7 +858,7 @@ func TestAttributeStateTracking(t *testing.T) {
 	var tests = []struct {
 		expr  string
 		env   []*exprpb.Decl
-		in    map[string]any
+		in    any
 		out   ref.Val
 		attrs []*AttributePattern
 		state map[int64]any
@@ -1083,14 +1081,14 @@ func TestAttributeStateTracking(t *testing.T) {
 				decls.NewVar("a", decls.NewMapType(decls.String, decls.Dyn)),
 				decls.NewVar("m", decls.NewMapType(decls.Bool, decls.String)),
 			},
-			in: map[string]any{
-				"a": map[string]any{},
-				"m": map[bool]string{true: "world"},
-			},
-			out: types.Unknown{5},
-			attrs: []*AttributePattern{
+			in: partialActivation(
+				map[string]any{
+					"a": map[string]any{},
+					"m": map[bool]string{true: "world"},
+				},
 				NewAttributePattern("a").QualString("b"),
-			},
+			),
+			out: types.Unknown{5},
 		},
 	}
 	for _, test := range tests {
@@ -1123,9 +1121,16 @@ func TestAttributeStateTracking(t *testing.T) {
 			if len(errors.GetErrors()) != 0 {
 				t.Fatalf(errors.ToDisplayString())
 			}
-			attrs := NewAttributeFactory(cont, reg, reg)
-			if tc.attrs != nil {
+			in, err := NewActivation(tc.in)
+			if err != nil {
+				t.Fatalf("NewActivation(%v) failed: %v", tc.in, err)
+			}
+			var attrs AttributeFactory
+			_, isPartial := in.(PartialActivation)
+			if isPartial {
 				attrs = NewPartialAttributeFactory(cont, reg, reg)
+			} else {
+				attrs = NewAttributeFactory(cont, reg, reg)
 			}
 			interp := NewStandardInterpreter(cont, reg, reg, attrs)
 			// Show that program planning will now produce an error.
@@ -1137,7 +1142,6 @@ func TestAttributeStateTracking(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			in, _ := NewPartialActivation(tc.in, tc.attrs...)
 			out := i.Eval(in)
 			if types.IsUnknown(tc.out) && types.IsUnknown(out) {
 				if !reflect.DeepEqual(tc.out, out) {
@@ -1261,15 +1265,3 @@ func findField(t testing.TB, reg ref.TypeRegistry, typeName, field string) *ref.
 	}
 	return ft
 }
-
-func optionalSignatures() []*exprpb.Decl {
-	return []*exprpb.Decl{
-		decls.NewFunction(operators.OptIndex,
-			decls.NewParameterizedOverload("map_optindex_optional_value", []*exprpb.Type{
-				decls.NewMapType(decls.NewTypeParamType("K"), decls.NewTypeParamType("V")),
-				decls.NewTypeParamType("K")},
-				decls.NewOptionalType(decls.NewTypeParamType("V")),
-				[]string{"K", "V"},
-			)),
-	}
-}

interpreter/interpretable.go

@@ -162,9 +162,6 @@ func (q *testOnlyQualifier) Qualify(vars Activation, obj any) (any, error) {
 }
 
 // QualifyIfPresent returns whether the target field in the test-only expression is present.
-//
-// This method should never be called as the has() macro and optional syntax are incompatible
-// when used on the same field.
 func (q *testOnlyQualifier) QualifyIfPresent(vars Activation, obj any, presenceOnly bool) (any, bool, error) {
 	// Only ever test for presence.
 	return q.ConstantQualifier.QualifyIfPresent(vars, obj, true)
@@ -922,7 +919,7 @@ func (e *evalWatchConstQual) QualifyIfPresent(vars Activation, obj any, presence
 		val = types.WrapErr(err)
 	} else if out != nil {
 		val = e.adapter.NativeToValue(out)
-	} else if out == nil && presenceOnly {
+	} else if presenceOnly {
 		val = types.Bool(present)
 	}
 	if present || presenceOnly {
@@ -965,7 +962,7 @@ func (e *evalWatchQual) QualifyIfPresent(vars Activation, obj any, presenceOnly
 		val = types.WrapErr(err)
 	} else if out != nil {
 		val = e.adapter.NativeToValue(out)
-	} else if out == nil && presenceOnly {
+	} else if presenceOnly {
 		val = types.Bool(present)
 	}
 	if present || presenceOnly {

interpreter/prune_test.go

@@ -17,14 +17,19 @@ package interpreter
 import (
 	"testing"
 
+	"github.com/google/cel-go/checker/decls"
 	"github.com/google/cel-go/common"
 	"github.com/google/cel-go/common/containers"
+	"github.com/google/cel-go/common/operators"
 	"github.com/google/cel-go/common/types"
 	"github.com/google/cel-go/common/types/ref"
 	"github.com/google/cel-go/interpreter/functions"
 	"github.com/google/cel-go/parser"
 	"github.com/google/cel-go/test"
-	"github.com/google/cel-go/test/proto3pb"
+
+	proto3pb "github.com/google/cel-go/test/proto3pb"
+
+	exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
 )
 
 type testInfo struct {
@@ -387,6 +392,41 @@ var testCases = []testInfo{
 		expr: `[has(a.b), has(c.d)].exists(x, x == true)`,
 		out:  `[false, has(c.d)].exists(x, x == true)`,
 	},
+	{
+		in: partialActivation(map[string]any{
+			"a": map[string]any{},
+		}, "c"),
+		expr: `[has(a.b), has(c.d)].exists(x, x == true)`,
+		out:  `[false, has(c.d)].exists(x, x == true)`,
+	},
+	{
+		in: partialActivation(map[string]any{
+			"a": map[string]string{},
+		}),
+		expr: `[?a[?0], a.b]`,
+		out:  `[a.b]`,
+	},
+	{
+		in: partialActivation(map[string]any{
+			"a": map[string]string{},
+		}, "a"),
+		expr: `[?a[?0], a.b].exists(x, x == true)`,
+		out:  `[?a[?0], a.b].exists(x, x == true)`,
+	},
+	{
+		in: partialActivation(map[string]any{
+			"a": map[string]string{},
+		}),
+		expr: `[?a[?0], a.b].exists(x, x == true)`,
+		out:  `[a.b].exists(x, x == true)`,
+	},
+	{
+		in: partialActivation(map[string]any{
+			"a": map[string]string{},
+		}),
+		expr: `[a[0], a.b].exists(x, x == true)`,
+		out:  `[a[0], a.b].exists(x, x == true)`,
+	},
 }
 
 func TestPrune(t *testing.T) {
@@ -483,3 +523,15 @@ func optionalFunctions() []*functions.Overload {
 		},
 	}
 }
+
+func optionalSignatures() []*exprpb.Decl {
+	return []*exprpb.Decl{
+		decls.NewFunction(operators.OptIndex,
+			decls.NewParameterizedOverload("map_optindex_optional_value", []*exprpb.Type{
+				decls.NewMapType(decls.NewTypeParamType("K"), decls.NewTypeParamType("V")),
+				decls.NewTypeParamType("K")},
+				decls.NewOptionalType(decls.NewTypeParamType("V")),
+				[]string{"K", "V"},
+			)),
+	}
+}

parser/helper.go

@@ -259,7 +259,8 @@ func (p *parserHelper) buildMacroCallArg(expr *exprpb.Expr) *exprpb.Expr {
 			Id: expr.GetId(),
 			ExprKind: &exprpb.Expr_ListExpr{
 				ListExpr: &exprpb.Expr_CreateList{
-					Elements: macroListArgs,
+					Elements:        macroListArgs,
+					OptionalIndices: listExpr.GetOptionalIndices(),
 				},
 			},
 		}

parser/unparser_test.go

@@ -151,6 +151,26 @@ func TestUnparse(t *testing.T) {
 			in:                 `[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)`,
 			requiresMacroCalls: true,
 		},
+		{
+			name:               "comp_chained_opt",
+			in:                 `[?a, b[?0], c].map(x, x >= 2, x * 4).filter(x, x <= 10)`,
+			requiresMacroCalls: true,
+		},
+		{
+			name:               "comp_map_opt",
+			in:                 `{?a: b[?0]}.map(k, x >= 2, x * 4)`,
+			requiresMacroCalls: true,
+		},
+		{
+			name:               "comp_map_opt",
+			in:                 `{a: has(b.c)}.exists(k, k != "")`,
+			requiresMacroCalls: true,
+		},
+		{
+			name:               "comp_nested",
+			in:                 `{a: [1, 2].all(i > 0)}.exists(k, k != "")`,
+			requiresMacroCalls: true,
+		},
 
 		// These expressions will not be wrapped because they haven't met the
 		// conditions required by the provided unparser options