From 4b786c02d6458728a3e196c8a3f404a56c725958 Mon Sep 17 00:00:00 2001
From: James Luck <james.luck@thetradedesk.com>
Date: Wed, 24 Nov 2021 08:28:23 +1100
Subject: [PATCH] Re-arranging some of the `Try` statements to ensure we get
 more meaningful error messages when parsing `Expr` (this is the main entry
 point for almost anyone trying to parse expressions).

---
 src/PromQL.Parser/Parser.cs              | 14 +++++++-------
 tests/PromQL.Parser.Tests/ParserTests.cs |  9 +++++++++
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/PromQL.Parser/Parser.cs b/src/PromQL.Parser/Parser.cs
index 33975ae..862d4df 100644
--- a/src/PromQL.Parser/Parser.cs
+++ b/src/PromQL.Parser/Parser.cs
@@ -218,10 +218,10 @@ from e in Parse.Ref(() => Expr).Between(Token.EqualTo(PromToken.LEFT_PAREN), Tok
             select new ParenExpression(e);
 
         public static TokenListParser<PromToken, Expr[]> FunctionArgs = Parse.Ref(() => Expr).ManyDelimitedBy(Token.EqualTo(PromToken.COMMA))
-            .Between(Token.EqualTo(PromToken.LEFT_PAREN), Token.EqualTo(PromToken.RIGHT_PAREN));
+            .Between(Token.EqualTo(PromToken.LEFT_PAREN).Try(), Token.EqualTo(PromToken.RIGHT_PAREN));
         
         public static TokenListParser<PromToken, FunctionCall> FunctionCall =
-            from id in Token.EqualTo(PromToken.IDENTIFIER).Where(x => Functions.Names.Contains(x.ToStringValue()))
+            from id in Token.EqualTo(PromToken.IDENTIFIER).Where(x => Functions.Names.Contains(x.ToStringValue())).Try()
             from args in FunctionArgs
             select new FunctionCall(id.ToStringValue(), args.ToImmutableArray());
 
@@ -308,13 +308,13 @@ from rhs in Parse.Ref(() => Expr)
             select new BinaryExpr(lhs, rhs, BinaryOperatorMap[op.Kind], vm);
 
         public static TokenListParser<PromToken, (bool without, ImmutableArray<string> labels)> AggregateModifier =
-            from kind in Token.EqualTo(PromToken.BY)
-                .Or(Token.EqualTo(PromToken.WITHOUT))
+            from kind in Token.EqualTo(PromToken.BY).Try()
+                .Or(Token.EqualTo(PromToken.WITHOUT).Try())
             from labels in GroupingLabels
             select (kind.Kind == PromToken.WITHOUT, labels);
 
         public static TokenListParser<PromToken, AggregateExpr> AggregateExpr =
-            from op in Token.EqualTo(PromToken.AGGREGATE_OP).Where(x => Operators.Aggregates.Contains(x.Span.ToStringValue()))
+            from op in Token.EqualTo(PromToken.AGGREGATE_OP).Where(x => Operators.Aggregates.Contains(x.Span.ToStringValue())).Try()
             from argsAndMod in (
                 from args in FunctionArgs
                 from mod in AggregateModifier.OptionalOrDefault((without: false, labels: ImmutableArray<string>.Empty))
@@ -332,8 +332,8 @@ from args in FunctionArgs
              from head in OneOf(
                  // TODO can we optimize order here?
                  Parse.Ref(() => ParenExpression).Cast<PromToken, ParenExpression, Expr>(),
-                 Parse.Ref(() => AggregateExpr).Cast<PromToken, AggregateExpr, Expr>().Try(),
-                 Parse.Ref(() => FunctionCall).Cast<PromToken, FunctionCall, Expr>().Try(),
+                 Parse.Ref(() => AggregateExpr).Cast<PromToken, AggregateExpr, Expr>(),
+                 Parse.Ref(() => FunctionCall).Cast<PromToken, FunctionCall, Expr>(),
                  Number.Cast<PromToken, NumberLiteral, Expr>().Try(),
                  Parse.Ref(() => UnaryExpr).Cast<PromToken, UnaryExpr, Expr>(),
                  MatrixSelector.Cast<PromToken, MatrixSelector, Expr>().Try(),
diff --git a/tests/PromQL.Parser.Tests/ParserTests.cs b/tests/PromQL.Parser.Tests/ParserTests.cs
index 10f33ed..d717229 100644
--- a/tests/PromQL.Parser.Tests/ParserTests.cs
+++ b/tests/PromQL.Parser.Tests/ParserTests.cs
@@ -400,6 +400,15 @@ public void Invalid_Expr()
             Assert.Throws<ParseException>(() => Parse(Parser.Expr, "!= 'blah'"));
         }
         
+        [Test]
+        public void Invalid_DurationExpr()
+        {
+            // Prior to this test, a very unhelpful error message would be returned (parsing would complain about an invalid
+            // opening parenthesis)
+            Assert.Throws<ParseException>(() => Parse(Parser.Expr, "sum(my_metric[window])")).Message
+                .Should().Be("Syntax error (line 1, column 15): unexpected identifier `window`, expected duration.");
+        }
+
         [Test]
         public void ParseExpression_With_Comments()
         {