Fix JS Programming decompiler: render childless boolean conditions #2536
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The decompiler was skipping boolean conditions with no children, assuming they were all "helper conditions" used as operands elsewhere. However, some childless conditions (like flight parameter checks with activators) need to be rendered for external reference (e.g., Global Functions, OSD). Changed decompileTree() to only skip childless conditions that cannot be externally referenced. Conditions that could be referenced externally are now rendered as empty if blocks with a comment. Fixes: Logic condition 23 (airspeed < 1111 with activator) was completely missing from decompiled output. Added comprehensive regression test suite with 7 test cases covering childless boolean conditions, activators, and external reference scenarios. All existing tests pass (27/27 test suites).
Contributor
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
608
to
621
| const condition = hoistedVarName || this.decompileCondition(node.lc, allConditions); | ||
|
|
||
| if (node.children.length === 0) { | ||
| // Condition with no children - skip it (it's a helper used elsewhere) | ||
| return ''; | ||
| // Only skip if this is a helper condition that can't be externally referenced | ||
| // Conditions that could be externally referenced (e.g., via Global Functions, OSD) | ||
| // should still be rendered even if they have no children | ||
| if (!this.couldBeExternallyReferenced(node.lc.operation)) { | ||
| return ''; | ||
| } | ||
| // Render empty if block with comment for external reference | ||
| lines.push(indentStr + `if (${condition}) {`); | ||
| lines.push(indentStr + ` /* LC ${node.lc.index}: for external reference */`); | ||
| lines.push(indentStr + '}'); | ||
| } else { |
Contributor
There was a problem hiding this comment.
✅ Suggestion: Add a defensive early return (or defaulting) when node, node.lc, or node.children are missing so malformed trees cannot crash decompilation. [Learned best practice, importance: 5]
New proposed code:
+if (!node?.lc || !Array.isArray(node.children)) {
+ return '';
+}
+
const hoistedVarName = this.hoistingManager?.getHoistedVarName(node.lc.index);
const condition = hoistedVarName || this.decompileCondition(node.lc, allConditions);
if (node.children.length === 0) {
// Only skip if this is a helper condition that can't be externally referenced
// Conditions that could be externally referenced (e.g., via Global Functions, OSD)
// should still be rendered even if they have no children
if (!this.couldBeExternallyReferenced(node.lc.operation)) {
return '';
}
// Render empty if block with comment for external reference
lines.push(indentStr + `if (${condition}) {`);
lines.push(indentStr + ` /* LC ${node.lc.index}: for external reference */`);
lines.push(indentStr + '}');
} else {
Comment on lines
62
to
65
| operation: 3, // LOWER_THAN | ||
| operandAType: 2, // FLIGHT | ||
| operandAValue: 11, // AIR_SPEED | ||
| operandBType: 0, // VALUE |
Contributor
There was a problem hiding this comment.
✅ Suggestion: Import and use the project enums/constants (e.g., OPERATION, OPERAND_TYPE) instead of hard-coded numeric operation/operand codes to keep tests aligned with the actual mapping. [Learned best practice, importance: 6]
Suggested change
| operation: 3, // LOWER_THAN | |
| operandAType: 2, // FLIGHT | |
| operandAValue: 11, // AIR_SPEED | |
| operandBType: 0, // VALUE | |
| operation: OPERATION.LOWER_THAN, | |
| operandAType: OPERAND_TYPE.FLIGHT, | |
| operandAValue: 11, // AIR_SPEED | |
| operandBType: OPERAND_TYPE.VALUE, |
The decompiler was hoisting expressions that read from GVARs to the top
of the decompiled code (as const declarations), causing them to use OLD
GVAR values instead of NEW values set by earlier logic conditions.
Root Cause:
ActivatorHoistingManager.identifyHoistedVars() didn't check if an LC
reads from GVARs before hoisting to global scope. GVAR values are dynamic
and can be set at runtime, so hoisting breaks execution order.
Fix:
- Added readsFromGvar() method to detect GVAR reads (recursively checking
LC operands for transitive dependencies)
- Skip hoisting for any LC that reads from GVARs
- Expressions using GVARs now render inline, preserving execution order
Example (BEFORE - broken):
const cond1 = (gvar[7] * 1000 / 45) - 500; // Uses OLD value
if (rc[8].low) { gvar[7] = 0; } // Sets NEW value
gvar[6] = cond1; // Wrong\!
Example (AFTER - correct):
if (rc[8].low) { gvar[7] = 0; } // Sets value first
gvar[6] = (gvar[7] * 1000 / 45) - 500; // Uses NEW value
Testing:
- Added comprehensive regression test suite (3 test cases)
- Tests verify GVAR assignments occur before GVAR usage
- All existing tests pass (27/28 suites)
Files modified:
- js/transpiler/transpiler/activator_hoisting.js
- js/transpiler/transpiler/tests/gvar_hoisting_order.test.cjs (new)
- js/transpiler/transpiler/tests/run_gvar_hoisting_tests.cjs (new)
The initial GVAR hoisting fix was too aggressive - it prevented hoisting ANY expression that read from GVARs, even when those GVARs were never written. This caused the "duplicate let declarations" test to fail. Refinement: - Added findWrittenGvars() to track which GVARs are actually being SET - Only prevent hoisting if reading from a GVAR that is WRITTEN - GVARs that are only READ can be safely hoisted This allows safe optimizations while still preventing the original bug. Test Results: - All 28 test suites now pass (was 27/28) - GVAR hoisting regression tests: PASS - Duplicate let declarations test: PASS (fixed) - User's original conditions: PASS Example distinction: - gvar[5] only read → hoisting ALLOWED (safe optimization) - gvar[7] written by LCs → hoisting PREVENTED (prevents bug)
1. Exclude DELAY from hoisting (like STICKY/TIMER) - DELAY maintains state (timeout, flags) across loop iterations - Should not be hoisted to prevent incorrect evaluation timing - Updated: identifyHoistedVars, activatorChainHasSticky, findStickyInActivatorChain 2. Add defensive check in decompileTree (Qodo suggestion #1) - Prevent crashes from malformed trees with missing node.lc or node.children - Adds robustness for edge cases 3. Use enums in test file (Qodo suggestion #2 - partial) - Import OPERATION and OPERAND_TYPE constants - Replace magic numbers with named constants (first test case) - Improves test maintainability Test Results: - All 28 test suites pass - No regressions introduced
EDGE maintains state (timeout, flags) across loop iterations like STICKY/TIMER/DELAY, so it should not be hoisted. Changes: 1. Added EDGE to stateful operations exclusion in identifyHoistedVars() 2. Added EDGE to activatorChainHasSticky() check 3. Added EDGE to findStickyInActivatorChain() for scope detection 4. Removed EDGE from complexOps list (now excluded earlier) 5. Updated edge_activator_in_sticky test to accept both patterns: - Hoisted: const cond1 = ... ? edge(...) : 0 - Inline: on: () => (... ? edge(...) : 0) Both patterns correctly preserve the activator relationship. Test Results: - All 28 test suites pass - edge_activator_in_sticky test now accepts inline pattern
Added detailed header comment explaining: - What hoisting is and why we do it - Hoisting criteria (all conditions that must be met) - Excluded operations (stateful, actions, GVAR dependencies) - GVAR dependency tracking and why it matters - Execution order preservation mechanisms - Example of hoisted code This helps developers understand the hoisting logic without needing to read through the implementation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
Summary
Fixes a bug in the JavaScript Programming decompiler where childless boolean conditions were being incorrectly skipped, causing them to be completely missing from the decompiled output.
Problem
Logic conditions with no children (no actions or nested conditions) that could be externally referenced (e.g., by Global Functions or OSD) were being skipped during decompilation. This made it appear as if these conditions didn't exist, confusing users.
Example: A condition checking
flight.airSpeed < 1111with an activator was completely missing from the decompiled JavaScript output.Changes
decompileTree()injs/transpiler/transpiler/decompiler.jsto check if childless conditions can be externally referenced before skipping them/* LC N: for external reference */Testing
Code Review
Reviewed with inav-code-review agent. No critical, important, or minor issues found. Clean implementation with precise fix and comprehensive testing.
Related Issues
Fixes the issue where flight parameter comparisons with activators were missing from decompiled JavaScript output.
PR Type
Bug fix, Tests
Description
Fix JS decompiler skipping childless boolean conditions with external references
Add comprehensive regression test suite with 7 test cases
Diagram Walkthrough
flowchart LR A["Childless Boolean Condition"] --> B{"Can be externally referenced?"} B -->|No| C["Skip rendering"] B -->|Yes| D["Render empty if block with comment"] E["Test Suite"] --> F["7 regression tests"] F --> G["All scenarios covered"]File Walkthrough
decompiler.js
Add external reference check for childless conditionsjs/transpiler/transpiler/decompiler.js
decompileTree()to check if childless conditions can beexternally referenced
couldBeExternallyReferenced()empty if blocks with external reference comment
referenced
test_childless_boolean_condition.js
Add regression tests for childless boolean conditionsjs/transpiler/transpiler/tests/test_childless_boolean_condition.js
activators
with children
detailed reporting