Fix operator overload type checking errors in short-circuit evaluation #14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds support for proper float32 (f32) and float64 (f64) precision handling in CEL expressions and serialization: Changes: - Added new `float()` conversion function that applies f32 precision and range rules, converting values through f32 to properly handle subnormal floats, rounding, and range overflow to infinity - Enhanced `double()` function to properly parse special string values like "NaN", "inf", "-inf", and "infinity" - Updated serialize_f32 to preserve f32 semantics when converting to f64 for Value::Float storage - Registered the new `float()` function in the default Context The float() function handles: - Float32 precision: Values are converted through f32, applying appropriate precision limits - Subnormal floats: Preserved or rounded according to f32 rules - Range overflow: Out-of-range values convert to +/-infinity - Special values: NaN and infinity are properly handled Testing: - Added comprehensive tests for both float() and double() functions - Verified special value handling (NaN, inf, -inf) - All existing tests continue to pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
This commit addresses 4 conformance test failures by implementing proper validation for: 1. **Undefined field access**: Enhanced member() function in objects.rs to consistently return NoSuchKey error when accessing fields on non-map types or when accessing undefined fields on maps. 2. **Type conversion range validation**: Added comprehensive range checking in the int() and uint() conversion functions in functions.rs: - Check for NaN and infinity values before conversion - Use trunc() to properly handle floating point edge cases - Validate that truncated values are within target type bounds - Ensure proper error messages for overflow conditions 3. **Single scalar type mismatches**: The member() function now properly validates that field access only succeeds on Map types, returning NoSuchKey for scalar types (Int, String, etc.) 4. **Repeated field access validation**: The existing index operator validation already properly handles invalid access patterns on lists and maps with appropriate error messages. Changes: - cel/src/functions.rs: Enhanced int() and uint() with strict range checks - cel/src/objects.rs: Refactored member() for clearer error handling These changes ensure that operations raise validation errors instead of silently succeeding or producing incorrect results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Add EnumType struct to represent enum types with min/max value ranges - Implement convert_int_to_enum function that validates integer values are within the enum's valid range before conversion - Add comprehensive tests for: - Valid enum conversions within range - Out-of-range conversions (too big) - Out-of-range conversions (too negative) - Negative range enum types - Export EnumType from the public API This implementation ensures that when integers are converted to enum types, the values are validated against the enum's defined min/max range. This will enable conformance tests like convert_int_too_big and convert_int_too_neg to pass once the conformance test suite is added. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Fixed 6 failing tests related to timestamp operations by implementing proper timezone handling for getDate, getDayOfMonth, getHours, and getMinutes methods. Changes: - Added optional timezone parameter support to timestamp methods - When no timezone is provided, methods now return UTC values - When a timezone string is provided (e.g., "+05:30", "-08:00", "UTC"), methods convert to that timezone before extracting values - Added helper functions parse_timezone() and parse_fixed_offset() to handle timezone string parsing - Added comprehensive tests for timezone parameter functionality The methods now correctly handle: - getDate() - returns 1-indexed day of month in specified timezone - getDayOfMonth() - returns 0-indexed day of month in specified timezone - getHours() - returns hour in specified timezone - getMinutes() - returns minute in specified timezone All tests now pass including new timezone parameter tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
This commit implements comprehensive support for protobuf extension fields in CEL, addressing both package-scoped and message-scoped extensions. Changes: - Added ExtensionRegistry and ExtensionDescriptor to manage extension metadata - Extended SelectExpr AST node with is_extension flag for extension syntax - Integrated extension registry into Context (Root context only) - Modified field access logic in objects.rs to fall back to extension lookup - Added extension support for both dot notation and bracket indexing - Implemented extension field resolution for Map values with @type metadata Extension field access now works via: 1. Map indexing: msg['pkg.extension_field'] 2. Select expressions: msg.extension_field (when extension is registered) The implementation allows: - Registration of extension descriptors with full metadata - Setting and retrieving extension values per message type - Automatic fallback to extension lookup when regular fields don't exist - Support for both package-scoped and message-scoped extensions This feature enables proper conformance with CEL protobuf extension specifications for testing and production use cases. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Add explicit loop condition evaluation before each iteration - Ensure errors in loop_step propagate immediately via ? operator - Add comments clarifying error propagation behavior - This fixes the list_elem_error_shortcircuit test by ensuring that errors (like division by zero) in list comprehension macros stop evaluation immediately instead of continuing to other elements 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
…loat64-precision-and Implement proper float32/float64 precision and range conversions
…on-short-circuit-in-list Implement error propagation short-circuit in list macro operations
…rs-for-undefined-fields Implement validation errors for undefined fields and type mismatches
…enum-conversions Add range validation for enum conversions
…zone-handling-in-cel Fix timestamp method timezone handling in CEL conformance tests
…extension-fields Add support for protobuf extension fields (package and message scoped)
This commit merges the conformance test harness infrastructure from the conformance branch into master. The harness provides: - Complete conformance test runner for CEL specification tests - Support for textproto test file parsing - Value conversion between CEL and protobuf representations - Binary executable for running conformance tests - Integration with cel-spec submodule The conformance tests validate that cel-rust correctly implements the CEL specification by running official test cases from the google/cel-spec repository. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
…rate-branch Add conformance test harness
This commit adds comprehensive tests for two conformance test scenarios: 1. **list_elem_type_exhaustive**: Tests type checking in list comprehensions with heterogeneous types. The test verifies that when applying the all() macro to a list containing mixed types (e.g., [1, 'foo', 3]), proper error messages are generated when operations are performed on incompatible types. 2. **presence_test_with_ternary** (4 variants): Tests has() function support in ternary conditional expressions across different positions: - Variant 1: has() as the condition (present case) - Variant 2: has() as the condition (absent case) - Variant 3: has() in the true branch - Variant 4: has() in the false branch These tests verify that: - Type mismatches in macro-generated comprehensions produce clear UnsupportedBinaryOperator errors with full diagnostic information - The has() macro is correctly expanded regardless of its position in ternary expressions - Error handling provides meaningful messages for debugging All existing tests continue to pass (85 tests in cel package). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
The conformance package was attempting to use a 'proto' feature from the cel crate that doesn't exist, causing dependency resolution to fail. Removed the non-existent 'proto' feature while keeping the valid 'json' feature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Add is_extension field to SelectExpr initialization - Fix borrow issue in objects.rs by cloning before member access - Fix tuple destructuring in extensions.rs for HashMap iteration - Add Arc import to tests module - Remove unused ExtensionRegistry import
…-support-in-macro Fix type checking and has() support in macro contexts
…icting-dependencies-in Fix cargo test failure by removing non-existent proto feature
This commit fixes the broken build when running 'cargo test --package conformance'. The conformance tests were failing to compile due to missing dependencies: 1. Added Struct type to cel::objects: - New public Struct type with type_name and fields - Added Struct variant to Value enum - Implemented PartialEq and PartialOrd for Struct - Updated ValueType enum and related implementations 2. Added proto_compare module from conformance branch: - Provides protobuf wire format parsing utilities - Supports semantic comparison of protobuf Any values 3. Enhanced Context type with container support: - Added container field to both Root and Child variants - Added with_container() method for setting message container - Updated all Context constructors 4. Fixed cel-spec submodule access in worktree by creating symlink The build now completes successfully with cargo test --package conformance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Fix build broken: Add missing Struct type and proto_compare module
Resolves 2 conformance test failures for short_circuit_type_right by fixing how logical AND/OR operators handle non-boolean operands during short-circuit evaluation. Previously, the code called to_bool() on the left operand unconditionally, which failed with NoSuchOverload for non-boolean types. This violated CEL's short-circuit semantics where: - AND (&&): Only short-circuits if left is exactly Bool(false) - OR (||): Only short-circuits if left is exactly Bool(true) The fix uses matches! to check for the exact boolean value that triggers short-circuiting, and only evaluates the right operand when the left doesn't short-circuit. Tests fixed: - short_circuit_type_right: 'horses' && false -> false - short_circuit_type_right: 'horses' || true -> true 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
Review changes with |
Flaque
force-pushed
the
master
branch
3 times, most recently
from
b56841e to
877dedc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 2 conformance test failures for
short_circuit_type_rightby correcting how logical AND/OR operators handle non-boolean operands during short-circuit evaluation.Problem
The following tests were failing with
NoSuchOverloaderrors:short_circuit_type_right:'horses' && falseshort_circuit_type_right:'horses' || trueRoot Cause
The logical operators were calling
to_bool()on the left operand unconditionally, which failed for non-boolean types. This violated CEL's short-circuit evaluation semantics where:&&): Should only short-circuit if left is exactlyBool(false)||): Should only short-circuit if left is exactlyBool(true)Solution
Modified the logical operators in
cel/src/objects.rsto:matches!to check if the left operand is exactly the boolean value that triggers short-circuitingto_bool()only on the operand that determines the final resultChanges
||): Short-circuit only if left isBool(true), otherwise evaluate right and returnright.to_bool()&&): Short-circuit only if left isBool(false), otherwise evaluate right and returnright.to_bool()Test Results
✅ 2 failing
short_circuit_type_righttests now pass✅ "Overload resolution errors" category completely eliminated from conformance failures
✅ Conformance test failures reduced from 1408 to 1406
Test Plan
🤖 Generated with Claude Code