Merge branch 'check-if-cond' (fix #272)

docs/checks.md

@@ -37,6 +37,7 @@ List of checks:
 - [ID naming convention](#id-naming-convention)
 - [Contexts and special functions availability](#ctx-spfunc-availability)
 - [Deprecated workflow commands](#check-deprecated-workflow-commands)
+- [Conditions always evaluated to true at `if:`](#if-cond-always-true)
 
 Note that actionlint focuses on catching mistakes in workflow files. If you want some general code style checks, please consider
 using a general YAML checker like [yamllint][].
@@ -2537,6 +2538,76 @@ GitHub deprecated the following workflow commands.
 actionlint detects these commands are used in `run:` and reports them as errors suggesting alternatives. See
 [the official document][workflow-commands-doc] for the comprehensive list of workflow commands to know the usage.
 
+<a name="if-cond-always-true"></a>
+## Conditions always evaluated to true at `if:`
+
+Example input:
+
+```yaml
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'Commit is pushed'
+        # OK
+        if: ${{ github.event_name == 'push' }}
+      - run: echo 'Commit is pushed'
+        # ERROR: It is always evaluated to true
+        if: |
+          ${{ github.event_name == 'push' }}
+      - run: echo 'Commit is pushed'
+        # ERROR: It is always evaluated to true
+        if: "${{ github.event_name == 'push' }} "
+      - run: echo 'Commit is pushed to main'
+        # OK
+        if: github.event_name == 'push' && github.ref_name == 'main'
+      - run: echo 'Commit is pushed to main'
+        # ERROR: It is always evaluated to true
+        if: ${{ github.event_name == 'push' }} && ${{ github.ref_name == 'main' }}
+```
+
+Output:
+
+```
+test.yaml:12:13: if: condition "${{ github.event_name == 'push' }}\n" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+   |
+12 |         if: |
+   |             ^
+test.yaml:16:13: if: condition "${{ github.event_name == 'push' }} " is always evaluated to true because extra characters are around ${{ }} [if-cond]
+   |
+16 |         if: "${{ github.event_name == 'push' }} "
+   |             ^~~~
+test.yaml:22:13: if: condition "${{ github.event_name == 'push' }} && ${{ github.ref_name == 'main' }}" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+   |
+22 |         if: ${{ github.event_name == 'push' }} && ${{ github.ref_name == 'main' }}
+   |             ^~~
+```
+
+[Playground](https://rhysd.github.io/actionlint#eJy1j0sKwjAUReddxaVIM6oLCHTkQqTVVxsxSWlenNTu3aT+KoJWwVF4nMO5xBqJ1rsmSfa2cjIBmBzHF+i8cbkNgq+8YZ8fyshG5Jhad7GAPJoStGksxMpqrRjKjVnaiqsEqFpi0ffYKW58taQjGV6bUhOKAiLaAsPwffN0v/CXfvo5inROFmyhS2We8+/KWXbDHdUPOI38sDjjP2F4Yr2OB+cMjO6qVQ==)
+
+Evaluation of `${{ }}` at `if:` condition is tricky. When the expression in `${{ }}` is evaluated to boolean value and there is
+no extra characters around the `${{ }}`, the condition is evaluated to the boolean value. Otherwise the condition is treated as
+string hence it is **always** evaluated to `true`.
+
+It means that multi-line string must not be used at `if:` condition (`if: |`) because the condition is always evaluated to true.
+Multi-line string inserts newline character at end of each line.
+
+```yaml
+if: |
+  ${{ false }}
+```
+
+is equivalent to
+
+```yaml
+if: "${{ false }}\n"
+```
+
+actionlint checks all `if:` conditions in workflow and reports error when some condition is always evaluated to true due to extra
+characters around `${{ }}`.
+
 ---
 
 [Installation](install.md) | [Usage](usage.md) | [Configuration](config.md) | [Go API](api.md) | [References](reference.md)

linter.go

@@ -503,6 +503,7 @@ func (l *Linter) check(
 			NewRuleWorkflowCall(path, localReusableWorkflows),
 			NewRuleExpression(localActions, localReusableWorkflows),
 			NewRuleDeprecatedCommands(),
+			NewRuleIfCond(),
 		}
 		if l.shellcheck != "" {
 			r, err := NewRuleShellcheck(l.shellcheck, proc)

rule_if_cond.go

@@ -0,0 +1,50 @@
+package actionlint
+
+import (
+	"strings"
+)
+
+// RuleIfCond is a rule to check if: conditions.
+type RuleIfCond struct {
+	RuleBase
+}
+
+// NewRuleIfCond creates new RuleIfCond instance.
+func NewRuleIfCond() *RuleIfCond {
+	return &RuleIfCond{
+		RuleBase: RuleBase{
+			name: "if-cond",
+			desc: "Checks for if: conditions which are always true/false",
+		},
+	}
+}
+
+// VisitStep is callback when visiting Step node.
+func (rule *RuleIfCond) VisitStep(n *Step) error {
+	rule.checkIfCond(n.If)
+	return nil
+}
+
+// VisitJobPre is callback when visiting Job node before visiting its children.
+func (rule *RuleIfCond) VisitJobPre(n *Job) error {
+	rule.checkIfCond(n.If)
+	return nil
+}
+
+func (rule *RuleIfCond) checkIfCond(n *String) {
+	if n == nil {
+		return
+	}
+	if !n.ContainsExpression() {
+		return
+	}
+	// Check number of ${{ }} for conditions like `${{ false }} || ${{ true }}` which are always evaluated to true
+	if strings.HasPrefix(n.Value, "${{") && strings.HasSuffix(n.Value, "}}") && strings.Count(n.Value, "${{") == 1 {
+		return
+	}
+	rule.errorf(
+		n.Pos,
+		"if: condition %q is always evaluated to true because extra characters are around ${{ }}",
+		n.Value,
+	)
+}

rule_if_cond_test.go

@@ -0,0 +1,66 @@
+package actionlint
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestRuleIfCond(t *testing.T) {
+	tests := []struct {
+		cond  string
+		valid bool
+	}{
+		{"", true},
+		{"true", true},
+		{"true || false", true},
+		{"${{ false }}", true},
+		{"${{ false }}\n", false},
+		{"${{ false }} ", false},
+		{" ${{ false }}", false},
+		{"${{ true }} && ${{ true }}", false},
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("%q at step", tc.cond), func(t *testing.T) {
+			var s Step
+			if len(tc.cond) > 0 {
+				s.If = &String{Value: tc.cond, Pos: &Pos{}}
+			}
+
+			r := NewRuleIfCond()
+			if err := r.VisitStep(&s); err != nil {
+				t.Fatal(err)
+			}
+
+			errs := r.Errs()
+			if tc.valid && len(errs) > 0 {
+				t.Fatalf("wanted no error but have %q", errs)
+			}
+			if !tc.valid && len(errs) != 1 {
+				t.Fatalf("wanted one error but have %q", errs)
+			}
+		})
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("%q at job", tc.cond), func(t *testing.T) {
+			var j Job
+			if len(tc.cond) > 0 {
+				j.If = &String{Value: tc.cond, Pos: &Pos{}}
+			}
+
+			r := NewRuleIfCond()
+			if err := r.VisitJobPre(&j); err != nil {
+				t.Fatal(err)
+			}
+
+			errs := r.Errs()
+			if tc.valid && len(errs) > 0 {
+				t.Fatalf("wanted no error but have %q", errs)
+			}
+			if !tc.valid && len(errs) != 1 {
+				t.Fatalf("wanted one error but have %q", errs)
+			}
+		})
+	}
+}

testdata/err/if_cond_always_evaluated_to_true.out

@@ -0,0 +1,7 @@
+test.yaml:12:13: if: condition "${{ false }}\n" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:19:13: if: condition "${{ false }} " is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:22:13: if: condition " ${{ false }}" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:47:13: if: condition "${{ false }} && ${{ false }}" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:49:9: if: condition "# ERROR: True\n${{ false }}\n" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:57:9: if: condition " ${{ false }}" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:63:9: if: condition "${{ false }} && ${{ false }}" is always evaluated to true because extra characters are around ${{ }} [if-cond]

testdata/err/if_cond_always_evaluated_to_true.yaml

@@ -0,0 +1,66 @@
+on: push
+
+jobs:
+  test-steps:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 1
+        # False
+        if: ${{ false }}
+      - run: echo 2
+        # ERROR: True
+        if: |
+          ${{ false }}
+      - run: echo 3
+        # False
+        if: '${{ false }}'
+      - run: echo 4
+        # ERROR: True
+        if: '${{ false }} '
+      - run: echo 5
+        # ERROR: True
+        if: ' ${{ false }}'
+      - run: echo 6
+        # False
+        if: 'false'
+      - run: echo 7
+        # False
+        if: 'false '
+      - run: echo 8
+        # False
+        if: |
+          false
+      - run: echo 9
+        # False
+        if: ' false'
+      - run: echo 10
+        # True
+        if: false || true
+      - run: echo 11
+        # False
+        if: true && false
+      - run: echo 12
+        # False
+        if: ${{ true && false }}
+      - run: echo 13
+        # ERROR: True
+        if: ${{ false }} && ${{ false }}
+  test-cond-1:
+    if: |
+      # ERROR: True
+      ${{ false }}
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 1
+  test-cond-2:
+    # ERROR: True
+    if: ' ${{ false }}'
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 1
+  test-cond-3:
+    # ERROR: True
+    if: ${{ false }} && ${{ false }}
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 1

testdata/examples/if_cond_always_true.out

@@ -0,0 +1,3 @@
+test.yaml:12:13: if: condition "${{ github.event_name == 'push' }}\n" is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:16:13: if: condition "${{ github.event_name == 'push' }} " is always evaluated to true because extra characters are around ${{ }} [if-cond]
+test.yaml:22:13: if: condition "${{ github.event_name == 'push' }} && ${{ github.ref_name == 'main' }}" is always evaluated to true because extra characters are around ${{ }} [if-cond]

testdata/examples/if_cond_always_true.yaml

@@ -0,0 +1,22 @@
+on: push
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'Commit is pushed'
+        # OK
+        if: ${{ github.event_name == 'push' }}
+      - run: echo 'Commit is pushed'
+        # ERROR: It is always evaluated to true
+        if: |
+          ${{ github.event_name == 'push' }}
+      - run: echo 'Commit is pushed'
+        # ERROR: It is always evaluated to true
+        if: "${{ github.event_name == 'push' }} "
+      - run: echo 'Commit is pushed to main'
+        # OK
+        if: github.event_name == 'push' && github.ref_name == 'main'
+      - run: echo 'Commit is pushed to main'
+        # ERROR: It is always evaluated to true
+        if: ${{ github.event_name == 'push' }} && ${{ github.ref_name == 'main' }}

testdata/format/test.sarif

@@ -129,6 +129,21 @@
               },
               "helpUri": "https://github.com/rhysd/actionlint/blob/main/docs/checks.md"
             },
+            {
+              "id": "if-cond",
+              "name": "IfCond",
+              "defaultConfiguration": {
+                "level": "error"
+              },
+              "properties": {
+                "description": "Checks for if: conditions which are always true/false",
+                "queryURI": "https://github.com/rhysd/actionlint/blob/main/docs/checks.md"
+              },
+              "fullDescription": {
+                "text": "Checks for if: conditions which are always true/false"
+              },
+              "helpUri": "https://github.com/rhysd/actionlint/blob/main/docs/checks.md"
+            },
             {
               "id": "job-needs",
               "name": "JobNeeds",