-
Notifications
You must be signed in to change notification settings - Fork 63
Optimize null-coalescing operator transpilation to if/else statements #1537
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
0cfae88
ce2492f
237b971
c6f3726
ad6757f
b03819a
b3a911b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,13 @@ | ||
| import { createAssignmentStatement, createBlock, createDottedSetStatement, createIfStatement, createIndexedSetStatement, createToken } from '../../astUtils/creators'; | ||
| import { isAssignmentStatement, isBinaryExpression, isBlock, isBody, isBrsFile, isDottedGetExpression, isDottedSetStatement, isGroupingExpression, isIndexedGetExpression, isIndexedSetStatement, isLiteralExpression, isUnaryExpression, isVariableExpression } from '../../astUtils/reflection'; | ||
| import { createAssignmentStatement, createBlock, createDottedSetStatement, createIfStatement, createIndexedSetStatement, createToken, createVariableExpression, createInvalidLiteral } from '../../astUtils/creators'; | ||
| import { isAssignmentStatement, isBinaryExpression, isBlock, isBody, isBrsFile, isCallExpression, isCallfuncExpression, isDottedGetExpression, isDottedSetStatement, isGroupingExpression, isIndexedGetExpression, isIndexedSetStatement, isLiteralExpression, isUnaryExpression, isVariableExpression } from '../../astUtils/reflection'; | ||
| import { createVisitor, WalkMode } from '../../astUtils/visitors'; | ||
| import type { BrsFile } from '../../files/BrsFile'; | ||
| import type { BeforeFileTranspileEvent } from '../../interfaces'; | ||
| import type { Token } from '../../lexer/Token'; | ||
| import { TokenKind } from '../../lexer/TokenKind'; | ||
| import type { Expression, Statement } from '../../parser/AstNode'; | ||
| import type { TernaryExpression } from '../../parser/Expression'; | ||
| import { LiteralExpression } from '../../parser/Expression'; | ||
| import type { TernaryExpression, NullCoalescingExpression } from '../../parser/Expression'; | ||
| import { BinaryExpression, LiteralExpression } from '../../parser/Expression'; | ||
| import { ParseMode } from '../../parser/Parser'; | ||
| import type { IfStatement } from '../../parser/Statement'; | ||
| import type { Scope } from '../../Scope'; | ||
|
|
@@ -41,6 +41,9 @@ export class BrsFilePreTranspileProcessor { | |
| const visitor = createVisitor({ | ||
| TernaryExpression: (ternaryExpression) => { | ||
| this.processTernaryExpression(ternaryExpression, visitor, walkMode); | ||
| }, | ||
| NullCoalescingExpression: (nullCoalescingExpression) => { | ||
| this.processNullCoalescingExpression(nullCoalescingExpression, visitor, walkMode); | ||
| } | ||
| }); | ||
| this.event.file.ast.walk(visitor, { walkMode: walkMode }); | ||
|
|
@@ -178,6 +181,138 @@ export class BrsFilePreTranspileProcessor { | |
| } | ||
| } | ||
|
|
||
| private processNullCoalescingExpression(nullCoalescingExpression: NullCoalescingExpression, visitor: ReturnType<typeof createVisitor>, walkMode: WalkMode) { | ||
| // Check if this null coalescing expression has complex expressions that require scope protection | ||
| const consequentInfo = util.getExpressionInfo(nullCoalescingExpression.consequent, this.event.file); | ||
| const alternateInfo = util.getExpressionInfo(nullCoalescingExpression.alternate, this.event.file); | ||
|
|
||
| let hasComplexExpression = [ | ||
| ...consequentInfo.expressions, | ||
| ...alternateInfo.expressions | ||
| ].find(e => isCallExpression(e) || isCallfuncExpression(e) || isDottedGetExpression(e) || isIndexedGetExpression(e)); | ||
|
|
||
| // Only optimize if there are no complex expressions | ||
| if (hasComplexExpression) { | ||
| return; | ||
| } | ||
|
|
||
| function getOwnerAndKey(statement: Statement) { | ||
| const parent = statement.parent; | ||
| if (isBlock(parent) || isBody(parent)) { | ||
| let idx = parent.statements.indexOf(statement); | ||
| if (idx > -1) { | ||
| return { owner: parent.statements, key: idx }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| //if the null coalescing expression is part of a simple assignment to a local variable, rewrite it as an `IfStatement` | ||
| let parent = nullCoalescingExpression.findAncestor(x => !isGroupingExpression(x)); | ||
| let operator: Token; | ||
| //operators like `+=` will cause the RHS to be a BinaryExpression due to how the parser handles this. let's do a little magic to detect this situation | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's exclude operators like Be sure to remove the logic below that was handling this. we should probably only have the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed compound assignment operators support in f31b7d8. The feature now only supports simple local variable assignment statements like |
||
| if ( | ||
| //parent is a binary expression | ||
| isBinaryExpression(parent) && | ||
| isAssignmentStatement(parent.parent) && isVariableExpression(parent.left) && parent.left.name === parent.parent.name | ||
| ) { | ||
| //keep the correct operator (i.e. `+=`) | ||
| operator = parent.operator; | ||
| //use the outer parent and skip this BinaryExpression | ||
| parent = parent.parent; | ||
| } | ||
| let ifStatement: IfStatement; | ||
|
|
||
| // Only support AssignmentStatement to local variables | ||
| if (isAssignmentStatement(parent)) { | ||
| if (operator) { | ||
| // For compound assignments like `a += user ?? 0`, we need to check the left side value first | ||
| // Create condition: user <> invalid | ||
| const condition = new BinaryExpression( | ||
| nullCoalescingExpression.consequent, | ||
| createToken(TokenKind.LessGreater, '<>', nullCoalescingExpression.questionQuestionToken.range), | ||
| createInvalidLiteral('invalid', nullCoalescingExpression.questionQuestionToken.range) | ||
| ); | ||
|
|
||
| ifStatement = createIfStatement({ | ||
| if: createToken(TokenKind.If, 'if', nullCoalescingExpression.questionQuestionToken.range), | ||
| condition: condition, | ||
| then: createToken(TokenKind.Then, 'then', nullCoalescingExpression.questionQuestionToken.range), | ||
| thenBranch: createBlock({ | ||
| statements: [ | ||
| createAssignmentStatement({ | ||
| name: parent.name, | ||
| equals: operator, | ||
| value: nullCoalescingExpression.consequent | ||
| }) | ||
| ] | ||
| }), | ||
| else: createToken(TokenKind.Else, 'else', nullCoalescingExpression.questionQuestionToken.range), | ||
| elseBranch: createBlock({ | ||
| statements: [ | ||
| createAssignmentStatement({ | ||
| name: parent.name, | ||
| equals: operator, | ||
| value: nullCoalescingExpression.alternate | ||
| }) | ||
| ] | ||
| }), | ||
| endIf: createToken(TokenKind.EndIf, 'end if', nullCoalescingExpression.questionQuestionToken.range) | ||
| }); | ||
|
|
||
| // Replace the parent statement with the if statement | ||
| let { owner, key } = getOwnerAndKey(parent as Statement) ?? {}; | ||
| if (owner && key !== undefined) { | ||
| this.event.editor.setProperty(owner, key, ifStatement); | ||
| } | ||
| } else { | ||
| // For simple assignments like `a = user ?? {}`, use the original logic | ||
| // Create condition: variableName = invalid | ||
| const condition = new BinaryExpression( | ||
| createVariableExpression(parent.name.text), | ||
| createToken(TokenKind.Equal, '=', nullCoalescingExpression.questionQuestionToken.range), | ||
| createInvalidLiteral('invalid', nullCoalescingExpression.questionQuestionToken.range) | ||
| ); | ||
|
|
||
| ifStatement = createIfStatement({ | ||
| if: createToken(TokenKind.If, 'if', nullCoalescingExpression.questionQuestionToken.range), | ||
| condition: condition, | ||
| then: createToken(TokenKind.Then, 'then', nullCoalescingExpression.questionQuestionToken.range), | ||
| thenBranch: createBlock({ | ||
| statements: [ | ||
| createAssignmentStatement({ | ||
| name: parent.name, | ||
| equals: parent.equals, | ||
| value: nullCoalescingExpression.alternate | ||
| }) | ||
| ] | ||
| }), | ||
| endIf: createToken(TokenKind.EndIf, 'end if', nullCoalescingExpression.questionQuestionToken.range) | ||
| }); | ||
|
|
||
| // First, we need to create the initial assignment statement | ||
| const initialAssignment = createAssignmentStatement({ | ||
| name: parent.name, | ||
| equals: parent.equals, | ||
| value: nullCoalescingExpression.consequent | ||
| }); | ||
|
|
||
| // Replace the parent with a sequence: first the initial assignment, then the if statement | ||
| let { owner, key } = getOwnerAndKey(parent as Statement) ?? {}; | ||
| if (owner && key !== undefined) { | ||
| // Replace with initial assignment first | ||
| this.event.editor.setProperty(owner, key, initialAssignment); | ||
| // Insert the if statement after | ||
| this.event.editor.addToArray(owner, key + 1, ifStatement); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (ifStatement) { | ||
| //we've injected an ifStatement, so now we need to trigger a walk to handle any nested null coalescing expressions | ||
| ifStatement.walk(visitor, { walkMode: walkMode }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Given a string optionally separated by dots, find an enum related to it. | ||
| * For example, all of these would return the enum: `SomeNamespace.SomeEnum.SomeMember`, SomeEnum.SomeMember, `SomeEnum` | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get rid of this junk, we don't need to check for this at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need clarification on this. If I remove the complex expression detection entirely, then expressions like
a = user.getAccount() ?? {}would be optimized to if/else instead of falling back tobslib_coalesce(). However, the existing tests expect complex expressions with function calls to use the function-based approach. Should I update the tests to match the new expected behavior, or keep some form of complex expression detection?