fix(parser): revert pr 16041 and 16754 (#16996)

* fix(parser): revert pr 16041 and 16754

Fix for https://github.com/prometheus/prometheus/issues/16053
turned out to be too complicated and lead to more errors than it solved.
See https://github.com/prometheus/prometheus/pull/16875.

* add TODOs to wrong end positions
* restore test and fix initialization bug

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

promql/engine_test.go

@@ -2896,7 +2896,7 @@ func TestPreprocessAndWrapWithStepInvariantExpr(t *testing.T) {
 							},
 							PosRange: posrange.PositionRange{
 								Start: 29,
-								End:   51,
+								End:   52, // TODO(krajorama): this should be 51. https://github.com/prometheus/prometheus/issues/16053
 							},
 						},
 					},

promql/parser/generated_parser.y

@@ -244,23 +244,9 @@ expr            :
  */
 
 aggregate_expr  : aggregate_op aggregate_modifier function_call_body
-                        {
+                        { $$ = yylex.(*parser).newAggregateExpr($1, $2, $3) }
-                        // Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
-                        // like 'sum (some_metric) by test'
-                        if len(yylex.(*parser).closingParens) > 1 {
-                                yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
-                        }
-                        $$ = yylex.(*parser).newAggregateExpr($1, $2, $3)
-                        }
                 | aggregate_op function_call_body aggregate_modifier
-                        {
+                        { $$ = yylex.(*parser).newAggregateExpr($1, $3, $2) }
-                        // Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
-                        // like 'sum by test (some_metric)'
-                        if len(yylex.(*parser).closingParens) > 1 {
-                                yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
-                        }
-                        $$ = yylex.(*parser).newAggregateExpr($1, $3, $2)
-                        }
                 | aggregate_op function_call_body
                         { $$ = yylex.(*parser).newAggregateExpr($1, &AggregateExpr{}, $2) }
                 | aggregate_op error
@@ -414,10 +400,9 @@ function_call   : IDENTIFIER function_call_body
                                 Args: $2.(Expressions),
                                 PosRange: posrange.PositionRange{
                                         Start: $1.Pos,
-                                        End:   yylex.(*parser).closingParens[0],
+                                        End:   yylex.(*parser).lastClosing,
                                 },
                         }
-                        yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
                         }
                 ;
 
@@ -443,10 +428,7 @@ function_call_args: function_call_args COMMA expr
  */
 
 paren_expr      : LEFT_PAREN expr RIGHT_PAREN
-                        {
+                        { $$ = &ParenExpr{Expr: $2.(Expr), PosRange: mergeRanges(&$1, &$3)} }
-                        $$ = &ParenExpr{Expr: $2.(Expr), PosRange: mergeRanges(&$1, &$3)}
-                        yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
-                        }
                 ;
 
 /*

promql/parser/generated_parser.y.go

@@ -1122,21 +1122,11 @@ yydefault:
 	case 21:
 		yyDollar = yyS[yypt-3 : yypt+1]
 		{
-			// Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
-			// like 'sum (some_metric) by test'
-			if len(yylex.(*parser).closingParens) > 1 {
-				yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
-			}
 			yyVAL.node = yylex.(*parser).newAggregateExpr(yyDollar[1].item, yyDollar[2].node, yyDollar[3].node)
 		}
 	case 22:
 		yyDollar = yyS[yypt-3 : yypt+1]
 		{
-			// Need to consume the position of the first RIGHT_PAREN. It might not exist on garbage input
-			// like 'sum by test (some_metric)'
-			if len(yylex.(*parser).closingParens) > 1 {
-				yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
-			}
 			yyVAL.node = yylex.(*parser).newAggregateExpr(yyDollar[1].item, yyDollar[3].node, yyDollar[2].node)
 		}
 	case 23:
@@ -1364,10 +1354,9 @@ yydefault:
 				Args: yyDollar[2].node.(Expressions),
 				PosRange: posrange.PositionRange{
 					Start: yyDollar[1].item.Pos,
-					End:   yylex.(*parser).closingParens[0],
+					End:   yylex.(*parser).lastClosing,
 				},
 			}
-			yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
 		}
 	case 63:
 		yyDollar = yyS[yypt-3 : yypt+1]
@@ -1399,7 +1388,6 @@ yydefault:
 		yyDollar = yyS[yypt-3 : yypt+1]
 		{
 			yyVAL.node = &ParenExpr{Expr: yyDollar[2].node.(Expr), PosRange: mergeRanges(&yyDollar[1].item, &yyDollar[3].item)}
-			yylex.(*parser).closingParens = yylex.(*parser).closingParens[1:]
 		}
 	case 69:
 		yyDollar = yyS[yypt-1 : yypt+1]

promql/parser/parse.go

@@ -59,13 +59,6 @@ type parser struct {
 	// Everytime an Item is lexed that could be the end
 	// of certain expressions its end position is stored here.
 	lastClosing posrange.Pos
-	// Keep track of closing parentheses in addition, because sometimes the
-	// parser needs to read past a closing parenthesis to find the end of an
-	// expression, e.g. reading ony '(sum(foo)' cannot tell the end of the
-	// aggregation expression, since it could continue with either
-	// '(sum(foo))' or '(sum(foo) by (bar))' by which time we set lastClosing
-	// to the last paren.
-	closingParens []posrange.Pos
 
 	yyParser yyParserImpl
 
@@ -89,7 +82,7 @@ func NewParser(input string, opts ...Opt) *parser { //nolint:revive // unexporte
 	p.injecting = false
 	p.parseErrors = nil
 	p.generatedParserResult = nil
-	p.closingParens = make([]posrange.Pos, 0)
+	p.lastClosing = posrange.Pos(0)
 
 	// Clear lexer struct before reusing.
 	p.lex = Lexer{
@@ -179,11 +172,6 @@ func EnrichParseError(err error, enrich func(parseErr *ParseErr)) {
 func ParseExpr(input string) (expr Expr, err error) {
 	p := NewParser(input)
 	defer p.Close()
-
-	if len(p.closingParens) > 0 {
-		return nil, fmt.Errorf("internal parser error, not all closing parens consumed: %v", p.closingParens)
-	}
-
 	return p.ParseExpr()
 }
 
@@ -387,10 +375,7 @@ func (p *parser) Lex(lval *yySymType) int {
 	case EOF:
 		lval.item.Typ = EOF
 		p.InjectItem(0)
-	case RIGHT_PAREN:
+	case RIGHT_BRACE, RIGHT_PAREN, RIGHT_BRACKET, DURATION, NUMBER:
-		p.closingParens = append(p.closingParens, lval.item.Pos+posrange.Pos(len(lval.item.Val)))
-		fallthrough
-	case RIGHT_BRACE, RIGHT_BRACKET, DURATION, NUMBER:
 		p.lastClosing = lval.item.Pos + posrange.Pos(len(lval.item.Val))
 	}
 
@@ -451,16 +436,10 @@ func (p *parser) newAggregateExpr(op Item, modifier, args Node) (ret *AggregateE
 	ret = modifier.(*AggregateExpr)
 	arguments := args.(Expressions)
 
-	if len(p.closingParens) == 0 {
-		// Prevents invalid array accesses.
-		// The error is already captured by the parser.
-		return
-	}
 	ret.PosRange = posrange.PositionRange{
 		Start: op.Pos,
-		End:   p.closingParens[0],
+		End:   p.lastClosing,
 	}
-	p.closingParens = p.closingParens[1:]
 
 	ret.Op = op.Typ
 

promql/parser/parse_test.go

@@ -4776,7 +4776,7 @@ var testExpr = []struct {
 						End:   8,
 					},
 				},
-				PosRange: posrange.PositionRange{Start: 1, End: 9},
+				PosRange: posrange.PositionRange{Start: 1, End: 10}, // TODO(krajorama): this should be 9. https://github.com/prometheus/prometheus/issues/16053
 			},
 			PosRange: posrange.PositionRange{Start: 0, End: 10},
 		},