stevedlawrence commented on code in PR #1652:
URL: https://github.com/apache/daffodil/pull/1652#discussion_r3305635266
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala:
##########
@@ -116,6 +117,7 @@ final class SchemaSet private (
requiredEvaluationsAlways(root)
requiredEvaluationsAlways(checkForDuplicateTopLevels())
+
requiredEvaluationsAlways(root.checkEndOfParentRestrictions(Some(LengthUnits.Characters)))
Review Comment:
Is it possible to put this on the `Root` class? It feels like that' the
right place for this check and any checks that algorithmically need to start
from the root like this one.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -269,6 +267,26 @@ trait Term
.getOrElse(false)
}
+ final lazy val immediatelyEnclosingParentForEOPElem: Option[Term] = {
Review Comment:
Can this be removed? It doesn't look like it's used anywhere, and it is has
subtle edge cases where it doesn't necessarily return what one might think it
would because it only knows about lexical parents.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -562,4 +580,21 @@ trait Term
}
}
+ final lazy val realElementChildren: Seq[ElementBase] = {
+ termChildren.flatMap {
+ case eb: ElementBase => Seq(eb)
+ case c: Choice => Nil
+ case mg: ModelGroup => mg.realElementChildren
+ }
+ }
+
+ lazy val flattenedChildren: IndexedSeq[Term] = termChildren.flatMap { c =>
Review Comment:
Does an IndexedSeq provide much benefit with how this is used? I don't think
we will usually know what index we an element is, so we'll likely have to do a
linear scans regardless. And the way this is used it looks like we do indexOf
and then get everything after that index. If this were a normal Seq, I think
similar logic could be implemented like this:
```scala
val elementPlusNextSiblings = flattendChildren.dropWhile(_ !=
specificEopChild)
Assert.invariant(!elementPlusNextSibligns.isEmpty)
val nextSiblings = elementPlusNextSiblings.tail
if (nextSiblings.isDefined) {
// SDE, children exist afterEOPelement
}
```
We don't really use the indexability, so this might avoid some overheads.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -252,6 +253,49 @@ trait ElementBaseGrammarMixin
}
final lazy val prefixedLengthBody = prefixedLengthElementDecl.parsedValue
+ def checkEndOfParentRestrictionsOnCurrentElement(optParentELU:
Option[LengthUnits]): Unit = {
+ val currentElement: ElementBase = this
+ if (currentElement.lengthKind != LengthKind.EndOfParent) {} else {
Review Comment:
I think this function is only called on elements with lengthKind=EOP. I
would suggest we change this to an assert that lengthKind == EOP and remove the
condition.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -562,4 +580,21 @@ trait Term
}
}
+ final lazy val realElementChildren: Seq[ElementBase] = {
+ termChildren.flatMap {
+ case eb: ElementBase => Seq(eb)
+ case c: Choice => Nil
+ case mg: ModelGroup => mg.realElementChildren
+ }
+ }
+
+ lazy val flattenedChildren: IndexedSeq[Term] = termChildren.flatMap { c =>
+ c match {
+ case eb: ElementBase => IndexedSeq(eb)
+ case mg: ModelGroup if mg.groupMembers.isEmpty => IndexedSeq(mg)
+ case s: SequenceTermBase => s.flattenedChildren
+ case c: ChoiceTermBase => c.flattenedChildren
Review Comment:
I'm not sure this is correct for ChoiceTermBase, at least in the context
where this is used to detect if there are siblings after this. For example, say
we have a schema below, where the first branch of a choice contains an EOP
element:
```xml
<element name="foo">
<complexType>
<choice>
<element name="bar" dfdl:lengthKind="endOfParent" ... />
<element name="baz" dfdl:lenghtKind="explicit" ... />
</choice>
</complexType>
</element>
```
In this case, I think the choices `flattenedChildren` will look like:
```
IndexedSeq(bar, baz)
```
And so it will look like baz follows an EOP element and would SDE. But I
think this this schema is legal?
Since choiceLengthKind doesn't allow EOP, maybe we just return Nil here,
similar to realElementChildren? And maybe choices need to run their checks on
individual branches? I'm not sure we can just flatten all branches of a choice
into a single list of children.
Or maybe we need a different way to determine if there are things after an
EOP element that doesn't require flattening. Maybe checkEndOfParentRestrictions
returns a boolean that indicates if it saw an EOP element, and then as we roll
up from the recursion we check the result? For xample, maybe something like:
```scala
def checkEndOfParentRestrcitions(...) {
...
val sawEOP = term.termChildren.foldLeft(false) { case (child, sawEOP)) =>
if (sawEOP) {
// the previous child saw an EOP element, and this child is after it,
so error
SDE(...)
}
child.checkEndOfParentRestrictions(...)
}
sawEOP
}
```
This doesn't handle non-represented elements or choice branches, but maybe
something along those lines? This is also nice in that it doesn't require
allocating any new lists like the flattenedChildren. We're already having to
walking the entire tree, so if we can do logic just be return values or
something that would be preferred. It's not the standard way we do things, but
this parent/sibling logic this reuires kindof makes the normal way difficult.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/AlignedMixin.scala:
##########
@@ -456,7 +456,11 @@ trait AlignedMixin extends GrammarMixin { self: Term =>
}
case LengthKind.Delimited => encodingLengthApprox
case LengthKind.Pattern => encodingLengthApprox
- case LengthKind.EndOfParent => LengthMultipleOf(1) // NYI
+ case LengthKind.EndOfParent =>
+ // technically the alignment of an EndOfParent element would be the
+ // alignment of its parent minus our current alignment (i.e
alignment of
+ // prior sibs) but since nothing can follow
Review Comment:
This looks like a fragment sentence? Also this comment talks about alignment
but this function is about calculating the approximate length. I think
`LengthMultipleOf(1)` is correct, but we might want to update the comment as to
why.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
+ parent.lengthKind match {
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ schemaDefinitionError(
+ "element is specified as dfdl:lengthKind=\"endOfParent\",
but its parent is an element with dfdl:lengthKind 'implicit' or 'delimited'."
+ )
+ case _ => // do nothing
+ }
+ parent.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case s: SequenceTermBase if eopChildren.nonEmpty => {
+ schemaDefinitionWhen(
+ s.separatorPosition == SeparatorPosition.Postfix,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:separatorPosition defined as 'postfix'."
+ )
+ schemaDefinitionWhen(
+ s.sequenceKind != SequenceKind.Ordered,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:sequenceKind defined as 'unordered'."
+ )
+ schemaDefinitionWhen(
+ s.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ s.realElementChildren.exists(e => e.floating == YesNo.Yes),
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with elements defining dfdl:floating='yes'."
+ )
+ schemaDefinitionWhen(
+ s.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a non-zero dfdl:trailingSkip."
+ )
+ eopChildren.foreach {
+ case e: ElementBase => {
+ s.checkChildrenForSiblingsAfterEOPElement(e)
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case c: ChoiceTermBase if eopChildren.nonEmpty => {
+ // TODO: The DFDL spec (12.3.6) explicitly mentions that an
EndOfParent element
+ // can be terminated by a choice with choiceLengthKind='explicit',
with no reference
+ // to implicit length choices. Later on in 12.3.6, it specified what
the parent Effective
+ // Length Units would be for an explicit length choice, again with no
reference to
+ // implicit length choices. The only way to get an EndOfParent element
within an
+ // implicit length choice would be to have it actually be terminated
by the choice's
+ // parent, similar to how we treat elements within an element that is
also EndOfParent.
+ // The only mention of implicit length choices in the DFDL Spec is to
mention SDEs when
+ // an EndOfParent element is enclosed by a choice with
choiceLengthKind='implicit'. We
+ // think that may be a typo, so for now we disallow
ChoiceLengthKind.Implicit
+ // enclosing an EndOfParent element.
+ // See Daffodil-3080
+ schemaDefinitionWhen(
+ c.choiceLengthKind == ChoiceLengthKind.Implicit,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but its
parent is a choice with dfdl:choiceLengthKind 'implicit'."
+ )
+ schemaDefinitionWhen(
+ c.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a choice with a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ c.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a choice with a non-zero dfdl:trailingSkip."
+ )
+ val optParentELU = c.myEffectiveLengthUnits(optLastNonEOPLU)
+ eopChildren.foreach {
+ case e: ElementBase => {
+ c.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case _ => // do nothing
+ }
+ // end checks
+ term.termChildren.foreach { e =>
+ lazy val optParentELU = term.myEffectiveLengthUnits(optLastNonEOPLU)
+ term match {
+ case parent @ (_: ElementBase | _: ChoiceTermBase | _:
SequenceTermBase) => {
+ e.checkEndOfParentRestrictions(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ }
+
+ def checkChildrenForSiblingsAfterEOPElement(specificChild: ElementBase) = {
Review Comment:
One thing worth considering is this line in the spec:
> It is a Schema Definition Error if:
> ...
> * any other represented element is defined between this element and the
end of the enclosing component.
I think the implication it is that it is legal to have non-represented
elements (i.e. IVC's) that appear after a an EOP element. So for example, this
is legal:
```xml
<element name="foo" dfdl:lengthKind="explicit" ...>
<complexType>
<sequence>
<element name="bar" dfdl:lengthKind="endOfParent" ... />
<element name="baz" dfdl:inputValueCalc="{ ... }" ... />
</sequence>
</complexType>
</element>
```
Maybe this can be done by just excluding IVC elements form
flattenedChildren, since we essentially ignore them in the context of checking
for siblings after EOP?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -605,7 +649,19 @@ trait ElementBaseGrammarMixin
case LengthKind.Pattern =>
schemaDefinitionError("Binary data elements cannot have
lengthKind='pattern'.")
case LengthKind.EndOfParent =>
- schemaDefinitionError("Binary data elements cannot have
lengthKind='endOfParent'.")
+ // only for packed binary data, length must be computed at runtime.
+ if (
+ representation == Representation.Binary
+ && optionBinaryCalendarRep.isDefined
+ && Seq(BinaryCalendarRep.Packed, BinaryCalendarRep.Bcd,
BinaryCalendarRep.Ibm4690Packed)
+ .contains(binaryCalendarRep)
+ || representation == Representation.Binary
+ && optionBinaryNumberRep.isDefined
+ && Seq(BinaryNumberRep.Packed, BinaryNumberRep.Bcd,
BinaryNumberRep.Ibm4690Packed)
+ .contains(binaryNumberRep)
Review Comment:
I feel like we don't usually use contains for this kind of thing and instead
just do a match case. I have no strong preference either way, but it might
match our existing conventions in this file a bit better. Maybe it gets too
messy though...
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -605,7 +649,19 @@ trait ElementBaseGrammarMixin
case LengthKind.Pattern =>
schemaDefinitionError("Binary data elements cannot have
lengthKind='pattern'.")
case LengthKind.EndOfParent =>
- schemaDefinitionError("Binary data elements cannot have
lengthKind='endOfParent'.")
+ // only for packed binary data, length must be computed at runtime.
+ if (
+ representation == Representation.Binary
Review Comment:
Do we need representation checks here? I would assume that if
binaryNumberKnownLengthInBits is called then that implies we're already
determined this is some kind of binary representation. For example, we don't
check representation in any of the other cases.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -922,6 +998,8 @@ trait ElementBaseGrammarMixin
byteOrderRaw // must be defined or SDE
}
(binaryNumberRep, lengthKind, binaryNumberKnownLengthInBits) match {
+ case (BinaryNumberRep.Binary, LengthKind.EndOfParent, _) =>
+ SDE("lengthKind='endOfParent' is not allowed with binary number
representation")
Review Comment:
Another example of and SDE that feels like we already check somewhere else.
Is it possible to only have one place?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1396,11 +1490,9 @@ trait ElementBaseGrammarMixin
case LengthKind.Implicit
if isSimpleType && impliedRepresentation == Representation.Binary
=>
new SpecifiedLengthImplicit(this, body, implicitBinaryLengthInBits)
- case LengthKind.EndOfParent if isComplexType =>
- notYetImplemented("lengthKind='endOfParent' for complex type")
- case LengthKind.EndOfParent =>
- notYetImplemented("lengthKind='endOfParent' for simple type")
- case LengthKind.Delimited | LengthKind.Implicit =>
+ case LengthKind.EndOfParent if (isComplexType ||
this.isInstanceOf[Root]) =>
+ new SpecifiedLengthEndOfParent(this, body)
Review Comment:
Why is this needed for simple Roots? It feels like the `body` and simple
root and non-root elements would both be the same thing, so it's not clear why
root simples need this SpecifiedLengthEndOfParent wrapper. Maybe add a comment
explaining why it's needed for root?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1396,11 +1490,9 @@ trait ElementBaseGrammarMixin
case LengthKind.Implicit
if isSimpleType && impliedRepresentation == Representation.Binary
=>
new SpecifiedLengthImplicit(this, body, implicitBinaryLengthInBits)
- case LengthKind.EndOfParent if isComplexType =>
- notYetImplemented("lengthKind='endOfParent' for complex type")
- case LengthKind.EndOfParent =>
- notYetImplemented("lengthKind='endOfParent' for simple type")
- case LengthKind.Delimited | LengthKind.Implicit =>
+ case LengthKind.EndOfParent if (isComplexType ||
this.isInstanceOf[Root]) =>
+ new SpecifiedLengthEndOfParent(this, body)
+ case LengthKind.Delimited | LengthKind.Implicit |
LengthKind.EndOfParent =>
Assert.impossibleCase(
"Delimited and ComplexType Implicit cases should not be reached"
Review Comment:
Should probably update this comment to mention EOP, or maybe reword it to be
more generic.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
+ parent.lengthKind match {
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ schemaDefinitionError(
+ "element is specified as dfdl:lengthKind=\"endOfParent\",
but its parent is an element with dfdl:lengthKind 'implicit' or 'delimited'."
+ )
+ case _ => // do nothing
Review Comment:
I feel like in general if we have a donothing case, we usually opt for
something like
```scala
schemaDefinitionWhen(
lengthKind == LengthKind.Implicit || lengthKind == LengthKind.Delimited,
"..."
)
```
Since it only two things, it feels a bit cleaner
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
+ parent.lengthKind match {
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ schemaDefinitionError(
+ "element is specified as dfdl:lengthKind=\"endOfParent\",
but its parent is an element with dfdl:lengthKind 'implicit' or 'delimited'."
+ )
+ case _ => // do nothing
+ }
+ parent.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case s: SequenceTermBase if eopChildren.nonEmpty => {
+ schemaDefinitionWhen(
+ s.separatorPosition == SeparatorPosition.Postfix,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:separatorPosition defined as 'postfix'."
+ )
+ schemaDefinitionWhen(
+ s.sequenceKind != SequenceKind.Ordered,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:sequenceKind defined as 'unordered'."
+ )
+ schemaDefinitionWhen(
+ s.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ s.realElementChildren.exists(e => e.floating == YesNo.Yes),
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with elements defining dfdl:floating='yes'."
+ )
+ schemaDefinitionWhen(
+ s.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a non-zero dfdl:trailingSkip."
+ )
+ eopChildren.foreach {
+ case e: ElementBase => {
+ s.checkChildrenForSiblingsAfterEOPElement(e)
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case c: ChoiceTermBase if eopChildren.nonEmpty => {
+ // TODO: The DFDL spec (12.3.6) explicitly mentions that an
EndOfParent element
+ // can be terminated by a choice with choiceLengthKind='explicit',
with no reference
+ // to implicit length choices. Later on in 12.3.6, it specified what
the parent Effective
+ // Length Units would be for an explicit length choice, again with no
reference to
+ // implicit length choices. The only way to get an EndOfParent element
within an
+ // implicit length choice would be to have it actually be terminated
by the choice's
+ // parent, similar to how we treat elements within an element that is
also EndOfParent.
+ // The only mention of implicit length choices in the DFDL Spec is to
mention SDEs when
+ // an EndOfParent element is enclosed by a choice with
choiceLengthKind='implicit'. We
+ // think that may be a typo, so for now we disallow
ChoiceLengthKind.Implicit
+ // enclosing an EndOfParent element.
+ // See Daffodil-3080
+ schemaDefinitionWhen(
+ c.choiceLengthKind == ChoiceLengthKind.Implicit,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but its
parent is a choice with dfdl:choiceLengthKind 'implicit'."
+ )
+ schemaDefinitionWhen(
+ c.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a choice with a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ c.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a choice with a non-zero dfdl:trailingSkip."
+ )
+ val optParentELU = c.myEffectiveLengthUnits(optLastNonEOPLU)
+ eopChildren.foreach {
+ case e: ElementBase => {
+ c.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case _ => // do nothing
+ }
+ // end checks
+ term.termChildren.foreach { e =>
+ lazy val optParentELU = term.myEffectiveLengthUnits(optLastNonEOPLU)
Review Comment:
I think we can move term.myEffectiveLengthUnits out of this foreach since it
doesn't depend on anything but term which doesn't change in the foreach.
In fact, I think we can move this to the top and then reference optParentELU
everywhere it's needed. We reference `this.myEffectiveLengthUnits` in a number
of places so we should just do it at the beginning to avoid the duplication.
And that function shoudl work in all circumstances, it just sometimes returns
None to indicate situations where using it isn't valid because we're EOP
children should be legal and we should error somewhere else.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
Review Comment:
Suggest we change this default case to case `c: ChoiceTermBase => None`
which I think is the only other case, and scala should error if we are missing
any other cases.
It's also maybe worth adding a comment about these None cases, that they
indicate elements where EOP children should be illegal children, and so it
doesn't make sense to make any decision that require this elements ELU. And we
use None ensure that doesn't happen.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
+ parent.lengthKind match {
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ schemaDefinitionError(
+ "element is specified as dfdl:lengthKind=\"endOfParent\",
but its parent is an element with dfdl:lengthKind 'implicit' or 'delimited'."
+ )
+ case _ => // do nothing
+ }
+ parent.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case s: SequenceTermBase if eopChildren.nonEmpty => {
+ schemaDefinitionWhen(
+ s.separatorPosition == SeparatorPosition.Postfix,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:separatorPosition defined as 'postfix'."
+ )
+ schemaDefinitionWhen(
+ s.sequenceKind != SequenceKind.Ordered,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:sequenceKind defined as 'unordered'."
+ )
+ schemaDefinitionWhen(
+ s.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ s.realElementChildren.exists(e => e.floating == YesNo.Yes),
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with elements defining dfdl:floating='yes'."
+ )
+ schemaDefinitionWhen(
+ s.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a non-zero dfdl:trailingSkip."
+ )
+ eopChildren.foreach {
+ case e: ElementBase => {
+ s.checkChildrenForSiblingsAfterEOPElement(e)
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
Review Comment:
I'm not sure we need to do these checks here? I'm pretty sure we don't need
the `e.checkEndOfParentRestrictionsOnCurrentElement` since these `eopChildren`
are all the same as what whatever contains this sequence, so we would just be
duplicating checks.
I feel like the way we flatten children for checking siblings also kindof
ignores sequences, but I'm not positive.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
+ parent.lengthKind match {
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ schemaDefinitionError(
Review Comment:
I think this SDE (and maybe most of these SDEs?) want to be something like
`child.SchemaDefintionError` so that the context of the SDE is the element that
specifies EOP instead of the parent. Once a user knows where the EOP elment is,
it should be easy to find the offending parent. But the other way is maybe
difficult, it could be hard to find the sole EOP child if there a bunch of
children.
##########
daffodil-core/src/main/scala/org/apache/daffodil/io/InputSource.scala:
##########
@@ -181,6 +181,12 @@ abstract class InputSource {
*/
def releasePosition(bytePos0b: Long): Unit
+ /**
+ * Get the number of bytes that are available to be read until the end of
+ * the data stream.
+ */
+ lazy val bytesTillEndOfDataStream: Long = -1L
Review Comment:
Suggest we make this a def so we require classes that extend InputSource
create implement this function.
Also, I'm wondering if we could run into any issues with this being a lazy
val? My concern is that `bytesUntilEndOfDataStream` is only correct at a single
point in the stream. So if we calcuulate this lazy val at one position, move
the bit position, and then access it again, then stored
bytesUntilEndOfDataStream is no longer correct. I can't think of an example
where this is actually possible, but we could avoid this entirely the API is
about just the EOD position. For example, what if instead we do something like
this:
```scala
/**
* Getteh byte position of End Of Data, using zero-based indexing.
*
* Throws some exception if end of data could not be found within an
implementation defined limit.
*/
def endOfDataPosition(): Long
```
This way implementations can try to find and store the EOD byte position
without needing to know the current byte position. And if anything like
endOfParent parsers need to calculate a length to the EOD they can get this
position and subtract their current position.
##########
daffodil-core/src/main/scala/org/apache/daffodil/io/InputSource.scala:
##########
@@ -564,6 +570,22 @@ class BucketingInputSource(
headBucketBytePosition0b += bytesRemoved
oldestBucketIndex = 0
}
+
+ override lazy val bytesTillEndOfDataStream: Long = {
+ val initialBytePosition = position()
+ val endOfDataNotReached = fillBucketsToIndex(maxNumberOfNonNullBuckets, 8)
Review Comment:
Two things:
1. Instead of passing in 8 for the second parameter for the
bytesNeededInBucket, I'd suggest we go with `bucketSize`. That way the goal is
to fill all of the last bucket instead of just the first 8 bytes and so we are
guaranteed to try have all `maxNumberOfNonNullBuckets` filled instead of the
last one just being possibly 8 bytes full.
2. The first parameter of `fillBucketsToIndex` isn't the number of buckets
to fill, it is the goal index of the bucket to fill up to. So for example, if
our current position was in bucket 100 and lets say `maxNumberOfNonNullBuckets`
was 2, then I think we want to try to fill up buckets 100 and 101 (so that we
have 2 full buckets, including our current bucket). So in this example the
first parameter would want to be 101 instead of `maxNumberOfNonNullBuckets`.
So I think the math for that all looks something like this:
```scala
val curBucketIndex = getBucketIndex(curBytePosition0b)
val goalBucketIndex = curBucketIndex + maxNumberOfNonNullBuckets - 1
val endOfDataNotReached = fillBucketsToIndex(goalBucket, bucketSize)
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/io/InputSource.scala:
##########
@@ -645,4 +667,11 @@ class ByteBufferInputSource(byteBuffer: ByteBuffer)
extends InputSource {
override def close(): Unit = {
// do nothing. No resources to release.
}
+
+ override lazy val bytesTillEndOfDataStream: Long = {
+ val initialPosition = position()
+ val br = byteBuffer.remaining
+ position(initialPosition)
+ br
+ }
Review Comment:
`byterBuffer.remaining` doesn't change the position so we don't need to
capture and restore it.
Note that if we switch to `endOfDataPosition`, that could be calculated when
the class is initialized, and it would just be something like `override val
endOfDataPostion = bb.remaining()`
##########
daffodil-core/src/main/scala/org/apache/daffodil/io/InputSource.scala:
##########
@@ -564,6 +570,22 @@ class BucketingInputSource(
headBucketBytePosition0b += bytesRemoved
oldestBucketIndex = 0
}
+
+ override lazy val bytesTillEndOfDataStream: Long = {
+ val initialBytePosition = position()
+ val endOfDataNotReached = fillBucketsToIndex(maxNumberOfNonNullBuckets, 8)
+ if (!endOfDataNotReached) {
+ val numBytesFilled = totalBytesBucketed - initialBytePosition
+ // reset the byte position to the initial byte position
+ position(initialBytePosition)
+ numBytesFilled
+ } else {
+ position(initialBytePosition)
+ throw new Exception(
+ s"Attempted to fill to end of data stream, but did not reach end of
data stream before maxCacheSizeInBytes: ${maxCacheSizeInBytes}."
+ )
Review Comment:
I'm wondering if we should return a Option/Maybe here instead of throwing an
exception. That way we can let callers decide how to handle the cases where an
implementation couldn't find EOD. I imagine in most cases they'll want to
create an SDE, but maybe there are other options. And we can just document that
the function might return None if the end of data stream couldn't be found.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/ParserTraits.scala:
##########
@@ -181,7 +181,14 @@ trait BitLengthFromBitLimitMixin {
}
def getLengthInBits(pstate: PState): Long = {
- val len = pstate.bitLimit0b.get - pstate.bitPos0b
- len
+ if (pstate.bitLimit0b.isDefined) {
+ val len = pstate.bitLimit0b.get - pstate.bitPos0b
+ len
+ } else if (isEndOfParent) {
+ val byteLimit = pstate.dataInputStream.bytesTillEndOfDataStream
+ byteLimit * 8
Review Comment:
Is it possible for our current bitPosition to not on a byte boundary? Or do
all EOP types required byte alignment?
I'm not sure if packed decimals can by half-byte aligned or not (I think
they can?), but I'm pretty xs:hexBinary does not require a specific alignment.
For example:
```xml
<element name="root" dfdl:lengthKind="endOfParent">
<complexType>
<sequene>
<element name="foo" type="xs:int" dfdl:lengthKind="4"
dfdl:lengthUnits="bits" ... />
<element name="leftOver" type="xs:hexBinary"
dfdl:lengthKind="endOfParent" ... />
</sequence>
</complexType>
</element>
```
This feels like a reasonable way to capture leftOver data, even if we aren't
byte aligned.
I think this is maybe another reason in favor for switching to something
like endOfDataPosition, then this becomes something like
```scala
val dis = pstate.dataInputStream
val len = (dis.endOfDataPosition * 8) - dis.curBitPos0b
```
Which works as expected even if curBitPosition is not byte aligned.
##########
daffodil-core/src/main/scala/org/apache/daffodil/io/InputSourceDataInputStream.scala:
##########
@@ -136,6 +136,8 @@ final class InputSourceDataInputStream private (val
inputSource: InputSource)
*/
def hasReachedEndOfData: Boolean = inputSource.hasReachedEndOfData
+ lazy val bytesTillEndOfDataStream: Long =
inputSource.bytesTillEndOfDataStream
Review Comment:
For consistency, I'd suggest we make this a def. This is essentially a
delegate to the underlying implementation, and we can let them decide best how
the result show be stored or recalcuated.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -252,6 +253,49 @@ trait ElementBaseGrammarMixin
}
final lazy val prefixedLengthBody = prefixedLengthElementDecl.parsedValue
+ def checkEndOfParentRestrictionsOnCurrentElement(optParentELU:
Option[LengthUnits]): Unit = {
+ val currentElement: ElementBase = this
+ if (currentElement.lengthKind != LengthKind.EndOfParent) {} else {
+ schemaDefinitionWhen(
+ currentElement.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but
specifies a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ currentElement.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but
specifies a non-zero dfdl:trailingSkip."
+ )
+ schemaDefinitionWhen(
+ currentElement.maxOccurs > 1,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but
specifies a maxOccurs greater than 1."
+ )
+ schemaDefinitionWhen(
+ currentElement.impliedRepresentation == Representation.Text
+ && (!currentElement.isKnownEncoding ||
!currentElement.knownEncodingIsFixedWidth ||
currentElement.knownEncodingWidthInBits != 8)
+ && (!optParentELU.contains(LengthUnits.Characters)),
Review Comment:
Using contains will return false if optParentELU is None, which will make
this a non-SDE.
Though, in theory we should never have a None here since if this is an EOP
element it must be in something that has an ELU. So maybe assert in this
function that opParentELU is defined. Or maybe better woudl be to change
optParentELU in this function not be an Option, so you can't even pass in an
invalid ELU. And it requires that callers are careful to make sure there is an
ELU.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -252,6 +253,49 @@ trait ElementBaseGrammarMixin
}
final lazy val prefixedLengthBody = prefixedLengthElementDecl.parsedValue
+ def checkEndOfParentRestrictionsOnCurrentElement(optParentELU:
Option[LengthUnits]): Unit = {
+ val currentElement: ElementBase = this
+ if (currentElement.lengthKind != LengthKind.EndOfParent) {} else {
+ schemaDefinitionWhen(
+ currentElement.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but
specifies a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ currentElement.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but
specifies a non-zero dfdl:trailingSkip."
+ )
+ schemaDefinitionWhen(
+ currentElement.maxOccurs > 1,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but
specifies a maxOccurs greater than 1."
+ )
+ schemaDefinitionWhen(
+ currentElement.impliedRepresentation == Representation.Text
+ && (!currentElement.isKnownEncoding ||
!currentElement.knownEncodingIsFixedWidth ||
currentElement.knownEncodingWidthInBits != 8)
+ && (!optParentELU.contains(LengthUnits.Characters)),
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but the
element has text representation, and does not have a single-byte character set
encoding, and the effective length units of the parent is not 'characters'."
+ )
+ if (currentElement.isSimpleType) {
+ schemaDefinitionUnless(
+ (currentElement.primType eq PrimType.String)
+ || (currentElement.representation == Representation.Text)
+ || (currentElement.primType eq PrimType.HexBinary)
+ || (currentElement.representation == Representation.Binary
+ && Seq(BinaryNumberRep.Packed, BinaryNumberRep.Bcd,
BinaryNumberRep.Ibm4690Packed)
+ .contains(currentElement.binaryNumberRep))
+ || (currentElement.representation == Representation.Binary
+ && currentElement.optionBinaryCalendarRep.isDefined
+ && Seq(
+ BinaryCalendarRep.Packed,
+ BinaryCalendarRep.Bcd,
+ BinaryCalendarRep.Ibm4690Packed
+ )
+ .contains(currentElement.binaryCalendarRep)),
+ "element is a simple type specified as
dfdl:lengthKind=\"endOfParent\", but isn't a string type, doesn't have text
representation, isn't a hexbinary type, or doesn't have binary representation
with packed decimal representation."
+ )
Review Comment:
I'm wondering if this last check as already handled by below changes? FOr
example, just below this you have a SDE for
```
SDE("lengthKind='endOfParent' only supported for packed binary
formats.")
```
Seems like it would only be possible to hit one of these SDEs. I don't think
we necessarily need to have all checks in this
`checkEndOfParentRestrictionsOnCurrentElement` if they are handled in other
places with potentially more specific error messages.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1072,6 +1163,7 @@ trait ElementBaseGrammarMixin
case PrimType.Boolean => {
lengthKind match {
case LengthKind.Prefixed => new BinaryBooleanPrefixedLength(this)
+ case LengthKind.EndOfParent => new
BinaryBooleanEndOfParentLength(this)
Review Comment:
It looks like the spec only allows EOP with packed binary elements. Is
boolean allowed?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
Review Comment:
Should we drop `my`? We don't really use that convention very often, and I
think in the context it's clear that calling `effectiveLengthUnits()` returns
the ELU of the current element.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
Review Comment:
I think this case is almost exactly the same as the Root case except the
root case does a `rootElem.checkEndOfParentResriciontiosnOfCurrrentElement`.
Can we merge the two and do something like:
```scala
if (parent.isInstanceOf[Root]) {
parent.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
}
```
Then everything else can remain the same?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
Review Comment:
Reading over this, I have to keep reminding myself that parent is not
reaching up to this elements parent, it's just `this`. I know using `parent`
better matches the wording in the spec, but would it make sense to name this as
`elem` or `e` or something, and then this case would be `case child:
ELemenetBase`? I think that still makes the parent children relationship clear,
but doesn't confuse with `parent`, which I tihnk in some instances we do use to
actually reach up to the parent element.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
+ parent.lengthKind match {
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ schemaDefinitionError(
+ "element is specified as dfdl:lengthKind=\"endOfParent\",
but its parent is an element with dfdl:lengthKind 'implicit' or 'delimited'."
+ )
+ case _ => // do nothing
+ }
+ parent.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
Review Comment:
Is this case possible? I think `childrenEndOfParent` is always Elements? Do
we want to change `childrenEndOfParent` to be a `Seq[ElementBase]`, then we can
avoid these match cases?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/TermGrammarMixin.scala:
##########
@@ -84,4 +96,181 @@ trait TermGrammarMixin extends AlignedMixin with
BitOrderMixin with TermRuntime1
MandatoryTextAlignment(this, knownEncodingAlignmentInBits, true)
}
+ def myEffectiveLengthUnits(optLastNonEOPLU: Option[LengthUnits]):
Option[LengthUnits] = {
+ val elu = this match {
+ case e: ElementBase =>
+ e.lengthKind match {
+ case LengthKind.EndOfParent => optLastNonEOPLU
+ case LengthKind.Explicit | LengthKind.Prefixed => Some(e.lengthUnits)
+ case LengthKind.Pattern => Some(LengthUnits.Characters)
+ case _ => None
+ }
+ case c: ChoiceTermBase if c.choiceLengthKind ==
ChoiceLengthKind.Explicit =>
+ Some(LengthUnits.Bytes)
+ // the spec doesn't actually account for this case, but logically it
makes
+ // sense that ELU of a sequence is the ELU of its parent that
technically is the
+ // last non-EndOfParent ELU.
+ case s: SequenceTermBase => optLastNonEOPLU
+ case _ => None
+ }
+ elu
+ }
+
+ final lazy val childrenEndOfParent: Seq[Term] =
LV(Symbol("childrenEndOfParent")) {
+ val gms = termChildren
+ val chls = gms.flatMap {
+ case eb: ElementBase if eb.lengthKind == LengthKind.EndOfParent =>
Seq(eb)
+ case eb: ElementBase => Nil
+ case c: ChoiceTermBase => Nil
+ case mg: ModelGroup => mg.childrenEndOfParent
+ }
+ chls
+ }.value
+
+ def checkEndOfParentRestrictions(optLastNonEOPLU: Option[LengthUnits]): Unit
= {
+ val term = this
+ lazy val eopChildren = this.childrenEndOfParent
+ // checks
+ term match {
+ case rootElem: Root if rootElem.lengthKind == LengthKind.EndOfParent => {
+
rootElem.checkEndOfParentRestrictionsOnCurrentElement(Some(LengthUnits.Characters))
+ eopChildren.foreach {
+ case e: ElementBase => {
+ rootElem.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optLastNonEOPLU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case parent: ElementBase if eopChildren.nonEmpty => {
+ eopChildren.foreach {
+ case e: ElementBase => {
+ val optParentELU = parent.myEffectiveLengthUnits(optLastNonEOPLU)
+ parent.lengthKind match {
+ case LengthKind.Implicit | LengthKind.Delimited =>
+ schemaDefinitionError(
+ "element is specified as dfdl:lengthKind=\"endOfParent\",
but its parent is an element with dfdl:lengthKind 'implicit' or 'delimited'."
+ )
+ case _ => // do nothing
+ }
+ parent.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case s: SequenceTermBase if eopChildren.nonEmpty => {
+ schemaDefinitionWhen(
+ s.separatorPosition == SeparatorPosition.Postfix,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:separatorPosition defined as 'postfix'."
+ )
+ schemaDefinitionWhen(
+ s.sequenceKind != SequenceKind.Ordered,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with dfdl:sequenceKind defined as 'unordered'."
+ )
+ schemaDefinitionWhen(
+ s.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ s.realElementChildren.exists(e => e.floating == YesNo.Yes),
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with elements defining dfdl:floating='yes'."
+ )
+ schemaDefinitionWhen(
+ s.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a sequence with a non-zero dfdl:trailingSkip."
+ )
+ eopChildren.foreach {
+ case e: ElementBase => {
+ s.checkChildrenForSiblingsAfterEOPElement(e)
+ e.checkEndOfParentRestrictionsOnCurrentElement(optLastNonEOPLU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case c: ChoiceTermBase if eopChildren.nonEmpty => {
+ // TODO: The DFDL spec (12.3.6) explicitly mentions that an
EndOfParent element
+ // can be terminated by a choice with choiceLengthKind='explicit',
with no reference
+ // to implicit length choices. Later on in 12.3.6, it specified what
the parent Effective
+ // Length Units would be for an explicit length choice, again with no
reference to
+ // implicit length choices. The only way to get an EndOfParent element
within an
+ // implicit length choice would be to have it actually be terminated
by the choice's
+ // parent, similar to how we treat elements within an element that is
also EndOfParent.
+ // The only mention of implicit length choices in the DFDL Spec is to
mention SDEs when
+ // an EndOfParent element is enclosed by a choice with
choiceLengthKind='implicit'. We
+ // think that may be a typo, so for now we disallow
ChoiceLengthKind.Implicit
+ // enclosing an EndOfParent element.
+ // See Daffodil-3080
+ schemaDefinitionWhen(
+ c.choiceLengthKind == ChoiceLengthKind.Implicit,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but its
parent is a choice with dfdl:choiceLengthKind 'implicit'."
+ )
+ schemaDefinitionWhen(
+ c.hasTerminator,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a choice with a dfdl:terminator."
+ )
+ schemaDefinitionWhen(
+ c.trailingSkip != 0,
+ "element is specified as dfdl:lengthKind=\"endOfParent\", but is in
a choice with a non-zero dfdl:trailingSkip."
+ )
+ val optParentELU = c.myEffectiveLengthUnits(optLastNonEOPLU)
+ eopChildren.foreach {
+ case e: ElementBase => {
+ c.checkChildrenForSiblingsAfterEOPElement(e)
+ Assert.invariant(
+ optParentELU.isDefined,
+ "Effective Length Units of parent should not be None"
+ )
+ e.checkEndOfParentRestrictionsOnCurrentElement(optParentELU)
+ }
+ case _ => // do nothing
+ }
+ }
+ case _ => // do nothing
+ }
+ // end checks
+ term.termChildren.foreach { e =>
+ lazy val optParentELU = term.myEffectiveLengthUnits(optLastNonEOPLU)
+ term match {
+ case parent @ (_: ElementBase | _: ChoiceTermBase | _:
SequenceTermBase) => {
+ e.checkEndOfParentRestrictions(optParentELU)
+ }
+ case _ => // do nothing
Review Comment:
What is this default case match and so not recursing into? It feels like we
should recurse into everything so this just wants to be
`term.checkEndOfParentRestrictions(optParentELU)`
##########
daffodil-core/src/main/scala/org/apache/daffodil/io/InputSource.scala:
##########
@@ -564,6 +570,22 @@ class BucketingInputSource(
headBucketBytePosition0b += bytesRemoved
oldestBucketIndex = 0
}
+
+ override lazy val bytesTillEndOfDataStream: Long = {
+ val initialBytePosition = position()
Review Comment:
I don't think `fillBucketsToIndex` changes the position so I don't think you
need to capture this and restore the position.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SpecifiedLengthParsers.scala:
##########
@@ -132,6 +132,17 @@ class SpecifiedLengthPatternParser(
}
}
+class SpecifiedLengthEndOfParentParser(
+ eParser: Parser,
+ erd: ElementRuntimeData
+) extends SpecifiedLengthParserBase(eParser, erd),
+ BitLengthFromBitLimitMixin(true) {
+
+ override protected def getBitLength(s: PState): MaybeULong = {
+ MaybeULong(super[BitLengthFromBitLimitMixin].getBitLength(s))
+ }
Review Comment:
I'm wondering if we need something different for
SpecifiedLengthEndOfParentParser, at least the context of complex types. If we
look at SpecifiedLengthParserBase, then logic it follows for complex types is
essentially
1. getBitLength
2. ignored checking isDefinedForLength
3. withBitLengthLimit { children.parse() }
4. skip any remaining bits not consumed by children
Note that the reason for step 2 to ignore isDefinedForLength is because
isDefinedForLength will cause the underlying input source too fill a bunch of
buckets for the full length of the complex types, but there are limits to how
much we can bucket.
Instead, we just call all the children, have them slowly read all the bytes
as needed, and then skip whatever is left over at the end. So we don't require
the full length of the complex type until we finish parsing the children.
But say we have an EOP complex root element. The first thing this does is
call getBiLength which will to scan the entire stream for the EOD, which does
exactly what we were trying to avoid by skipping isDefinedForLength in step 2.
So it feels like we need special logic for complex EOP elements so that they
do something different if there is no current bit limit. For example, the logic
wants to be more like this for complex EOP elements:
```scala
if (bitLimit.isDefined) {
val len = getBitLength // will use bitLimit
withBitLengthLength { children.parse }
val reminaingBits = ...
dis.skip(remaiingBits)
} else {
children.parse()
val len = getBitLength // will scan for EOD
val reminaingBits = ...
dis.skip(remainingBits)
}
```
So we kindof need to reverse the order of getBitLengthand children.parse.
I'm not sure if it makes sense to change SpecifiedLengthParserBase to know
about EOP and check for bitLimit or SpecifiedLengthEndOfParentParser wants to
be it's own thing, maybe with helper functions that the Base parser has for the
common things like skipping remaining bits.
Note this is only an issue for complex types, simple types need the length
no matter what.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
