optional: allow duplicate if statements

Introduces double condition check serialization using InconsistentConditionValue

pdl-compiler/src/analyzer.rs

@@ -137,7 +137,6 @@ pub enum ErrorCode {
     InvalidConditionIdentifier = 47,
     InvalidConditionValue = 48,
     E49 = 49,
-    ReusedConditionIdentifier = 50,
     InvalidFieldOffset = 51,
     InvalidFieldSize = 52,
     InvalidPacketSize = 53,
@@ -1517,7 +1516,6 @@ fn check_optional_fields(file: &File) -> Result<(), Diagnostics> {
     let mut diagnostics: Diagnostics = Default::default();
     for decl in &file.declarations {
         let mut local_scope: HashMap<String, &Field> = HashMap::new();
-        let mut condition_ids: HashMap<String, &Field> = HashMap::new();
         for field in decl.fields() {
             if let Some(ref cond) = field.cond {
                 match &field.desc {
@@ -1598,20 +1596,6 @@ fn check_optional_fields(file: &File) -> Result<(), Diagnostics> {
                     ),
                     _ => unreachable!(),
                 }
-                if let Some(prev_field) = condition_ids.insert(cond.id.to_owned(), field) {
-                    diagnostics.push(
-                        Diagnostic::error()
-                            .with_code(ErrorCode::ReusedConditionIdentifier)
-                            .with_message("reused condition identifier".to_owned())
-                            .with_labels(vec![
-                                field.loc.primary(),
-                                prev_field
-                                    .loc
-                                    .secondary()
-                                    .with_message(format!("`{}` is first used here", cond.id)),
-                            ]),
-                    )
-                }
             }
             if let Some(id) = field.id() {
                 local_scope.insert(id.to_owned(), field);
@@ -1817,25 +1801,24 @@ fn desugar_flags(file: &mut File) {
             | DeclDesc::Struct { fields, .. }
             | DeclDesc::Group { fields, .. } => {
                 // Gather information about condition flags.
-                let mut condition_ids: HashMap<String, (String, usize)> = HashMap::new();
+                let mut condition_ids: HashMap<String, Vec<(String, usize)>> = HashMap::new();
                 for field in fields.iter() {
                     if let Some(ref cond) = field.cond {
-                        condition_ids.insert(
-                            cond.id.to_owned(),
-                            (field.id().unwrap().to_owned(), cond.value.unwrap()),
+
+                        condition_ids.entry(cond.id.to_owned()).or_default().push(
+                            (field.id().unwrap().to_owned(), cond.value.unwrap())
                         );
                     }
                 }
                 // Replace condition flags in the fields.
                 for field in fields.iter_mut() {
-                    if let Some((optional_field_id, set_value)) =
+                    if let Some(optional_field_ids) =
                         field.id().and_then(|id| condition_ids.get(id))
                     {
                         field.desc = FieldDesc::Flag {
                             id: field.id().unwrap().to_owned(),
-                            optional_field_id: optional_field_id.to_owned(),
-                            set_value: *set_value,
-                        }
+                            optional_field_ids: optional_field_ids.to_owned(),
+                        };
                     }
                 }
             }
@@ -3084,22 +3067,6 @@ mod test {
         );
     }
 
-    #[test]
-    fn test_e50() {
-        raises!(
-            ReusedConditionIdentifier,
-            r#"
-        little_endian_packets
-        packet B {
-            c : 1,
-            _reserved_ : 7,
-            x : 8 if c = 1,
-            y : 8 if c = 0,
-        }
-        "#
-        );
-    }
-
     #[test]
     fn test_e51() {
         raises!(

pdl-compiler/src/ast.rs

@@ -144,7 +144,7 @@ pub enum FieldDesc {
     /// Special case of Scalar for fields used as condition for
     /// optional fields. The width is always 1.
     #[serde(rename = "flag_field")]
-    Flag { id: String, optional_field_id: String, set_value: usize },
+    Flag { id: String, optional_field_ids: Vec<(String, usize)> },
     #[serde(rename = "typedef_field")]
     Typedef { id: String, type_id: String },
     #[serde(rename = "group_field")]

pdl-compiler/src/backends/rust/encoder.rs

@@ -237,12 +237,45 @@ impl Encoder {
         let shift = self.bit_shift;
 
         match &field.desc {
-            ast::FieldDesc::Flag { optional_field_id, set_value, .. } => {
+            ast::FieldDesc::Flag { optional_field_ids, .. } => {
+                let packet_name = &self.packet_name;
+                let field_id = field.id().unwrap();
+                let (optional_field_id, set_value) = &optional_field_ids[0];
                 let optional_field_id = optional_field_id.to_ident();
+
                 let cond_value_present =
                     syn::parse_str::<syn::LitInt>(&format!("{}", set_value)).unwrap();
                 let cond_value_absent =
                     syn::parse_str::<syn::LitInt>(&format!("{}", 1 - set_value)).unwrap();
+
+                if optional_field_ids.len() >= 2 {
+
+                    self.tokens.extend(quote! {
+                        let mut cond_value_is_zero = false;
+                        let mut cond_value_is_one = false;
+                    });
+
+                    for (optional_cond_field, value) in optional_field_ids {
+                        let optional_cond_field_tok = optional_cond_field.to_ident();
+                        if *value == 1 {
+                            self.tokens.extend(quote! { cond_value_is_one |= self.#optional_cond_field_tok.is_some();} );
+                            self.tokens.extend(quote! { cond_value_is_zero |= self.#optional_cond_field_tok.is_none();} );
+                        } else {
+                            self.tokens.extend(quote! { cond_value_is_one |= self.#optional_cond_field_tok.is_none();} );
+                            self.tokens.extend(quote! { cond_value_is_zero |= self.#optional_cond_field_tok.is_some();} );
+                        }
+                    }
+
+                    self.tokens.extend(quote! {
+                        if cond_value_is_zero && cond_value_is_one {
+                            return Err(EncodeError::InconsistentConditionValue {
+                                packet: #packet_name,
+                                field: #field_id,
+                            })
+                        }
+                    });
+                }
+
                 self.bit_fields.push(BitField {
                     value: quote! {
                         if self.#optional_field_id.is_some() {

pdl-compiler/src/backends/rust_legacy/serializer.rs

@@ -143,7 +143,10 @@ impl<'a> FieldSerializer<'a> {
         let shift = self.shift;
 
         match &field.desc {
-            ast::FieldDesc::Flag { optional_field_id, set_value, .. } => {
+            ast::FieldDesc::Flag { optional_field_ids, .. } => {
+                assert!(optional_field_ids.len() == 1, "condition flag reuse not supported by legacy generator");
+
+                let (optional_field_id, set_value) = &optional_field_ids[0];
                 let optional_field_id = optional_field_id.to_ident();
                 let cond_value_present =
                     syn::parse_str::<syn::LitInt>(&format!("{}", set_value)).unwrap();

pdl-runtime/src/lib.rs

@@ -76,6 +76,8 @@ pub enum EncodeError {
         expected_size: usize,
         element_index: usize,
     },
+    #[error("{packet}.{field} value cannot be uniquely determined")]
+    InconsistentConditionValue { packet: &'static str, field: &'static str},
 }
 
 /// Trait implemented for all toplevel packet declarations.

pdl-tests/tests/semantic.rs

@@ -0,0 +1,80 @@
+// Copyright 2024 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use pdl_derive::pdl_inline;
+
+#[pdl_inline(
+    r#"
+little_endian_packets
+
+enum Enum16 : 16 {
+   X = 0x1234,
+   Y = 0x5678,
+}
+
+packet CondTest {
+  cond: 1,
+  _reserved_ : 7,
+  a: 8 if cond = 0,
+  b: Enum16 if cond = 1,
+}
+"#
+)]
+#[cfg(test)]
+mod optional_field {
+    #[test]
+    fn test_value_0() {
+      // Success
+      let test_value_0 = CondTest {
+        a: Some(255),
+        b: None,
+      };
+      let mut buf = vec![];
+      assert!(test_value_0.encode(&mut buf).is_ok());
+
+      let decoded_cond = CondTest::decode_full(&buf).unwrap();
+      assert_eq!(decoded_cond.a, test_value_0.a);
+      assert_eq!(decoded_cond.b, test_value_0.b);
+    }
+
+    #[test]
+    fn test_value_1() {
+      // Success
+      let test_value_1 = CondTest {
+        a: None,
+        b: Some(Enum16::X),
+      };
+      let mut buf = vec![];
+      assert!(test_value_1.encode(&mut buf).is_ok());
+
+      let decoded_cond = CondTest::decode_full(&buf).unwrap();
+      assert_eq!(decoded_cond.a, test_value_1.a);
+      assert_eq!(decoded_cond.b, test_value_1.b);
+    }
+
+    #[test]
+    fn test_value_inconsistent() {
+      let test_value_none = CondTest {
+        a: None,
+        b: None,
+      };
+      assert!(matches!(test_value_none.encode_to_vec(), Err(EncodeError::InconsistentConditionValue { .. })));
+
+      let test_value_both = CondTest {
+        a: Some(255),
+        b: Some(Enum16::X),
+      };
+      assert!(matches!(test_value_both.encode_to_vec(), Err(EncodeError::InconsistentConditionValue { .. })));
+    }
+}