Expression evaluator fixes

pkg/common/executor.go

@@ -136,7 +136,6 @@ func (e Executor) Then(then Executor) Executor {
 			case Warning:
 				log.Warning(err.Error())
 			default:
-				log.Debugf("%+v", err)
 				return err
 			}
 		}

pkg/exprparser/interpreter.go

@@ -274,7 +274,7 @@ func (impl *interperterImpl) evaluateNot(notNode *actionlint.NotOpNode) (interfa
 		return nil, err
 	}
 
-	return !impl.isTruthy(reflect.ValueOf(operand)), nil
+	return !IsTruthy(operand), nil
 }
 
 func (impl *interperterImpl) evaluateCompare(compareNode *actionlint.CompareOpNode) (interface{}, error) {
@@ -434,7 +434,8 @@ func (impl *interperterImpl) compareNumber(left float64, right float64, kind act
 	}
 }
 
-func (impl *interperterImpl) isTruthy(value reflect.Value) bool {
+func IsTruthy(input interface{}) bool {
+	value := reflect.ValueOf(input)
 	switch value.Kind() {
 	case reflect.Bool:
 		return value.Bool()
@@ -452,10 +453,7 @@ func (impl *interperterImpl) isTruthy(value reflect.Value) bool {
 
 		return value.Float() != 0
 
-	case reflect.Map:
-		return true
-
-	case reflect.Slice:
+	case reflect.Map, reflect.Slice:
 		return true
 
 	default:
@@ -503,14 +501,14 @@ func (impl *interperterImpl) evaluateLogicalCompare(compareNode *actionlint.Logi
 
 	switch compareNode.Kind {
 	case actionlint.LogicalOpNodeKindAnd:
-		if impl.isTruthy(leftValue) {
+		if IsTruthy(left) {
 			return impl.getSafeValue(rightValue), nil
 		}
 
 		return impl.getSafeValue(leftValue), nil
 
 	case actionlint.LogicalOpNodeKindOr:
-		if impl.isTruthy(leftValue) {
+		if IsTruthy(left) {
 			return impl.getSafeValue(leftValue), nil
 		}
 

pkg/runner/expression.go

@@ -2,7 +2,6 @@ package runner
 
 import (
 	"fmt"
-	"math"
 	"regexp"
 	"strings"
 
@@ -128,7 +127,9 @@ type expressionEvaluator struct {
 }
 
 func (ee expressionEvaluator) evaluate(in string, isIfExpression bool) (interface{}, error) {
+	log.Debugf("evaluating expression '%s'", in)
 	evaluated, err := ee.interpreter.Evaluate(in, isIfExpression)
+	log.Debugf("expression '%s' evaluated to '%t'", in, evaluated)
 	return evaluated, err
 }
 
@@ -141,9 +142,6 @@ func (ee expressionEvaluator) evaluateScalarYamlNode(node *yaml.Node) error {
 		return nil
 	}
 	expr, _ := rewriteSubExpression(in, false)
-	if in != expr {
-		log.Debugf("expression '%s' rewritten to '%s'", in, expr)
-	}
 	res, err := ee.evaluate(expr, false)
 	if err != nil {
 		return err
@@ -214,18 +212,12 @@ func (ee expressionEvaluator) Interpolate(in string) string {
 	}
 
 	expr, _ := rewriteSubExpression(in, true)
-	if in != expr {
-		log.Debugf("expression '%s' rewritten to '%s'", in, expr)
-	}
-
 	evaluated, err := ee.evaluate(expr, false)
 	if err != nil {
 		log.Errorf("Unable to interpolate expression '%s': %s", expr, err)
 		return ""
 	}
 
-	log.Debugf("expression '%s' evaluated to '%s'", expr, evaluated)
-
 	value, ok := evaluated.(string)
 	if !ok {
 		panic(fmt.Sprintf("Expression %s did not evaluate to a string", expr))
@@ -237,37 +229,13 @@ func (ee expressionEvaluator) Interpolate(in string) string {
 // EvalBool evaluates an expression against given evaluator
 func EvalBool(evaluator ExpressionEvaluator, expr string) (bool, error) {
 	nextExpr, _ := rewriteSubExpression(expr, false)
-	if expr != nextExpr {
-		log.Debugf("expression '%s' rewritten to '%s'", expr, nextExpr)
-	}
 
 	evaluated, err := evaluator.evaluate(nextExpr, true)
 	if err != nil {
 		return false, err
 	}
 
-	var result bool
-
-	switch t := evaluated.(type) {
-	case bool:
-		result = t
-	case string:
-		result = t != ""
-	case int:
-		result = t != 0
-	case float64:
-		if math.IsNaN(t) {
-			result = false
-		} else {
-			result = t != 0
-		}
-	default:
-		return false, fmt.Errorf("Unable to map return type to boolean for '%s'", expr)
-	}
-
-	log.Debugf("expression '%s' evaluated to '%t'", nextExpr, result)
-
-	return result, nil
+	return exprparser.IsTruthy(evaluated), nil
 }
 
 func escapeFormatString(in string) string {
@@ -334,5 +302,9 @@ func rewriteSubExpression(in string, forceFormat bool) (string, error) {
 		return in, nil
 	}
 
-	return fmt.Sprintf("format('%s', %s)", strings.ReplaceAll(formatOut, "'", "''"), strings.Join(results, ", ")), nil
+	out := fmt.Sprintf("format('%s', %s)", strings.ReplaceAll(formatOut, "'", "''"), strings.Join(results, ", "))
+	if in != out {
+		log.Debugf("expression '%s' rewritten to '%s'", in, out)
+	}
+	return out, nil
 }

pkg/runner/run_context.go

@@ -275,7 +275,18 @@ func (rc *RunContext) steps() []*model.Step {
 
 // Executor returns a pipeline executor for all the steps in the job
 func (rc *RunContext) Executor() common.Executor {
-	return newJobExecutor(rc).If(rc.isEnabled)
+	return func(ctx context.Context) error {
+		isEnabled, err := rc.isEnabled(ctx)
+		if err != nil {
+			return err
+		}
+
+		if isEnabled {
+			return newJobExecutor(rc)(ctx)
+		}
+
+		return nil
+	}
 }
 
 // Executor returns a pipeline executor for all the steps in the job
@@ -405,17 +416,16 @@ func (rc *RunContext) hostname() string {
 	return *hostname
 }
 
-func (rc *RunContext) isEnabled(ctx context.Context) bool {
+func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) {
 	job := rc.Run.Job()
 	l := common.Logger(ctx)
 	runJob, err := EvalBool(rc.ExprEval, job.If.Value)
 	if err != nil {
-		common.Logger(ctx).Errorf("  \u274C  Error in if: expression - %s", job.Name)
-		return false
+		return false, fmt.Errorf("  \u274C  Error in if-expression: \"if: %s\" (%s)", job.If.Value, err)
 	}
 	if !runJob {
 		l.Debugf("Skipping job '%s' due to '%s'", job.Name, job.If.Value)
-		return false
+		return false, nil
 	}
 
 	img := rc.platformImage()
@@ -428,9 +438,9 @@ func (rc *RunContext) isEnabled(ctx context.Context) bool {
 			platformName := rc.ExprEval.Interpolate(runnerLabel)
 			l.Infof("\U0001F6A7  Skipping unsupported platform -- Try running with `-P %+v=...`", platformName)
 		}
-		return false
+		return false, nil
 	}
-	return true
+	return true, nil
 }
 
 func mergeMaps(maps ...map[string]string) map[string]string {

pkg/runner/step_context.go

@@ -171,13 +171,7 @@ func (sc *StepContext) interpolateEnv(exprEval ExpressionEvaluator) {
 func (sc *StepContext) isEnabled(ctx context.Context) (bool, error) {
 	runStep, err := EvalBool(sc.NewExpressionEvaluator(), sc.Step.If.Value)
 	if err != nil {
-		common.Logger(ctx).Errorf("  \u274C  Error in if: expression - %s", sc.Step)
-		exprEval, err := sc.setupEnv(ctx)
-		if err != nil {
-			return false, err
-		}
-		sc.RunContext.ExprEval = exprEval
-		return false, err
+		return false, fmt.Errorf("  \u274C  Error in if-expression: \"if: %s\" (%s)", sc.Step.If.Value, err)
 	}
 
 	return runStep, nil

pkg/runner/testdata/issue-597/spelling.yaml

@@ -1,10 +1,10 @@
 name: issue-597
 on: push
-  
+
 
 jobs:
   my_first_job:
-    
+
     runs-on: ubuntu-latest
     steps:
       - name: My first false step
@@ -14,7 +14,7 @@ jobs:
           ref: refs/pull/${{github.event.pull_request.number}}/merge
           fetch-depth: 5
       - name: My first true step
-        if: ${{"endsWith('Hello world', 'ld')"}}
+        if: ${{endsWith('Hello world', 'ld')}}
         uses: actions/hello-world-javascript-action@main
         with:
           who-to-greet: "Renst the Octocat"

pkg/runner/testdata/uses-composite/push.yml

@@ -17,7 +17,7 @@ jobs:
         secret_input: ${{secrets.test_input_optional || 'NO SUCH SECRET'}}
       env:
         secret_input: ${{secrets.test_input_optional || 'NO SUCH SECRET'}}
-    - if: steps.composite.outputs.test_output != "test_output_value"
+    - if: steps.composite.outputs.test_output != 'test_output_value'
       run: |
         echo "steps.composite.outputs.test_output=${{ steps.composite.outputs.test_output }}"
         exit 1
@@ -38,7 +38,7 @@ jobs:
         test_input_optional_with_default_overriden: 'test_input_optional_with_default_overriden'
         test_input_required_with_default_overriden: 'test_input_required_with_default_overriden'
 
-    - if: steps.composite2.outputs.test_output != "test_output_value"
+    - if: steps.composite2.outputs.test_output != 'test_output_value'
       run: |
         echo "steps.composite.outputs.test_output=${{ steps.composite2.outputs.test_output }}"
         exit 1