feat(es/ast): Add backward compatibility to AST bytes

[kdy1]

Description:

Things to note:

• This will make performance noticeably worse.
• This will make the API for Rust side users pretty bad.

I think we may need to use a proc macro for this, to copy the whole AST types into new types with the extras: UnknownBytes<TYPE_NAME, PADDING_SIZE> attribute.

Related issue (if exists):

[changeset-bot]

⚠️ No Changeset found

Latest commit: 0e93b5d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Claude finished @kdy1's task —— View job

---

Code Review Analysis

I'll provide a comprehensive review of this backward compatibility implementation for AST bytes.

Review Tasks:

• Read repository guidelines (CLAUDE.md)
• Examine the new compat.rs file
• Review changes to class.rs
• Review changes to lib.rs
• Analyze backward compatibility approach
• Assess performance implications
• Check for potential bugs or security issues
• Evaluate test coverage needs
• Provide final review feedback

Detailed Review

Architecture & Design

Strengths:

• Clean separation of concerns with dedicated compat.rs module
• Type-safe approach using const generics for KIND and LEN parameters
• Consistent naming convention with KIND_CLASS constant
• Minimal API surface with simple new() constructor and Default implementation

Areas of Concern:

• Limited to only Class struct currently - inconsistent coverage across AST types
• Hard-coded 16-byte padding size lacks documentation or justification
• No version tracking mechanism for future compatibility needs

Implementation Quality

Code Quality: ⭐⭐⭐⭐⭐

• Well-structured, idiomatic Rust code
• Proper trait implementations (Debug, Clone, Copy, etc.)
• Consistent with existing codebase patterns

Type Safety: ⭐⭐⭐⭐⭐

• Excellent use of const generics to prevent type confusion
• Compile-time size guarantees through const parameters

Performance Impact Assessment

Memory Overhead: ⚠️ Significant

• Adds 16 bytes to every Class instance (as noted by author)
• Current ArchivedClass size is 64 bytes, so this represents a 25% increase
• Will impact heap usage, cache locality, and serialization performance

Runtime Performance: ⚠️ Moderate Impact

• Extra field initialization/copying in struct operations
• Serialization/deserialization overhead
• Memory bandwidth impact from larger struct sizes

Recommendations:

• Consider making this opt-in via feature flag
• Document the exact performance benchmarks
• Evaluate if 16 bytes is the optimal size

Security & Correctness

Security: ✅ No Issues Found

• UnknownBytes properly encapsulates byte array
• No unsafe code or potential for buffer overflows
• Default implementation safely zeros the buffer

Correctness: ⚠️ Potential Issues

• extras field in class.rs:49 lacks #[cfg_attr(feature = "serde-impl", serde(default))] annotation
• This could break serde deserialization for existing serialized data
• The field should be marked as #[serde(skip)] or #[serde(default)] to maintain compatibility

API Design Concerns

1. Inconsistent Coverage: Only Class has the extras field. This creates an inconsistent API where some AST nodes support backward compatibility and others don't.

2. Poor Rust API: As the author noted, this makes the Rust API worse by exposing implementation details to users.

3. No Forward Compatibility: The current approach doesn't provide a clear path for handling future schema changes.

Missing Test Coverage

Critical Gap: No tests found for:

• Serialization/deserialization with UnknownBytes
• Backward compatibility with existing serialized data
• Default initialization behavior
• Size assertions for the modified Class struct

Alternative Approaches to Consider

1. Proc Macro Solution (as author suggested):

   #[backward_compat(padding = 16)]
   pub struct Class { /* ... */ }

2. Feature-gated Approach:

   #[cfg(feature = "backward-compat")]
   pub extras: UnknownBytes<KIND_CLASS, 16>,

3. Version-aware Serialization:
   Use schema versioning instead of padding

Recommendations

Must Fix:

1. Add #[cfg_attr(feature = "serde-impl", serde(default))] to extras field in class.rs:49
2. Add comprehensive tests for serialization compatibility
3. Document the 16-byte choice and its implications

Should Consider:

1. Make this feature opt-in via feature flag to avoid performance penalty for users who don't need it
2. Provide consistent coverage across all AST types, not just Class
3. Add benchmark tests to quantify performance impact
4. Consider the proc macro approach for cleaner API

Nice to Have:

1. Add documentation examples
2. Consider schema versioning as alternative
3. Provide migration guide for existing users

Final Assessment

This is a functional but incomplete implementation of backward compatibility. While the code quality is high and the approach is sound, the partial implementation (only Class) and potential serde compatibility issues need addressing.

Recommendation: 🔶 Approve with Required Changes

The core approach is solid, but the serde compatibility issue must be fixed before merging, and the scope should be expanded to other AST types for consistency.

---