Skip to content
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.

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

fix: add parentheses if needed when reduceConditionals optimization is applied #411

Merged
merged 1 commit into from
Dec 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 40 additions & 2 deletions packages/parse/src/ast/reduce-expr.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ describe('reduceExpr', () => {
expect(reduceExpr(binaryOp(call('IF THEN ELSE', one, two, zero), '*', two))).toEqual(num(4))
})

it('should not remove parens when the child expression resolves to a constant', () => {
it('should remove parens when the child expression resolves to a simple value', () => {
// (x*1) == x
expect(reduceExpr(parens(binaryOp(x, '*', one)))).toEqual(x)
})
Expand All @@ -172,11 +172,31 @@ describe('reduceExpr', () => {
expect(reduceExpr(call('MIN', binaryOp(one, '+', one), x))).toEqual(call('MIN', two, x))
})

it('should reduce calls involving constants when possible', () => {
it('should reduce IF THEN ELSE conditionals involving constants when possible', () => {
expect(reduceExpr(call('IF THEN ELSE', x, zero, one))).toEqual(call('IF THEN ELSE', x, zero, one))
expect(reduceExpr(call('IF THEN ELSE', one, x, y))).toEqual(x)
expect(reduceExpr(call('IF THEN ELSE', zero, x, y))).toEqual(y)
})

it('should remove parens when IF THEN ELSE is reduced and branch expression resolves to a simple value', () => {
// IF THEN ELSE(1, x, y + 1) * 2 == x * 2
const whenTrue = x
const whenFalse = binaryOp(y, '+', one)
expect(reduceExpr(binaryOp(call('IF THEN ELSE', one, whenTrue, whenFalse), '*', two))).toEqual(
binaryOp(x, '*', two)
)
})

it('should not remove parens when IF THEN ELSE is reduced and branch expression cannot be reduced further', () => {
// IF THEN ELSE(1, x + y, y + 1) * 2 == (x + 1) * 2
const whenTrue = binaryOp(x, '+', y)
const whenFalse = binaryOp(y, '+', one)
expect(reduceExpr(binaryOp(call('IF THEN ELSE', one, whenTrue, whenFalse), '*', two))).toEqual(
binaryOp(parens(binaryOp(x, '+', y)), '*', two)
)
})

it('should reduce function calls involving constants when possible', () => {
expect(reduceExpr(call('ABS', num(-1)))).toEqual(one)
expect(reduceExpr(call('COS', one))).toEqual(num(Math.cos(1)))
expect(reduceExpr(call('EXP', two))).toEqual(num(Math.exp(2)))
Expand Down Expand Up @@ -233,6 +253,24 @@ describe('reduceConditionals', () => {
)
})

it('should remove parens when IF THEN ELSE is reduced and branch expression resolves to a simple value', () => {
// IF THEN ELSE(1, x, y + 1) * 2 == x * 2
const whenTrue = x
const whenFalse = binaryOp(y, '+', one)
expect(reduceConditionals(binaryOp(call('IF THEN ELSE', one, whenTrue, whenFalse), '*', two))).toEqual(
binaryOp(x, '*', two)
)
})

it('should not remove parens when IF THEN ELSE is reduced and branch expression cannot be reduced further', () => {
// IF THEN ELSE(1, x + y, y + 1) * 2 == (x + 1) * 2
const whenTrue = binaryOp(x, '+', y)
const whenFalse = binaryOp(y, '+', one)
expect(reduceConditionals(binaryOp(call('IF THEN ELSE', one, whenTrue, whenFalse), '*', two))).toEqual(
binaryOp(parens(binaryOp(x, '+', y)), '*', two)
)
})

it('should try to resolve variable refs in the condition but not elsewhere', () => {
interface Variable {
refId: string
Expand Down
78 changes: 50 additions & 28 deletions packages/parse/src/ast/reduce-expr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,18 +217,7 @@ export function reduceExpr(expr: Expr, opts?: ReduceExprOptions): Expr {

case 'parens': {
const child = reduceExpr(expr.expr, opts)
switch (child.kind) {
case 'number':
case 'string':
case 'keyword':
case 'variable-ref':
// When the child expression resolves to something simple, drop the parens
// TODO: Are there other cases where we should drop the parens?
return child
default:
// Otherwise, preserve the parens
return parens(child)
}
return applyParens(child)
}

case 'lookup-def':
Expand All @@ -245,7 +234,19 @@ export function reduceExpr(expr: Expr, opts?: ReduceExprOptions): Expr {
// If condition is a constant, we can eliminate the unused branch
const conditionExpr = reduceExpr(expr.args[0], opts)
if (conditionExpr.kind === 'number') {
return conditionExpr.value !== 0 ? reduceExpr(expr.args[1], opts) : reduceExpr(expr.args[2], opts)
// The condition resolved to a simple numeric constant. If it is non-zero,
// replace the `IF THEN ELSE` call with the "true" branch, otherwise replace
// it with the "false" branch.
const branchExpr = conditionExpr.value !== 0 ? reduceExpr(expr.args[1], opts) : reduceExpr(expr.args[2], opts)

// Wrap the branch expression in parentheses if needed to ensure that the intent
// of the original expression remains the same. For example:
// a = IF THEN ELSE(1, b + c, d - e) * 10
// In this case, the parentheses are important, and need to be preserved:
// a = (b + c) * 10
// If the parentheses are not needed (like when the branch resolve to a simple
// constant or variable reference), the parentheses will be dropped.
return applyParens(branchExpr)
}
}

Expand Down Expand Up @@ -309,6 +310,7 @@ export function reduceExpr(expr: Expr, opts?: ReduceExprOptions): Expr {
*
* @param expr The expression to reduce.
* @param opts The reduce options.
* @returns A possibly reduced expression.
*/
export function reduceConditionals(expr: Expr, opts?: ReduceExprOptions): Expr {
switch (expr.kind) {
Expand Down Expand Up @@ -336,18 +338,7 @@ export function reduceConditionals(expr: Expr, opts?: ReduceExprOptions): Expr {

case 'parens': {
const child = reduceConditionals(expr.expr, opts)
switch (child.kind) {
case 'number':
case 'string':
case 'keyword':
case 'variable-ref':
// When the child expression resolves to something simple, drop the parens
// TODO: Are there other cases where we should drop the parens?
return child
default:
// Otherwise, preserve the parens
return parens(child)
}
return applyParens(child)
}

case 'lookup-def':
Expand All @@ -368,9 +359,17 @@ export function reduceConditionals(expr: Expr, opts?: ReduceExprOptions): Expr {
// The condition resolved to a simple numeric constant. If it is non-zero,
// replace the `IF THEN ELSE` call with the "true" branch, otherwise replace
// it with the "false" branch.
return conditionExpr.value !== 0
? reduceConditionals(expr.args[1], opts)
: reduceConditionals(expr.args[2], opts)
const branchExpr =
conditionExpr.value !== 0 ? reduceConditionals(expr.args[1], opts) : reduceConditionals(expr.args[2], opts)

// Wrap the branch expression in parentheses if needed to ensure that the intent
// of the original expression remains the same. For example:
// a = IF THEN ELSE(1, b + c, d - e) * 10
// In this case, the parentheses are important, and need to be preserved:
// a = (b + c) * 10
// If the parentheses are not needed (like when the branch resolve to a simple
// constant or variable reference), the parentheses will be dropped.
return applyParens(branchExpr)
}
}

Expand All @@ -388,3 +387,26 @@ export function reduceConditionals(expr: Expr, opts?: ReduceExprOptions): Expr {
assertNever(expr)
}
}

/**
* Reduce the given expression that may need to be wrapped in parentheses. If the parentheses
* are not needed, the given `child` expression will be returned as is, otherwise it will be
* wrapped in a "parens" node.
*
* @param child The child of a parens expression to reduce.
* @returns A possibly reduced expression.
*/
function applyParens(child: Expr): Expr {
switch (child.kind) {
case 'number':
case 'string':
case 'keyword':
case 'variable-ref':
// When the child expression resolves to something simple, drop the parens
// TODO: Are there other cases where we should drop the parens?
return child
default:
// Otherwise, preserve the parens
return parens(child)
}
}