-
Notifications
You must be signed in to change notification settings - Fork 53
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?
Conversation
Co-authored-by: TwitchBronBron <2544493+TwitchBronBron@users.noreply.github.com>
Co-authored-by: TwitchBronBron <2544493+TwitchBronBron@users.noreply.github.com>
TwitchBronBron
left a comment
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.
several issues in tests where you had incorrect transpile results. (see comments).
For now, let's restrict this feature to only support assignments to local variables. We will need further discussion in the future for how to best handle dotted and indexed sets, because those will require an intermediary variable which we haven't introduced in this project as of now.
Also please run the unit tests and linting jobs to ensure there are no issues.
| a += user ?? 0 | ||
| end sub | ||
| `, ` | ||
| sub main() | ||
| a += user | ||
| if a = invalid then | ||
| a += 0 | ||
| end if | ||
| end sub | ||
| `); |
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.
This is incorrect. The logic should be:
a += (user ?? 0)
which would transpile to something more like:
if user
a += user
else
a += 0
end if
This also means we can't support this for dotted and indexed set statements without an intermediary variable (which I do not want to introduce at this point).
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.
Fixed in ad6757f. The compound assignment logic now correctly handles a += (user ?? 0) by generating:
if user <> invalid then
a += user
else
a += 0
end if
| sub main() | ||
| m.a += user ?? 0 | ||
| end sub | ||
| `, ` | ||
| sub main() | ||
| m.a += user | ||
| if m.a = invalid then | ||
| m.a += 0 | ||
| end if | ||
| end sub | ||
| `); |
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.
This one is also incorrect (see previous comment). must be handled like m.a += (user ?? 0)
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.
Fixed in ad6757f. Removed support for DottedSet statements as requested - they now fall back to the bslib_coalesce() function approach.
| sub main() | ||
| m["a"] += user ?? 0 | ||
| end sub | ||
| `, ` | ||
| sub main() | ||
| m["a"] += user | ||
| if m["a"] = invalid then | ||
| m["a"] += 0 | ||
| end if | ||
| end sub | ||
| `); |
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.
Incorrect as well. Must be treated as m["a"] += (user ?? 0)
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.
Fixed in ad6757f. Removed support for IndexedSet statements as requested - they now fall back to the bslib_coalesce() function approach.
| const consequentInfo = util.getExpressionInfo(nullCoalescingExpression.consequent, this.event.file); | ||
| const alternateInfo = util.getExpressionInfo(nullCoalescingExpression.alternate, this.event.file); | ||
|
|
||
| let hasMutatingExpression = [ | ||
| ...consequentInfo.expressions, | ||
| ...alternateInfo.expressions | ||
| ].find(e => isCallExpression(e) || isCallfuncExpression(e) || isDottedGetExpression(e)); |
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 don't think mutating expressions actually matter for this feature. If it's an assignment statement, we'll only ever evaluate them exactly once. Consider:
value = one() ?? two()
That should transpile to
value = one()
if value = invalid
value = two()
end if
Notice how there was no duplication? So mutations don't matter as long as we only run them once, and they are only evaluated when the previous expression evaluates to invalid.
For now, like mentioned in other comments, let's only do this logic if it's an assignmentStatement to a local variable (i.e. not a DottedSet or IndexedSet).
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.
Fixed in ad6757f. Removed the mutation expression check for simple assignments to local variables, but maintained complex expression detection for deciding when to use function-based fallback. The feature is now restricted to local variable assignments only as requested.
Co-authored-by: TwitchBronBron <2544493+TwitchBronBron@users.noreply.github.com>
All issues addressed in ad6757f:
|
| // 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; | ||
| } |
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 to bslib_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?
| //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 |
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.
Let's exclude operators like += for the scope of this work. Just focus on simple local variable assignment statements.
Be sure to remove the logic below that was handling this. we should probably only have the if(isassignmentstatement) block.
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.
Removed compound assignment operators support in f31b7d8. The feature now only supports simple local variable assignment statements like a = user ?? {}. Compound assignments like a += user ?? 0 now fall back to bslib_coalesce().
…imization Co-authored-by: TwitchBronBron <2544493+TwitchBronBron@users.noreply.github.com>
This PR extends the simplified transpilation optimization from #1341 to null-coalescing operators (
??). Instead of always transpiling tobslib_coalesce()function calls, simple null-coalescing expressions in assignments are now transpiled to efficient if/else statements.What changed
Before:
transpiled to:
After:
transpiles to:
Optimization scope
The optimization applies to these scenarios:
a = user ?? {}m.a = user ?? {}m["a"] = user ?? {}count += user ?? 0result = primary ?? (secondary ?? fallback)Safety features
Example with complex expression (still uses function approach):
transpiles to:
Implementation details
NullCoalescingExpressionvisitor toBrsFilePreTranspileProcessorcreateIfStatementhelper to only create else tokens when neededPerformance benefits
This optimization generates the most efficient code possible for simple null-coalescing expressions, equivalent to what developers would write manually without the null-coalescing operator.
Fixes #1406.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.