Fix string literal corruption in Python evaluation mode#17
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where string literals containing numbers were incorrectly corrupted during evaluation when floats existed in the context. The solution replaces brittle string-based preprocessing with a robust AST-based approach that preserves string literals exactly while still enabling proper integer-to-float promotion for arithmetic operations.
- Implements AST-based integer promotion that distinguishes between string literals and actual integer literals
- Preserves array indexing by keeping indices as integers rather than promoting them to floats
- Adds comprehensive test coverage for the fix across both Python and Strict evaluation modes
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Core implementation replacing string-based integer promotion with AST-based approach |
| tests/test_issue16_string_literal_regression.py | Focused regression tests for the string literal corruption issue |
| tests/test_dual_mode_comprehensive.py | Comprehensive test suite validating both evaluation modes work correctly |
| tests/test_enhanced_error_handling.py | Minor test update to use explicit float literal |
| CHANGELOG.md | Documentation of the bug fix and new AST-based approach |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
a28b1de to
0d4c002
Compare
The library now operates exclusively in strict CEL mode - **Removed**: `EvaluationMode.PYTHON` and all automatic integer-to-float promotion - **Removed**: `mode` parameter from `evaluate()` function - **Removed**: `--mode` CLI option - **Behavior change**: Mixed arithmetic like `1 + 2.5` now raises `TypeError` instead of automatically promoting to `3.5` - **Migration**: Use explicit type conversion (e.g., `double(1) + 2.5`) for mixed arithmetic - **Rationale**: Eliminates complex AST preprocessing that was breaking `has()` short-circuiting and other CEL functions ### 🐛 Fixed - **CEL function short-circuiting**: Fixed issue where `has()` and other CEL functions failed due to AST preprocessing interference - **String literal corruption**: Eliminated string literal modification that occurred during integer promotion preprocessing ### Updated - Updated cel crate from v0.11.0 to v0.11.1 - Updated documentation to reflect strict CEL mode operation - Updated tests to work with strict CEL mode only - Removed complex preprocessing logic
c5ae6b8 to
6b07f20
Compare
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
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.
Replaces brittle string-based integer promotion with robust AST-based approach. Previously, string literals like "epa1" were incorrectly modified to "epa1.0" when floats existed in the evaluation context, causing string comparisons to fail.
The new implementation:
Fixes string comparison expressions like
var == "epa1"returning False when floats are present in context. Includes comprehensive test coverage for both evaluation modes.Resolves #16