-
Notifications
You must be signed in to change notification settings - Fork 500
feat: add limited type checking for validate command #5076
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR implements limited type checking for the validation command, starting with the simplest case of *ast.LiteralExpr expressions. The implementation includes type checking for literal values, arrays, objects, unary expressions, and binary expressions. The PR also handles capsule types correctly, allowing conversions such as discovery.Target to map types.
Key Changes
- Added comprehensive type checking functions in
syntax/typecheck/typecheck.gofor various expression types (literal, array, object, unary, binary) - Refactored binary and unary operation functions from
syntax/vmpackage to a newsyntax/internal/transformpackage for reuse - Enhanced the
valuepackage to exposeTryCapsuleConvertand addEncapsulateWithRvfor better capsule handling
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
syntax/typecheck/typecheck.go |
Added type checking functions for literal, array, object, unary, and binary expressions with proper diagnostic reporting |
syntax/typecheck/typecheck_test.go |
Extended test suite with new test cases covering wrong literal types, arrays, objects, and capsule type checking |
syntax/vm/vm.go |
Refactored to use transform.BinaryOp and transform.UnaryOp instead of internal functions |
syntax/internal/transform/unary.go |
Moved unary operation logic from vm package, renamed function to UnaryOp for public access |
syntax/internal/transform/binary.go |
Moved binary operation logic from vm package, renamed function to BinaryOp for public access |
syntax/internal/value/value.go |
Added EncapsulateWithRv function to create capsule values from reflect.Value |
syntax/internal/value/decode.go |
Made TryCapsuleConvert public and simplified redundant error checking conditions |
internal/validator/testdata/default/*.txtar |
Updated test data files to use appropriate types for proper validation testing |
internal/validator/testdata/default/invalid_types.diags |
Added expected diagnostics for new invalid type test cases |
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.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
syntax/internal/value/value.go:105
- The function documentation incorrectly states "Encapsulate panics" when it should say "EncapsulateWithRv panics" to match the actual function name.
// Encode creates a new Value from v. If v is a pointer, v must be considered
// immutable and not change while the Value is used.
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.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
4df3f02 to
70583f5
Compare
1c12de4 to
dde0acd
Compare
PR Description
Doing type checking without evaluation is not easy. But I figured we could start with the simplest case
*ast.LiteralExpr.Because we always know what the values are it's pretty straight forward to add. I also added support for other expressions but it all boils down to performing type checking on Literal expressions.
I also try to handle capsules correctly, where e.g. discovery.Target can be converted into a map.
Which issue(s) this PR fixes
Part of: #3712
Notes to the Reviewer
Next up we can try to type check
*ast.AccessExrpbut for that we need a bit more information passed and I figured I can to that in a follow up pr.PR Checklist