Merge branch 'topic/bbannier/issue-1617'

CHANGES

@@ -1,3 +1,14 @@
+1.10.0-dev.55 | 2023-12-12 18:38:24 +0100
+
+  * GH-1617: Fix handling of `%synchronize-*` attributes for units in lists. (Benjamin Bannier, Corelight)
+
+    We previously would not detect `%synchronize-at` or `%synchronize-from`
+    attributes if the unit was not directly in a field, i.e., we mishandled
+    the common case of synchronizing on a unit in a list.
+
+    With this patch we now handle these attributes, regardless of how the
+    unit appears.
+
 1.10.0-dev.53 | 2023-12-12 13:05:41 +0100
 
   * Avoid unnecessary copy when constructing `Stream` from `Bytes`. (Benjamin Bannier, Corelight)

VERSION

@@ -1 +1 @@
-1.10.0-dev.53
+1.10.0-dev.55

spicy/toolchain/src/compiler/codegen/parser-builder.cc

@@ -1125,7 +1125,8 @@ struct ProductionVisitor
         }
 
         // Validation.
-        if ( auto while_ = p->tryAs<production::While>(); while_ && while_->expression() )
+        auto while_ = p->tryAs<production::While>();
+        if ( while_ && while_->expression() )
             hilti::logger().error("&synchronize cannot be used on while loops with conditions");
 
         // Helper to validate the parser state after search for a lookahead.
@@ -1141,10 +1142,21 @@ struct ProductionVisitor
         };
 
         // Handle synchronization via `synchronize-at` or `synchronize-after` unit properties.
-        if ( const auto& unit = p->tryAs<production::Unit>() ) {
-            const auto& type = unit->unitType();
-            const auto synchronize_at = type.propertyItem("%synchronize-at");
-            const auto synchronize_after = type.propertyItem("%synchronize-after");
+        // We can either see a unit for synchronization in a list (generating a
+        // `while` production), or directly.
+        std::optional<type::Unit> unitType;
+        if ( while_ ) {
+            if ( const auto& field = while_->meta().field() )
+                if ( auto unit = field->parseType().elementType().tryAs<type::Unit>() )
+                    unitType = *unit;
+        }
+
+        else if ( const auto& unit = p->tryAs<production::Unit>() )
+            unitType = unit->unitType();
+
+        if ( unitType ) {
+            const auto synchronize_at = unitType->propertyItem("%synchronize-at");
+            const auto synchronize_after = unitType->propertyItem("%synchronize-after");
 
             std::optional<Expression> e;
 

tests/Baseline/spicy.types.unit.synchronize-at/output

@@ -1,14 +1,14 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-foo::Test {
+foo::Test1 {
   foo: foo::Foo {
     data: foobar
   }
   bar: foo::Bar {
     data: 123
   }
 }
-[fatal error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: failed to match regular expression (<...>/synchronize-at.spicy:12:11)
-foo::Test {
+[fatal error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: failed to match regular expression (<...>/synchronize-at.spicy:17:11)
+foo::Test1 {
   foo: foo::Foo {}
   bar: foo::Bar {
     data: bar4567

tests/Baseline/spicy.types.unit.synchronize-at/output2

@@ -0,0 +1,15 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+foo::Test2 {
+  xs: [
+    foo::X {
+      x: \xff
+    }
+  ]
+}
+foo::Test2 {
+  xs: [
+    foo::X {
+      x: \xff
+    }
+  ]
+}

tests/spicy/types/unit/synchronize-at.spicy

@@ -1,11 +1,16 @@
 # @TEST-DOC: Validates synchronization via `%synchronize-at` property.
 #
 # @TEST-EXEC: spicyc -d -j %INPUT -o foo.hlto
-# @TEST-EXEC: printf 'foobar123' | spicy-dump -d foo.hlto >>output 2>&1
-# @TEST-EXEC-FAIL: printf '1234567' | spicy-dump -d foo.hlto >>output 2>&1
-# @TEST-EXEC: printf '123bar4567' | spicy-dump -d foo.hlto >>output 2>&1
+
+# @TEST-EXEC: ${SCRIPTS}/printf 'foobar123' | spicy-dump -d foo.hlto -p foo::Test1 >>output 2>&1
+# @TEST-EXEC-FAIL: ${SCRIPTS}/printf '1234567' | spicy-dump -d foo.hlto -p foo::Test1 >>output 2>&1
+# @TEST-EXEC: ${SCRIPTS}/printf '123bar4567' | spicy-dump -d foo.hlto -p foo::Test1 >>output 2>&1
 # @TEST-EXEC: btest-diff output
 
+# @TEST-EXEC: ${SCRIPTS}/printf '\xFF\xFF\xFF' | spicy-dump -d foo.hlto -p foo::Test2 >>output2 2>&1
+# @TEST-EXEC: ${SCRIPTS}/printf '\xCC\xFF\xFF' | spicy-dump -d foo.hlto -p foo::Test2 >>output2 2>&1
+# @TEST-EXEC: btest-diff output2
+
 module foo;
 
 type Foo = unit {
@@ -17,9 +22,23 @@ type Bar = unit {
     data: bytes &eod;
 };
 
-public type Test = unit {
+public type Test1 = unit {
     foo: Foo;
     bar: Bar &synchronize;
 
     on %synced { print self; confirm; }
 };
+
+##########################################
+
+public type Test2 = unit {
+    xs: (X &synchronize)[] foreach { if (|self.xs| > 0) stop; }
+    on %synced { confirm; }
+};
+
+type X = unit {
+    %synchronize-at = b"\xFF";
+    # Use a `&requires` here to avoid us generating a lookahead parse for
+    # `Test2::xs` above. Due to this we also need an explicit `stop` there.
+    x: bytes &size=1 &requires=(|$$| == 0 || $$==b"\xFF");
+};