Use of %synchronize-at unexpectedly rejected

[bbannier]

One would expect that the following code can be used to specify a literal to synchronize on independently of usual lookahead parsing:

module sync;

public type PDUs = unit {
    : (PDU &synchronize)[];
    on %synced { confirm; }
};

type PDU = unit {
    %synchronize-at = b"\x00";
    : uint32;
};

We currently reject this code.

$ spicyc -j sync.spicy
[error] sync.spicy:10:7: &synchronize cannot be used on field, look-ahead contains non-literals
[error] spicyc: aborting after errors

[bbannier]

We currently do not seem to account for %synchronize-at being used on a unit which is used in a container with lookahead. Indeed, the only test case for %synchronize-at we have is for a unit directly containing a unit with %synchronize-at.

Since synchronizing on a message in a container of unspecified size (i.e., involving lookahead) is pretty much the most common case of synchronization for TCP this is definitely something we should fix.

[bbannier]

Also backported to release/1.8.

[pauldokas]

Is there any chance of getting this fix into the 1.9 branch? Possibly in a 1.9.1 release?

[bbannier]

Backported to release/1.9.

@pauldokas Since I had already backported this to release/1.8 I now backported it to release/1.9 for consistency as well.

Please note that we typically only backport patches which fix incorrect runtime behavior (e.g., types showing incorrect behavior at runtime; grammar accepted, but incorrect parser generated). The issue fixed here does not fall under this since it is about adding to an incomplete feature (unintentionally incomplete and inconsistent with e.g., the docs, but not effect on runtime behavior). Backporting to release/1.8 was already outside of what we usually consider. If you have a general interest in such kind of fixes in the future it would be best to instead use the latest release, or alternatively cherry-pick changes of interest to you into a fork.