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.

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


		Assert.invariant(stopValues.nonEmpty)

		final override def pouStatus = PoUStatus.HasPoU

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants