better error reporting, tests

Author: PhilippImhof

classes/local/answer_parser.php

@@ -251,7 +251,7 @@ private function is_acceptable_algebraic_formula(bool $fornumericalformula = fal
                 // formulas, students are allowed to use variables, so the expression is syntactically
                 // valid and will be interpreted as 'sin*30' which is most certainly wrong. The
                 // following check will make sure that students do not use function names as variables.
-                if (array_key_exists($token->value, functions::FUNCTIONS + evaluator::PHPFUNCTIONS)) {
+                if (self::is_valid_function_name($token->value)) {
                     return false;
                 }
                 /* TODO: maybe we should reject unknown variables, because that avoids mistakes

classes/local/parser.php

@@ -239,10 +239,14 @@ private function parse_general_expression(?int $stopat = null): expression {
             $nextvalue = $nexttoken->value;
 
             // If the current token is a PREFIX and the next one is an IDENTIFIER, we will consider
-            // that one as a FUNCTION. If the next token has already been classified as a function,
-            // there is nothing to do. Otherwise, the PREFIX is simply useless and we ignore it.
+            // that one as a FUNCTION, unless it is not a known function name. If the PREFIX is not
+            // followed by an identifier, we silently ignore it for maximum backwards compatibility, as
+            // legacy versions used to remove backslashes from variable definitions.
             if ($type === token::PREFIX) {
-                if ($nexttype === token::IDENTIFIER || $nexttype === token::FUNCTION) {
+                if ($nexttype === token::IDENTIFIER) {
+                    if (!self::is_valid_function_name($nextvalue)) {
+                        $this->die(get_string('error_prefix', 'qtype_formulas'));
+                    }
                     $nexttype = ($nexttoken->type = token::FUNCTION);
                 }
             }
@@ -264,7 +268,7 @@ private function parse_general_expression(?int $stopat = null): expression {
             // break existing questions.
             if ($type === token::IDENTIFIER) {
                 $isnotavariable = !$this->is_known_variable($currenttoken);
-                $isknownfunction = array_key_exists($value, functions::FUNCTIONS + evaluator::PHPFUNCTIONS);
+                $isknownfunction = self::is_valid_function_name($value);
                 $nextisparen = $nexttype === token::OPENING_PAREN;
                 if ($isnotavariable && $isknownfunction && $nextisparen) {
                     $type = ($currenttoken->type = token::FUNCTION);
@@ -369,6 +373,16 @@ private function parse_general_expression(?int $stopat = null): expression {
         return new expression(shunting_yard::infix_to_rpn($expression));
     }
 
+    /**
+     * Check if a given name is a valid function name.
+     *
+     * @param string $identifier the name to check
+     * @return bool
+     */
+    public static function is_valid_function_name(string $identifier): bool {
+        return array_key_exists($identifier, functions::FUNCTIONS + evaluator::PHPFUNCTIONS);
+    }
+
     /**
      * Retrieve the parsed statements.
      *

tests/evaluator_test.php

@@ -1506,8 +1506,8 @@ public static function provide_invalid_assignments(): array {
                 'Individual chars of a string cannot be modified.',
                 's = "foo"; s[1] = "x"',
             ],
-            'assignment with invalid function' => [
-                "Unknown function: 'idontexist'",
+            'prefix with invalid function' => [
+                'Syntax error: invalid use of prefix character \.',
                 'a = \idontexist(5)',
             ],
             'assignment to constant' => [
@@ -2322,6 +2322,22 @@ public function test_impossible_stuff(): void {
         }
         self::assertNotNull($e);
 
+        // Trying to execute an unknown function should yield the appropriate error message.
+        $statement = new expression([
+            new token(token::VARIABLE, 'a'),
+            new token(token::NUMBER, 5),
+            new token(token::NUMBER, 1),
+            new token(token::FUNCTION, 'idontexist'),
+            new token(token::OPERATOR, '='),
+        ]);
+        $e = null;
+        try {
+            $evaluator->evaluate($statement);
+        } catch (Exception $e) {
+            self::assertStringEndsWith("Unknown function: 'idontexist'", $e->getMessage());
+        }
+        self::assertNotNull($e);
+
         // When executing the ternary operator, we must have enough stuff (and the right stuff) on the stack.
         $statement = new expression([
             new token(token::OPERATOR, '?'),
@@ -2350,6 +2366,8 @@ public function test_impossible_stuff(): void {
             self::assertStringEndsWith('Evaluation error: not enough arguments for ternary operator.', $e->getMessage());
         }
         self::assertNotNull($e);
+
+
     }
 
     /**

tests/parser_test.php

@@ -287,27 +287,4 @@ public function test_impossible_stuff($expected, $input): void {
         }
         self::assertNotNull($e);
     }
-
-    /**
-     * Test that the shunting yard algorithm correctly deals with a PREFIX token
-     * that might have gotten into the list of tokens.
-     */
-    public function test_shunting_yard_prefix_token(): void {
-        $tokens = [
-            new token(token::VARIABLE, 'a'),
-            new token(token::OPERATOR, '='),
-            new token(token::PREFIX, '\\'),
-            new token(token::FUNCTION, 'sin'),
-            new token(token::OPENING_PAREN, '('),
-            new token(token::NUMBER, '2'),
-            new token(token::CLOSING_PAREN, ')'),
-        ];
-
-        $output = shunting_yard::infix_to_rpn($tokens);
-        $output = array_map(function($token) {
-            return $token->value;
-        }, $output);
-
-        self::assertEquals('a,2,1,sin,=', implode(',', $output));
-    }
 }

tests/questiontype_test.php

@@ -324,6 +324,10 @@ public static function provide_single_part_data_for_form_validation(): array {
                     'subqtext' => [0 => ['text' => '{_0} and {_0}']],
                 ],
             ],
+            [
+                ['answer[0]' => '1:1:Syntax error: invalid use of prefix character \.'],
+                ['answer' => [0 => '\idontexist(20)']],
+            ],
             [
                 ['answer[0]' => get_string('error_model_answer_no_content', 'qtype_formulas')],
                 ['answer' => [0 => '#']],