Ensure isNilled state is always correct

In some cases, it is possible for the InfosetWalker to walk into a nillable complex element that is not "finished", but has children and so is known to be not nilled. Even though we know it isn't nilled, we don't ever set the isNilled state--we only do that for nilled elements. This means that if an InfosetOutputter calls isNilled, then it would fail since that requires the state to be set. To resolve this, this completely removes the concept of isNilledSet which avoids the problem. isNilled now defaults to false and is set to true only when an element is determined to be nil by the NilOrParserValue combinator. Although there is a brief period of time where an infoset element's isNilled state is not known (i.e. after an element is initialized but before the NilOrParserValue combinator determines and sets isNilled), there is no way for anything to access this state during this time. This also means isNilled is always correct by the time the InfosetWalker walks into a nillable element, so isNilled can be safely used and is always accurate. This also removes the concept of maybeIsNilled, which was just a workaround to handle the fact that non-nillable elements never set nilled state. Now everything can use reference isNilled since it is always accurate by the time a parser might access it. DAFFODIL-2962
apache · Dec 19, 2024 · 9293589 · 9293589
1 parent 48131bf
commit 9293589
Show file tree

Hide file tree

Showing 14 changed files with 317 additions and 90 deletions.
diff --git a/...ime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/ElementUnparser.scala b/...ime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/ElementUnparser.scala
@@ -20,7 +20,6 @@ package org.apache.daffodil.unparsers.runtime1
 import org.apache.daffodil.lib.exceptions.Assert
 import org.apache.daffodil.lib.util.Maybe
 import org.apache.daffodil.lib.util.Maybe._
-import org.apache.daffodil.lib.util.MaybeBoolean
 import org.apache.daffodil.lib.util.MaybeULong
 import org.apache.daffodil.runtime1.dpath.SuspendableExpression
 import org.apache.daffodil.runtime1.dsom.CompiledExpression
@@ -447,7 +446,7 @@ sealed trait RegularElementUnparserStartEndStrategy extends ElementUnparserStart
       if (parentNodeMaybe.isDefined) {
         val parentComplex = parentNodeMaybe.get.asComplex
         Assert.invariant(!parentComplex.isFinal)
-        if (parentComplex.maybeIsNilled == MaybeBoolean.True) {
+        if (parentComplex.isNilled) {
           // cannot add content to a nilled complex element
           UnparseError(
             One(erd.schemaFileLocation),

diff --git a/...r/src/main/scala/org/apache/daffodil/unparsers/runtime1/NilEmptyCombinatorUnparsers.scala b/...r/src/main/scala/org/apache/daffodil/unparsers/runtime1/NilEmptyCombinatorUnparsers.scala
@@ -61,8 +61,7 @@ case class ComplexNilOrContentUnparser(
   def unparse(state: UState): Unit = {
     Assert.invariant(Maybe.WithNulls.isDefined(state.currentInfosetNode))
     val inode = state.currentInfosetNode.asComplex
-    val maybeIsNilled = inode.maybeIsNilled
-    if (maybeIsNilled.isDefined && maybeIsNilled.get)
+    if (inode.isNilled)
       nilUnparser.unparse1(state)
     else
       contentUnparser.unparse1(state)

diff --git a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetImpl.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetImpl.scala
@@ -807,6 +807,14 @@ sealed trait DIElementSharedMembersMixin {
   protected final var _contentLength: ContentLengthState = null
   protected final var _valueLength: ValueLengthState = null
 
+  /**
+   * Default isNilled to false. Note that this is safe since this field cannot be accessed in
+   * the time between an element being created and the determination that an element is nil by a
+   * NilParser or an InfosetInputter. The only things that are evaluated during this time are
+   * things like discriminators/setVar, but the kinds that can evaluate expression that might
+   * access isNilledState are evaluated at the end of the element, well after it is determined
+   * to be nilled or not
+   */
   protected final var _isNilled: Boolean = false
   protected final var _validity: MaybeBoolean = MaybeBoolean.Nope
 
@@ -1098,8 +1106,6 @@ sealed trait DIElement
   final def runtimeData = erd
   protected final var _parent: DIComplex = null
 
-  protected final var _isNilledSet: Boolean = false
-
   def parent = _parent
 
   def diParent = _parent.asInstanceOf[DIComplex]
@@ -1116,30 +1122,16 @@ sealed trait DIElement
   }
 
   /**
-   * Used for just testing whether a node has the nil
-   * indicators set. That is, dodges the expression evaluation
-   * complexity where specific exceptions are thrown when you
-   * ask about data that isn't known yet.
+   * Tells if the element is nilled or not. For non-nillable elements this always returns false.
    */
-  final def maybeIsNilled: MaybeBoolean = {
-    if (!_isNilledSet) MaybeBoolean.Nope
-    else MaybeBoolean(_isNilled)
+  def isNilled: Boolean = {
+    if (!erd.isNillable) false
+    else _isNilled
   }
 
-  /**
-   * Tells if the element is nilled or not.
-   *
-   * Throws InfosetNoDataException if we don't yet
-   * know if it is nil or not (i.e., hasn't be set, nor has
-   * anything been set to indicate that it won't be nilled.)
-   */
-  def isNilled: Boolean
-
   def setNilled(): Unit = {
     Assert.invariant(erd.isNillable)
-    Assert.invariant(!_isNilled)
     _isNilled = true
-    _isNilledSet = true
   }
 
   /**
@@ -1423,30 +1415,12 @@ sealed class DISimple(override val erd: ElementRuntimeData)
         }
       }
     }
-    _isNilled = false
-    _isNilledSet = true
     _isDefaulted = false
     _validity = MaybeBoolean.Nope // we have not tested this new value.
     _unionMemberRuntimeData = Nope
   }
 
-  /**
-   * @return true if the element is nilled, false otherwise.
-   * @throws InfosetNoDataException if neither data value nor setNull has happened yet
-   *                                so the nil status is undetermined.
-   */
-  override def isNilled: Boolean = {
-    if (!erd.isNillable) false
-    else if (_isNilledSet) {
-      _isNilled
-    } else {
-      erd.toss(InfosetNoDataException(this, erd))
-    }
-  }
-
   def resetValue(): Unit = {
-    _isNilled = false
-    _isNilledSet = false
     _isDefaulted = false
     _validity = MaybeBoolean.Nope // we have not tested this new value.
     _unionMemberRuntimeData = Nope
@@ -1479,7 +1453,6 @@ sealed class DISimple(override val erd: ElementRuntimeData)
       if (erd.optDefaultValue.isDefined) {
         val defaultVal = erd.optDefaultValue
         if (defaultVal == DataValue.UseNilForDefault) {
-          Assert.invariant(erd.isNillable)
           this.setNilled()
         } else {
           _value = defaultVal.getNullablePrimitive
@@ -1685,18 +1658,6 @@ sealed class DIComplex(override val erd: ElementRuntimeData)
 
   final def isEmpty: Boolean = false
 
-  final override def isNilled: Boolean = {
-    if (!erd.isNillable) false
-    else if (_isNilledSet) {
-      _isNilled
-    } else if (this.isFinal) {
-      // TODO: should we check that there are no children?
-      false
-    } else {
-      erd.toss(new InfosetNoDataException(this, erd))
-    }
-  }
-
   override def requireFinal(): Unit = {
     if (!isFinal) throw nfe
   }

diff --git a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetInputter.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetInputter.scala
@@ -340,8 +340,7 @@ abstract class InfosetInputter
       // An open complex element is on top of stack.
       // This start event must be for a child element
       val c = top.asComplex
-      val optNilled = c.maybeIsNilled
-      if (optNilled.isDefined && optNilled.get) {
+      if (c.isNilled) {
         // cannot add content to a nilled complex element
         UnparseError(
           One(c.erd.schemaFileLocation),

diff --git a/...ntime1/src/main/scala/org/apache/daffodil/runtime1/infoset/ScalaXMLInfosetOutputter.scala b/...ntime1/src/main/scala/org/apache/daffodil/runtime1/infoset/ScalaXMLInfosetOutputter.scala
@@ -29,7 +29,6 @@ import org.apache.daffodil.lib.xml.XMLUtils
 import org.apache.daffodil.runtime1.api.DFDLPrimType
 import org.apache.daffodil.runtime1.api.InfosetArray
 import org.apache.daffodil.runtime1.api.InfosetComplexElement
-import org.apache.daffodil.runtime1.api.InfosetElement
 import org.apache.daffodil.runtime1.api.InfosetSimpleElement
 
 class ScalaXMLInfosetOutputter(showFreedInfo: Boolean = false) extends InfosetOutputter {
@@ -87,7 +86,7 @@ class ScalaXMLInfosetOutputter(showFreedInfo: Boolean = false) extends InfosetOu
     val attributes = getAttributes(diSimple)
 
     val children =
-      if (!isNilled(diSimple) && diSimple.hasValue) {
+      if (!diSimple.isNilled && diSimple.hasValue) {
         val text =
           if (diSimple.metadata.dfdlType == DFDLPrimType.String) {
             XMLUtils.remapXMLIllegalCharactersToPUA(diSimple.dataValueAsString)
@@ -149,19 +148,4 @@ class ScalaXMLInfosetOutputter(showFreedInfo: Boolean = false) extends InfosetOu
     )
     resultNode.get
   }
-
-  /**
-   * Helper function to determine if an element is nilled or not, taking into
-   * account whether or not the nilled state has been set yet.
-   *
-   * @param elem the element to check the nilled state of
-   * @return true if the nilled state has been set and is true. false if the
-   *         nilled state is false or if the nilled state has not been set yet
-   *         (e.g. during debugging)
-   */
-  private def isNilled(elem: InfosetElement): Boolean = {
-    val diElement = elem.asInstanceOf[DIElement]
-    val maybeIsNilled = diElement.maybeIsNilled
-    maybeIsNilled.isDefined && maybeIsNilled.get == true
-  }
 }
diff --git a/...me1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimitedParsers.scala b/...me1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimitedParsers.scala
@@ -150,8 +150,7 @@ class LiteralNilDelimitedEndOfDataParser(
         (isFieldEmpty && isEmptyAllowed) || // Empty, but must advance past padChars if there were any.
         isNilLiteral
       ) { // Not empty, but matches.
-        // Contains a nilValue, Success!
-        state.thisElement.setNilled()
+        // Contains a nilValue, Succes ParseResult indiciates nilled
         captureValueLengthOfString(state, field)
         if (result.matchedDelimiterValue.isDefined) state.saveDelimitedParseResult(parseResult)
         return

diff --git a/...ain/scala/org/apache/daffodil/runtime1/processors/parsers/NilEmptyCombinatorParsers.scala b/...ain/scala/org/apache/daffodil/runtime1/processors/parsers/NilEmptyCombinatorParsers.scala
@@ -34,12 +34,16 @@ abstract class NilOrValueParser(ctxt: TermRuntimeData, nilParser: Parser, valueP
     pstate.withPointOfUncertainty("NilOrValueParser", ctxt) { pou =>
       nilParser.parse1(pstate)
 
-      if (pstate.processorStatus ne Success) {
-        // did not find a nil. reset back to the pou and try to parse the value
+      if (pstate.processorStatus eq Success) {
+        // Success indicates we found a nil representation. Set the infoset to nil.
+        // withPointOfUncertainty will discard the pou
+        pstate.infoset.setNilled()
+      } else {
+        // Faiulre indiciated we did not find a nil representation. Reset back to the pou and
+        // try to parse the value. We do not need to change the nilled state in the infoset
+        // since it defaults to not nilled
         pstate.resetToPointOfUncertainty(pou)
         valueParser.parse1(pstate)
-      } else {
-        // no-op. We found nil, withPointOfUncertainty will discard the pou
       }
     }
 

diff --git a/...-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilParsers.scala b/...-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilParsers.scala
@@ -46,14 +46,12 @@ abstract class LiteralNilOfSpecifiedLengthParserBase(erd: ElementRuntimeData)
     val isFieldEmpty = field.length() == 0
 
     if (isFieldEmpty && isEmptyAllowed) {
-      // Valid!
-      start.thisElement.setNilled()
+      // Valid! Success ParseResult indicates nilled
     } else if (isFieldEmpty && !isEmptyAllowed) {
       // Fail!
       PE(start, "%s - Empty field found but not allowed!", eName)
     } else if (isFieldNilLit(field)) {
-      // Contains a nilValue, Success!
-      start.thisElement.setNilled()
+      // Contains a nilValue, Success ParseResult indicates nilled
     } else {
       // Fail!
       PE(start, "%s - Does not contain a nil literal!", eName)

diff --git a/...apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala b/...apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala
@@ -345,9 +345,7 @@ class NonPositionalGroupSeparatedSequenceChildParseResultHelper(
       val maybeElem = pstate.infosetLastChild
       Assert.invariant(maybeElem.isDefined)
       val elem = maybeElem.get
-      val maybeIsNilled =
-        elem.maybeIsNilled // can't just call isNilled because that throws exceptions on not defined
-      if (maybeIsNilled.isDefined && maybeIsNilled.get) {
+      if (elem.isNilled) {
         ParseAttemptStatus.NilRep
       } else {
         ParseAttemptStatus.NormalRep

diff --git a/...cala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala b/...cala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala
@@ -163,9 +163,7 @@ trait ElementSequenceChildParseResultHelper extends SequenceChildParseResultHelp
       val maybeElem = pstate.infosetLastChild
       Assert.invariant(maybeElem.isDefined)
       val elem = maybeElem.get
-      val maybeIsNilled =
-        elem.maybeIsNilled // can't just call isNilled because that throws exceptions on not defined
-      if (maybeIsNilled.isDefined && maybeIsNilled.get) {
+      if (elem.isNilled) {
         ParseAttemptStatus.NilRep
       } else {
         // not nilled

diff --git a/daffodil-test/src/test/resources/org/apache/daffodil/section13/nillable/nillable.tdml b/daffodil-test/src/test/resources/org/apache/daffodil/section13/nillable/nillable.tdml
@@ -20,6 +20,7 @@
   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
   xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ct="http://w3.ibm.com/xmlns/dfdl/ctInfoset"
   xmlns:ex="http://example.com"
+  xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
   defaultRoundTrip="true">
 
   <tdml:defineSchema name="s1">
@@ -355,4 +356,74 @@
       </tdml:dfdlInfoset>
     </tdml:infoset>
   </tdml:parserTestCase>
+
+  <tdml:defineSchema name="s3">
+    <xs:include schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd" />
+
+    <dfdl:format ref="ex:GeneralFormat" 
+      nilValueDelimiterPolicy="both"
+      lengthKind="implicit" />
+
+    <xs:element name="r1">
+      <xs:complexType>
+        <xs:sequence>
+          <xs:element name="fields" nillable="true" dfdl:nilKind="literalValue" dfdl:nilValue="%ES;" dfdl:initiator="fields:">
+            <xs:complexType>
+              <xs:sequence>
+                <xs:element name="a" type="xs:string" dfdl:terminator="," dfdl:lengthKind="explicit" dfdl:length="1" /> 
+                <xs:element name="b" type="xs:string" dfdl:terminator="," dfdl:lengthKind="explicit" dfdl:length="1" /> 
+                <xs:element name="c" type="xs:string" dfdl:terminator="," dfdl:lengthKind="explicit" dfdl:length="1" /> 
+              </xs:sequence>
+            </xs:complexType>
+          </xs:element>
+        </xs:sequence>
+      </xs:complexType>
+    </xs:element>
+
+  </tdml:defineSchema>
+
+  <tdml:defineConfig name="infosetWalkerNoSkip">
+    <daf:tunables>
+      <daf:infosetWalkerSkipMin>0</daf:infosetWalkerSkipMin>
+    </daf:tunables>
+  </tdml:defineConfig>
+
+  <!--
+    This test ideally should be run with the environment variable DAFFODIL_TDML_API_INFOSETS=all.
+    This is because the normal ScalaXMLInfosetOutputter used for TDML tests lazily accesses some
+    infoset properites like isNillable. Other infoset outputters, like XMLTextInfosetOutputter are
+    more greedy and, prior to a fix, could access the isNillable property before the state was set.
+
+    Note that we disable infoset walker skipping to force the access of isNillable state
+  -->
+  <tdml:parserTestCase name="complexNillable_01" model="s3" config="infosetWalkerNoSkip">
+    <tdml:document>
+      <tdml:documentPart type="text">fields:1,2,3,</tdml:documentPart>
+    </tdml:document>
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <ex:r1>
+          <fields>
+            <a>1</a>
+            <b>2</b>
+            <c>3</c>
+          </fields>
+        </ex:r1>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+  </tdml:parserTestCase>
+
+  <tdml:parserTestCase name="complexNillable_02" model="s3" config="infosetWalkerNoSkip">
+    <tdml:document>
+      <tdml:documentPart type="text">fields:</tdml:documentPart>
+    </tdml:document>
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <ex:r1>
+          <fields xsi:nil="true" />
+        </ex:r1>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+  </tdml:parserTestCase>
+
 </tdml:testSuite>