-
Notifications
You must be signed in to change notification settings - Fork 370
Fix transpiler validation and improve error messages #2514
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: maintenance-9.x
Are you sure you want to change the base?
Fix transpiler validation and improve error messages #2514
Conversation
This commit fixes three issues in the JavaScript Programming tab:
1. **Fix silent code dropping for invalid function calls**
- Previously, code like `inav.override.flightAxis(180);` was silently
discarded without any error message
- Now generates clear error: "Cannot call 'X' as a function"
- Added extractCalleeNameForError() helper to show full call path
- Location: js/transpiler/transpiler/parser.js
2. **Add comprehensive intermediate object detection**
- Detects when users try to use objects as if they were properties
- Catches: assignments, function calls, expressions, comparisons
- Examples caught:
* `inav.override.flightAxis.yaw = 180` (missing .angle)
* `gvar[0] = inav.flight + 1` (flight is an object)
* `inav.override.vtx(3)` (vtx is object, not function)
- Provides helpful suggestions showing available properties
- Locations:
* js/transpiler/transpiler/property_access_checker.js
* js/transpiler/transpiler/analyzer.js
3. **Fix missing i18n text in JavaScript Programming tab header**
- Added i18n.localize() calls in tab initialization
- Added CSS to fix paragraph spacing (margin-bottom: 10px)
- Locations:
* tabs/javascript_programming.js (lines 84, 91)
* tabs/javascript_programming.html (line 217-219)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Changed addError to addWarning with type 'error' in parser - Made transpile() fail when parser errors are present - Prevents silent code dropping from succeeding compilation
This commit completes the transpiler validation improvements by: 1. Enhanced intermediate object detection (property_access_checker.js): - Now uses raw API definitions instead of processed structure - Simplified logic per user suggestion: just check typeof === 'object' - Catches 2-level objects (e.g., inav.override.vtx) - Catches 3-level objects (e.g., inav.override.flightAxis.yaw) - Catches category-only access (e.g., inav.flight) 2. Improved error messages (analyzer.js): - Shows available properties for intermediate objects - Differentiates simple vs deeply nested objects - Provides actionable suggestions 3. Comprehensive test coverage: - test_examples.mjs: All 22 examples still compile (no regressions) - test_validation_fixes.mjs: 8/8 validation tests pass All invalid code now generates helpful error messages instead of: - Being silently dropped - Failing later in codegen with cryptic messages - Passing through to generate invalid logic conditions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
Address Qodo code review feedback: check result.warnings.errors instead of result.errors for semantic/validation errors.
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
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.
High-level Suggestion
Refactor the new validation logic to use the existing processed API structure instead of directly importing raw API definitions. This will prevent inconsistencies by ensuring all components rely on a single source for API information. [High-level, importance: 8]
Solution Walkthrough:
Before:
// property_access_checker.js
import apiDefinitions from '../api/definitions/index.js'; // Raw definitions
class PropertyAccessChecker {
constructor(context) {
this.inavAPI = context.inavAPI; // Processed structure
}
isPropertyAnObject(category, parentProp, childProp) {
// Bypasses processed structure and uses raw definitions directly
const categoryDef = apiDefinitions[category];
// ... logic using raw definitions
}
// ... other methods use this.inavAPI (processed)
}After:
// api_mapping_utility.js
function buildAPIStructure(apiDefinitions) {
// ... existing processing
// Enhance the structure to include original hierarchy or metadata
// needed for validation.
processedStructure.rawHierarchy = apiDefinitions; // or some other form
return processedStructure;
}
// property_access_checker.js
class PropertyAccessChecker {
constructor(context) {
this.inavAPI = context.inavAPI; // Processed structure with extra metadata
}
isPropertyAnObject(category, parentProp, childProp) {
// Uses the enhanced processed structure, no direct import of raw definitions
const categoryDef = this.inavAPI.rawHierarchy[category];
// ... logic using the single source of truth
}
}| const nestedPropKeys = Object.keys(firstProp.properties); | ||
| const suggestions = propKeys.slice(0, 2).flatMap(p => { | ||
| const nested = categoryDef.properties[p]; | ||
| if (nested && nested.properties) { | ||
| const firstNestedProp = Object.keys(nested.properties)[0]; | ||
| return [`inav.override.${parts[1]}.${p}.${firstNestedProp}`]; | ||
| } | ||
| return []; | ||
| }).join(', '); |
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.
Suggestion: In getImprovedWritabilityError, check that nested.properties is not empty before accessing its first key to prevent generating error suggestions containing undefined. [possible issue, importance: 6]
| const nestedPropKeys = Object.keys(firstProp.properties); | |
| const suggestions = propKeys.slice(0, 2).flatMap(p => { | |
| const nested = categoryDef.properties[p]; | |
| if (nested && nested.properties) { | |
| const firstNestedProp = Object.keys(nested.properties)[0]; | |
| return [`inav.override.${parts[1]}.${p}.${firstNestedProp}`]; | |
| } | |
| return []; | |
| }).join(', '); | |
| const nestedPropKeys = Object.keys(firstProp.properties); | |
| const suggestions = propKeys.slice(0, 2).flatMap(p => { | |
| const nested = categoryDef.properties[p]; | |
| if (nested && nested.properties) { | |
| const nestedKeys = Object.keys(nested.properties); | |
| if (nestedKeys.length > 0) { | |
| const firstNestedProp = nestedKeys[0]; | |
| return [`inav.override.${parts[1]}.${p}.${firstNestedProp}`]; | |
| } | |
| } | |
| return []; | |
| }).join(', '); |
| extractCalleeNameForError(callee) { | ||
| if (callee.type === 'Identifier') { | ||
| return callee.name; | ||
| } | ||
| if (callee.type === 'MemberExpression') { | ||
| // Try to reconstruct the full path | ||
| const parts = []; | ||
| let current = callee; | ||
|
|
||
| while (current) { | ||
| if (current.type === 'MemberExpression') { | ||
| if (current.property) { | ||
| parts.unshift(current.property.name || current.property.value); | ||
| } | ||
| current = current.object; | ||
| } else if (current.type === 'Identifier') { | ||
| parts.unshift(current.name); | ||
| break; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return parts.join('.'); | ||
| } | ||
| return '<unknown>'; | ||
| } |
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.
Suggestion: Update extractCalleeNameForError to correctly handle computed properties in MemberExpression (e.g., inav.rc[0]) to generate more accurate error messages. [possible issue, importance: 7]
| extractCalleeNameForError(callee) { | |
| if (callee.type === 'Identifier') { | |
| return callee.name; | |
| } | |
| if (callee.type === 'MemberExpression') { | |
| // Try to reconstruct the full path | |
| const parts = []; | |
| let current = callee; | |
| while (current) { | |
| if (current.type === 'MemberExpression') { | |
| if (current.property) { | |
| parts.unshift(current.property.name || current.property.value); | |
| } | |
| current = current.object; | |
| } else if (current.type === 'Identifier') { | |
| parts.unshift(current.name); | |
| break; | |
| } else { | |
| break; | |
| } | |
| } | |
| return parts.join('.'); | |
| } | |
| return '<unknown>'; | |
| } | |
| extractCalleeNameForError(callee) { | |
| if (callee.type === 'Identifier') { | |
| return callee.name; | |
| } | |
| if (callee.type === 'MemberExpression') { | |
| // Try to reconstruct the full path | |
| const parts = []; | |
| let current = callee; | |
| while (current && current.type === 'MemberExpression') { | |
| if (current.property) { | |
| if (current.property.type === 'Identifier') { | |
| parts.unshift(current.property.name); | |
| } else if (current.property.type === 'Literal') { | |
| parts.unshift(`[${current.property.raw}]`); | |
| } | |
| } | |
| current = current.object; | |
| } | |
| if (current && current.type === 'Identifier') { | |
| parts.unshift(current.name); | |
| } | |
| return parts.join(''); // Use join('') for bracket notation | |
| } | |
| return '<unknown>'; | |
| } |
| getImprovedWritabilityError(target, line) { | ||
| // Strip 'inav.' prefix if present | ||
| const normalizedTarget = target.startsWith('inav.') ? target.substring(5) : target; | ||
| const parts = normalizedTarget.split('.'); | ||
|
|
||
| // Only applies to override namespace for now | ||
| if (parts[0] !== 'override') { | ||
| return null; | ||
| } | ||
|
|
||
| // Check if trying to assign to a 3-level intermediate object | ||
| // E.g., override.flightAxis.yaw (should be override.flightAxis.yaw.angle or .rate) | ||
| if (parts.length === 3) { | ||
| const overrideDef = this.getOverrideDefinition(parts[1], parts[2]); | ||
|
|
||
| if (overrideDef && overrideDef.type === 'object' && overrideDef.properties) { | ||
| const availableProps = Object.keys(overrideDef.properties); | ||
| const suggestions = availableProps.map(p => `inav.override.${parts[1]}.${parts[2]}.${p}`).join(', '); | ||
| return `Cannot assign to '${target}' - it's an object, not a property. Available properties: ${suggestions}`; | ||
| } | ||
| } | ||
|
|
||
| // Check if trying to assign to a 2-level intermediate object | ||
| // E.g., override.vtx (should be override.vtx.power, etc.) | ||
| // or override.flightAxis (should be override.flightAxis.roll.angle, etc.) | ||
| if (parts.length === 2) { | ||
| const categoryDef = this.getOverrideCategoryDefinition(parts[1]); | ||
|
|
||
| if (categoryDef && categoryDef.type === 'object' && categoryDef.properties) { | ||
| const propKeys = Object.keys(categoryDef.properties); | ||
|
|
||
| // Check if properties are simple (like vtx.power) or nested (like flightAxis.roll.angle) | ||
| const firstProp = categoryDef.properties[propKeys[0]]; | ||
|
|
||
| if (firstProp && firstProp.type === 'object' && firstProp.properties) { | ||
| // Deeply nested (like flightAxis.roll.angle) | ||
| const nestedPropKeys = Object.keys(firstProp.properties); | ||
| const suggestions = propKeys.slice(0, 2).flatMap(p => { | ||
| const nested = categoryDef.properties[p]; | ||
| if (nested && nested.properties) { | ||
| const firstNestedProp = Object.keys(nested.properties)[0]; | ||
| return [`inav.override.${parts[1]}.${p}.${firstNestedProp}`]; | ||
| } | ||
| return []; | ||
| }).join(', '); | ||
| return `Cannot assign to '${target}' - it's an object, not a property. Examples: ${suggestions}, ...`; | ||
| } else { | ||
| // Simple properties (like vtx.power, vtx.band, vtx.channel) | ||
| const suggestions = propKeys.map(p => `inav.override.${parts[1]}.${p}`).join(', '); | ||
| return `Cannot assign to '${target}' - it's an object, not a property. Available properties: ${suggestions}`; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Get override definition for a specific property | ||
| * @private | ||
| */ | ||
| getOverrideDefinition(category, property) { | ||
| try { | ||
| // Access raw API definitions, not processed structure | ||
| const overrideDefs = apiDefinitions.override; | ||
| if (!overrideDefs) return null; | ||
|
|
||
| // For nested objects like flightAxis, check if the property itself has properties | ||
| if (overrideDefs[category] && overrideDefs[category].properties) { | ||
| return overrideDefs[category].properties[property]; | ||
| } | ||
|
|
||
| return null; | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } |
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.
Suggestion: Avoid duplicating raw apiDefinitions traversal in the analyzer; reuse PropertyAccessChecker’s object/children introspection helpers so “what is an object vs leaf” logic stays consistent in one place. [Learned best practice, importance: 5]
| getImprovedWritabilityError(target, line) { | |
| // Strip 'inav.' prefix if present | |
| const normalizedTarget = target.startsWith('inav.') ? target.substring(5) : target; | |
| const parts = normalizedTarget.split('.'); | |
| // Only applies to override namespace for now | |
| if (parts[0] !== 'override') { | |
| return null; | |
| } | |
| // Check if trying to assign to a 3-level intermediate object | |
| // E.g., override.flightAxis.yaw (should be override.flightAxis.yaw.angle or .rate) | |
| if (parts.length === 3) { | |
| const overrideDef = this.getOverrideDefinition(parts[1], parts[2]); | |
| if (overrideDef && overrideDef.type === 'object' && overrideDef.properties) { | |
| const availableProps = Object.keys(overrideDef.properties); | |
| const suggestions = availableProps.map(p => `inav.override.${parts[1]}.${parts[2]}.${p}`).join(', '); | |
| return `Cannot assign to '${target}' - it's an object, not a property. Available properties: ${suggestions}`; | |
| } | |
| } | |
| // Check if trying to assign to a 2-level intermediate object | |
| // E.g., override.vtx (should be override.vtx.power, etc.) | |
| // or override.flightAxis (should be override.flightAxis.roll.angle, etc.) | |
| if (parts.length === 2) { | |
| const categoryDef = this.getOverrideCategoryDefinition(parts[1]); | |
| if (categoryDef && categoryDef.type === 'object' && categoryDef.properties) { | |
| const propKeys = Object.keys(categoryDef.properties); | |
| // Check if properties are simple (like vtx.power) or nested (like flightAxis.roll.angle) | |
| const firstProp = categoryDef.properties[propKeys[0]]; | |
| if (firstProp && firstProp.type === 'object' && firstProp.properties) { | |
| // Deeply nested (like flightAxis.roll.angle) | |
| const nestedPropKeys = Object.keys(firstProp.properties); | |
| const suggestions = propKeys.slice(0, 2).flatMap(p => { | |
| const nested = categoryDef.properties[p]; | |
| if (nested && nested.properties) { | |
| const firstNestedProp = Object.keys(nested.properties)[0]; | |
| return [`inav.override.${parts[1]}.${p}.${firstNestedProp}`]; | |
| } | |
| return []; | |
| }).join(', '); | |
| return `Cannot assign to '${target}' - it's an object, not a property. Examples: ${suggestions}, ...`; | |
| } else { | |
| // Simple properties (like vtx.power, vtx.band, vtx.channel) | |
| const suggestions = propKeys.map(p => `inav.override.${parts[1]}.${p}`).join(', '); | |
| return `Cannot assign to '${target}' - it's an object, not a property. Available properties: ${suggestions}`; | |
| } | |
| } | |
| } | |
| return null; | |
| } | |
| /** | |
| * Get override definition for a specific property | |
| * @private | |
| */ | |
| getOverrideDefinition(category, property) { | |
| try { | |
| // Access raw API definitions, not processed structure | |
| const overrideDefs = apiDefinitions.override; | |
| if (!overrideDefs) return null; | |
| // For nested objects like flightAxis, check if the property itself has properties | |
| if (overrideDefs[category] && overrideDefs[category].properties) { | |
| return overrideDefs[category].properties[property]; | |
| } | |
| return null; | |
| } catch (error) { | |
| return null; | |
| } | |
| } | |
| getImprovedWritabilityError(target, line) { | |
| const normalizedTarget = target.startsWith('inav.') ? target.substring(5) : target; | |
| const parts = normalizedTarget.split('.'); | |
| if (parts[0] !== 'override') return null; | |
| // override.flightAxis.yaw -> suggest deeper props using the checker’s source-of-truth helpers | |
| if (parts.length === 3) { | |
| const [_, parentProp, childProp] = parts; | |
| if (this.propertyAccessChecker.isPropertyAnObject('override', parentProp, childProp)) { | |
| const deeperProps = this.propertyAccessChecker.getNestedObjectProperties('override', parentProp, childProp) || []; | |
| if (deeperProps.length > 0) { | |
| const suggestions = deeperProps.map(p => `inav.override.${parentProp}.${childProp}.${p}`).join(', '); | |
| return `Cannot assign to '${target}' - it's an object, not a property. Available properties: ${suggestions}`; | |
| } | |
| } | |
| } | |
| ... | |
| return null; | |
| } |
User description
Summary
Improves JavaScript Programming transpiler validation to catch invalid code with helpful error messages instead of silently dropping code or failing with cryptic messages.
Changes
Validation Improvements:
inav.override.flightAxis(180))inav.flightwithout a property)Error Message Enhancements:
Test Coverage:
test_examples.mjs- regression test for all 22 examplestest_validation_fixes.mjs- tests for new validation featuresi18n Fixes:
Files Changed
js/transpiler/transpiler/parser.js- Invalid function call detectionjs/transpiler/transpiler/analyzer.js- Improved error messages for assignmentsjs/transpiler/transpiler/property_access_checker.js- Enhanced intermediate object detectionjs/transpiler/transpiler/index.js- Fail transpilation on parser errorstabs/javascript_programming.js&.html- i18n display fixeslocale/en/messages.json- i18n texttest_examples.mjs&test_validation_fixes.mjs- New test filesTesting
Examples
Before:
inav.override.vtx = 3;silently fails or shows "Cannot assign to 'inav.override.vtx'. Not a valid INAV writable property."After: Clear error: "Cannot assign to 'inav.override.vtx' - it's an object, not a property. Available properties: inav.override.vtx.power, inav.override.vtx.band, inav.override.vtx.channel"
PR Type
Bug fix, Enhancement
Description
This description is generated by an AI tool. It may have inaccuracies
Detects invalid function calls and intermediate object usage with clear error messages instead of silently dropping code
Improves error messages by showing available properties for nested objects and suggesting correct access patterns
Fails transpilation when parser errors occur, preventing invalid code from compiling
Adds comprehensive test coverage with regression tests for all 22 examples and 8 validation scenarios
Fixes missing i18n localization in JavaScript Programming tab and improves paragraph spacing
Diagram Walkthrough
File Walkthrough
4 files
Detect invalid function calls with error reportingFail transpilation when parser errors presentAdd i18n localization to tab initializationFix paragraph spacing in warning text2 files
Improve error messages for invalid assignmentsDetect intermediate objects and nested property validation1 files
Update JavaScript beta warning message text2 files
Add regression test for all 22 examplesAdd validation test suite for error detection