feat: provide better panic msgs and docs#153
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis set of changes introduces robust type assertion checks and centralized panic handling across schema validation and parsing logic. Type assertions on context value pointers are now performed using safe, checked idioms; if a type mismatch is detected, a standardized panic function is invoked with detailed diagnostic information. New constants and a utility for formatted panics are added, and a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Schema
participant Context
participant PanicUtil
User->>Schema: Validate(data, valuePtr)
Schema->>Context: Prepare context with valuePtr
Schema->>Schema: Type assertion on valuePtr (safe check)
alt Type assertion fails
Schema->>PanicUtil: Panicf("Type cast error...", context info)
PanicUtil-->>Schema: Panic triggered
else Type assertion succeeds
Schema->>Schema: Continue validation logic
end
Schema-->>User: Validation result or panic
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying zog with
|
| Latest commit: |
57f1f2a
|
| Status: | ⚡️ Build in progress... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
struct.go (1)
167-167: Consider updating this panic to match the new patternThis panic message could benefit from the same improvements applied at line 103 - using p.Panicf with the predefined constant and including context information. This would ensure consistency in error reporting throughout the codebase.
- panic(fmt.Sprintf("Struct is missing expected schema key: %s", key)) + p.Panicf(p.PanicMissingStructField, ctx.String(), key)preprocess.go (1)
52-53: Consider standardizing this panic message as wellWhile you've improved type assertion error messages, this panic message for incompatible types still uses a direct
panic(fmt.Sprintf(...))call rather than the newp.Panicffunction with a standardized message constant.Consider standardizing this panic message by creating a constant in
internals/panics.goand usingp.Panicf:- panic(fmt.Sprintf("Preprocessed should be passed in schema.Validate() a value pointer that is compatible with its returned type T. Either *T or **T. Got %T", v)) + p.Panicf(p.PanicIncompatiblePreprocessType, v)You would need to add the constant to
internals/panics.go:const PanicIncompatiblePreprocessType = "Zog Panic: Type Compatibility Error\nPreprocessed should be passed in schema.Validate() a value pointer that is compatible with its returned type T. Either *T or **T. Got %T\nFor more information see: https://zog.dev/panics#type-cast-errors"docs/docs/panics.md (1)
1-140: Well-structured documentation that clearly explains panic scenariosThe new documentation thoroughly explains when and why the library panics, with clear examples for different scenarios. This will be very helpful for users debugging issues.
The documentation provides comprehensive coverage of panic scenarios with practical examples and solutions.
Consider these minor grammar improvements for better readability:
- Line 14: Add a comma after "In practice this means that" and after "but"
- Line 14: Add a comma after "Most of the time"
- Line 80: Add a determiner before "Same thing will happen"
-In practice this means that Zog will never panic if the input data is wrong but it will panic if you configure it wrong. Most of the time "configured it wrong" means that you have made a mistake in your schema definition which puts Zog into an invalid state and results in a schema that can never succeed. +In practice, this means that Zog will never panic if the input data is wrong, but it will panic if you configure it wrong. Most of the time, "configured it wrong" means that you have made a mistake in your schema definition which puts Zog into an invalid state and results in a schema that can never succeed.-Same thing will happen if you incorrectly set the type in a z.Custom schema: +The same thing will happen if you incorrectly set the type in a z.Custom schema:🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: A comma might be missing here.
Context: ... fundamental assumptions is broken. In practice this means that Zog will never panic if...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~14-~14: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...l never panic if the input data is wrong but it will panic if you configure it wrong...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~14-~14: A comma might be missing here.
Context: ... if you configure it wrong. Most of the time "configured it wrong" means that you ha...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~80-~80: A determiner appears to be missing. Consider inserting it.
Context: ... but the value is of type MyString ``` Same thing will happen if you incorrectly se...(AI_EN_LECTOR_MISSING_DETERMINER)
internals/panics.go (1)
1-14: Consider adding documentation comments for exported identifiersThe constants and function are exported and should have documentation comments that explain their purpose and usage.
Consider adding documentation comments for the exported identifiers:
+// PanicTypeCast is the format string for panics caused by type assertion failures on value pointers const ( PanicTypeCast = "Zog Panic: Type Cast Error\n Current context: %s\n Expected valPtr type to correspond with type defined in schema. But it does not. Expected type: *%T, got: %T\nFor more information see: https://zog.dev/panics#type-cast-errors" PanicTypeCastCoercer = "Zog Panic: Type Cast Error\n Current context: %s\n Expected coercer return value to correspond with type defined in schema. But it does not. Expected type: *%T, got: %T\nFor more information see: https://zog.dev/panics#type-cast-errors" PanicMissingStructField = "Zog Panic: Struct Schema Definition Error\n Current context: %s\n Provided struct is missing expected schema key: %s.\n This means you have made a mistake in your schema definition.\nFor more information see: https://zog.dev/panics#schema-definition-errors" ) +// Panicf formats a panic message using fmt.Sprintf and triggers a panic with the resulting message func Panicf(format string, args ...any) { panic(fmt.Sprintf(format, args...)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
custom.go(2 hunks)docs/docs/core-design-decisions.md(1 hunks)docs/docs/panics.md(1 hunks)internals/contexts.go(2 hunks)internals/panics.go(1 hunks)internals/utils.go(1 hunks)preprocess.go(1 hunks)struct.go(1 hunks)struct_validate_test.go(1 hunks)zogSchema.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
struct.go (1)
internals/panics.go (2)
Panicf(11-13)PanicMissingStructField(8-8)
internals/contexts.go (1)
internals/utils.go (1)
SafeString(10-22)
preprocess.go (1)
internals/panics.go (2)
Panicf(11-13)PanicTypeCast(6-6)
🪛 LanguageTool
docs/docs/panics.md
[uncategorized] ~14-~14: A comma might be missing here.
Context: ... fundamental assumptions is broken. In practice this means that Zog will never panic if...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~14-~14: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...l never panic if the input data is wrong but it will panic if you configure it wrong...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~14-~14: A comma might be missing here.
Context: ... if you configure it wrong. Most of the time "configured it wrong" means that you ha...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~80-~80: A determiner appears to be missing. Consider inserting it.
Context: ... but the value is of type MyString ``` Same thing will happen if you incorrectly se...
(AI_EN_LECTOR_MISSING_DETERMINER)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
docs/docs/core-design-decisions.md (1)
15-15: Great documentation improvement!Converting the plain text reference to a clickable link improves navigation and helps users find detailed information about panic scenarios more easily.
internals/contexts.go (1)
213-215: Excellent addition for improved error diagnostics!The String() method provides a comprehensive string representation of SchemaCtx that will significantly improve panic messages, making them more informative and useful for debugging. The use of SafeString for handling potentially nil values is a good safety measure.
struct_validate_test.go (1)
219-232: Great test addition for validating panic behavior!This test effectively verifies that the validation correctly panics when there's a mismatch between schema field type (string) and struct field type (int). It provides good coverage for the improved panic handling implemented in this PR.
struct.go (1)
103-103: Improved panic message with better diagnostics!Using the centralized p.Panicf with the predefined panic message constant and including the context information via ctx.String() greatly improves the error diagnostics. The link to documentation in the panic message will help developers resolve issues more quickly.
internals/utils.go (1)
14-21: Robust & succinct – approvedThe recursive dereference loop elegantly handles arbitrarily-nested pointers while short-circuiting on
nil, resulting in predictable<nil>output for panic diagnostics. Nice touch!zogSchema.go (1)
90-95: 👍 Good coercer-cast safeguardAdding the checked assertion with a dedicated
PanicTypeCastCoercerpath uniformly enforces type safety after coercion. No further issues spotted here.preprocess.go (1)
36-41: Improved type assertion safety with better error messagingThe updated code replaces a direct type assertion with a safe type assertion plus explicit error handling. When a type mismatch occurs, it now provides detailed diagnostic information through a standardized panic message.
This change correctly aligns with the PR's goal of providing better panic messages and improves type safety by explicitly handling type assertion failures.
internals/panics.go (2)
5-9: Well-designed panic constants with detailed diagnostic informationThe panic message constants provide detailed diagnostic information, including the context, expected and actual types, and documentation links. This will make debugging much easier for users.
The panic message formatting is clear and includes all necessary details for effective debugging.
11-13: Clean utility function for standardized panic handlingThe
Panicffunction centralizes panic formatting and invocation, ensuring consistency across the codebase.This utility function will help maintain consistent panic handling throughout the library.
| ptr, ok := ctx.ValPtr.(*T) | ||
| if !ok { | ||
| p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr) | ||
| } | ||
| c.test.Func(ptr, ctx) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same nil-pointer hole exists during validation
c.test.Func(ptr, ctx) may dereference ptr. Mirror the safety guard suggested above:
ptr, ok := ctx.ValPtr.(*T)
if !ok {
p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr)
}
+if ptr == nil {
+ p.Panicf(p.PanicNilPointer, ctx.String(), ctx.DType, ctx.ValPtr)
+}
c.test.Func(ptr, ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ptr, ok := ctx.ValPtr.(*T) | |
| if !ok { | |
| p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr) | |
| } | |
| c.test.Func(ptr, ctx) | |
| } | |
| ptr, ok := ctx.ValPtr.(*T) | |
| if !ok { | |
| p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr) | |
| } | |
| if ptr == nil { | |
| p.Panicf(p.PanicNilPointer, ctx.String(), ctx.DType, ctx.ValPtr) | |
| } | |
| c.test.Func(ptr, ctx) | |
| } |
🤖 Prompt for AI Agents (early access)
In custom.go around lines 79 to 84, the code calls c.test.Func(ptr, ctx) without checking if ptr is nil, which can cause a nil-pointer dereference. Add a nil check for ptr after the type assertion and before calling c.test.Func. If ptr is nil, handle it appropriately, such as by panicking or returning an error, to prevent unsafe dereferencing.
| ptr, ok := ctx.ValPtr.(*T) | ||
| if !ok { | ||
| p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr) | ||
| } | ||
| *ptr = d | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against nil destination pointers to avoid runtime panics
ok only guarantees that ctx.ValPtr is of type *T; it might still be a nil pointer.
Dereferencing it on the next line (*ptr = d) will trigger a panic: runtime error: invalid memory address if the caller passes nil.
Given these helpers are frequently reused by end-users, crashing the whole process contradicts the goal of the PR (better-controlled panics).
ptr, ok := ctx.ValPtr.(*T)
if !ok {
p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr)
}
+if ptr == nil {
+ // NB: add PanicNilPointer to internals/panics.go for consistency.
+ p.Panicf(p.PanicNilPointer, ctx.String(), ctx.DType, ctx.ValPtr)
+}
*ptr = dAdding the extra check keeps the invariant “a panic only occurs through Panicf with actionable diagnostics” intact and eliminates a blind spot that would surface as an unstructured Go panic.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In custom.go around lines 51 to 56, the code checks if ctx.ValPtr is of type *T but does not verify if the pointer is nil before dereferencing it. To fix this, add a nil check for ptr after the type assertion and before dereferencing. If ptr is nil, invoke p.Panicf with appropriate diagnostic information to maintain controlled panics and avoid runtime panics caused by dereferencing nil pointers.
| destPtr, ok := ctx.ValPtr.(*T) | ||
| if !ok { | ||
| p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle nil destPtr before dereferencing
*destPtr = *defaultVal (and several subsequent writes) assume a non-nil pointer.
Insert the defensive check used for type-mismatch to also cover nil pointers:
destPtr, ok := ctx.ValPtr.(*T)
if !ok {
p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr)
}
+if destPtr == nil {
+ p.Panicf(p.PanicNilPointer, ctx.String(), ctx.DType, ctx.ValPtr)
+}🤖 Prompt for AI Agents (early access)
In zogSchema.go around lines 56 to 60, before dereferencing destPtr, add a check to ensure destPtr is not nil, similar to the existing type assertion check. If destPtr is nil, call p.Panicf with appropriate arguments to handle the error defensively and prevent nil pointer dereference.
| valPtr, ok := ctx.ValPtr.(*T) | ||
| if !ok { | ||
| p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parallel nil-pointer risk during validation
primitiveValidation dereferences valPtr multiple times. Replicate the nil-pointer guard:
valPtr, ok := ctx.ValPtr.(*T)
if !ok {
p.Panicf(p.PanicTypeCast, ctx.String(), ctx.DType, ctx.ValPtr)
}
+if valPtr == nil {
+ p.Panicf(p.PanicNilPointer, ctx.String(), ctx.DType, ctx.ValPtr)
+}🤖 Prompt for AI Agents (early access)
In zogSchema.go around lines 113 to 116, the code casts ctx.ValPtr to *T but does not check if valPtr is nil before dereferencing it multiple times later in primitiveValidation. Add a nil-pointer check after the type assertion to ensure valPtr is not nil, and handle the nil case appropriately to prevent runtime panics during validation.
* feat: provide better panic msgs and docs * fix: docs link
1 participant
Summary by CodeRabbit