-
Notifications
You must be signed in to change notification settings - Fork 0
Fix default value encoding and bootstrap test failures #2
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
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
- Define ArenaDefault<'arena> trait with Builder associated type - Enables default values for types with arena lifetimes - Auto-implement ArenaDefault in View derive macro - Generate setter calls for Plain/Required fields with explicit defaults - Optional fields default to None, Repeated fields to empty (no setters needed) - Update code generator and build system to support new pattern Part of refactor: arena allocation with default value support
- Remove old benchmark files from defiant/benches/ - Remove benchmark harness entries from defiant/Cargo.toml - Update allocations benchmark to use new patterns - Update google_message2_decode benchmark - Consolidate benchmark infrastructure in top-level benchmarks/ crate Part of refactor: arena allocation with default value support
- Update arena tests to use arena_default() builders - Update message encoding tests for new default patterns - Update integration tests across all test modules Part of refactor: arena allocation with default value support
- Update README with ArenaDefault trait usage examples - Document builder pattern with default values - Update BENCHMARKS.md with new benchmark results - Add examples of arena-allocated message creation Part of refactor: arena allocation with default value support
- Add missing key_len calculation for repeated message fields in View types - Bug was only calculating length_varint + content per message - Forgot to multiply key_len by number of messages in the repeated field - Update conformance test runner for arena allocation patterns - Use arena.alloc_str() for error messages and string fields - Remove 2 passing tests from failing_tests.txt - Fixes conformance tests: ValidDataRepeated.MESSAGE (proto2 and proto3) - Improves conformance from 1298 to 1300 successes (2 failures remaining) The remaining 2 failures are for UnknownVarint which requires unknown field preservation - a more complex feature requiring structural changes.
- Check String/Bytes fields against actual default value, not just .is_empty() - Only use .is_empty() optimization when default is actually empty - For custom defaults (e.g., default = "forty two"), compare against the value - Fixes check_default_values test: messages with defaults now encode to 0 bytes This ensures protobuf spec compliance: fields with default values should not be encoded to save space.
Code generator changes (defiant-build): - Add self:: prefix for user-defined types named 'Option' or 'Result' - Prevents conflict with ::core::option::Option - Only applies to bare identifiers without module paths Derive macro changes (defiant-derive): - Filter out self/super/crate paths from Option detection - Prevents treating self::Option as ::core::option::Option<T> - Fixes 'Failed to extract type from Option' panic Allows protobuf messages to have fields of type 'Option' (a user-defined type) without conflicting with Rust's standard Option<T>.
Bootstrap test changes: - Update path from prost-types to defiant-types - Reflects package rename in this fork Protobuf build changes: - Check both possible conformance_test_runner locations - Handles build/bin/conformance_test_runner (new CMake layout) - Falls back to build/conformance_test_runner (old layout) - Adds better error context showing attempted path Fixes bootstrap test failures on fresh builds.
- Generated code now uses self::Option for user-defined Option type - Resolves conflicts with ::core::option::Option - Allows compilation of Google's protobuf descriptor types - Generated by bootstrap test with updated code generator This is generated code from the .proto definitions.
- Use ArenaDefault::arena_default() pattern in tests - Add explicit type annotations where needed for inference - Fix method calls with as_ref() for borrowed types - Remove unused imports Tests now compile and pass with all fixes applied. Test suite: 30 passing, 0 failing, 2 ignored.
Arena changes: - Implement PartialOrd and Ord for ArenaMap - Required for certain message field comparisons Encoding changes: - Use closure pattern to avoid variable shadowing in varint macro - Pattern: (|param| expr)(value) instead of direct variable usage Gitignore changes: - Add context-snapshot.yaml for session management - Add stay-focused trigger files These are supporting changes for the main fixes.
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.
Major Changes
1. ArenaDefault Trait Pattern
Adds a new pattern for constructing messages with default values in arena-allocated contexts.
Add ArenaDefault trait and derive macro
ArenaDefault<'arena>trait withBuilderassociated type#[derive(View)]Update test suite to use ArenaDefault
Type::arena_default(&arena)Update documentation
2. Project Reorganization (1 commit)
Reorganize benchmarks into dedicated benchmarks/ directory
defiant/benches/tobenchmarks/BENCHMARKS.mdwith new structure3. Encoding Bug Fixes (2 commits)
Fix repeated message encoded_len
key_lencalculation for repeated message fieldslength_varint + contentper messagekey_lenby number of messagesFix String/Bytes default value encoding
.is_empty()optimization when default is actually emptycheck_default_valuestest: messages with defaults encode to 0 bytes4. Option Type Conflict Resolution (3 commits)
Google's protobuf has a message type named
Optionthat conflicted with Rust'sstd::option::Option<T>.Resolve Option type name conflicts
self::Optionfor user-defined types named "Option" or "Result"self::/super::/crate::paths from Option detectionRegenerate protobuf bindings
self::OptionqualifierFix bootstrap test paths
prost-types→defiant-types(package rename)build/bin/andbuild/locations5. Supporting Improvements (2 commits)
Update test suite for fixed patterns
as_ref()for borrowed typesAdd ArenaMap ordering traits and minor improvements
PartialOrdandOrdforArenaMap.gitignorefor context snapshots and skill triggersFiles:
tests/src/*.rs,defiant/src/arena.rs,defiant/src/encoding.rs,.gitignoreImpact