Merge branch 'issue-22' (fix #22)

rhysd · Aug 4, 2021 · 0cc6c67 · 0cc6c67
2 parents 13053ab + adbaeb6
commit 0cc6c67
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 159 deletions.
diff --git a/expr_parser.go b/expr_parser.go
@@ -18,10 +18,9 @@ func errorAtToken(t *Token, msg string) *ExprError {
 // ExprParser is a parser for expression syntax. To know the details, see
 // https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions
 type ExprParser struct {
-	cur         *Token
-	lexer       *ExprLexer
-	err         *ExprError
-	sawOperator *Token
+	cur   *Token
+	lexer *ExprLexer
+	err   *ExprError
 }
 
 // NewExprParser creates new ExprParser instance.
@@ -67,13 +66,6 @@ func (p *ExprParser) peek() *Token {
 	return p.cur
 }
 
-// Note: List of operators: https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#operators
-func (p *ExprParser) seeingOperator(tok *Token) {
-	if p.sawOperator == nil {
-		p.sawOperator = tok
-	}
-}
-
 func (p *ExprParser) parseIdent() ExprNode {
 	ident := p.next() // eat ident
 	switch p.peek().Kind {
@@ -128,7 +120,7 @@ func (p *ExprParser) parseIdent() ExprNode {
 }
 
 func (p *ExprParser) parseNestedExpr() ExprNode {
-	lparen := p.next() // eat '('
+	p.next() // eat '('
 
 	nested := p.parseLogicalOr()
 	if nested == nil {
@@ -142,7 +134,6 @@ func (p *ExprParser) parseNestedExpr() ExprNode {
 		return nil
 	}
 
-	p.seeingOperator(lparen)
 	return nested
 }
 
@@ -216,7 +207,7 @@ func (p *ExprParser) parsePostfixOp() ExprNode {
 	for {
 		switch p.peek().Kind {
 		case TokenKindDot:
-			dot := p.next() // eat '.'
+			p.next() // eat '.'
 			switch p.peek().Kind {
 			case TokenKindStar:
 				p.next() // eat '*'
@@ -232,9 +223,8 @@ func (p *ExprParser) parsePostfixOp() ExprNode {
 				)
 				return nil
 			}
-			p.seeingOperator(dot)
 		case TokenKindLeftBracket:
-			lbra := p.next() // eat '['
+			p.next() // eat '['
 			idx := p.parseLogicalOr()
 			if idx == nil {
 				return nil
@@ -245,7 +235,6 @@ func (p *ExprParser) parsePostfixOp() ExprNode {
 				return nil
 			}
 			p.next() // eat ']'
-			p.seeingOperator(lbra)
 		default:
 			return ret
 		}
@@ -257,13 +246,12 @@ func (p *ExprParser) parsePrefixOp() ExprNode {
 	if t.Kind != TokenKindNot {
 		return p.parsePostfixOp()
 	}
-	not := p.next() // eat '!' token
+	p.next() // eat '!' token
 
 	o := p.parsePostfixOp()
 	if o == nil {
 		return nil
 	}
-	p.seeingOperator(not)
 
 	return &NotOpNode{o, t}
 }
@@ -291,15 +279,13 @@ func (p *ExprParser) parseCompareBinOp() ExprNode {
 	default:
 		return l
 	}
-	op := p.next() // eat the operator token
+	p.next() // eat the operator token
 
 	r := p.parseCompareBinOp()
 	if r == nil {
 		return nil
 	}
 
-	p.seeingOperator(op)
-
 	return &CompareOpNode{k, l, r}
 }
 
@@ -311,12 +297,11 @@ func (p *ExprParser) parseLogicalAnd() ExprNode {
 	if p.peek().Kind != TokenKindAnd {
 		return l
 	}
-	and := p.next() // eat &&
+	p.next() // eat &&
 	r := p.parseLogicalAnd()
 	if r == nil {
 		return nil
 	}
-	p.seeingOperator(and)
 	return &LogicalOpNode{LogicalOpNodeKindAnd, l, r}
 }
 
@@ -328,12 +313,11 @@ func (p *ExprParser) parseLogicalOr() ExprNode {
 	if p.peek().Kind != TokenKindOr {
 		return l
 	}
-	or := p.next() // eat ||
+	p.next() // eat ||
 	r := p.parseLogicalOr()
 	if r == nil {
 		return nil
 	}
-	p.seeingOperator(or)
 	return &LogicalOpNode{LogicalOpNodeKindOr, l, r}
 }
 
@@ -351,7 +335,6 @@ func (p *ExprParser) Parse(l *ExprLexer) (ExprNode, *ExprError) {
 	p.err = nil
 	p.lexer = l
 	p.cur = l.Next()
-	p.sawOperator = nil
 
 	root := p.parseLogicalOr()
 	if err := p.Err(); err != nil {

diff --git a/expr_parser_test.go b/expr_parser_test.go
@@ -893,120 +893,6 @@ func TestParseExpressionTokenPosition(t *testing.T) {
 	}
 }
 
-func TestParseExpressionOperatorDetection(t *testing.T) {
-	testCases := []struct {
-		what  string
-		op    string
-		input string
-	}{
-		{
-			what:  "nested expression",
-			op:    "(",
-			input: "(foo)",
-		},
-		{
-			what:  "index access",
-			op:    "[",
-			input: "arr[idx]",
-		},
-		{
-			what:  "property access",
-			op:    ".",
-			input: "github.event",
-		},
-		{
-			what:  "not",
-			op:    "!",
-			input: "!foo",
-		},
-		{
-			what:  "less",
-			op:    "<",
-			input: "a < b",
-		},
-		{
-			what:  "less equal",
-			op:    "<=",
-			input: "a <= b",
-		},
-		{
-			what:  "greater",
-			op:    ">",
-			input: "a > b",
-		},
-		{
-			what:  "greater equal",
-			op:    ">=",
-			input: "a >= b",
-		},
-		{
-			what:  "equal",
-			op:    "==",
-			input: "a == b",
-		},
-		{
-			what:  "not equal",
-			op:    "!=",
-			input: "a != b",
-		},
-		{
-			what:  "and",
-			op:    "&&",
-			input: "a && b",
-		},
-		{
-			what:  "or",
-			op:    "||",
-			input: "a || b",
-		},
-		{
-			what:  "multiple operators",
-			op:    "==",
-			input: "(a == b) || (c < d)",
-		},
-		{
-			what:  "no operator",
-			op:    "",
-			input: "true",
-		},
-		{
-			what:  "func call is not operator",
-			op:    "",
-			input: "contains(foo, 'hello')",
-		},
-		{
-			what:  "operator inside expression",
-			op:    ".",
-			input: "contains(github.repository, 'hello')",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.what, func(t *testing.T) {
-			l := NewExprLexer(tc.input + "}}")
-			p := NewExprParser()
-			if _, err := p.Parse(l); err != nil {
-				t.Fatal("Parse error:", err)
-			}
-
-			op := p.sawOperator
-			if tc.op == "" {
-				if op != nil {
-					t.Fatalf("no operator was expected but found %q in %q", op.Kind.String(), tc.input)
-				}
-			} else {
-				if op == nil {
-					t.Fatalf("operator was not found in %q", tc.input)
-				}
-
-				if op.Kind.String() != tc.op {
-					t.Fatalf("Wanted operator %q but got %q", tc.op, op.Kind.String())
-				}
-			}
-		})
-	}
-}
-
 func TestParseReturnFirstErrorOnMultipleErrors(t *testing.T) {
 	p := NewExprParser()
 	_, want := p.Parse(NewExprLexer(".}}"))

diff --git a/rule_expression.go b/rule_expression.go
@@ -422,16 +422,6 @@ func (rule *RuleExpression) checkIfCondition(str *String) {
 			return
 		}
 
-		if p.sawOperator != nil {
-			op := p.sawOperator
-			pos := convertExprLineColToPos(op.Line, op.Column, str.Pos.Line, str.Pos.Col)
-			rule.errorf(
-				pos,
-				"this expression must be contained within ${{ }} like `if: ${{ ... }}` since it contains operator %q. see https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idif for more details",
-				p.sawOperator.Kind.String(),
-			)
-		}
-
 		condTy = rule.checkSemanticsOfExprNode(expr, line, col)
 	}
 

diff --git a/testdata/examples/operator_outside_expr.out b/testdata/examples/operator_outside_expr.out
diff --git a/testdata/examples/operator_outside_expr.yaml b/testdata/examples/operator_outside_expr.yaml
diff --git a/testdata/ok/operator_outside_expr.yaml b/testdata/ok/operator_outside_expr.yaml
@@ -0,0 +1,14 @@
+on: push
+jobs:
+  test:
+    strategy:
+      matrix:
+        ver: [0, 1]
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'my repo'
+        # This was not ok until https://github.com/github/docs/pull/8786
+        if: contains(github.repository, 'rhysd')
+      - run: echo 'my repo'
+        # This was not ok until https://github.com/github/docs/pull/8786
+        if: github.repository != 'rhysd/foo' && matrix.ver > 0 || !(github.token == '')