Fix panic in comparison operators with non-comparable types#990
Fix panic in comparison operators with non-comparable types#990lawrence3699 wants to merge 1 commit intolmorg:masterfrom
Conversation
When comparing a non-comparable type (e.g. array) with a number using
<, >, <=, or >= in non-strict mode, expGtLt panicked with
"interface conversion: interface {} is float64, not string" because
compareTypes() can return mismatched Go types — the non-comparable
side is JSON-marshaled to a string while the comparable side retains
its original type (e.g. float64).
Add a safe type assertion in the string comparison branch, converting
the right-hand value to a string via types.ConvertGoType when it is
not already a string.
Fixes lmorg#982
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in expression comparison operators (<, >, <=, >=) when non-comparable types (eg arrays) are compared against comparable types in non-strict mode, addressing issue #982.
Changes:
- Adds a safe conversion path for the RHS operand in the string-comparison branch to prevent
interface conversionpanics. - Adds regression tests for strict-mode error behavior and non-strict-mode non-panicking behavior when comparing arrays with numbers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lang/expressions/exp06.go |
Prevents panic by safely converting rv to string when the comparison is routed through the string branch. |
lang/expressions/exp06_test.go |
Adds regression tests covering strict vs non-strict comparisons involving non-comparable types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v, convErr := types.ConvertGoType(rv, types.String) | ||
| if convErr != nil { | ||
| return raiseError(tree.expression, tree.currentSymbol(), 0, fmt.Sprintf( | ||
| "cannot %s with %s types", tree.currentSymbol().key, left.Primitive, | ||
| )) |
There was a problem hiding this comment.
When converting rv to a string fails, the returned error message drops the underlying convErr details and doesn’t mention the right-hand operand type/value. Consider including convErr.Error() (and ideally both operand primitives/Go types) so users can understand why the comparison cannot be performed instead of only seeing cannot < with <left> types.
| { | ||
| Expression: `2 < %[1,2,3]`, | ||
| Expected: false, | ||
| }, |
There was a problem hiding this comment.
The regression test for non-strict mode only covers < and > cases. Since the fix is inside expGtLt (shared by <, <=, > and >=), add non-strict assertions for <= and >= (both operand orders) to ensure those operators also won’t panic and to lock in expected behavior.
| }, | |
| }, | |
| { | |
| Expression: `%[1,2,3] <= 2`, | |
| Expected: false, | |
| }, | |
| { | |
| Expression: `2 >= %[1,2,3]`, | |
| Expected: true, | |
| }, | |
| { | |
| Expression: `%[1,2,3] >= 2`, | |
| Expected: true, | |
| }, | |
| { | |
| Expression: `2 <= %[1,2,3]`, | |
| Expected: false, | |
| }, |
Fixes #982
Failing scenario: In non-strict mode,
@PWD < 2(or any expression comparing a non-comparable type like an array with a number via<,>,<=,>=) panics:Root cause:
compareTypes()returns mismatched Go types when one operand is non-comparable — the non-comparable side gets JSON-marshaled to a string while the comparable side retains its original Go type (e.g.float64). InexpGtLt, the string case branch doesrv.(string)without checking the actual type, causing the panic.Fix: Add a safe type assertion in the string comparison branch. When
rvis not already a string, convert it viatypes.ConvertGoType(rv, types.String)before comparing.Before:
@PWD < 2→ panicAfter:
@PWD < 2→ returns a boolean (string comparison)Validation:
go test ./lang/expressions/— all existing tests passgo vet ./lang/expressions/— clean