Fix bytes operations: validation, comparison, indexing, and concatenation #18
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 implements proper struct type resolution for CEL expressions,
fixing the issue where creating nested messages returned the nested type
instead of wrapping it in the parent struct type.
Changes:
- Added Struct type with type_name and fields to objects.rs
- Added Struct variant to Value enum
- Implemented Expr::Struct resolution in Value::resolve()
- Updated all Value match statements (Debug, PartialEq, type_of, is_zero)
- Added Struct field access support in member() method
- Updated size() function to handle Struct
- Added JSON serialization support for Struct
When creating expressions like TestAllTypes{nested_message: NestedMessage{}},
the result now correctly has type TestAllTypes with the nested message
properly nested within its fields.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
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
…es-when Resolved merge conflict in cel/src/objects.rs member() method by integrating Struct field access into the new error handling pattern from master. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
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
Added support for type conversion (denotation) functions that were causing conformance test failures. These functions enable explicit type casting in CEL expressions. Implemented conversion functions: - list(): Convert to list type (identity for lists) - map(): Convert to map type (identity for maps) - null_type(): Convert to null_type (identity for null) - dyn(): Convert to dynamic type (identity function for all types) These functions follow CEL spec semantics where they act as type markers for the type checker while providing runtime identity conversion for compatible types. Non-compatible type conversions return appropriate errors. Added unit tests for all new conversion functions to verify correct behavior. Fixes conformance test failures: - int_denotation (already implemented) - uint_denotation (already implemented) - double_denotation (already implemented) - string_denotation (already implemented) - bytes_denotation (already implemented) - list_denotation - map_denotation - null_type_denotation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
This commit implements all 7 missing standard math functions that were causing 18+ conformance test failures: - isNaN(): Check if a value is NaN (4 test failures) - isInf(): Check if a value is infinite (2 test failures) - isFinite(): Check if a value is finite (4 test failures) - ceil(): Round up to nearest integer (2 test failures) - floor(): Round down to nearest integer (2 test failures) - trunc(): Truncate to integer toward zero (2 test failures) - round(): Round to nearest integer (2 test failures) Implementation details: - All functions are implemented in cel/src/functions.rs - Functions are registered in Context::default() in cel/src/context.rs - isNaN/isInf/isFinite return bool for float type checks - ceil/floor/trunc/round work with both float and integer inputs - Integer inputs are converted to float for consistency - Comprehensive test coverage added for all functions - All tests pass successfully These functions are part of the CEL standard library specification and enable full conformance with the CEL standard. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
…sion-functions Implement Missing CEL Type Conversion Functions
…ath-functions Implement essential math functions
…struct-types-when Correctly resolve parent struct types when creating nested messages
…tion This commit addresses all bytes operation conformance failures: 1. **Validation**: Fixed string() function to reject invalid UTF-8 bytes - Changed from String::from_utf8_lossy() to String::from_utf8() - Now returns error for invalid UTF-8 instead of silently replacing - Fixes test: bytes_invalid 2. **Comparison**: Added bytes comparison support to PartialOrd - Bytes can now be compared with <, >, <=, >= operators - Fixes tests: lt_bytes, gt_bytes, lte_bytes, gte_bytes, etc. 3. **Indexing**: Added bytes indexing support to INDEX operation - Bytes can now be indexed with [int] and [uint] - Returns single-byte Bytes value at the specified index - Properly handles out-of-bounds errors 4. **Concatenation**: Added bytes concatenation support to ADD operation - Bytes can now be concatenated with + operator - Optimized with Arc for efficient in-place mutation All bytes operation conformance tests now pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
Review changes with |
This commit resolves the compilation errors when running `cargo test --package conformance`: 1. **Removed duplicate Struct definition**: The `Struct` type was defined twice in `cel/src/objects.rs` (lines 75-108 and 221-254), causing compilation errors. Removed the duplicate definition. 2. **Fixed duplicate pattern in PartialEq**: The `Value::PartialEq` implementation had a duplicate pattern match for `(Value::Struct(a), Value::Struct(b))`, which was unreachable. Removed the duplicate. 3. **Initialized cel-spec git submodule**: The conformance build script requires proto files from the cel-spec submodule. Added the cel-spec repository as a proper git submodule. After these changes, `cargo build --package conformance` completes successfully with only warnings (no errors). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
…-in-conformance-package Fix cargo test errors in conformance package
…ing-bytes-operations-handling-invalid-bytes
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
This PR fixes all bytes operation conformance failures identified in the issue. All bytes-related conformance tests now pass.
Changes Made
1. Validation - Invalid UTF-8 Handling (cel/src/functions.rs:162-166)
string()function to properly reject invalid UTF-8 bytesString::from_utf8_lossy()(which silently replaces invalid bytes) toString::from_utf8()(which returns an error)bytes_invalid2. Comparison Operations (cel/src/objects.rs:623)
(Value::Bytes(a), Value::Bytes(b))case toPartialOrdimplementation<,>,<=,>=operatorslt_bytes,gt_bytes,lte_bytes,gte_bytes,not_lt_bytes_same,not_lt_bytes_width,gt_bytes_one,gt_bytes_one_to_empty,not_gt_bytes_sorting,not_lte_bytes_length,gte_bytes_to_empty,not_gte_bytes_empty_to_nonempty,gte_bytes_samelength,bytes_gt_left_false3. Indexing Operations (cel/src/objects.rs:905-918)
IntandUIntindicesBytesvalue at the specified indexIndexOutOfBoundserror4. Concatenation Operations (cel/src/objects.rs:1296-1308)
Addtrait implementation+operatorArc::make_mutfor efficient in-place mutationTest Results
All bytes operation conformance tests now pass. No "Bytes operations" failures remain in the conformance test suite.
Test Plan
🤖 Generated with Claude Code