From 35b39777d51e88d4256b6dffee210a83e93c9d0b Mon Sep 17 00:00:00 2001 From: Philipp Imhof <52650214+PhilippImhof@users.noreply.github.com> Date: Sat, 31 May 2025 18:04:56 +0200 Subject: [PATCH 1/5] silently discard useless PREFIX --- classes/local/parser.php | 9 +++++---- tests/evaluator_test.php | 24 ++++++++++++++++-------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/classes/local/parser.php b/classes/local/parser.php index 1d3d75fe..cec01f05 100644 --- a/classes/local/parser.php +++ b/classes/local/parser.php @@ -240,12 +240,10 @@ private function parse_general_expression(?int $stopat = null): expression { // 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, this is a syntax error. + // there is nothing to do. Otherwise, the PREFIX is simply useless and we ignore it. if ($type === token::PREFIX) { if ($nexttype === token::IDENTIFIER || $nexttype === token::FUNCTION) { $nexttype = ($nexttoken->type = token::FUNCTION); - } else { - $this->die(get_string('error_prefix', 'qtype_formulas')); } } @@ -364,7 +362,10 @@ private function parse_general_expression(?int $stopat = null): expression { } // Otherwise, let's read the next token and append it to the list of tokens for this statement. $currenttoken = $this->read_next(); - $expression[] = $currenttoken; + // FIXME: update comment -- we do not add the PREFIX token to the list + if ($currenttoken->type !== token::PREFIX) { + $expression[] = $currenttoken; + } } // Feed the expression to the shunting yard algorithm and return the result. diff --git a/tests/evaluator_test.php b/tests/evaluator_test.php index 369e4f9e..adc787e5 100644 --- a/tests/evaluator_test.php +++ b/tests/evaluator_test.php @@ -454,6 +454,22 @@ public static function provide_simple_expressions(): array { */ public static function provide_valid_assignments(): array { return [ + 'accepting useless prefix before number' => [ + ['a' => new variable('a', 2, variable::NUMERIC)], + 'a = \2', + ], + 'accepting useless prefix' => [ + ['a' => new variable('a', 4, variable::NUMERIC)], + 'a = \ (3 + 1)', + ], + 'accepting useless prefix, not swallowing parens' => [ + ['a' => new variable('a', 20, variable::NUMERIC)], + 'a = \ (3 + 1) * 5', + ], + 'correctly reading a whole bunch of useless and useful PREFIXES' => [ + ['a' => new variable('a', 20, variable::NUMERIC)], + 'a \ = \ ( \ 3 \ + \ 1 \ ) \ * \ 5 * \cos(0)', + ], 'one number' => [ ['a' => new variable('a', 1, variable::NUMERIC)], 'a = 1;', @@ -1502,10 +1518,6 @@ public static function provide_invalid_assignments(): array { 'Left-hand side of assignment must be a variable.', 'π = 3', ], - 'invalid use of prefix with number' => [ - 'Syntax error: invalid use of prefix character \.', - 'a = \ 2', - ], 'invalid argument for unary operator' => [ "Number expected, found 'foo'.", 'a = -"foo"', @@ -1514,10 +1526,6 @@ public static function provide_invalid_assignments(): array { "Number expected, found 'foo'.", 's = "foo"; a = -s', ], - 'invalid use of prefix with paren' => [ - 'Syntax error: invalid use of prefix character \.', - 'a = \ (3 + 1)', - ], 'assignment to invalid variable' => [ '1:1:Invalid variable name: _a.', '_a=3;', From cc417bc444d202942432248f59e74cb89a94d12f Mon Sep 17 00:00:00 2001 From: Philipp Imhof <52650214+PhilippImhof@users.noreply.github.com> Date: Sat, 31 May 2025 22:01:20 +0200 Subject: [PATCH 2/5] correctly display student answer containing a backslash --- renderer.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/renderer.php b/renderer.php index 0812d3d0..20cca9be 100644 --- a/renderer.php +++ b/renderer.php @@ -496,7 +496,18 @@ public function get_part_formulation(question_attempt $qa, question_display_opti } foreach ($inputs as $placeholder => $replacement) { - $subqreplaced = preg_replace('/'.$boxes[$placeholder]['placeholder'].'/', $replacement, $subqreplaced, 1); + // The replacement text *might* contain a backslash and in the worst case this might + // lead to an erroneous backreference, e. g. if the student's answer was \1. Thus, + // we better use preg_replace_callback() instead of just preg_replace(), as this allows + // us to ignore such unintentional backreferences. + $subqreplaced = preg_replace_callback( + '/' . $boxes[$placeholder]['placeholder'] . '/', + function ($matches) use ($replacement) { + return $replacement; + }, + $subqreplaced, + 1 + ); } return $subqreplaced; } From b482aa30f2bc33c68480a85761c04438153091f2 Mon Sep 17 00:00:00 2001 From: Philipp Imhof <52650214+PhilippImhof@users.noreply.github.com> Date: Sun, 1 Jun 2025 10:19:00 +0200 Subject: [PATCH 3/5] add tests, cleanup --- classes/local/parser.php | 2 +- tests/parser_test.php | 23 +++++++++++++++++++++++ tests/renderer_test.php | 16 ++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/classes/local/parser.php b/classes/local/parser.php index cec01f05..aba96412 100644 --- a/classes/local/parser.php +++ b/classes/local/parser.php @@ -361,8 +361,8 @@ private function parse_general_expression(?int $stopat = null): expression { break; } // Otherwise, let's read the next token and append it to the list of tokens for this statement. + // Note that we do not add the PREFIX token to the list, because it has already served its purpose. $currenttoken = $this->read_next(); - // FIXME: update comment -- we do not add the PREFIX token to the list if ($currenttoken->type !== token::PREFIX) { $expression[] = $currenttoken; } diff --git a/tests/parser_test.php b/tests/parser_test.php index abb77ee3..435ebef1 100644 --- a/tests/parser_test.php +++ b/tests/parser_test.php @@ -287,4 +287,27 @@ 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)); + } } diff --git a/tests/renderer_test.php b/tests/renderer_test.php index d23a34e3..2fc7a8ee 100644 --- a/tests/renderer_test.php +++ b/tests/renderer_test.php @@ -103,6 +103,22 @@ public function test_answer_not_unique(): void { $this->check_output_contains_lang_string('uniquecorrectansweris', 'qtype_formulas', '5'); } + public function test_show_response_with_backslash(): void { + $q = $this->get_test_formulas_question('testsinglenum'); + $this->start_attempt_at_question($q, 'immediatefeedback', 1); + + $this->render(); + $this->check_current_state(question_state::$todo); + $this->check_output_contains_text_input('0_0', '', true); + + // Submit wrong answer. The \1 must not be gobbled. + $this->process_submission(['0_0' => '\1', '-submit' => 1]); + $this->render(); + $this->check_current_state(question_state::$gradedwrong); + $this->check_output_contains(qtype_formulas_test_helper::DEFAULT_INCORRECT_FEEDBACK); + $this->check_output_contains_text_input('0_0', '\1', false); + } + public function test_render_question_with_combined_unit_field(): void { $q = $this->get_test_formulas_question('testsinglenumunit'); $q->parts[0]->unitpenalty = 0.5; From b2ee74b9b7ff5c7ae7f4aea798f993ca0727a1d2 Mon Sep 17 00:00:00 2001 From: Philipp Imhof <52650214+PhilippImhof@users.noreply.github.com> Date: Sun, 1 Jun 2025 18:07:25 +0200 Subject: [PATCH 4/5] bugfix, cleanup --- classes/local/parser.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/classes/local/parser.php b/classes/local/parser.php index aba96412..87d27a0e 100644 --- a/classes/local/parser.php +++ b/classes/local/parser.php @@ -361,11 +361,8 @@ private function parse_general_expression(?int $stopat = null): expression { break; } // Otherwise, let's read the next token and append it to the list of tokens for this statement. - // Note that we do not add the PREFIX token to the list, because it has already served its purpose. $currenttoken = $this->read_next(); - if ($currenttoken->type !== token::PREFIX) { - $expression[] = $currenttoken; - } + $expression[] = $currenttoken; } // Feed the expression to the shunting yard algorithm and return the result. From 11b6426c7e937951c766954fbc9cd41387569bac Mon Sep 17 00:00:00 2001 From: Philipp Imhof <52650214+PhilippImhof@users.noreply.github.com> Date: Sun, 1 Jun 2025 22:04:46 +0200 Subject: [PATCH 5/5] better error reporting, tests --- classes/local/answer_parser.php | 2 +- classes/local/parser.php | 22 ++++++++++++++++++---- tests/evaluator_test.php | 22 ++++++++++++++++++++-- tests/parser_test.php | 23 ----------------------- tests/questiontype_test.php | 4 ++++ 5 files changed, 43 insertions(+), 30 deletions(-) diff --git a/classes/local/answer_parser.php b/classes/local/answer_parser.php index 0898ed0e..ce1808f7 100644 --- a/classes/local/answer_parser.php +++ b/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 diff --git a/classes/local/parser.php b/classes/local/parser.php index 87d27a0e..bcb72ae8 100644 --- a/classes/local/parser.php +++ b/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. * diff --git a/tests/evaluator_test.php b/tests/evaluator_test.php index adc787e5..2c64ca3a 100644 --- a/tests/evaluator_test.php +++ b/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); + + } /** diff --git a/tests/parser_test.php b/tests/parser_test.php index 435ebef1..abb77ee3 100644 --- a/tests/parser_test.php +++ b/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)); - } } diff --git a/tests/questiontype_test.php b/tests/questiontype_test.php index ea92bbf3..416f68d6 100644 --- a/tests/questiontype_test.php +++ b/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 => '#']],