Changed and() and or() to stop at the argument that decides the result

libpromises/evalfunction.c

@@ -693,30 +693,105 @@ static FnCallResult FnCallGetACLs(
 
 /*********************************************************************/
 
-static FnCallResult FnCallAnd(EvalContext *ctx,
-                              ARG_UNUSED const Policy *policy,
-                              ARG_UNUSED const FnCall *fp,
-                              const Rlist *finalargs)
+/**
+ * Check whether #rval has unresolved variables, expanding scalars without
+ * calling any functions. Function call arguments are checked recursively.
+ */
+static bool RvalHasUnresolvedVariables(const EvalContext *ctx, const char *ns, Rval rval)
 {
+    switch (rval.type)
+    {
+    case RVAL_TYPE_FNCALL:
+        for (const Rlist *rp = RvalFnCallValue(rval)->args; rp != NULL; rp = rp->next)
+        {
+            if (RvalHasUnresolvedVariables(ctx, ns, rp->val))
+            {
+                return true;
+            }
+        }
+        return false;
+    case RVAL_TYPE_SCALAR:
+    {
+        Rlist *expanded = NULL;
+        RlistAppendRval(&expanded, ExpandPrivateRval(ctx, ns, NULL, rval.item, rval.type));
+        const bool unresolved = RlistIsUnresolved(expanded);
+        RlistDestroy(expanded);
+        return unresolved;
+    }
+    default:
+        return false;
+    }
+}
 
-    for (const Rlist *arg = finalargs; arg; arg = arg->next)
+/**
+ * and() and or(): evaluate the arguments left to right and stop at the first
+ * one that decides the result, without evaluating the rest.
+ */
+static FnCallResult AndOr(EvalContext *ctx,
+                          const Policy *policy,
+                          const FnCall *fp,
+                          const Rlist *finalargs,
+                          bool is_and)
+{
+    assert(fp != NULL);
+
+    /* Like any function, skip for now if any argument has unresolved
+     * variables. Checked without calling any functions. */
+    const Bundle *caller_bundle = fp->caller ? PromiseGetBundle(fp->caller) : NULL;
+    const char *caller_ns = caller_bundle ? caller_bundle->ns : NULL;
+    for (const Rlist *arg = finalargs; arg != NULL; arg = arg->next)
     {
-        SyntaxTypeMatch err = CheckConstraintTypeMatch(fp->name, arg->val, CF_DATA_TYPE_STRING, "", 1);
-        if (err != SYNTAX_TYPE_MATCH_OK && err != SYNTAX_TYPE_MATCH_ERROR_UNEXPANDED)
+        if (RvalHasUnresolvedVariables(ctx, caller_ns, arg->val))
         {
-            FatalError(ctx, "Function '%s', %s", fp->name, SyntaxTypeMatchToString(err));
+            return FnFailure();
         }
     }
 
-    for (const Rlist *arg = finalargs; arg; arg = arg->next)
+    /* Then expand the arguments one at a time, stopping at the first one
+     * that decides the result. */
+    for (const Rlist *arg = finalargs; arg != NULL; arg = arg->next)
     {
-        if (!IsDefinedClass(ctx, RlistScalarValue(arg)))
+        /* Expand just this argument, it can expand to several (e.g. a list). */
+        Rlist *single_arg = NULL;
+        RlistAppendRval(&single_arg, RvalCopy(arg->val));
+        FnCall *single_fp = FnCallNew(fp->name, single_arg);
+        single_fp->caller = fp->caller;
+        Rlist *expanded = NewExpArgs(ctx, policy, single_fp, NULL);
+        FnCallDestroy(single_fp);
+
+        /* A nested function call that failed stays unexpanded. */
+        if (RlistIsUnresolved(expanded))
         {
-            return FnReturnContext(false);
+            RlistDestroy(expanded);
+            return FnFailure();
+        }
+
+        for (const Rlist *rp = expanded; rp != NULL; rp = rp->next)
+        {
+            SyntaxTypeMatch err = CheckConstraintTypeMatch(fp->name, rp->val, CF_DATA_TYPE_STRING, "", 1);
+            if (err != SYNTAX_TYPE_MATCH_OK && err != SYNTAX_TYPE_MATCH_ERROR_UNEXPANDED)
+            {
+                FatalError(ctx, "Function '%s', %s", fp->name, SyntaxTypeMatchToString(err));
+            }
+
+            if (IsDefinedClass(ctx, RlistScalarValue(rp)) != is_and)
+            {
+                RlistDestroy(expanded);
+                return FnReturnContext(!is_and);
+            }
         }
+        RlistDestroy(expanded);
     }
 
-    return FnReturnContext(true);
+    return FnReturnContext(is_and);
+}
+
+static FnCallResult FnCallAnd(EvalContext *ctx,
+                              const Policy *policy,
+                              const FnCall *fp,
+                              const Rlist *finalargs)
+{
+    return AndOr(ctx, policy, fp, finalargs, true);
 }
 
 /*******************************************************************/
@@ -5497,6 +5572,7 @@ static FnCallResult FilterInternal(EvalContext *ctx,
                                    bool invert,
                                    long max)
 {
+    assert(fp != NULL);
     Regex *rx = NULL;
     if (do_regex)
     {
@@ -5553,8 +5629,11 @@ static FnCallResult FilterInternal(EvalContext *ctx,
                 RlistAppendScalar(&returnlist, val);
                 match_count++;
 
+                /* One match decides the result of these. */
                 if (strcmp(fp->name, "some")     == 0 ||
-                    strcmp(fp->name, "regarray") == 0)
+                    strcmp(fp->name, "regarray") == 0 ||
+                    strcmp(fp->name, "reglist")  == 0 ||
+                    strcmp(fp->name, "none")     == 0)
                 {
                     free(val);
                     break;
@@ -7351,33 +7430,11 @@ static FnCallResult FnCallOn(ARG_UNUSED EvalContext *ctx, ARG_UNUSED const Polic
 /*********************************************************************/
 
 static FnCallResult FnCallOr(EvalContext *ctx,
-                             ARG_UNUSED const Policy *policy,
-                             ARG_UNUSED const FnCall *fp,
+                             const Policy *policy,
+                             const FnCall *fp,
                              const Rlist *finalargs)
 {
-    char id[CF_BUFSIZE];
-
-    snprintf(id, CF_BUFSIZE, "built-in FnCall or-arg");
-
-/* We need to check all the arguments, ArgTemplate does not check varadic functions */
-    for (const Rlist *arg = finalargs; arg; arg = arg->next)
-    {
-        SyntaxTypeMatch err = CheckConstraintTypeMatch(id, arg->val, CF_DATA_TYPE_STRING, "", 1);
-        if (err != SYNTAX_TYPE_MATCH_OK && err != SYNTAX_TYPE_MATCH_ERROR_UNEXPANDED)
-        {
-            FatalError(ctx, "in %s: %s", id, SyntaxTypeMatchToString(err));
-        }
-    }
-
-    for (const Rlist *arg = finalargs; arg; arg = arg->next)
-    {
-        if (IsDefinedClass(ctx, RlistScalarValue(arg)))
-        {
-            return FnReturnContext(true);
-        }
-    }
-
-    return FnReturnContext(false);
+    return AndOr(ctx, policy, fp, finalargs, false);
 }
 
 /*********************************************************************/
@@ -11848,7 +11905,7 @@ const FnCallType CF_FNCALL_TYPES[] =
     FnCallTypeNew("ago", CF_DATA_TYPE_INT, AGO_ARGS, &FnCallAgoDate, "Convert a time relative to now to an integer system representation",
                   FNCALL_OPTION_NONE, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, DEFAULT_ARGC),
     FnCallTypeNew("and", CF_DATA_TYPE_CONTEXT, AND_ARGS, &FnCallAnd, "Calculate whether all arguments evaluate to true",
-                  FNCALL_OPTION_VARARG, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, ARGC(0, -1)),
+                  FNCALL_OPTION_VARARG|FNCALL_OPTION_DELAYED_EVALUATION, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, ARGC(0, -1)),
     FnCallTypeNew("basename", CF_DATA_TYPE_STRING, BASENAME_ARGS, &FnCallBasename, "Retrieves the basename of a filename.",
                   FNCALL_OPTION_VARARG, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, ARGC(1, 2)),
     FnCallTypeNew("bundlesmatching", CF_DATA_TYPE_STRING_LIST, BUNDLESMATCHING_ARGS, &FnCallBundlesMatching, "Find all the bundles that match a regular expression and tags.",
@@ -12046,7 +12103,7 @@ const FnCallType CF_FNCALL_TYPES[] =
     FnCallTypeNew("on", CF_DATA_TYPE_INT, DATE_ARGS, &FnCallOn, "Convert an exact date/time to an integer system representation",
                   FNCALL_OPTION_NONE, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, DEFAULT_ARGC),
     FnCallTypeNew("or", CF_DATA_TYPE_CONTEXT, OR_ARGS, &FnCallOr, "Calculate whether any argument evaluates to true",
-                  FNCALL_OPTION_VARARG, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, ARGC(0, -1)),
+                  FNCALL_OPTION_VARARG|FNCALL_OPTION_DELAYED_EVALUATION, FNCALL_CATEGORY_DATA, SYNTAX_STATUS_NORMAL, ARGC(0, -1)),
     FnCallTypeNew("packagesmatching", CF_DATA_TYPE_CONTAINER, PACKAGESMATCHING_ARGS, &FnCallPackagesMatching, "List the installed packages (\"name,version,arch,manager\") matching regex arg1=name,arg2=version,arg3=arch,arg4=method",
                   FNCALL_OPTION_NONE, FNCALL_CATEGORY_SYSTEM, SYNTAX_STATUS_NORMAL, DEFAULT_ARGC),
     FnCallTypeNew("packageupdatesmatching", CF_DATA_TYPE_CONTAINER, PACKAGESMATCHING_ARGS, &FnCallPackagesMatching, "List the available patches (\"name,version,arch,manager\") matching regex arg1=name,arg2=version,arg3=arch,arg4=method.  Enterprise only.",

tests/acceptance/01_vars/02_functions/and_or_short_circuit.cf

@@ -0,0 +1,124 @@
+######################################################
+#
+# Test that and() and or() stop evaluating arguments early
+#
+#####################################################
+body common control
+{
+  inputs => { "../../default.sub.cf" };
+  bundlesequence => { default("$(this.promise_filename)") };
+  version => "1.0";
+}
+
+#######################################################
+bundle agent init
+{
+  vars:
+    "markers" slist => { "and_ran", "or_ran", "and_skipped", "or_skipped" };
+
+  files:
+    "$(G.testfile).$(markers)" delete => tidy;
+}
+
+#######################################################
+bundle agent test
+{
+  meta:
+    "description" -> { "CFE-4272", "CFE-4751" }
+      string => "Test that and() and or() stop at the argument that decides the result";
+
+    # The sub policy runs touch with returnszero(..., "noshell").
+    "test_skip_unsupported" string => "windows";
+
+  methods:
+    "namespaced" usebundle => ns_and_or:local;
+
+  commands:
+    # The and() and or() calls are in a separate agent run because functions
+    # in classes promises are evaluated before the init bundle cleans up.
+    "$(sys.cf_agent) -Kf $(this.promise_filename).sub" contain => in_shell;
+}
+
+#######################################################
+bundle agent check
+{
+  vars:
+    "false_in_middle" slist => { "any", "!any", "any" };
+    "true_in_middle" slist => { "!any", "any", "!any" };
+
+  classes:
+    # A function returning a list expands to several arguments, each of which
+    # can decide the result. (@(list) is not expanded here, it is a single
+    # class expression.)
+    "and_list_false" expression => and("any", getvalues("false_in_middle"));
+    "or_list_true" expression => or("!any", getvalues("true_in_middle"));
+
+    # A failing call after the deciding argument isn't made, so it no longer
+    # causes the whole or() to be skipped.
+    "or_true_before_failing_call"
+      expression => or(
+        "any",
+        islessthan(filestat("$(G.testfile).does_not_exist", "size"), "1")
+      );
+
+    # Unresolved variables still skip evaluation of and() and or() (so not()
+    # of it is skipped too), even after an argument that decides the result.
+    "and_false_before_unresolved"
+      expression => not(and("!any", "$(no_such_bundle.no_such_var)"));
+
+    "and_false_before_unresolved_in_fncall"
+      expression => not(
+        and("!any", strcmp("$(no_such_bundle.no_such_var)", "x"))
+      );
+
+    "or_true_before_unresolved"
+      expression => or("any", "$(no_such_bundle.no_such_var)");
+
+    "or_true_before_unresolved_in_fncall"
+      expression => or("any", strcmp("$(no_such_bundle.no_such_var)", "x"));
+
+    "ok"
+      and => {
+        fileexists("$(G.testfile).and_ran"),
+        fileexists("$(G.testfile).or_ran"),
+        not(fileexists("$(G.testfile).and_skipped")),
+        not(fileexists("$(G.testfile).or_skipped")),
+        "!and_false_before_unresolved",
+        "!and_false_before_unresolved_in_fncall",
+        "!or_true_before_unresolved",
+        "!or_true_before_unresolved_in_fncall",
+        "!and_list_false",
+        "or_list_true",
+        "or_true_before_failing_call",
+        "ns_and_or:and_local_var",
+        "ns_and_or:or_local_var",
+      };
+
+  reports:
+    ok::
+      "$(this.promise_filename) Pass";
+
+    !ok::
+      "$(this.promise_filename) FAIL";
+}
+
+body file control
+{
+  namespace => "ns_and_or";
+}
+
+# Bundle-qualified variables in and()/or() resolve in the caller's namespace.
+bundle agent local
+{
+  vars:
+    "v" string => "x";
+
+  classes:
+    "and_local_var"
+      expression => and("any", strcmp("$(local.v)", "x")),
+      scope => "namespace";
+
+    "or_local_var"
+      expression => or("!any", strcmp("$(local.v)", "x")),
+      scope => "namespace";
+}

tests/acceptance/01_vars/02_functions/and_or_short_circuit.cf.sub

@@ -0,0 +1,22 @@
+body common control
+{
+  inputs => { "../../default.sub.cf" };
+}
+
+bundle agent main
+{
+  classes:
+    # The first argument doesn't decide the result, so the second one must be
+    # evaluated.
+    "and_ran"
+      expression => and("any", returnszero("$(G.touch) $(G.testfile).and_ran", "noshell"));
+    "or_ran"
+      expression => or("!any", returnszero("$(G.touch) $(G.testfile).or_ran", "noshell"));
+
+    # The first argument decides the result, so the second one must not be
+    # evaluated.
+    "and_skipped"
+      expression => and("!any", returnszero("$(G.touch) $(G.testfile).and_skipped", "noshell"));
+    "or_skipped"
+      expression => or("any", returnszero("$(G.touch) $(G.testfile).or_skipped", "noshell"));
+}