diff --git a/README.md b/README.md index 691ac87..cc58060 100644 --- a/README.md +++ b/README.md @@ -56,8 +56,6 @@ These are intentionally out of scope: These are gaps we intend to address in future releases: -- **Closed-enum unknown values in packed-repeated view decode** are silently dropped (not routed to unknown fields). The owned decoder handles this correctly; the view decoder handles singular, optional, oneof, unpacked repeated, and map values correctly. Packed blobs have no per-element tag to borrow, so the zero-copy `UnknownFieldsView<'a>` has no span to reference. - ## Semver and API stability Buffa is pre-1.0. We follow the [Rust community convention](https://doc.rust-lang.org/cargo/reference/semver.html) for 0.x crates: breaking changes increment the **minor** version (0.1.x → 0.2.0), additive changes increment the **patch** version (0.1.0 → 0.1.1). Pin to a minor version (`buffa = "x.y"`; the full `x.y.z` that `cargo add buffa` writes is equivalent under caret semantics) to avoid surprises. diff --git a/buffa-codegen/src/view.rs b/buffa-codegen/src/view.rs index ff07a33..6ac5163 100644 --- a/buffa-codegen/src/view.rs +++ b/buffa-codegen/src/view.rs @@ -44,6 +44,21 @@ fn closed_enum_view_unknown_route(preserve_unknown_fields: bool) -> TokenStream } } +/// Token stream that records a closed-enum unknown packed element as +/// synthetic varint wire bytes in `view.__buffa_unknown_fields`. +fn closed_enum_view_unknown_varint_route( + field_number: u32, + preserve_unknown_fields: bool, +) -> TokenStream { + if preserve_unknown_fields { + quote! { + view.__buffa_unknown_fields.push_varint(#field_number, __raw as u64, ctx)?; + } + } else { + quote! {} + } +} + /// Convert a borrowed bytes view to the owned field type. /// /// When `use_bytes_type()` is active for this field, emits @@ -1399,7 +1414,13 @@ pub(crate) fn repeated_decode_arm( let push_known = quote! { view.#ident.push(__v); }; let packed_elem = if ty == Type::TYPE_ENUM { if closed { - closed_enum_decode("e! { &mut pcur }, push_known.clone()) + let unknown_route = + closed_enum_view_unknown_varint_route(field_number, preserve_unknown_fields); + closed_enum_decode_with_unknown( + "e! { &mut pcur }, + push_known.clone(), + unknown_route, + ) } else { quote! { view.#ident.push(::buffa::EnumValue::from(::buffa::types::decode_int32_packed(&mut pcur)?)); } } @@ -1433,7 +1454,8 @@ pub(crate) fn repeated_decode_arm( if closed { // Unpacked: each element has its own tag, so `before_tag` captures // the per-element span. Packed (above) can't do this — the tag - // covers the whole blob — so packed unknowns are still dropped. + // covers the whole blob — so packed unknowns are recorded as + // synthetic varint records instead. let unknown_route = closed_enum_view_unknown_route(preserve_unknown_fields); closed_enum_decode_with_unknown("e! { &mut cur }, push_known, unknown_route) } else { diff --git a/buffa-descriptor/src/generated/google.protobuf.descriptor.__view.rs b/buffa-descriptor/src/generated/google.protobuf.descriptor.__view.rs index 1d9fde9..be59dd9 100644 --- a/buffa-descriptor/src/generated/google.protobuf.descriptor.__view.rs +++ b/buffa-descriptor/src/generated/google.protobuf.descriptor.__view.rs @@ -8136,6 +8136,9 @@ impl<'a> ::buffa::MessageView<'a> for FieldOptionsView<'a> { __raw, ) { view.targets.push(__v); + } else { + view.__buffa_unknown_fields + .push_varint(19u32, __raw as u64, ctx)?; } } } else if tag.wire_type() == ::buffa::encoding::WireType::Varint { diff --git a/buffa-test/src/tests/closed_enum.rs b/buffa-test/src/tests/closed_enum.rs index 7ac2cc5..a3bae42 100644 --- a/buffa-test/src/tests/closed_enum.rs +++ b/buffa-test/src/tests/closed_enum.rs @@ -271,6 +271,79 @@ fn test_view_closed_enum_repeated_unpacked_unknown_preserved() { assert!(!view2.__buffa_unknown_fields.is_empty()); } +#[test] +fn test_view_closed_enum_repeated_packed_unknown_preserved() { + use crate::proto2::__buffa::view::ClosedEnumContextsView; + use crate::proto2::{ClosedEnumContexts, Priority}; + use buffa::encoding::{encode_varint, Tag, WireType}; + use buffa::{Message, MessageView}; + + let mut payload = Vec::new(); + encode_varint(0, &mut payload); + encode_varint(99, &mut payload); + encode_varint(2, &mut payload); + let mut wire = Vec::new(); + Tag::new(3, WireType::LengthDelimited).encode(&mut wire); + encode_varint(payload.len() as u64, &mut wire); + wire.extend_from_slice(&payload); + + let owned_direct = ClosedEnumContexts::decode(&mut wire.as_slice()).unwrap(); + let view = ClosedEnumContextsView::decode_view(&wire).unwrap(); + let vals: Vec<_> = view.rep_packed.iter().copied().collect(); + assert_eq!(vals, vec![Priority::LOW, Priority::HIGH]); + assert!(!view.__buffa_unknown_fields.is_empty()); + + let via_view = view.to_owned_message().unwrap(); + assert_eq!(via_view.encode_to_vec(), owned_direct.encode_to_vec()); +} + +#[test] +fn test_view_closed_enum_repeated_packed_unknown_preserves_order_before_later_unknown() { + use crate::proto2::__buffa::view::ClosedEnumContextsView; + use crate::proto2::ClosedEnumContexts; + use buffa::encoding::{encode_varint, Tag, WireType}; + use buffa::{Message, MessageView}; + + let mut payload = Vec::new(); + encode_varint(99, &mut payload); + let mut wire = Vec::new(); + Tag::new(3, WireType::LengthDelimited).encode(&mut wire); + encode_varint(payload.len() as u64, &mut wire); + wire.extend_from_slice(&payload); + wire.extend(varint_field(100, 7)); + + let owned_direct = ClosedEnumContexts::decode(&mut wire.as_slice()).unwrap(); + let view = ClosedEnumContextsView::decode_view(&wire).unwrap(); + assert!(!view.__buffa_unknown_fields.is_empty()); + + let via_view = view.to_owned_message().unwrap(); + assert_eq!(via_view.encode_to_vec(), owned_direct.encode_to_vec()); +} + +#[test] +fn test_view_closed_enum_repeated_packed_unknowns_consume_unknown_field_limit() { + use crate::proto2::__buffa::view::ClosedEnumContextsView; + use buffa::encoding::{encode_varint, Tag, WireType}; + use buffa::MessageView; + + let mut payload = Vec::new(); + encode_varint(99, &mut payload); + encode_varint(100, &mut payload); + let mut wire = Vec::new(); + Tag::new(3, WireType::LengthDelimited).encode(&mut wire); + encode_varint(payload.len() as u64, &mut wire); + wire.extend_from_slice(&payload); + + let limit = core::cell::Cell::new(1); + let err = ClosedEnumContextsView::decode_view_ctx( + &wire, + buffa::DecodeContext::new(buffa::RECURSION_LIMIT, &limit), + ) + .unwrap_err(); + + assert_eq!(err, buffa::DecodeError::UnknownFieldLimitExceeded); +} + #[test] fn test_view_closed_enum_oneof_unknown_to_unknown_fields() { use crate::proto2::__buffa::view::ClosedEnumContextsView; diff --git a/buffa/src/view.rs b/buffa/src/view.rs index 2db653a..fd869a7 100644 --- a/buffa/src/view.rs +++ b/buffa/src/view.rs @@ -1724,21 +1724,41 @@ impl<'a, K, V> IntoIterator for MapView<'a, K, V> { } } +/// One unknown-field wire record: either a span borrowed from the input +/// buffer, or synthetic bytes for a value with no borrowable source span. +#[derive(Clone, Debug)] +enum UnknownFieldsViewRecord<'a> { + Borrowed(&'a [u8]), + Owned(alloc::vec::Vec), +} + +impl UnknownFieldsViewRecord<'_> { + fn as_slice(&self) -> &[u8] { + match self { + Self::Borrowed(span) => span, + Self::Owned(bytes) => bytes, + } + } +} + /// A borrowed view of unknown fields. /// -/// Stores raw byte slices from the input buffer rather than decoded values, -/// enabling zero-copy round-tripping of unknown fields. Each stored span -/// holds **one or more consecutive** complete `(tag, value)` records: -/// adjacent unknown fields are coalesced into a single span, so a long run -/// of unknown fields costs one `Vec` slot rather than one per field. +/// Stores unknown fields as wire records rather than decoded values, enabling +/// zero-copy round-tripping for borrowable input spans. Borrowed spans may +/// hold **one or more consecutive** complete `(tag, value)` records: adjacent +/// unknown fields are coalesced into a single span, so a long run of unknown +/// fields costs one `Vec` slot rather than one per field. Synthetic records +/// cover cases where the view decoder must preserve a value that has no +/// borrowable source span. /// Coalescing bounds the view's memory only — the decode-time /// unknown-field allowance is still charged per field (see /// [`push_record`](Self::push_record)). #[derive(Clone, Default)] pub struct UnknownFieldsView<'a> { - /// Raw byte spans from the input buffer, each one or more complete - /// `(tag, value)` records. - raw_spans: alloc::vec::Vec<&'a [u8]>, + /// Unknown wire records in decode order. Borrowed records may each hold + /// one or more complete `(tag, value)` records; owned records are + /// synthetic wire bytes for values that have no borrowable source span. + records: alloc::vec::Vec>, /// The input-buffer tail starting at the first byte of the last span, /// kept so [`push_record`](Self::push_record) can extend that span over /// an adjacent record by re-slicing `last_tail` — never by widening the @@ -1765,7 +1785,7 @@ pub struct UnknownFieldsView<'a> { impl core::fmt::Debug for UnknownFieldsView<'_> { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { f.debug_struct("UnknownFieldsView") - .field("raw_spans", &self.raw_spans) + .field("records", &self.records) .finish_non_exhaustive() } } @@ -1778,7 +1798,7 @@ impl<'a> UnknownFieldsView<'a> { #[doc(hidden)] pub fn push_raw(&mut self, span: &'a [u8]) { - self.raw_spans.push(span); + self.records.push(UnknownFieldsViewRecord::Borrowed(span)); // A manually pushed span has no known position in the input buffer, // so coalescing must not extend it — and it was never charged // against a decode allowance, so to_owned falls back to the default @@ -1821,7 +1841,9 @@ impl<'a> UnknownFieldsView<'a> { let charge = crate::encoding::register_unknown_record(&tail[..span_len], ctx)?; self.to_owned_budget = self.to_owned_budget.saturating_add(charge.fields); self.to_owned_depth = self.to_owned_depth.max(charge.depth); - if let (Some(last), Some(prev_tail)) = (self.raw_spans.last_mut(), self.last_tail) { + if let (Some(UnknownFieldsViewRecord::Borrowed(last)), Some(prev_tail)) = + (self.records.last_mut(), self.last_tail) + { let prev_len = last.len(); // Contiguous if the new record begins exactly one past the end // of the previous span. Both checks are plain pointer/length @@ -1834,54 +1856,79 @@ impl<'a> UnknownFieldsView<'a> { return Ok(()); } } - self.raw_spans.push(&tail[..span_len]); + self.records + .push(UnknownFieldsViewRecord::Borrowed(&tail[..span_len])); self.last_tail = Some(tail); Ok(()) } + #[doc(hidden)] + pub fn push_varint( + &mut self, + field_number: u32, + value: u64, + ctx: crate::DecodeContext<'_>, + ) -> Result<(), crate::DecodeError> { + use crate::encoding::{encode_varint, Tag, WireType}; + + ctx.register_unknown_field()?; + self.to_owned_budget = self.to_owned_budget.saturating_add(1); + + let mut bytes = alloc::vec::Vec::new(); + Tag::new(field_number, WireType::Varint).encode(&mut bytes); + encode_varint(value, &mut bytes); + self.records.push(UnknownFieldsViewRecord::Owned(bytes)); + // Synthetic bytes have no position in the input buffer, so a later + // borrowed record must start a fresh span. + self.last_tail = None; + Ok(()) + } + /// Returns `true` if no unknown fields were recorded. pub fn is_empty(&self) -> bool { - self.raw_spans.is_empty() + self.records.is_empty() } /// Total byte length of all unknown field data. pub fn encoded_len(&self) -> usize { - self.raw_spans.iter().map(|s| s.len()).sum() + self.records + .iter() + .map(|record| record.as_slice().len()) + .sum() } - /// Write all unknown-field bytes verbatim. Each span holds one or more - /// complete `(tag, value)` records as they appeared on the wire, so - /// concatenating the spans produces a valid encoding. + /// Write all unknown-field bytes in decode order. Each record holds one + /// or more complete `(tag, value)` records, so concatenating them + /// produces a valid encoding. pub fn write_to(&self, buf: &mut impl BufMut) { - for span in &self.raw_spans { - buf.put_slice(span); + for record in &self.records { + buf.put_slice(record.as_slice()); } } - /// Convert to an owned [`UnknownFields`](crate::UnknownFields) by parsing all stored raw byte spans. + /// Convert to an owned [`UnknownFields`](crate::UnknownFields) by parsing all stored wire records. /// - /// Each span holds one or more consecutive (tag + value) records as they - /// appeared on the wire. Parsing uses + /// Each record holds one or more consecutive (tag + value) records as + /// they should be re-emitted. Parsing uses /// [`crate::encoding::decode_unknown_field`] under exactly the budget - /// [`push_record`](Self::push_record) charged at decode time — the same - /// field count and group-nesting depth — so replay cannot exhaust - /// either. Views holding manually pushed spans (via - /// [`push_raw`](Self::push_raw)) additionally get + /// [`push_record`](Self::push_record) and [`push_varint`](Self::push_varint) + /// charged at decode time — the same field count and group-nesting depth — + /// so replay cannot exhaust either. Views holding manually pushed spans + /// (via [`push_raw`](Self::push_raw)) additionally get /// [`DEFAULT_UNKNOWN_FIELD_LIMIT`](crate::DEFAULT_UNKNOWN_FIELD_LIMIT) /// slots and the full [`RECURSION_LIMIT`](crate::RECURSION_LIMIT), since /// those spans were never charged. - /// A coalesced span re-materializes one owned `UnknownField` per - /// record, so this conversion is where a long run of unknown fields + /// A coalesced borrowed span re-materializes one owned `UnknownField` per + /// wire record, so this conversion is where a long run of unknown fields /// actually allocates. /// /// # Errors /// - /// Returns `Err` if a stored span is malformed or exceeds the replay - /// budget. Neither can occur when the view was produced by wire - /// decoding — every span was parsed off the wire, and the budget equals - /// what decoding charged — so any view that decoded successfully - /// converts successfully. Only views holding - /// [`push_raw`](Self::push_raw) spans can fail here. + /// Returns `Err` if a stored record is malformed or exceeds the replay + /// budget. Neither can occur when the view was produced by wire decoding + /// — every borrowed record was parsed off the wire, and the budget equals + /// what decoding charged — so any such view converts successfully. Only + /// views holding [`push_raw`](Self::push_raw) spans can fail here. pub fn to_owned(&self) -> Result { use crate::encoding::{decode_unknown_field, Tag}; @@ -1897,8 +1944,8 @@ impl<'a> UnknownFieldsView<'a> { let limit = core::cell::Cell::new(budget); let ctx = crate::DecodeContext::new(depth, &limit); let mut out = crate::UnknownFields::new(); - for span in &self.raw_spans { - let mut cur: &[u8] = span; + for record in &self.records { + let mut cur = record.as_slice(); while !cur.is_empty() { let tag = Tag::decode(&mut cur)?; let field = decode_unknown_field(tag, &mut cur, ctx)?; @@ -2517,7 +2564,7 @@ mod tests { ufv.push_record(&buf[2..], 2, ctx).unwrap(); ufv.push_record(&buf[4..], 2, ctx).unwrap(); assert_eq!(ufv.encoded_len(), 6); - assert_eq!(ufv.raw_spans.len(), 1, "coalesced into one span"); + assert_eq!(ufv.records.len(), 1, "coalesced into one span"); let mut out = alloc::vec::Vec::new(); ufv.write_to(&mut out); assert_eq!(out, buf); @@ -2648,7 +2695,7 @@ mod tests { for i in 0..3 { ufv.push_record(&buf[2 * i..], 2, ctx).unwrap(); } - assert_eq!(ufv.raw_spans.len(), 1, "coalesced into one span"); + assert_eq!(ufv.records.len(), 1, "coalesced into one span"); assert_eq!(ctx.remaining_unknown_fields(), 0, "limit exactly used"); let owned = ufv.to_owned().expect("decode succeeded, so convert must"); assert_eq!(owned.iter().count(), 3); @@ -3005,6 +3052,23 @@ mod tests { assert!(uf.to_owned().is_err()); } + #[test] + fn unknown_fields_view_to_owned_includes_synthetic_varint_records() { + let ctx = record_ctx(1); + let mut uf = UnknownFieldsView::new(); + uf.push_varint(3, 99, ctx).unwrap(); + assert_eq!(ctx.remaining_unknown_fields(), 0); + + let owned = uf.to_owned().expect("owned unknown field"); + let fields: Vec<_> = owned.iter().collect(); + assert_eq!(fields.len(), 1); + assert_eq!(fields[0].number, 3); + assert!(matches!( + fields[0].data, + crate::UnknownFieldData::Varint(99) + )); + } + // ── OwnedView ────────────────────────────────────────────────────── // Minimal types to test OwnedView without depending on generated code.