Skip to content

fix: implement proper serde handling for unknown AST node types #281

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

Closed
wants to merge 2 commits into from

Conversation

mattsse
Copy link
Member

@mattsse mattsse commented Jun 10, 2025

Summary

Fixes #280 by implementing proper serde handling for the NodeType enum's Other variant to catch unknown AST node types.

Problem

The NodeType enum had an Other(String) variant intended to handle unknown AST node types, but it wasn't working because:

  • Serde's default enum serialization uses tagged format: {"Other": "UnknownType"}
  • Solidity AST uses simple strings: "UnknownType"
  • Unknown types caused deserialization errors instead of falling back to Other

Root Cause Analysis

When encountering an unknown node type like "SomeNewNodeType", the deserializer would:

  1. Try to match it against known variants (fail)
  2. Try to deserialize as a tagged enum (fail - no object structure)
  3. Return a deserialization error instead of using the Other variant

Solution

Implemented custom Serialize and Deserialize traits for NodeType:

  • Custom serialization: All variants serialize as simple strings
  • Custom deserialization: Known strings map to specific variants, unknown strings map to Other(String)
  • Backwards compatibility: All existing functionality remains unchanged

Implementation Details

// Before (broken)
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
pub enum NodeType {
    Assignment,
    // ... other variants
    Other(String),
}

// After (working)  
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum NodeType {
    Assignment,
    // ... other variants
    Other(String), 
}

impl serde::Serialize for NodeType { /* custom implementation */ }
impl<'de> serde::Deserialize<'de> for NodeType { /* custom implementation */ }

Testing

Added comprehensive tests covering:

  • Unknown node type deserialization → Other variant
  • Known node type deserialization (unchanged behavior)
  • Roundtrip serialization for all node types
  • Complete AST parsing with unknown node types
  • Mixed known/unknown node structures

Test Results

cargo test -p foundry-compilers-artifacts-solc lowfidelity
# running 7 tests
# test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured

Benefits

  • Future-proof: Can handle new AST node types from future Solidity versions
  • Zero breaking changes: All existing code continues to work unchanged
  • Robust error handling: Gracefully handles unknown types instead of failing
  • Comprehensive testing: Ensures reliability across different scenarios

AI Development Process

This fix was developed using Claude Code with the following prompts:

Initial prompt:

fix reported issue #280 by following the suggestion and add an Other variant that catches unknown AST nodes

Follow-up request:

also add additional serde tests that ensure this is still working as before but now can handle unknown types properly

The development process included:

  1. Research Phase: Used search tools to understand the codebase and locate the issue
  2. Problem Analysis: Identified the serde serialization mismatch through code examination
  3. Solution Design: Implemented custom serde traits to handle both known and unknown variants
  4. Testing: Created comprehensive test suite covering edge cases and backwards compatibility
  5. Validation: Ran full test suite to ensure no regressions

This ensures the library can handle future Solidity compiler versions without breaking existing code.

Closes #280

## Problem

The `NodeType` enum had an `Other(String)` variant intended to catch unknown
AST node types, but it wasn't working correctly. The issue was that serde's
default enum serialization uses tagged format (e.g., `{"Other": "UnknownType"}`),
but Solidity AST serializes node types as simple strings (e.g., `"UnknownType"`).

When encountering an unknown node type like `"SomeNewNodeType"`, the deserializer
would fail instead of falling back to the `Other` variant.

## Solution

Implemented custom `Serialize` and `Deserialize` traits for `NodeType`:

- **Custom serialization**: All variants serialize as simple strings
- **Custom deserialization**: Known strings map to specific variants, unknown
  strings map to `Other(String)`
- **Backwards compatibility**: All existing functionality remains unchanged

## Testing

Added comprehensive tests covering:
- Unknown node type deserialization → `Other` variant
- Known node type deserialization (unchanged behavior)
- Roundtrip serialization for all node types
- Complete AST parsing with unknown node types
- Mixed known/unknown node structures

This ensures the library can handle future Solidity compiler versions that
introduce new AST node types without breaking existing code.

Closes #280

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@mattsse mattsse requested review from DaniPopes and klkvr as code owners June 10, 2025 11:12
Replace NodeType:: with Self:: in match patterns to follow Rust idioms
and reduce unnecessary structure name repetition.

No functional changes - purely cosmetic improvements.
@mattsse
Copy link
Member Author

mattsse commented Jun 10, 2025

better to just add the missing variants

@mattsse mattsse closed this Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NodeType enum fails to deserialize unknown AST node types
1 participant