Skip to content

Replaced std.debug.print with @compileError for unsupported types#2

Merged
RomarQ merged 2 commits intomainfrom
rq/try-ci
Dec 30, 2025
Merged

Replaced std.debug.print with @compileError for unsupported types#2
RomarQ merged 2 commits intomainfrom
rq/try-ci

Conversation

@RomarQ
Copy link
Owner

@RomarQ RomarQ commented Dec 30, 2025

No description provided.

@claude
Copy link

claude bot commented Dec 30, 2025

Claude finished @RomarQ's task in 44s —— View job


Review Summary

Status: ✅ Approved

This is a small but valuable improvement that shifts unsupported type detection from runtime to compile-time.


Changes Overview

The PR replaces runtime error handling with compile-time errors in two files:

File Change
src/encoder.zig 2 locations: unsupported pointer types and catch-all else branch
src/util.zig 2 locations: same pattern in calculateEncodedSize

Analysis

✅ Correctness

The change is correct. Using @compileError for unsupported types is the right approach because:

  1. The encode and calculateEncodedSize functions use comptime type information via @typeInfo
  2. Unsupported types can be detected at compile-time since the type is known statically
  3. This catches type errors during compilation rather than at runtime

✅ Zig Idioms

This follows Zig best practices:

  • Prefer compile-time checks over runtime checks when possible
  • Use @compileError to provide clear, actionable error messages
  • Error messages include the type name for easy debugging

✅ Memory Safety

No memory safety concerns - this is purely a compile-time change.

✅ Error Handling

The previous approach had a subtle issue: error.UnsupportedType would be returned at runtime, but since these functions operate on comptime-known types, the error could never actually occur dynamically. The new approach correctly makes this a compile-time error.

✅ Code Quality

  • Removes unnecessary std.debug.print calls that would pollute stderr
  • Removes the now-unused error.UnsupportedType from the error set
  • Error messages are descriptive and include context (e.g., "for SCALE encoding" vs "for SCALE size calculation")

Minor Observation

The error messages are well-crafted and distinguish between the two contexts:

  • "Unsupported pointer type for SCALE encoding: "
  • "Unsupported type for SCALE encoding: "
  • "Unsupported pointer type for SCALE size calculation: "
  • "Unsupported type for SCALE size calculation: "

This helps developers quickly identify where the issue originates.


Verdict

LGTM! This is a clean improvement that catches errors earlier in the development cycle. No changes requested.

@claude
Copy link

claude bot commented Dec 30, 2025

Claude finished @RomarQ's task in 1m 54s —— View job


Review Summary

Status:Approved with Strong Recommendation

This PR makes an excellent improvement by converting runtime error handling to compile-time errors for unsupported types in the SCALE codec.


Changes Overview

The PR modifies 2 files with 4 total replacements:

File Lines Changed Change Description
src/encoder.zig 270, 282 Replace std.debug.print + runtime error with @compileError
src/util.zig 53, 86 Replace std.debug.print + runtime error with @compileError

Before:

std.debug.print("Unsupported type: {s}\n", .{@typeName(T)});
return error.UnsupportedType;

After:

@compileError("Unsupported pointer type for SCALE encoding: " ++ @typeName(T));

Detailed Analysis

1. Correctness

The change is absolutely correct. Here's why:

  • Both encode() and calculateEncodedSize() are generic functions that dispatch based on @typeInfo(T), which is evaluated at compile-time
  • The type T is known statically via @TypeOf(value) — there's no runtime type uncertainty
  • Unsupported types can and should be caught during compilation, not at runtime
  • This prevents impossible-to-reach error paths from polluting the error set

Key insight: Since these functions use comptime type introspection, any unsupported type will always fail at the same point in the switch statement during compilation. There's no valid code path where an unsupported type could be detected only at runtime.

2. Zig Idioms & Best Practices

This change exemplifies excellent Zig practices:

  • Compile-time over runtime: Zig strongly encourages catching errors as early as possible. Using @compileError for statically-knowable failures is the idiomatic approach.

  • Clear error messages: Each error message is context-specific:

    • "Unsupported pointer type for SCALE encoding: " (encoder.zig:270)
    • "Unsupported type for SCALE encoding: " (encoder.zig:282)
    • "Unsupported pointer type for SCALE size calculation: " (util.zig:53)
    • "Unsupported type for SCALE size calculation: " (util.zig:86)

    This helps developers immediately understand which function and which code path triggered the error.

  • Type information in errors: Including @typeName(T) in the error message provides actionable debugging information.

3. Memory Safety

No memory safety concerns. This is purely a compile-time change that doesn't affect runtime memory behavior.

4. Error Handling

Major improvement:

  • Before: error.UnsupportedType was part of the inferred error set, but could never actually occur at runtime (since unsupported types are statically known)
  • After: The error is properly categorized as a compile-time issue, removing unreachable error paths from the runtime error set
  • Side benefit: Removed unnecessary std.debug.print calls that would pollute stderr

Note: The error set EncodeError correctly does NOT include UnsupportedType — it only contains BufferTooSmall and InvalidCompactValue, which are legitimate runtime errors.

5. Code Quality

Improvements:

  • Removes debug noise (std.debug.print to stderr)
  • Cleaner error reporting (compile errors are more visible than runtime errors in development)
  • Reduces runtime error surface area
  • More maintainable (developers get instant feedback when using unsupported types)

Consistency:
All four locations follow the same pattern, ensuring uniform error handling across the codebase.

⚠️ 6. Test Coverage

Observation: There are no compile-time tests for unsupported types.

While this isn't strictly necessary (the compiler will reject invalid code), it could be valuable to add negative compilation tests to document which types are intentionally unsupported.

Recommendation (optional enhancement):
Consider adding test cases that verify certain types fail to compile, such as:

test "unsupported types fail at compile time" {
    // This would be a compile-time test framework, if Zig supports it
    // Alternatively, document unsupported types in comments
}

However, this is not blocking — the current implementation is sound without these tests.

7. Documentation

The error messages themselves serve as inline documentation. Each error clearly states:

  1. What failed (pointer type vs general type)
  2. Where it failed (encoding vs size calculation)
  3. Which type caused the issue (via @typeName(T))

No additional documentation is needed for this change.


Comparison to Previous Review

The previous Claude review (from earlier today) correctly identified this as a valuable improvement. This updated review confirms:

  • No regressions have been introduced since the last review
  • The implementation remains correct and idiomatic
  • All observations from the previous review still hold

Minor Observations

Excellent Error Message Differentiation

The PR distinguishes between:

  1. Pointer-specific errors (e.g., unsupported pointer types like multi-item pointers)
  2. General type errors (e.g., floats, enums without union tags, etc.)

This granularity helps developers quickly identify the issue.

Alignment with SCALE Spec

The SCALE codec specification defines a limited set of supported types. By making type validation compile-time, this implementation enforces spec compliance at the earliest possible stage.


Verdict

LGTM! 🚀

This is a textbook example of improving code quality through Zig's compile-time capabilities. The change:

  • Catches errors earlier in the development cycle
  • Removes unreachable runtime error paths
  • Provides clearer, more actionable error messages
  • Follows Zig best practices

No changes requested.


Recommended Next Steps (Optional)

  1. Consider documenting supported types in a comment or doc comment near the encode() function
  2. Add examples of unsupported types in documentation (e.g., "raw pointers, function pointers, and multi-item pointers are not supported")

These are suggestions for future enhancements, not blockers for this PR.


@RomarQ RomarQ merged commit d72c0fd into main Dec 30, 2025
2 checks passed
@RomarQ RomarQ deleted the rq/try-ci branch December 30, 2025 21:09
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.

1 participant