From 9293589405e180efe624694ca1dbc2a38401d6d2 Mon Sep 17 00:00:00 2001 From: Steve Lawrence Date: Mon, 16 Dec 2024 08:37:22 -0500 Subject: [PATCH] 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 --- .../unparsers/runtime1/ElementUnparser.scala | 3 +- .../NilEmptyCombinatorUnparsers.scala | 3 +- .../runtime1/infoset/InfosetImpl.scala | 63 +----- .../runtime1/infoset/InfosetInputter.scala | 3 +- .../infoset/ScalaXMLInfosetOutputter.scala | 18 +- .../processors/parsers/DelimitedParsers.scala | 3 +- .../parsers/NilEmptyCombinatorParsers.scala | 12 +- .../processors/parsers/NilParsers.scala | 6 +- ...aratedSequenceChildParseResultHelper.scala | 4 +- .../SequenceChildParseResultHelper.scala | 4 +- .../daffodil/section13/nillable/nillable.tdml | 71 ++++++ .../section23/dfdl_functions/Functions.tdml | 208 ++++++++++++++++++ .../section13/nillable/TestNillable.scala | 2 + .../TestDFDLExpressions.scala | 7 + 14 files changed, 317 insertions(+), 90 deletions(-) diff --git a/daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/ElementUnparser.scala b/daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/ElementUnparser.scala index 9bf9c12a9c..1117e5c9da 100644 --- a/daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/ElementUnparser.scala +++ b/daffodil-runtime1-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/daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/NilEmptyCombinatorUnparsers.scala b/daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/NilEmptyCombinatorUnparsers.scala index 3ba8342c9e..b12986f6f1 100644 --- a/daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/unparsers/runtime1/NilEmptyCombinatorUnparsers.scala +++ b/daffodil-runtime1-unparser/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 index 0ff990525d..1aed39f826 100644 --- 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 index 825697658f..30c0492b59 100644 --- 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/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/ScalaXMLInfosetOutputter.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/ScalaXMLInfosetOutputter.scala index 628d323264..98b17b8aea 100644 --- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/ScalaXMLInfosetOutputter.scala +++ b/daffodil-runtime1/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/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimitedParsers.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimitedParsers.scala index f777b5da5a..edd34e8255 100644 --- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/DelimitedParsers.scala +++ b/daffodil-runtime1/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/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilEmptyCombinatorParsers.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilEmptyCombinatorParsers.scala index 7da3db844b..4c41b324c9 100644 --- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilEmptyCombinatorParsers.scala +++ b/daffodil-runtime1/src/main/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/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilParsers.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilParsers.scala index 9e9e2b93f9..e12d1055fa 100644 --- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/NilParsers.scala +++ b/daffodil-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/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala index c465f096fa..f245cda4b7 100644 --- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala +++ b/daffodil-runtime1/src/main/scala/org/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/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala index 3e464c666b..f000a4a7f0 100644 --- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SequenceChildParseResultHelper.scala +++ b/daffodil-runtime1/src/main/scala/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 index da41170d19..a0603e777f 100644 --- 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"> @@ -355,4 +356,74 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + 0 + + + + + + + fields:1,2,3, + + + + + + 1 + 2 + 3 + + + + + + + + + fields: + + + + + + + + + + diff --git a/daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml b/daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml index 09f9131018..a5f414b3b3 100644 --- a/daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml +++ b/daffodil-test/src/test/resources/org/apache/daffodil/section23/dfdl_functions/Functions.tdml @@ -9789,6 +9789,53 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + true + + + + + + + + + + + fields: + + + + + + false + + + + + + + + + + + fields:1 + + + + + + 1 + + false + + + + + + + + + + + + + + + + + true + + + + + + + + + + + fields: + + + + + + false + + + + + + + + + + + fields: + + + + + + + + + + + + + + + + + + + + + + + + +