Partial evaluation should leave erroneous policy in place

[patjakdev]

Currently the partial evaluator folds errors into error("error text goes here") … Instead it should just leave the original expression, so as to maintain that partial evaluation results in valid policy.

(This will take some care, the ifThenElse, and, or, are heavily tied to how it works now. Likely the main change will be in tryPartial to no longer bubble up the error, but to do the same as the tryFold implementation does, and just return the folded node.)

[patjakdev]

Hacky POC:

diff --git a/internal/eval/evalers.go b/internal/eval/evalers.go
index e4a42b4..60033e5 100644
--- a/internal/eval/evalers.go
+++ b/internal/eval/evalers.go
@@ -1184,11 +1184,6 @@ func (n *ipIsInRangeEval) Eval(env *Env) (types.Value, error) {
 // extensionEval
 
 func newExtensionEval(name types.Path, args []Evaler) Evaler {
-	// error is not part of the cedar spec, so leaving it out of the list of extensions that can be parsed, etc
-	if name == partialErrorName && len(args) == 1 {
-		return newPartialErrorEval(args[0])
-	}
-
 	if i, ok := extensions.ExtMap[name]; ok {
 		if i.Args != len(args) {
 			return newErrorEval(fmt.Errorf("%w: %s takes %d parameter(s)", errArity, name, i.Args))
diff --git a/internal/eval/partial.go b/internal/eval/partial.go
index 9d9b5c2..deb6429 100644
--- a/internal/eval/partial.go
+++ b/internal/eval/partial.go
@@ -186,7 +186,7 @@ func tryPartial(env *Env, nodes []ast.IsNode,
 		eval := mkEval(values)
 		v, err := eval.Eval(env)
 		if err != nil {
-			return nil, err
+			return mkNode(nodes), nil
 		}
 		if IsVariable(v) {
 			return mkNode(nodes), errVariable
@@ -422,10 +422,8 @@ func partialIfThenElse(env *Env, v ast.NodeTypeIfThenElse) (ast.IsNode, error) {
 		then = extError(thenErr)
 	}
 	else_, elseErr := partial(env, v.Else)
-	if errors.Is(elseErr, errIgnore) {
+	if elseErr != nil && !errors.Is(elseErr, errVariable) {
 		return nil, elseErr
-	} else if elseErr != nil && !errors.Is(elseErr, errVariable) {
-		else_ = extError(elseErr)
 	}
 	return ast.NodeTypeIfThenElse{If: if_, Then: then, Else: else_}, nil
 }
@@ -448,10 +446,8 @@ func partialAnd(env *Env, v ast.NodeTypeAnd) (ast.IsNode, error) {
 		)
 	}
 	right, rightErr := partial(env, v.Right)
-	if errors.Is(rightErr, errIgnore) {
+	if rightErr != nil && !errors.Is(rightErr, errVariable) {
 		return nil, rightErr
-	} else if rightErr != nil && !errors.Is(rightErr, errVariable) {
-		right = extError(rightErr)
 	}
 	return ast.NodeTypeAnd{BinaryNode: ast.BinaryNode{Left: left, Right: right}}, nil
 }
@@ -474,10 +470,8 @@ func partialOr(env *Env, v ast.NodeTypeOr) (ast.IsNode, error) {
 		)
 	}
 	right, rightErr := partial(env, v.Right)
-	if errors.Is(rightErr, errIgnore) {
+	if rightErr != nil && !errors.Is(rightErr, errVariable) {
 		return nil, rightErr
-	} else if rightErr != nil && !errors.Is(rightErr, errVariable) {
-		right = extError(rightErr)
 	}
 	return ast.NodeTypeOr{BinaryNode: ast.BinaryNode{Left: left, Right: right}}, nil
 }
@@ -520,22 +514,3 @@ func (n *partialHasEval) Eval(env *Env) (types.Value, error) {
 	}
 	return types.Boolean(ok), nil
 }
-
-// partialErrorEval
-type partialErrorEval struct {
-	arg Evaler
-}
-
-func newPartialErrorEval(err Evaler) *partialErrorEval {
-	return &partialErrorEval{
-		arg: err,
-	}
-}
-
-func (n *partialErrorEval) Eval(env *Env) (types.Value, error) {
-	v, err := evalString(n.arg, env)
-	if err != nil {
-		return nil, err
-	}
-	return zeroValue(), errors.New(string(v))
-}
diff --git a/internal/eval/partial_test.go b/internal/eval/partial_test.go
index e1d75e3..705e06c 100644
--- a/internal/eval/partial_test.go
+++ b/internal/eval/partial_test.go
@@ -519,6 +519,8 @@ func TestPartialAnd(t *testing.T) {
 	falseN := ast.False()
 	valueN := ast.String("test")
 	keepN := ast.Context()
+	trueExp := ast.Long(42).GreaterThan(ast.Long(41))
+	falseExp := ast.Long(42).GreaterThan(ast.Long(43))
 	_, _, _, _, _ = errorN, trueN, falseN, valueN, keepN
 
 	tests := []struct {
@@ -530,9 +532,9 @@ func TestPartialAnd(t *testing.T) {
 
 		{"andTrueTrue", trueN.And(trueN), ast.True(), testutil.OK},
 		{"andTrueFalse", trueN.And(falseN), ast.False(), testutil.OK},
-		{"andTrueValue", trueN.And(valueN), nil, testutil.Error},
+		{"andTrueValue", trueN.And(valueN), trueN.And(valueN), testutil.OK},
 		{"andTrueKeep", trueN.And(keepN), trueN.And(keepN), testutil.OK},
-		{"andTrueError", trueN.And(errorN), nil, testutil.Error},
+		{"andTrueError", trueN.And(errorN), trueN.And(errorN), testutil.OK},
 
 		{"andFalseTrue", falseN.And(trueN), ast.False(), testutil.OK},
 		{"andFalseFalse", falseN.And(falseN), ast.False(), testutil.OK},
@@ -550,13 +552,13 @@ func TestPartialAnd(t *testing.T) {
 		{"andKeepFalse", keepN.And(falseN), keepN.And(falseN), testutil.OK},
 		{"andKeepValue", keepN.And(valueN), keepN.And(valueN), testutil.OK},
 		{"andKeepKeep", keepN.And(keepN), keepN.And(keepN), testutil.OK},
-		{"andKeepError", keepN.And(errorN), keepN.And(ast.ExtensionCall(partialErrorName, ast.String("type error: expected long, got string"))), testutil.OK},
+		{"andKeepError", keepN.And(errorN), keepN.And(errorN), testutil.OK},
 
-		{"andErrorTrue", errorN.And(trueN), nil, testutil.Error},
-		{"andErrorFalse", errorN.And(falseN), nil, testutil.Error},
-		{"andErrorValue", errorN.And(valueN), nil, testutil.Error},
-		{"andErrorKeep", errorN.And(keepN), nil, testutil.Error},
-		{"andErrorError", errorN.And(errorN), nil, testutil.Error},
+		{"andErrorTrue", errorN.And(trueN), errorN.And(trueN), testutil.OK},
+		{"andErrorFalse", errorN.And(falseN), errorN.And(falseN), testutil.OK},
+		{"andErrorValue", errorN.And(valueN), errorN.And(valueN), testutil.OK},
+		{"andErrorKeep", errorN.And(keepN), errorN.And(keepN), testutil.OK},
+		{"andErrorError", errorN.And(errorN), errorN.And(errorN), testutil.OK},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -1215,39 +1217,6 @@ func TestPartialPanic(t *testing.T) {
 	})
 }
 
-func TestPartialErrorEval(t *testing.T) {
-	t.Parallel()
-	tests := []struct {
-		name string
-		env  Env
-		in   Evaler
-		out  types.Value
-		err  func(testutil.TB, error)
-	}{
-		{"happy",
-			Env{},
-			newPartialErrorEval(newLiteralEval(types.String("err"))),
-			nil, testutil.Error,
-		},
-
-		{"err",
-			Env{},
-			newPartialErrorEval(newLiteralEval(types.Long(42))),
-			nil, testutil.Error,
-		},
-	}
-
-	for _, tt := range tests {
-		tt := tt
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			out, err := tt.in.Eval(InitEnv(&tt.env))
-			testutil.Equals(t, out, tt.out)
-			tt.err(t, err)
-		})
-	}
-}
-
 func TestPartialHasEval(t *testing.T) {
 	t.Parallel()
 	tests := []struct {