Skip to content

Negative parsing fix #115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
+117 −14
Open

Negative parsing fix #115

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f2e78eb
Remove unused export
joshestein Sep 7, 2022
f59e000
Convert all minus operators to functions
joshestein Sep 7, 2022
893e2e2
Conditional treatment of '-' funcs when finding associations
joshestein Sep 7, 2022
3aeddfa
Find binary functions accounting for '-' funcs
joshestein Sep 7, 2022
d1284cb
Update tests
joshestein Sep 7, 2022
268e26e
Remove dead code
joshestein Sep 7, 2022
b67dd11
Treat ± as −
joshestein Sep 7, 2022
3f518ed
Missing required space
joshestein Nov 12, 2022
80fba5c
Make unary minus functions always require brackets
joshestein Nov 12, 2022
3895c2e
Add minus parsing tests
joshestein Nov 12, 2022
e2434b3
Leading factorial/percentage tests
joshestein Nov 12, 2022
2f92642
Evaluation tests
joshestein Nov 12, 2022
7370e75
Update negative parsing
joshestein Nov 12, 2022
4865477
Missing `test.end()`
joshestein Nov 12, 2022
7dc7597
Update comment
joshestein Nov 27, 2023
1861dd0
Combine separate binary merging fns into single fn
joshestein Jan 16, 2024
2b1af8b
Update comments
joshestein Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/functions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {flatten, isOneOf, join, repeat, unique, words} from '@mathigon/core';
import {evaluate, interval, Interval} from './eval';
import {collapseTerm} from './parser';
import {BRACKETS, escape, isSpecialFunction, VOICE_STRINGS} from './symbols';
import {CustomFunction, ExprElement, ExprMap, ExprNumber, MathMLMap, VarMap} from './elements';
import {ExprElement, ExprMap, ExprNumber, MathMLMap, VarMap} from './elements';
import {ExprError} from './errors';


Expand All @@ -19,6 +19,7 @@ const COMMA = '<mo value="," lspace="0">,</mo>';
function needsBrackets(expr: ExprElement, parentFn: string): boolean {
if (!PRECEDENCE.includes(parentFn)) return false;
if (expr instanceof ExprTerm) return true;
if (expr instanceof ExprFunction && expr.fn === '−') return true;
if (!(expr instanceof ExprFunction)) return false;
if (!PRECEDENCE.includes(expr.fn)) return false;
if (SUBSUP.includes(expr.fn) && SUBSUP.includes(parentFn)) return true;
Expand Down
63 changes: 54 additions & 9 deletions src/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,22 @@ function findBinaryFunction(tokens: ExprElement[], fn: string) {
}
}

// Some minuses have been parsed as functions (i.e. an `ExprFunction` with a single argument).
// If they are preceded by something, we need to treat the expression as a unary minus.
Copy link
Contributor

@pjhenning pjhenning Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, what do you think about rephrasing this comment to something like this:

// Some minuses are initially parsed as unary functions (i.e. the string "- b" is parsed an `ExprFunction` with
// `.fn` = "−" and an `ExprIdentifier` "b" as its single argument.)
// If any unary minus function is preceded by another token, we need to merge the pair of tokens into a binary minus
// function.

Copy link
Author

@joshestein joshestein Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely clearer!

function findBinaryMinusFunctions(tokens: ExprElement[]) {
for (let i = 1; i < tokens.length; i++) {
const token = tokens[i];
if (token instanceof ExprFunction && token.fn === '−') {
const a = tokens[i - 1];
const b = token.args[0];

const args = [removeBrackets(a), removeBrackets(b)];
tokens.splice(i - 1, 2, new ExprFunction('−', args));
i -= 1;
}
}
}
Copy link
Contributor

@swarty swarty Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can not understand this function, this is not return and do not modify anything except function param, just loop?

Copy link
Contributor

@swarty swarty Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. looks like it should return tokens as result or throw some error as in findBinaryFunction function?
  2. redeclaring/modifying function param makes code readability so hard.



// -----------------------------------------------------------------------------
// Match Brackets
Expand Down Expand Up @@ -241,12 +257,28 @@ function findAssociativeFunction(tokens: ExprElement[], symbol: string, implicit
clearBuffer();
result.push(t);
lastWasSymbol = false;
} else if (t instanceof ExprFunction && t.fn === '−') {
// We treat leading minuses as a special case, because they can be either multiplied (associative multiplication)
// or treated as subtraction (binary function).
if (lastWasSymbol && buffer.length) {
// When we have an explicit symbol (already parsed), we want to combine elements.
lastWasSymbol = false;
buffer.push(t);
clearBuffer();
} else if (buffer.length) {
// When we have an implicit symbol, we want to keep the minus as a unary operator.
// We remove previous elements from the buffer, and keep the minus as a separate token in results.
clearBuffer();
result.push(t);
} else {
// When we have no previous elements, we push the minus to the buffer to be combined with the next element.
buffer.push(t);
}
} else {
// If implicit=true, we allow implicit multiplication, except where the
// second factor is a number. For example, "3 5" is invalid.
const noImplicit = (!implicit || t instanceof ExprNumber);
if (buffer.length && !lastWasSymbol &&
noImplicit) throw ExprError.invalidExpression();
if (buffer.length && !lastWasSymbol && noImplicit) throw ExprError.invalidExpression();
buffer.push(t);
lastWasSymbol = false;
}
Expand All @@ -256,6 +288,8 @@ function findAssociativeFunction(tokens: ExprElement[], symbol: string, implicit
return result;
}

/* Reduces an array of tokens into a single nested expression.
* For example, [2, +, 3] becomes a new ExprFunction('+', argument = [2, 3]). */
export function collapseTerm(tokens: ExprElement[]): ExprElement {
// Filter out whitespace.
tokens = tokens.filter(t => !(t instanceof ExprSpace));
Expand All @@ -272,9 +306,9 @@ export function collapseTerm(tokens: ExprElement[]): ExprElement {
}

// Match percentage and factorial operators.
if (isOperator(tokens[0], '%!')) throw ExprError.startOperator(tokens[0]);
if (isOperator(tokens[0], '% !')) throw ExprError.startOperator(tokens[0]);
for (let i = 0; i < tokens.length; ++i) {
if (!isOperator(tokens[i], '%!')) continue;
if (!isOperator(tokens[i], '% !')) continue;
tokens.splice(i - 1, 2, new ExprFunction((tokens[i] as ExprOperator).o, [tokens[i - 1]]));
i -= 1;
}
Expand All @@ -296,19 +330,30 @@ export function collapseTerm(tokens: ExprElement[]): ExprElement {
}
}

// Replace all operator minuses, not preceded by numbers, with functions. Each function takes only one argument,
// the next token in sequence.
// Move backwards to correctly handle nested expressions. For example, " - - a" should be parsed as function "−" with
// argument [function "-" with argument ["a"]].
for (let i = tokens.length - 1; i >= 0; i--) {
// Treat ± as a minus.
if (isOperator(tokens[i], '− ±')) {
if (tokens[i - 1] instanceof ExprNumber) continue;

tokens.splice(i, 2, new ExprFunction('−', [tokens[i + 1]]));
}
}

// Match multiplication operators.
tokens = findAssociativeFunction(tokens, '× * ·', true);

// Match - and ± operators, including a unary -/± at the start of an expression.
if (isOperator(tokens[0], '− ±')) {
tokens.splice(0, 2, new ExprFunction((tokens[0] as ExprOperator).o, [tokens[1]]));
}
findBinaryFunction(tokens, '− ±');
findBinaryFunction(tokens, '- −');
Copy link
Contributor

@pjhenning pjhenning Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be a part of this PR but perhaps some day we should rename this function and findBinaryMinusFunctions to have more descriptive names, and/or add a doc comment to the former. To help distinguish what they do.


// Match + operators.
if (isOperator(tokens[0], '+')) tokens = tokens.slice(1);
tokens = findAssociativeFunction(tokens, '+');

findBinaryMinusFunctions(tokens);

if (tokens.length > 1) throw ExprError.invalidExpression();
return tokens[0];
}
16 changes: 16 additions & 0 deletions test/evaluate-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,29 @@ tape('Functions', (test) => {
tape('Order and Brackets', (test) => {
test.equal(value('2 a b', {a: 3, b: 5}), 30);
test.equal(value('2 + 3 + 5'), 10);
test.equal(value('2 - 3 - 5'), -6);
test.equal(value('-2 - 3 - 5'), -10);
test.equal(value('2 + 3 * 5'), 17);
test.equal(value('2 * 3 - 5'), 1);
test.equal(value('2 * (5 - 3)'), 4);
test.equal(value('2 * (6 - 8 / 2)'), 4);
test.equal(value('2 * (5 - 8 / 2)'), 2);
test.equal(value('+ 2 + 3'), 5);
test.equal(value('- 2 * 3'), -6);
test.equal(value('- - 2'), 2);
test.equal(value('3 * - 2'), -6);
test.equal(value('3 + - 2'), 1);
test.equal(value('-3 + - 2'), -5);
test.equal(value('3 - - 2'), 5);
test.equal(value('3 - - - 2'), 1);
test.equal(value('3 - - - 2 * 4'), -5);
test.equal(value('3 - - (- 2 * 4)'), -5);
test.equal(value('3 - - -(2 * 4)'), -5);
test.equal(value('+ 3 - - - 2'), 1);
test.equal(value('3 - - - 2 + 5'), 6);
test.equal(value('3 - - - (2 + 5)'), -4);
test.equal(value('3 - - - 2 + 5 - 3'), 3);
test.equal(value('3 - - - 2 + 5 * 3'), 16);
test.end();
});

Expand Down
16 changes: 13 additions & 3 deletions test/parsing-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ tape('Comparison Operators', (test) => {
});

tape('Unary Minus', (test) => {
test.throws(() => expr('1 * -1').collapse());
test.throws(() => expr('1 + -1').collapse());
test.doesNotThrow(() => expr('x = -1').collapse());
test.end();
});
Expand Down Expand Up @@ -93,13 +91,14 @@ tape('brackets', (test) => {

tape('errors', (test) => {
test.throws(() => expr('a + + b').collapse());
test.throws(() => expr('a * - b').collapse());
test.throws(() => expr('a + (a +)').collapse());
test.throws(() => expr('a + (*)').collapse());
test.throws(() => expr('(+) - a').collapse());
test.throws(() => expr('2 =').collapse());
test.throws(() => expr('2 = 1 =').collapse());
test.throws(() => expr('< 1').collapse());
test.throws(() => expr('!2').collapse());
test.throws(() => expr('%2').collapse());
test.end();
});

Expand Down Expand Up @@ -128,3 +127,14 @@ tape('mixed numbers', (test) => {
test.throws(() => expr('1/2 1/2').collapse());
test.end();
});

tape('Minus operators', (test) => {
test.equal(expr('a * - b').collapse().toString(), 'a × (−b)');
test.equal(expr('a + - b').collapse().toString(), 'a + (−b)');
test.equal(expr('a - - b').collapse().toString(), 'a − (−b)');
test.equal(expr('10% - b').collapse().toString(), '10% − b');
test.equal(expr('-(a + b)').collapse().toString(), '−(a + b)');
test.equal(expr('a - - - b').collapse().toString(), 'a − (−(−b))');
test.equal(expr('- - - b').collapse().toString(), '−(−(−b))');
test.end();
});