Implement occursCountKind="stopValue" by jadams-tresys · Pull Request #1728 · apache/daffodil

jadams-tresys · 2026-09-10T19:35:20Z

Bulk of this was done with AI. I tried to give it a thorough review and cleaned up a few things that seemed at odds with our usual coding style (see commit bf40370) but I'll admit I'm not super familiar with all the parts of Daffodil that were involved in this change.

Test coverage seems pretty good, but I think a few more need to be added to verify the various SDE/ProcessingError conditions outlined in the spec. I will add those soon, but wanted to get this up for review to make sure things are headed in the right direction.

DAFFODIL-501

olabusayoT

Some comments about style.

olabusayoT · 2026-10-14T18:42:55Z

+  <!--
+      Codegen backends cannot parse stopValueElement; generating C code for it fails with
+      "Code generation not supported for: RepOrderedStopValueSequenceChild"
+
+      daffodil generate c -r stopValueElement -s variablelen.dfdl.xsd
+  -->
+


It looks pretty straightfoward to add the ReoOrderedStopValueSequenceChild support to the C generator, any reason why we're not doing this?

I'd suggest we hold off for now, if we do want it we should do it as a separate PR.

For now only implemented parse support

Also remove some unused test files

One of the requirements in the DFDL spec is that if a stopValue shows up in the infoset, it should result in a processing error.

- ParticleMixin: restore LV(Symbol(...)) for hasStopValue; cook dfdl:occursStopValue separately for parsing and unparsing - SequenceChild: use if/else instead of matching on a boolean - SeparatedSequenceParsers: comment explaining why checkParseAttemptStatus is applied after the separator has been parsed - Remove DAFFODIL-501 from source comments and shorten long ones - Clarify in the variablelen test files that only the Daffodil runtime, not the C code generator, supports stopValue

stevedlawrence · 2026-10-19T17:45:37Z

+  <!--
+      Codegen backends cannot parse stopValueElement; generating C code for it fails with
+      "Code generation not supported for: RepOrderedStopValueSequenceChild"
+
+      daffodil generate c -r stopValueElement -s variablelen.dfdl.xsd
+  -->
+


I'd suggest we hold off for now, if we do want it we should do it as a separate PR.

stevedlawrence · 2026-10-19T17:58:26Z

+    // while building the grammar of an element whose required evaluations have not run yet
+    schemaDefinitionWhen(
+      !isSimpleType,
+      "Property occursCountKind='stopValue' requires a simple type. The value of each occurrence is compared with the dfdl:occursStopValue."


The value of each occurrence is compared with the dfdl:occursStopValue.

I don't think we need this diagnostic sentence, we don't usually explain why certain restrictions exist. Users can look to the spec if they need justification for our SDEs.

stevedlawrence · 2026-10-21T16:49:33Z

+
+  // Unparsing an occursCountKind='stopValue' array emits all of the infoset occurrences
+  // followed by one synthesized terminating occurrence whose value is the first
+  // dfdl:occursStopValue (that occurrence consumes data but is not part of the infoset,


SHould this be something like "(that occurrence produces data but ...)"? Nothing in unparse consumes data

stevedlawrence · 2026-10-21T17:06:21Z

+
+  Assert.invariant(stopValues.nonEmpty)
+
+  final override def pouStatus = PoUStatus.HasPoU


Section 9.3.3 says that StopValues do not have points of uncertainties:

When parsing an array or optional element, points of uncertainty only occur for certain values of dfdl:occursCountKind[23], as follows:
...
stopValue No point of uncertainty (The stop value must always be present, even when XSD minOccurs is 0).

Is this correct, or does pouStatus related to something other this table?

stevedlawrence · 2026-10-21T17:22:19Z

+    if (!status.isSuccess || !pstate.isSuccess) return status
+    val maybeElem = pstate.infosetLastChild
+    maybeElem.get match {
+      case elem: DISimple if (elem.erd eq erd) && !elem.isNilled && elem.hasValue => {


If we are in checkStopValue and the parse attempt was successful doesn't that mean we must have parsed at least one element, it must have been a DISImple, and that simple must have the erd we are looking for?

So it feels like the only conditions that really matter are if it's nilled (which does not match StopValue) and, if it's not nilled it must have a value. So this could be simplified to:

if (!status.isSuccess || !pstate.isSuccess) return status val maybeElem =pstate.infosetLastChild val simple = maybeElement.get.asSimple if (!simple.isNilled && stopValues.exists(stopValueMatch(elem.dataValue.getAnyRef, _)) { ParseAttemptStatus.StopValueRep } else status

stevedlawrence · 2026-10-21T18:45:09Z

              )
            } else {
              Assert.invariant(ev.isEnd && ev.erd.isComplexType)
+              // Zero infoset occurrences, but an occursCountKind='stopValue'


Same here, does this want to happen after the checkFinalOccursCOunt, or it doesn't matter if we don't change some state?

stevedlawrence · 2026-10-21T18:47:46Z

+   */
+  final def checkStopValueInInfoset(state: UState, maybeElem: Option[DISimple]): Unit = {
+    maybeElem.foreach { elem =>
+      if (!elem.isNilled && elem.hasValue) {


Do we need elem.hasValue? Once we've gotten if it isn't nilled doesn't it have to have a value? Similarly, don't we have to have an element so maybeElement doesn't need to be optional?

stevedlawrence · 2026-10-21T18:51:40Z

+        if (stopValues.exists { sv => stopValueMatch(dataValue, sv.getAnyRef) }) {
+          UE(
+            state,
+            "Found stop value '%s' in the infoset for array element %s. Occurrences of a dfdl:occursCountKind='stopValue' array must not have a value equal to one of the dfdl:occursStopValue values; the terminating occurrence is added automatically when unparsing.",


Can we make this a little less verbose? Maybe something like "Stop value %s cannot appear in the infoset". That's mabye a bit too terse, but it does convey the same information.

Note we also don't need to include "array element %s", since that should appear in the SDE context.

stevedlawrence · 2026-10-21T19:15:37Z

          <y>8</y>
          <y>9</y>
-          <z>some string</z>
+          <z>,some string</z>


I'm wondering if the change to this test is correct? The comma after 10 is definitely a delimiter since it is needed to determine where the 10 ends. So it's it feels wrong for it to appear in the data of the next element--we already determined it's a delimiter and not data.

It almost feels like delimited stopValues do not play well with infix separators? Usually delimited infix separated arrays end by seeing an out of scope delimiter. So if you see an in-scope delimiter, you know there must be at least one more element. But with stopValue, an in-scope delimiter doesn't necessarily indicate there is another value because the stopValue could end the array. So the question is what do you do with the delimiter? If we consume it as a delimiter it feels like we're treating it as if separatorPosition was postifx. But if we don't consume it we're treating it as data, but we already identified it as a delimiter.

I think the same issue exists with prefix separators to, since infix and prefiex are essentially the same thing, there just isn't a separate at the very begnning.

I'm not sure what the right answer is. Maybe we say that lengthKind="delimited" and occursCountKind="stopValue" cannot be in a sequence with infix/prefix separator position? I'm not sure if that's too strict, maybe there are causes where this can work?

stevedlawrence · 2026-10-21T19:16:17Z

-  @Ignore @Test def BB000 = test
+  // DAFFODIL-501: occursCountKind='stopValue' is now implemented; this test was
+  // enabled with its expected infoset updated to match the DFDL specification (see
+  // BB.tdml for details).


I don't think we need this comment.

stevedlawrence

Looks good. I think there are some things that don't match the spec, and some things that it's not even clear what the right behavior is

stevedlawrence · 2026-10-22T12:08:23Z

+    </xs:complexType>
+  </xs:element>
+
+  <!-- stopValue on a nillable element -->


There's a number of conditionas int eh code that check !isNilled. I didn't see where we disallow nillable for stopValue, but if it's not allowed as this test asserts I would imagine we could remove thos isNilled checks since that isn't possible.

Note I couldn't find anything in the spec that disallows nillable on stopValue,but it does kindof seem like a reasonable restriction. We could always lift the restriction in the future if there were valid use cases for it, but it seems unlikely.

stevedlawrence · 2026-10-22T12:09:23Z

+  limitations under the License.
+-->
+
+<xs:schema


It's not standard to put these in a separate dfdl.xsd file, suggest we embed these in the occursStopValue.tdml file

stevedlawrence · 2026-10-22T12:15:17Z

+    </tdml:infoset>
+  </tdml:parserTestCase>
+
+  <!-- Data ends without the stop value ever being found -->


I think this test is wrong, the spec states:

It is a Processing Error if a stop value occurrence is not found in the data (including when there are zero other occurrences).

So if we hit EOD, there was no stopValue and it should be a processing error. Note that elsewhere it says there is no POU, so ithink it means we backtrack passed the entire array to whatever PoU surrounds the array? In this specific test, there is no surronding PoU so I would expect this to fail with a ParseError.

stevedlawrence · 2026-10-22T12:20:59Z

+    <tdml:document><![CDATA[abcd07]]></tdml:document>
+    <tdml:errors>
+      <tdml:error>Parse Error</tdml:error>
+      <tdml:error>Insufficient bits</tdml:error>


I think this test shoudl have a different error message. We hit EOD without finding a stop value so that should be a parse error about stop value not value. It looks like in this case it doesn't care ethat a stop value wasn't value and continues on and fails when it tries to parse the following required element. But I think it should never even attempt to parse that element.

stevedlawrence · 2026-10-22T12:25:11Z

  @Test def expression_01 = test
  @Test def expression_16 = test
  @Test def expression_17_error = test
-  @Test def stopValue = test


Instead of disabling this, maybe we should create separate tdml test for the C implementation showing that it expects an error with the DaffodilC implementation?

stevedlawrence · 2026-10-22T12:33:25Z

+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+  </tdml:unparserTestCase>
+


Thoughts on adding tests for:

delimited length stopValues. We have the BB000 test, but it might be useful to have delimited length postfix, infix, and prefix test here to isolate them. Based on another comment, I'm not even entirely sure how the latter two should work with a trailing delimiter.

Add a test where an array element fails to parse becuse it's not valid. For example svIntRoot but instead of a array element being 00001 it's like ABCDE (i.e. not a valid int). I think that should fail and cause it to backtrack the entire array.

Add a test about character classes in stop values that should not be allowed. For example, I think %NL; or %WSP; should fail to compile, assuming we don't allow those. I think allowing them allows too much complication since the comparison logic becomes something similar to delimtier scanning. We could always add that in the future if it was needed.

Add a test about character classes in stop values that should be allowed? Maybe use byte enties. %ES might also be worth testing if we support that (which seems reaonsable, and empty fields seems like a reasonable way to stop an array of delimited thing).

stevedlawrence · 2026-10-22T12:37:07Z

+
+  final lazy val occursStopValuesForParse = occursStopValues(forUnparse = false)
+
+  final lazy val occursStopValuesForUnparse = occursStopValues(forUnparse = true)


Do we need separate cookers for stop value. Seems like we want to cook the stop values to their literal values for both unparse and parse? Based on a comment in cookedOccursStopValue it sounds like we sometimes don't convert %ES to a empty string, but I think we always want to. For parse we would want to compare the logical value to empty string, and on unparse we would want to unparse an empty string. So it seems like they should be cooked the same.

jadams-tresys requested review from olabusayoT and stevedlawrence September 10, 2026 19:35

olabusayoT reviewed Sep 30, 2026

View reviewed changes

olabusayoT reviewed Oct 14, 2026

View reviewed changes

Josh Adams added 9 commits October 19, 2026 07:48

Initial implementation of occursCountKind="stopValue"

b8af2a4

For now only implemented parse support

Add generated tests

a2a7d20

Add unparse support for stopValue

f065583

By hand cleanup of AI changes

bb6fe0d

Add test for exceeding maxOccurs

1646ad4

Add more negative test cases

b493fb7

Also remove some unused test files

Add check for stopValue appearring in infoset

516e042

One of the requirements in the DFDL spec is that if a stopValue shows up in the infoset, it should result in a processing error.

Fix test errors caused by rebase onto main

6a0ff59

jadams-tresys force-pushed the DAFFODIL-501 branch from 583716d to 6a0ff59 Compare October 19, 2026 12:46

stevedlawrence reviewed Oct 21, 2026

View reviewed changes

stevedlawrence requested changes Oct 22, 2026

View reviewed changes


		Assert.invariant(stopValues.nonEmpty)

		final override def pouStatus = PoUStatus.HasPoU


		final lazy val occursStopValuesForParse = occursStopValues(forUnparse = false)

		final lazy val occursStopValuesForUnparse = occursStopValues(forUnparse = true)

Conversation

jadams-tresys commented Sep 10, 2026

Uh oh!

olabusayoT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants