Skip to content

Fix duplicate condition bug in transpiler for synthesized operators#2456

Merged
sensei-hacker merged 1 commit intoiNavFlight:maintenance-9.xfrom
sensei-hacker:fix-transpiler-not-operator-precedence
Dec 4, 2025
Merged

Fix duplicate condition bug in transpiler for synthesized operators#2456
sensei-hacker merged 1 commit intoiNavFlight:maintenance-9.xfrom
sensei-hacker:fix-transpiler-not-operator-precedence

Conversation

@sensei-hacker
Copy link
Member

@sensei-hacker sensei-hacker commented Dec 4, 2025

User description

Summary

Fixes a bug where the JavaScript Programming tab transpiler was generating duplicate logic conditions when code used both direct comparison operators (<, >, ==) and their synthesized counterparts (>=, <=, !=) with the same operands.

Problem

When user writes:

if (flight.gpsSats < 6) {
  gvar[0] = 0; // No GPS
}

if (flight.gpsSats >= 6) {
  gvar[0] = 1; // Good GPS
}

Expected behavior:

  • Condition 0: gpsSats < 6
  • Condition 1: NOT(LC 0) - reuses condition 0
  • Total: 2 logic conditions

Buggy behavior:

  • Condition 0: gpsSats < 6
  • Condition 2: gpsSats < 6 (DUPLICATE)
  • Condition 3: NOT(LC 2)
  • Total: 4 logic conditions (wastes 2 slots)

Root Cause

The condition cache maintained separate namespaces for direct operations (binary) and synthesized operations (binary_synth). When generating >= as NOT(x < 6), the code didn't check if x < 6 already existed in the binary cache before creating a new condition.

Solution

Modified generateBinary() in condition_generator.js to check if the inverse comparison already exists in the cache before generating a new one. If found, reuse the existing LC index.

Changes

  • js/transpiler/transpiler/condition_generator.js: Added cache lookup for inverse comparison before generating new condition
  • js/transpiler/transpiler/tests/test_not_precedence.js: New test verifying >= reuses existing <
  • js/transpiler/transpiler/tests/test_duplicate_detection_comprehensive.js: Comprehensive tests for >=, <=, != reuse

Testing

All tests pass:

node js/transpiler/transpiler/tests/test_not_precedence.js
# ✅ PASS: No duplicates detected. Condition 0 was correctly reused.

node js/transpiler/transpiler/tests/test_duplicate_detection_comprehensive.js
# ✅ ALL TESTS PASSED (>= reuses <, <= reuses >, != reuses ==, multiple reuses)

Impact

  • Before: Wasted 2+ logic condition slots for common patterns
  • After: Efficiently reuses existing conditions
  • User benefit: More available slots for complex logic (64 slot limit)
  • Risk: Low - only affects condition generation, doesn't change logic behavior
  • Regression: None - existing correct behavior unchanged

Files Changed

  • js/transpiler/transpiler/condition_generator.js (17 insertions, 2 deletions)
  • js/transpiler/transpiler/tests/test_not_precedence.js (132 lines, new file)
  • js/transpiler/transpiler/tests/test_duplicate_detection_comprehensive.js (125 lines, new file)

PR Type

Bug fix, Tests


Description

  • Fixes duplicate condition generation when synthesized operators reuse inverse comparisons

  • Added cache lookup for inverse comparisons in generateBinary() method

  • Prevents wasting logic condition slots on duplicate conditions

  • Added comprehensive tests for >=, <=, != operator reuse scenarios


Diagram Walkthrough

flowchart LR
  A["User Code:<br/>x < 6 and x >= 6"] --> B["Transpiler:<br/>Check Cache"]
  B --> C{"Inverse<br/>Exists?"}
  C -->|Yes| D["Reuse LC Index"]
  C -->|No| E["Generate New<br/>Condition"]
  D --> F["Result:<br/>2 Conditions"]
  E --> F
Loading

File Walkthrough

Relevant files
Bug fix
condition_generator.js
Add inverse comparison cache lookup logic                               

js/transpiler/transpiler/condition_generator.js

  • Added cache lookup for inverse comparisons before generating new
    conditions
  • Checks binary cache for inverse operator when processing synthesized
    operators
  • Reuses existing LC index if inverse comparison already cached
  • Caches newly generated inverse comparisons for future reuse
+15/-2   
Tests
test_not_precedence.js
Test duplicate detection for >= synthesis                               

js/transpiler/transpiler/tests/test_not_precedence.js

  • New test file verifying >= operator reuses existing < condition
  • Tests that x >= 6 correctly reuses condition 0 from x < 6
  • Validates no duplicate conditions are generated
  • Confirms condition 1 is NOT(LC 0) as expected
+132/-0 
test_duplicate_detection_comprehensive.js
Comprehensive tests for synthesized operator reuse             

js/transpiler/transpiler/tests/test_duplicate_detection_comprehensive.js

  • New comprehensive test suite for synthesized operator reuse
  • Tests >= reusing <, <= reusing >, != reusing == scenarios
  • Tests multiple reuses of same condition
  • Validates no duplicate conditions across all test cases
+125/-0 

When code uses both direct (<, >, ==) and synthesized (>=, <=, !=)
comparison operators with the same operands, the transpiler was creating
duplicate conditions instead of reusing existing ones.

Example:
  if (x < 6) { ... }   // Creates condition 0: x < 6
  if (x >= 6) { ... }  // Was creating duplicate condition 2: x < 6
                       // Should reuse condition 0

Root cause:
The condition cache had separate namespaces for 'binary' (direct ops)
and 'binary_synth' (synthesized ops). When generating >= as NOT(x < 6),
the code didn't check if 'x < 6' already existed in the binary cache.

Fix:
In generateBinary(), when processing synthesized operators (>=, <=, !=),
check if the inverse comparison already exists in the cache before
generating a new one. If found, reuse the existing LC index.

Changes:
- js/transpiler/transpiler/condition_generator.js: Added cache lookup
  for inverse comparison before generating new condition
- Added comprehensive tests for duplicate detection

Testing:
- test_not_precedence.js: Verifies >= reuses existing <
- test_duplicate_detection_comprehensive.js: Tests >=, <=, != reuse
- All tests pass

Impact: Reduces wasted logic condition slots, prevents FC slot exhaustion
@qodo-code-review
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The new code and tests do not introduce or modify any audit logging for critical actions,
which may be acceptable for a transpiler but cannot be confirmed from the diff alone.

Referred Code
#!/usr/bin/env node

/**
 * Test: Duplicate condition detection for >= synthesis
 *
 * When user writes:
 *   if (x < 6) { ... }
 *   if (x >= 6) { ... }
 *
 * The >= is synthesized as NOT(x < 6).
 * Duplicate detection should catch that "x < 6" already exists as condition 0,
 * and reuse it instead of creating a duplicate at condition 2.
 */

import { Transpiler } from '../index.js';

console.log('=== Test: Duplicate Detection for >= Synthesis ===\n');

const code = `
const { flight, gvar } = inav;



 ... (clipped 111 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing null checks: The added cache lookup and reuse logic does not show handling for invalid or null
operands/operator, which might be handled elsewhere but is not evident in the diff.

Referred Code
// Check if the inverse comparison already exists in the cache
// This handles cases like:
//   if (x < 6) { ... }  // Creates cache entry for "binary:<:x:6:activator"
//   if (x >= 6) { ... } // Should reuse the existing "x < 6" condition
const inverseCacheKey = this.getCacheKey('binary', inverseOp, left, right, activatorId);
let comparisonId;

if (this.conditionCache.has(inverseCacheKey)) {
  // Reuse existing inverse comparison
  comparisonId = this.conditionCache.get(inverseCacheKey);
} else {
  // Generate the inverse comparison and cache it
  comparisonId = this.pushLogicCommand(inverseOp, left, right, activatorId);
  this.conditionCache.set(inverseCacheKey, comparisonId);
}

// Then negate it with NOT
const resultId = this.pushLogicCommand(OPERATION.NOT,
  { type: OPERAND_TYPE.LC, value: comparisonId },

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Console outputs: Tests output transpilation details to console; while no sensitive data is present in the
diff, confirming absence of sensitive values at runtime requires broader context.

Referred Code
console.log('=== Comprehensive Duplicate Detection Test ===\n');

const tests = [
  {
    name: '>= reuses existing <',
    code: `
const { flight, gvar } = inav;
if (flight.altitude < 100) { gvar[0] = 0; }
if (flight.altitude >= 100) { gvar[1] = 1; }
`,
    expectedConditions: 2,
    description: 'altitude >= 100 should reuse existing "altitude < 100"'
  },
  {
    name: '<= reuses existing >',
    code: `
const { flight, gvar } = inav;
if (flight.altitude > 100) { gvar[0] = 0; }
if (flight.altitude <= 100) { gvar[1] = 1; }
`,
    expectedConditions: 2,


 ... (clipped 92 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: The new logic assumes valid operators and operands when generating and caching conditions;
validation of external inputs to the transpiler is not visible in the diff.

Referred Code
// Check if the inverse comparison already exists in the cache
// This handles cases like:
//   if (x < 6) { ... }  // Creates cache entry for "binary:<:x:6:activator"
//   if (x >= 6) { ... } // Should reuse the existing "x < 6" condition
const inverseCacheKey = this.getCacheKey('binary', inverseOp, left, right, activatorId);
let comparisonId;

if (this.conditionCache.has(inverseCacheKey)) {
  // Reuse existing inverse comparison
  comparisonId = this.conditionCache.get(inverseCacheKey);
} else {
  // Generate the inverse comparison and cache it
  comparisonId = this.pushLogicCommand(inverseOp, left, right, activatorId);
  this.conditionCache.set(inverseCacheKey, comparisonId);
}

// Then negate it with NOT
const resultId = this.pushLogicCommand(OPERATION.NOT,
  { type: OPERAND_TYPE.LC, value: comparisonId },

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Avoid direct process termination

Replace direct process.exit calls with setting process.exitCode and return
early, enabling graceful completion and better integration with test runners.

js/transpiler/transpiler/tests/test_not_precedence.js [38-132]

 if (!result.success) {
   console.log('ERROR:', result.error);
   console.log('Errors:', result.errors);
-  process.exit(1);
+  process.exitCode = 1;
+  return;
 }
 ...
 if (duplicates.length > 0) {
   ...
-  process.exit(1);
+  process.exitCode = 1;
+  return;
 } else {
   if (conditions.length === 2) {
     ...
     if (cond1.operation === '12' && cond1.operandAType === '4' && cond1.operandAValue === conditions[0].slot) {
       ...
-      process.exit(0);
+      process.exitCode = 0;
+      return;
     } else {
       ...
-      process.exit(1);
+      process.exitCode = 1;
+      return;
     }
   } else {
     ...
-    process.exit(1);
+    process.exitCode = 1;
+    return;
   }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate objects and flows before use; avoid abrupt termination with process.exit in test logic to prevent skipped cleanup and brittle CI behavior.

Low
General
Improve duplicate detection using a Set

Refactor the duplicate detection logic in the test file to use a Set for
improved efficiency and readability, changing the complexity from O(n^2) to
O(n).

js/transpiler/transpiler/tests/test_duplicate_detection_comprehensive.js [91-102]

-// Check for duplicates
+// Check for duplicates more efficiently
+const seen = new Set();
 const duplicates = [];
 for (let i = 0; i < conditions.length; i++) {
-  for (let j = i + 1; j < conditions.length; j++) {
-    if (conditions[i].operation === conditions[j].operation &&
-        conditions[i].operandAType === conditions[j].operandAType &&
-        conditions[i].operandAValue === conditions[j].operandAValue &&
-        conditions[i].operandBValue === conditions[j].operandBValue) {
-      duplicates.push({ first: i, second: j });
-    }
+  const c = conditions[i];
+  const key = `${c.operation}:${c.operandAType}:${c.operandAValue}:${c.operandBValue}`;
+  if (seen.has(key)) {
+    // To find the original, we'd need a map, but for just detecting existence this is enough.
+    // For a better error message, we can find the first occurrence.
+    const firstIndex = conditions.findIndex(cond => `${cond.operation}:${cond.operandAType}:${cond.operandAValue}:${cond.operandBValue}` === key);
+    duplicates.push({ first: firstIndex, second: i });
+  } else {
+    seen.add(key);
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that using a Set is more efficient for duplicate detection than a nested loop. However, this change is for a test file where the number of items is very small, so the performance gain is negligible. It is a good practice for code quality but has a low impact on the PR.

Low
  • More

@sensei-hacker sensei-hacker merged commit c23ffd1 into iNavFlight:maintenance-9.x Dec 4, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant