Add fixed-size Rust primitive type support

[carllerche]

This change adds support for Rust's pointer-sized integer types (isize and usize) to match the existing support for fixed-size integers (i8, i16, i32, i64, u8, u16, u32, u64).

Changes include:

• Add Isize and Usize variants to stmt::Type and stmt::Value enums
• Implement conversion methods and TryFrom traits for isize/usize
• Add Primitive trait implementations
• Map to appropriate database types (8 bytes on 64-bit, 4 bytes on 32-bit)
• Update all database drivers (SQLite, MySQL, PostgreSQL, DynamoDB)
• Add IntoExpr implementations for code generation
• Add comprehensive tests for both types

[claude]

Pull Request Review: Add fixed-size Rust primitive type support

I've reviewed this PR which adds support for isize and usize types. Overall, this is a well-designed and clean implementation that follows Toasty's architecture principles. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Architecture Decision: Mapping isize/usize to i64/u64 internally is the right approach. This avoids:

   • Platform-dependent behavior in the database layer
   • Complexity in drivers and SQL generation
   • Schema migration issues across platforms

2. Minimal Surface Area: The implementation only touches necessary layers:

   • Core value conversions (crates/toasty-core/src/stmt/num.rs)
   • Primitive trait implementations (crates/toasty/src/stmt/primitive.rs)
   • Expression generation (crates/toasty/src/stmt/into_expr.rs)
   • Comprehensive tests (tests/tests/tys.rs)

3. Consistent Patterns: The code follows established patterns for similar numeric types, making it easy to understand and maintain.

4. Good Test Coverage: Tests cover:

   • Round-trip persistence
   • Multiple records with different values
   • Update chains
   • Edge cases (MIN/MAX values)
   • Raw storage verification

---

⚠️ Potential Issues & Concerns

1. Platform-Specific Test Values May Fail on 32-bit Systems

Location: tests/tests/tys.rs:86, 91

The test arrays include literal values that exceed 32-bit range:

isize => &[isize::MIN, -1000000000000, ..., 4611686018427387903, isize::MAX]
usize => &[..., 9223372036854775807, 10000000000000000000, usize::MAX]

Problem: On 32-bit platforms:

• isize::MAX = 2,147,483,647 (not 9,223,372,036,854,775,807)
• usize::MAX = 4,294,967,295 (not 18,446,744,073,709,551,615)
• The literal 1000000000000 exceeds isize::MAX on 32-bit and will fail to compile

Impact: Tests will fail to compile on 32-bit targets.

Recommendation: Use platform-aware test values or conditional compilation:

isize => &[isize::MIN, isize::MIN/2, -1, 0, 1, isize::MAX/2, isize::MAX] => ty_isize;
usize => &[usize::MIN, 0, 1, usize::MAX/4, usize::MAX/2, usize::MAX] => ty_usize;

2. Missing Overflow Documentation

Location: Commit message and crates/toasty-core/src/stmt/num.rs:297-323

Problem: The PR description states types "map to appropriate database types (8 bytes on 64-bit, 4 bytes on 32-bit)" but this is misleading. The implementation:

• Always maps to i64/u64 (8 bytes) regardless of platform
• Values are checked at conversion time (lines 302-307, 316-321)
• On 32-bit platforms, values outside the 32-bit range will fail during TryFrom<Value>

Recommendation: Update the PR description to clarify that isize/usize map to 64-bit types internally, with runtime overflow checks on 32-bit platforms.

3. Potential Data Loss on 32-bit Platforms

Scenario:

1. User saves usize::MAX (= 4,294,967,295 on 32-bit) to database
2. Database stores it as u64 value
3. User later deploys on 64-bit system, inserts larger value (e.g., 5,000,000,000)
4. User redeploys on 32-bit system
5. Loading the value fails with overflow error

Severity: Low (32-bit platforms are increasingly rare for server deployments)

Recommendation: Consider documenting this limitation in code comments or user-facing documentation.

---

🔍 Code Quality

Positive:

• Clean, idiomatic Rust code
• Proper error handling with descriptive messages
• Consistent with existing numeric type implementations
• No unsafe code or security concerns

Minor Suggestion:

Add inline comments explaining the design decision:

// Pointer-sized integers map to fixed-size types internally to avoid
// platform-dependent database schemas. Values are checked for overflow
// at the Rust boundary (model load/store), not in the database layer.
impl Primitive for isize {
    fn ty() -> stmt::Type {
        stmt::Type::I64  // Always 64-bit internally, regardless of platform
    }
    // ...
}

---

🛡️ Security Considerations

No security concerns identified. The implementation:

• Uses safe conversions with explicit overflow checks
• Follows Rust's type safety guarantees
• Does not introduce injection risks or memory safety issues

---

🎯 Performance

No performance concerns. The implementation:

• Uses zero-cost abstractions (trait-based dispatch)
• Performs conversions at model boundaries, not in hot loops
• Adds no runtime overhead compared to explicit i64/u64 usage

---

📋 Recommendations

Before merging:

1. ✅ Fix test values to work on 32-bit platforms (high priority)
2. ✅ Update PR description to clarify the 64-bit internal representation
3. 🔵 Consider adding a doc comment explaining the design trade-off (optional)

After merging:

1. Consider documenting this behavior in user-facing documentation
2. Add a test that explicitly validates overflow behavior on value load

---

📊 Summary

This is a high-quality implementation that makes the right design choices. The main issue is platform-specific test values that will break on 32-bit systems. Once fixed, this PR is ready to merge.

Overall Rating: ⭐⭐⭐⭐ (4/5) - Solid implementation with minor test issues

---

Files reviewed:

• ✅ crates/toasty-core/src/stmt/num.rs - Clean implementation
• ✅ crates/toasty/src/stmt/primitive.rs - Follows established patterns
• ✅ crates/toasty/src/stmt/into_expr.rs - Correct conversions
• ⚠️ tests/tests/tys.rs - Test values need platform-awareness

Great work! 🎉

[claude]

PR Review: Add fixed-size Rust primitive type support

Thank you for this contribution! I've reviewed the PR against Toasty's architecture and the documented pattern for adding primitive types (see docs/CHANGE_GUIDE.md § "Adding a New Primitive Type"). While the implementation looks well-structured, there are several critical missing pieces that need to be addressed.

❌ Critical Issues

1. Missing Type and Value Enum Variants

The PR title mentions "fixed-size" primitives, but the implementation actually adds pointer-sized types (isize/usize). However, the core type system is incomplete:

Missing in crates/toasty-core/src/stmt/ty.rs:

• No Isize or Usize variants in the Type enum
• The implementation maps to I64/U64 in Primitive::ty(), but this loses type information at the schema level

Missing in crates/toasty-core/src/stmt/value.rs:

• No Isize(isize) or Usize(usize) variants in the Value enum
• The implementation converts through i64/u64, which means values are stored as their fixed-size equivalents

Analysis: According to the documented pattern (docs/CHANGE_GUIDE.md steps 2-3), new primitive types should have dedicated variants in both enums. The current approach of converting through fixed-size types will work but:

• Loses semantic type information (can't distinguish isize from i64 in the AST)
• May cause issues with cross-type conversions in Value::is_a()
• Doesn't follow the established pattern for other primitives

2. Missing Helper Methods in Type

Missing in crates/toasty-core/src/stmt/num.rs:

The impl_num! macro (lines 65-106) generates helper methods for all other numeric types:

• Type::is_i8(), is_i16(), etc.
• Value::to_i8(), to_i8_unwrap(), etc.

But isize/usize are not included in this macro invocation, which means:

• No Type::is_isize() or Type::is_usize() methods
• No Value::to_isize() or Value::to_usize() methods
• The Value::is_a() method (value.rs:137-178) doesn't handle these types

This is inconsistent with other numeric types and will break code that expects these helper methods.

3. Missing Value::infer_ty() Support

Issue in crates/toasty-core/src/stmt/value.rs:180-200:

The Value::infer_ty() method doesn't handle isize/usize variants (because they don't exist). This is critical for type inference throughout the engine.

4. Platform-Specific Test Values

Issue in tests/tests/tys.rs:86,91:

isize => &[isize::MIN, -1000000000000, -1, 0, 1, 1000000000000, 4611686018427387903, isize::MAX] => ty_isize;
usize => &[usize::MIN, 0, 1, 1000000000000, 9223372036854775807, 10000000000000000000, usize::MAX] => ty_usize;

Problem: The literal values 1000000000000, 4611686018427387903, and 10000000000000000000 will not compile on 32-bit platforms because they exceed the range of 32-bit integers.

On 32-bit systems:

• isize::MIN = -2,147,483,648
• isize::MAX = 2,147,483,647
• usize::MAX = 4,294,967,295

Fix needed: Use conditional compilation or platform-appropriate test values:

#[cfg(target_pointer_width = "64")]
isize => &[isize::MIN, -1000000000000, -1, 0, 1, 1000000000000, isize::MAX] => ty_isize;

#[cfg(target_pointer_width = "32")]
isize => &[isize::MIN, -1000000, -1, 0, 1, 1000000, isize::MAX] => ty_isize;

⚠️ Design Question

Should pointer-sized types map to dedicated variants or convert through fixed-size types?

There are two valid approaches:

Option A: Dedicated variants (recommended for consistency)

• Add Isize and Usize to Type enum
• Add Isize(isize) and Usize(usize) to Value enum
• Include in impl_num! macro
• Follows the same pattern as all other numeric types
• Preserves type semantics in the AST

Option B: Proxy through fixed-size types (current approach)

• No new enum variants
• Convert to/from i64/u64 at boundaries
• Simpler implementation but loses type information
• May be acceptable if pointer-sized types are rarely used

The current PR is caught between these approaches - it doesn't add enum variants but also doesn't fully implement the proxy pattern. I recommend Option A for consistency with the existing codebase.

✅ What's Good

1. TryFrom implementations (num.rs:297-317) correctly handle conversions with proper error messages
2. IntoExpr implementations (into_expr.rs) correctly proxy through i64/u64
3. Primitive trait implementations (primitive.rs) use the same proxy pattern
4. Test structure follows the existing pattern with the def_num_ty_tests! macro

📋 Required Changes

According to docs/CHANGE_GUIDE.md, adding a primitive type requires:

1. ✅ toasty-core/src/stmt/ty.rs - ❌ Missing Type::Isize and Type::Usize variants
2. ✅ toasty-core/src/stmt/value.rs - ❌ Missing Value::Isize and Value::Usize variants
3. ⚠️ toasty-core/src/stmt/num.rs - Partial implementation; needs macro inclusion
4. ❓ toasty-codegen/src/schema/ty.rs - May need mapping from Rust isize/usize types
5. ❓ toasty-sql/src/serializer/ty.rs - Should already handle via db::Type::Integer/UnsignedInteger
6. ❓ Driver value.rs files - Likely work via i64/u64 conversions but untested
7. ⚠️ tests/tests/tys.rs - Added but with platform-specific bugs

🔍 Additional Concerns

1. Visit/VisitMut traits: If you add new Type or Value variants, verify that visitor patterns still work
2. Driver testing: No evidence that this works with actual databases. The tests use the tests!() macro but it's unclear if they pass
3. 32-bit CI: Does the CI test on 32-bit platforms? If not, the compilation errors won't be caught

🎯 Recommendation

Before merging, please:

1. Decide between Option A (dedicated variants) or Option B (full proxy pattern)
2. If Option A: Add enum variants and include in impl_num! macro
3. If Option B: Document why proxy pattern is acceptable for pointer-sized types
4. Fix platform-specific test value compilation errors
5. Verify tests actually pass (run cargo test ty_isize ty_usize)
6. Consider whether Toasty should even support pointer-sized integers (they're rarely used for database fields)

Let me know if you'd like help implementing any of these changes!

[claude]

Pull Request Review

Thank you for working on adding isize/usize support to Toasty! I've reviewed the changes and have identified several critical issues and concerns.

Critical Issues

1. Missing Core Enum Variants ❌

The most critical issue is that this PR does not actually add Isize/Usize variants to the core enums where they're required:

• Missing from crates/toasty-core/src/stmt/ty.rs: The Type enum needs Isize and Usize variants
• Missing from crates/toasty-core/src/stmt/value.rs: The Value enum needs Isize(isize) and Usize(usize) variants
• Missing from impl_num! macro: The macro invocation at num.rs:65-106 should include entries for isize/usize

The PR description claims to "Add Isize and Usize variants to stmt::Type and stmt::Value enums", but these changes are not present in the diff. Without these fundamental changes, the code added in this PR cannot actually work.

Expected pattern (following the guide in docs/CHANGE_GUIDE.md § "Adding a New Primitive Type"):

// In ty.rs Type enum:
pub enum Type {
    // ... existing variants
    I64,
    Isize,
    U64,
    Usize,
    // ...
}

// In value.rs Value enum:
pub enum Value {
    // ... existing variants  
    I64(i64),
    Isize(isize),
    U64(u64),
    Usize(usize),
    // ...
}

// In num.rs impl_num! invocation:
impl_num! {
    // ... existing entries
    I64(i64) { to_i64 to_i64_unwrap is_i64 }
    Isize(isize) { to_isize to_isize_unwrap is_isize }
    U64(u64) { to_u64 to_u64_unwrap is_u64 }
    Usize(usize) { to_usize to_usize_unwrap is_usize }
}

2. Incorrect PR Title ⚠️

The title "Add fixed-size Rust primitive type support" is misleading. This PR adds support for pointer-sized integers (isize/usize), not fixed-size integers. Fixed-size integers (i8, i16, i32, i64, u8, u16, u32, u64) already exist in Toasty.

Suggested title: "Add pointer-sized integer support (isize/usize)"

3. Missing Database Driver Updates ❌

According to docs/CHANGE_GUIDE.md, adding a primitive type requires updating all database drivers' value.rs files (step 6). The PR description claims to "Update all database drivers", but I don't see changes to:

• crates/toasty-driver-postgresql/src/value.rs
• crates/toasty-driver-mysql/src/value.rs
• crates/toasty-driver-sqlite/src/lib.rs
• crates/toasty-driver-dynamodb/src/* (if applicable)

Each driver needs to handle conversion of Isize/Usize variants to their database-specific types.

4. Missing SQL Type Mapping ❌

The PR needs to add SQL type mappings in crates/toasty-sql/src/serializer/ty.rs. Currently, it only handles db::Type::Integer(size) ranges, but doesn't define how isize/usize map to database types for schema generation.

Expected addition:

// In toasty-sql/src/serializer/ty.rs
db::Type::Integer(size) => match size {
    // ... existing mappings
    // On 64-bit: isize/usize = 8 bytes
    // On 32-bit: isize/usize = 4 bytes
    #[cfg(target_pointer_width = "64")]
    POINTER_SIZE => fmt!(cx, f, "BIGINT"),
    #[cfg(target_pointer_width = "32")]  
    POINTER_SIZE => fmt!(cx, f, "INTEGER"),
    // ...
}

5. Missing Visit/VisitMut Implementation ⚠️

If Type and Value enums gain new variants, the visitor traits in toasty-core/src/stmt/visit*.rs may need updates. Please verify whether visitor methods need to handle the new variants.

Design Concerns

6. Platform-Dependent Behavior 🤔

The current implementation maps isize/usize to i64/u64, which assumes a 64-bit platform. However:

• On 32-bit systems, isize/usize are 4 bytes
• On 64-bit systems, they're 8 bytes

Questions to consider:

• Should Toasty support 32-bit platforms for isize/usize?
• Should the schema generation use #[cfg(target_pointer_width)] to select the appropriate size?
• What happens if a database is created on a 64-bit system and read on a 32-bit system (or vice versa)?

The current approach of always using I64/U64 internally is pragmatic for 64-bit systems, but this assumption should be documented or made explicit with compile-time checks.

7. Semantic Concerns 💭

Question: What is the use case for isize/usize in an ORM?

Pointer-sized integers are typically used for:

• Array/vector indexing
• Memory sizes/offsets
• Platform-specific APIs

These use cases rarely need database persistence. In most domain models, developers should use explicitly-sized types (i32, i64) to ensure consistent behavior across platforms and prevent issues when moving data between systems.

Recommendation: Consider documenting when isize/usize should (and shouldn't) be used in models, perhaps with a note in the guide or a compile warning.

Code Quality

8. Good: Implementation Follows Patterns ✅

Where code exists, it follows Toasty's established patterns:

• Conversion through fixed-size equivalents is reasonable
• TryFrom implementations handle overflow correctly
• Test structure mirrors existing numeric type tests
• Error messages are clear

Testing

9. Incomplete Test Coverage ⚠️

The tests in tests/tests/tys.rs look reasonable, but:

• Need to verify tests actually compile and run (depends on fixing core enum issues)
• Should test boundary conditions between 32/64-bit ranges if supporting both
• Consider testing database round-trips with values near isize::MAX/MIN

Summary

This PR cannot be merged in its current state due to missing the core Type and Value enum variants. The implementation appears incomplete - either some commits are missing, or this was pushed prematurely.

Required changes before merge:

1. ✅ Add Isize/Usize variants to stmt::Type enum
2. ✅ Add Isize/Usize variants to stmt::Value enum
3. ✅ Add isize/usize to impl_num! macro invocation
4. ✅ Update all database drivers' value conversion logic
5. ✅ Add SQL type mapping in toasty-sql
6. ✅ Update visitor traits if needed
7. ✅ Fix PR title to accurately reflect changes
8. ✅ Document platform-dependent behavior

Recommended additions:

• Consider adding documentation on when to use isize/usize vs fixed-size types
• Add compile-time assertions about pointer width assumptions
• Consider whether 32-bit support is needed

Please let me know if you have questions about any of these points. Happy to help work through the implementation!