-
Notifications
You must be signed in to change notification settings - Fork 2
fix: Nullable return types #2
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
Conversation
|
What is the inference logic for required? Simply whether the field has a pointer? Let's say a field has |
|
When schema fields have required: false, generated Go structs now use pointer types (e.g., *string instead of string). This allows mocks to return nil for null values without compile errors. Changes: - signature.go: add lookupField() returning full Field with Required info - signature.go: inferReturnType() wraps type in * when !field.Required - mock.go: emit ptr[T any] helper for pointer value construction - mock.go: findOutputValue() uses ptr() wrapper for non-nil pointer values Fixes nullable return type codegen bug where bio: null in test cases caused 'cannot use nil as string value' compile errors.
…ndling - Add Required bool field to scaf.ReturnInfo in dialect.go - Add FieldInfo struct and LookupFieldInfo() in type_inference.go - Add inferFieldInfo() to extract Required from property access expressions - extractProjectionItem() now sets ret.Required from schema field lookup - signature.go uses ret.Required with proper isNilableType() helper - Pointer wrapping for nullable fields happens in codegen layer - Add TestTypeInference_RequiredField to verify Required propagation
ab10fab to
2b1c816
Compare
Replace complex AST traversal (inferFieldInfo, getPostfixExpr, FieldInfo, LookupFieldInfo) with simple lookupFieldFromExpression() that: - Parses 'variable.property' pattern from expression string - Rejects complex expressions (operators, predicates, indexing) - Looks up binding to get model, then field's Required from schema This addresses reviewer feedback about inferFieldInfo being too permissive for predicate expressions like 'u.bio IS NULL'.
- Move TestTypeInference_RequiredField to analyzer_test.go (tests analyzer behavior) - Add test case for complex expressions defaulting to required - Remove redundant needsPointerHelpers assignment in findOutputValue - Remove goLiteralWithType wrapper, call goLiteral directly - Remove unused baseType variable
rlch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove fallback and ensure tests pass after. Otherwise LGTM
Also can you submit another PR for CI on PRs?
The fallback scanned all models matching by field name only, which could return wrong field type/Required for ambiguous names like 'id' or 'name'. Now relies solely on analyzer's ReturnInfo which has proper model context.
Problem
When a schema field has
required: false(the default), the generated Go struct uses non-pointer types. This causes issues when the DSL specifiesnullas an expected value:Generated struct:
Generated mock tries to return
nilfor astringfield - compile error.Solution
Pass
Requiredmetadata throughReturnInfofrom the analyzer layer, letting codegen (signature.go) decide pointer wrapping. Type inference stays clean (returnsstring), whileRequiredbool travels separately for codegen to use with proper type inspection viaisNilableType().Changes
dialect.go: AddRequired boolfield toReturnInfodialects/cypher/type_inference.go: AddFieldInfostruct andinferFieldInfo()to extract Required from property accessdialects/cypher/analyzer.go: Setret.RequiredinextractProjectionItem()language/go/signature.go: Useret.Required+isNilableType()helper to wrap nullable primitives in pointerslanguage/go/mock.go: Emitptr[T any]helper, wrap non-nil pointer values withptr()