improve messaging for improperly structured requirements (#400)

Jamiras · Jun 26, 2023 · 1c937bc · 1c937bc
1 parent ec013f8
commit 1c937bc
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 40 deletions.
diff --git a/Source/Parser/Expressions/FunctionCallExpression.cs b/Source/Parser/Expressions/FunctionCallExpression.cs
@@ -145,11 +145,12 @@ private bool Evaluate(InterpreterScope scope, bool inAssignment, out ExpressionB
                 functionDefinition.Evaluate(functionParametersScope, out result);
             }
 
+            if (result != null && result.Location.Start.Line == 0)
+                CopyLocation(result);
+
             var error = result as ErrorExpression;
             if (error != null)
             {
-                if (error.Location.Start.Line == 0)
-                    CopyLocation(error);
                 result = ErrorExpression.WrapError(error, FunctionName.Name + " call failed", FunctionName);
                 return false;
             }

diff --git a/Source/Parser/Expressions/FunctionDefinitionExpression.cs b/Source/Parser/Expressions/FunctionDefinitionExpression.cs
@@ -451,14 +451,15 @@ protected RequirementExpressionBase GetRequirementParameter(InterpreterScope sco
                     }
                 }
 
-                parseError = new ErrorExpression(name + " is not a requirement", originalParameter ?? parameter);
+                var parameterError = new ErrorExpression(name + " is not a requirement", originalParameter ?? parameter);
 
-                if (!ReferenceEquals(invalidClause, parameter))
+                if (invalidClause != null)
                 {
-                    ((ErrorExpression)parseError).InnerError =
+                    parameterError.InnerError =
                         new ErrorExpression("Cannot convert " + invalidClause.Type.ToString().ToLower() + " to requirement", invalidClause);
                 }
 
+                parseError = parameterError;
                 return null;
             }
 

diff --git a/Source/Parser/Internal/ExpressionBase.cs b/Source/Parser/Internal/ExpressionBase.cs
@@ -55,8 +55,11 @@ internal ExpressionBase MakeReadOnly()
         /// </summary>
         internal void CopyLocation(ExpressionBase target)
         {
-            if (Location.End.Line != 0 && !target.IsReadOnly)
-                target.Location = Location;
+            if (Location.End.Line != 0)
+            {
+                if (!target.IsReadOnly || target.Location.End.Line == 0)
+                    target.Location = Location;
+            }
         }
 
         /// <summary>

diff --git a/Tests/Parser/AchievementScriptInterpreterTests.cs b/Tests/Parser/AchievementScriptInterpreterTests.cs
@@ -416,7 +416,7 @@ public void TestOnce()
         public void TestOnceMalformed()
         {
             var parser = Parse("achievement(\"T\", \"D\", 5, once(byte(0x1234)) == 1)", false);
-            Assert.That(GetInnerErrorMessage(parser), Is.EqualTo("1:31 comparison is not a requirement"));
+            Assert.That(GetInnerErrorMessage(parser), Is.EqualTo("1:31 Cannot convert memoryaccessor to requirement"));
         }
 
         [Test]
@@ -834,18 +834,14 @@ public void TestFunctionCallInFunctionInExpression()
             var tokenizer = Tokenizer.CreateTokenizer(input);
             var parser = new AchievementScriptInterpreter();
             Assert.That(parser.Run(tokenizer), Is.False);
-            Assert.That(parser.ErrorMessage, Is.EqualTo("2:30 always_true has no meaning outside of a trigger clause"));
+            Assert.That(parser.ErrorMessage, Is.EqualTo(
+                "10:40 Invalid value for parameter: trigger\r\n" +
+                "- 10:40 foo call failed\r\n" +
+                "- 2:30 always_true call failed\r\n" +
+                "- 2:30 always_true has no meaning outside of a trigger clause"));
         }
         */
 
-        [Test]
-        public void TestErrorInFunctionInExpression()
-        {
-            var parser = Parse("function foo() => byte(1)\n" +
-                               "achievement(\"Title\", \"Description\", 5, once(foo()))\n", false);
-            Assert.That(GetInnerErrorMessage(parser), Is.EqualTo("2:45 comparison is not a requirement"));
-        }
-
         [Test]
         public void TestErrorInFunctionParameterLocation()
         {

diff --git a/Tests/Parser/Expressions/Trigger/BehavioralRequirementExpression_Tests.cs b/Tests/Parser/Expressions/Trigger/BehavioralRequirementExpression_Tests.cs
@@ -60,21 +60,21 @@ public void TestBuildTrigger(string input, string expected)
         }
 
         [Test]
-        [TestCase("never(6+2)")] // numeric
-        [TestCase("never(byte(0x1234))")] // no comparison
-        [TestCase("never(f)")] // function reference
-        [TestCase("unless(6+2)")] // numeric
-        [TestCase("unless(byte(0x1234))")] // no comparison
-        [TestCase("unless(f)")] // function reference
-        [TestCase("trigger_when(6+2)")] // numeric
-        [TestCase("trigger_when(byte(0x1234))")] // no comparison
-        [TestCase("trigger_when(f)")] // function reference
-        public void TestUnsupportedComparisons(string input)
+        [TestCase("never(6+2)", "integerconstant")] // numeric
+        [TestCase("never(byte(0x1234))", "memoryaccessor")] // no comparison
+        [TestCase("never(f)", "variable")] // function reference
+        [TestCase("unless(6+2)", "integerconstant")] // numeric
+        [TestCase("unless(byte(0x1234))", "memoryaccessor")] // no comparison
+        [TestCase("unless(f)", "variable")] // function reference
+        [TestCase("trigger_when(6+2)", "integerconstant")] // numeric
+        [TestCase("trigger_when(byte(0x1234))", "memoryaccessor")] // no comparison
+        [TestCase("trigger_when(f)", "variable")] // function reference
+        public void TestUnsupportedComparisons(string input, string unsupportedType)
         {
             var scope = TriggerExpressionTests.CreateScope();
             scope.AssignVariable(new VariableExpression("f"), new FunctionReferenceExpression("f2"));
 
-            TriggerExpressionTests.AssertParseError(input, scope, "comparison is not a requirement");
+            TriggerExpressionTests.AssertParseError(input, scope, "Cannot convert " + unsupportedType + " to requirement");
         }
 
         [Test]

diff --git a/Tests/Parser/Functions/AchievementFunctionTests.cs b/Tests/Parser/Functions/AchievementFunctionTests.cs
@@ -136,5 +136,53 @@ public void TestInvalidComparisonInHelperFunction()
                 "- 3:26 b call failed\r\n" +
                 "- 2:17 Cannot compare function reference and IntegerConstant"));
         }
+
+        [Test]
+        public void TestIncompleteComparison()
+        {
+            var input = "function a() => byte(0x1234)\n" +
+                        "achievement(\"T\", \"D\", 5, a())";
+
+            var tokenizer = new PositionalTokenizer(Tokenizer.CreateTokenizer(input));
+            var parser = new AchievementScriptInterpreter();
+            Assert.That(parser.Run(tokenizer), Is.False);
+            Assert.That(parser.ErrorMessage, Is.EqualTo(
+                "2:1 achievement call failed\r\n" +
+                "- 2:26 trigger is not a requirement\r\n" +
+                "- 1:17 Cannot convert memoryaccessor to requirement"));
+        }
+
+        [Test]
+        public void TestIncompleteComparisonInHelperFunction()
+        {
+            var input = "function a() => byte(0x1234)\n" +
+                        "function b() => a() + 1\r\n" +
+                        "achievement(\"T\", \"D\", 5, b())";
+
+            var tokenizer = new PositionalTokenizer(Tokenizer.CreateTokenizer(input));
+            var parser = new AchievementScriptInterpreter();
+            Assert.That(parser.Run(tokenizer), Is.False);
+            Assert.That(parser.ErrorMessage, Is.EqualTo(
+                "3:1 achievement call failed\r\n" +
+                "- 3:26 trigger is not a requirement\r\n" +
+                // this actually reports the entire "a() + 1" as invalid
+                "- 2:17 Cannot convert memoryaccessor to requirement"));
+        }
+
+        [Test]
+        public void TestIncompleteComparisonInHelperFunctionLogical()
+        {
+            var input = "function a() => byte(0x1234)\n" +
+                        "function b() => a() && byte(0x2345) == 6\r\n" +
+                        "achievement(\"T\", \"D\", 5, b())";
+
+            var tokenizer = new PositionalTokenizer(Tokenizer.CreateTokenizer(input));
+            var parser = new AchievementScriptInterpreter();
+            Assert.That(parser.Run(tokenizer), Is.False);
+            Assert.That(parser.ErrorMessage, Is.EqualTo(
+                "3:1 achievement call failed\r\n" +
+                "- 3:26 trigger is not a requirement\r\n" +
+                "- 1:17 Cannot convert memoryaccessor to requirement"));
+        }
     }
 }
diff --git a/Tests/Parser/Functions/OnceFunctionTests.cs b/Tests/Parser/Functions/OnceFunctionTests.cs
@@ -45,16 +45,15 @@ public void TestBuildTrigger(string input, string expected)
             TriggerExpressionTests.AssertSerialize(clause, expected);
         }
 
-        [Test]
-        [TestCase("once(6+2)")] // numeric
-        [TestCase("once(byte(0x1234))")] // no comparison
-        [TestCase("once(f)")] // function reference
-        public void TestUnsupportedComparisons(string input)
+        [TestCase("once(6+2)", "integerconstant")] // numeric
+        [TestCase("once(byte(0x1234))", "memoryaccessor")] // no comparison
+        [TestCase("once(f)", "variable")] // function reference
+        public void TestUnsupportedComparisons(string input, string unsupportedType)
         {
             var scope = TriggerExpressionTests.CreateScope();
             scope.AssignVariable(new VariableExpression("f"), new FunctionReferenceExpression("f2"));
 
-            TriggerExpressionTests.AssertParseError(input, scope, "comparison is not a requirement");
+            TriggerExpressionTests.AssertParseError(input, scope, "Cannot convert " + unsupportedType + " to requirement");
         }
     }
 }
diff --git a/Tests/Parser/Functions/RepeatedFunctionTests.cs b/Tests/Parser/Functions/RepeatedFunctionTests.cs
@@ -74,16 +74,15 @@ public void TestBuildTrigger(string input, string expected)
             TriggerExpressionTests.AssertSerialize(clause, expected);
         }
 
-        [Test]
-        [TestCase("repeated(5, 6+2)")] // numeric
-        [TestCase("repeated(5, byte(0x1234))")] // no comparison
-        [TestCase("repeated(5, f)")] // function reference
-        public void TestUnsupportedComparisons(string input)
+        [TestCase("repeated(5, 6+2)", "integerconstant")] // numeric
+        [TestCase("repeated(5, byte(0x1234))", "memoryaccessor")] // no comparison
+        [TestCase("repeated(5, f)", "variable")] // function reference
+        public void TestUnsupportedComparisons(string input, string unsupportedType)
         {
             var scope = TriggerExpressionTests.CreateScope();
             scope.AssignVariable(new VariableExpression("f"), new FunctionReferenceExpression("f2"));
 
-            TriggerExpressionTests.AssertParseError(input, scope, "comparison is not a requirement");
+            TriggerExpressionTests.AssertParseError(input, scope, "Cannot convert " + unsupportedType + " to requirement");
         }
 
         [Test]