You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by ja...@apache.org on 2020/05/04 16:19:16 UTC
[incubator-daffodil] branch master updated: Fixed issues with
resetting variables
This is an automated email from the ASF dual-hosted git repository.
jadams pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
The following commit(s) were added to refs/heads/master by this push:
new 839d44a Fixed issues with resetting variables
839d44a is described below
commit 839d44ae371071ed3738c5d93b4ac605b6295cd0
Author: Josh Adams <ja...@tresys.com>
AuthorDate: Fri May 1 10:13:59 2020 -0400
Fixed issues with resetting variables
When parsing choices (or Unordered Sequences) there are cases where a
varaible can be read or set while speculatively parsing a choice branch.
If that branch ultimately fails to parse, we need to rewind the changes
we made, including resetting changes made to variables.
There were 4 separate fixes that were needed to get this resetting to
behave correctly:
1. A change in logic for the VariableInstance.reset() function to
account for variables with default values
2. Change the readVariable function to signal the PState to track only
the initial read of a variable as a change that may need to be reset.
Subsequence reads do not alter the state of the variable.
3. Clear the PState.changedVariablesStack.top after resetting all
variables as it is possible to have multiple calls to PState.reset()
within the same point of uncertainty, which means the
changedVariablesStack may not be pushed/popped with each reset.
4. Change the base sequence parser to ensure that all calls to
PState.reset() occur between the push and popPointOfUncertainty
calls.
These issues were discovered in some ofthe schema projects, but test
cases have been added to the main test suite to verify the fixes.
DAFFODIL-2334
---
.../apache/daffodil/dsom/DFDLDefineVariable.scala | 2 +-
.../externalvars/TestExternalVariablesLoader.scala | 4 +-
.../org/apache/daffodil/dpath/DPathRuntime.scala | 5 +-
.../apache/daffodil/processors/RuntimeData.scala | 2 +-
.../apache/daffodil/processors/VariableMap1.scala | 34 +++++++---
.../daffodil/processors/parsers/PState.scala | 10 ++-
.../processors/parsers/SequenceParserBases.scala | 3 +-
.../daffodil/section07/variables/variables.tdml | 75 ++++++++++++++++++++++
.../section07/variables/TestVariables.scala | 3 +
9 files changed, 118 insertions(+), 20 deletions(-)
diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLDefineVariable.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLDefineVariable.scala
index 8931995..7d713b4 100644
--- a/daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLDefineVariable.scala
+++ b/daffodil-core/src/main/scala/org/apache/daffodil/dsom/DFDLDefineVariable.scala
@@ -64,7 +64,7 @@ class DFDLDefineVariable(node: Node, doc: SchemaDocument)
final lazy val primType = PrimType.fromNameString(typeQName.local).getOrElse(
this.SDE("Variables must have primitive type. Type was '%s'.", typeQName.toPrettyString))
- final def newVariableInstance = variableRuntimeData.newVariableInstance
+ final def createVariableInstance = variableRuntimeData.createVariableInstance
final override lazy val runtimeData = variableRuntimeData
diff --git a/daffodil-core/src/test/scala/org/apache/daffodil/externalvars/TestExternalVariablesLoader.scala b/daffodil-core/src/test/scala/org/apache/daffodil/externalvars/TestExternalVariablesLoader.scala
index 3446a89..2b64618 100644
--- a/daffodil-core/src/test/scala/org/apache/daffodil/externalvars/TestExternalVariablesLoader.scala
+++ b/daffodil-core/src/test/scala/org/apache/daffodil/externalvars/TestExternalVariablesLoader.scala
@@ -110,9 +110,9 @@ class TestExternalVariablesLoader extends Logging {
// Verify that the external variables override the previous values
// in the VariableMap
- val value1 = vmap.readVariable(v_no_default_vrd, Fakes.fakeElem)
+ val value1 = vmap.readVariable(v_no_default_vrd, Fakes.fakeElem, Maybe.Nope)
assertEquals(1, value1.getAnyRef)
- val value2 = vmap.readVariable(v_with_default_vrd, Fakes.fakeElem)
+ val value2 = vmap.readVariable(v_with_default_vrd, Fakes.fakeElem, Maybe.Nope)
assertEquals(2, value2.getAnyRef)
}
diff --git a/daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPathRuntime.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPathRuntime.scala
index 07cc4d0..f25da41 100644
--- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPathRuntime.scala
+++ b/daffodil-runtime1/src/main/scala/org/apache/daffodil/dpath/DPathRuntime.scala
@@ -181,10 +181,7 @@ case class VRef(vrd: VariableRuntimeData, context: ThrowsSDE)
override def run(dstate: DState) {
Assert.invariant(dstate.vmap != null)
- if (dstate.parseOrUnparseState.isDefined)
- if (dstate.parseOrUnparseState.get.isInstanceOf[PState])
- dstate.parseOrUnparseState.get.asInstanceOf[PState].variableRead(vrd)
- dstate.setCurrentValue(dstate.vmap.readVariable(vrd, context))
+ dstate.setCurrentValue(dstate.vmap.readVariable(vrd, context, dstate.parseOrUnparseState))
}
override def toXML = toXML("$" + vrd.globalQName.toPrettyString)
diff --git a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
index 33c0ebc..07db069 100644
--- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
+++ b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/RuntimeData.scala
@@ -994,6 +994,6 @@ final class VariableRuntimeData(
}
}
- def newVariableInstance: VariableInstance = VariableInstance(state, value, this, maybeDefaultValueExpr)
+ def createVariableInstance: VariableInstance = VariableInstance(state, value, this, maybeDefaultValueExpr)
}
diff --git a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
index 441fed6..e997890 100644
--- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
+++ b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
@@ -30,6 +30,7 @@ import org.apache.daffodil.util.Maybe
import org.apache.daffodil.util.Maybe.Nope
import org.apache.daffodil.util.MStackOf
import org.apache.daffodil.xml.{GlobalQName, UnspecifiedNamespace, NamedQName, QNameBase, RefQName}
+import org.apache.daffodil.processors.parsers.PState
import scala.collection.mutable.Map
@@ -86,10 +87,15 @@ case class VariableInstance(
def reset() = {
Assert.invariant(this.state != VariableUndefined)
- this.value = this.priorValue
- this.state = this.priorState
- this.priorValue = DataValue.NoValue
- this.priorState = VariableUndefined
+ (this.state, this.priorState, this.defaultValueExpr.isDefined) match {
+ case (VariableRead, VariableSet, _) => this.state = VariableSet
+ case (VariableRead, _, true) => this.state = VariableDefined
+ case (VariableSet, _, _) => {
+ this.state = this.priorState
+ this.value = this.priorValue
+ }
+ case (_, _, _) => Assert.impossible("Should have SDE before reaching this")
+ }
}
}
@@ -159,7 +165,7 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
def this(topLevelVRDs: Seq[VariableRuntimeData] = Nil) =
this(Map(topLevelVRDs.map {
vrd =>
- val variab = vrd.newVariableInstance
+ val variab = vrd.createVariableInstance
val stack = new MStackOf[VariableInstance]
stack.push(variab)
(vrd.globalQName, stack)
@@ -216,8 +222,7 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
* Returns the value of a variable and sets the state of the variable to be
* VariableRead.
*/
- def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE): DataValuePrimitive = {
- val referringContext: VariableRuntimeData = vrd
+ def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE, maybePstate: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
val stack = vTable.get(varQName)
if (stack.isDefined) {
@@ -225,10 +230,13 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
variable.state match {
case VariableRead if (variable.value.isDefined) => variable.value.getNonNullable
case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ if (maybePstate.isDefined && maybePstate.get.isInstanceOf[PState])
+ maybePstate.get.asInstanceOf[PState].markVariableRead(vrd)
+
variable.setState(VariableRead)
variable.value.getNonNullable
}
- case _ => throw new VariableHasNoValue(varQName, referringContext)
+ case _ => throw new VariableHasNoValue(varQName, vrd)
}
} else
referringContext.SDE("Variable map (compilation): unknown variable %s", varQName) // Fix DFDL-766
@@ -249,8 +257,14 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
}
case VariableRead => {
- referringContext.SDE("Cannot set variable %s after reading the default value. State was: %s. Existing value: %s",
+ /**
+ * TODO: This should be an SDE, but due to a bug (DAFFODIL-1443) in
+ * the way we evaluate escapeSchemes it could lead us to setting the
+ * variable read too early */
+ pstate.SDW("Cannot set variable %s after reading the default value. State was: %s. Existing value: %s",
variable.rd.globalQName, VariableSet, variable.value)
+ variable.setValue(VariableUtils.convert(newValue.getAnyRef.toString, variable.rd))
+ variable.setState(VariableSet)
}
case _ => {
@@ -268,7 +282,7 @@ class VariableMap private(vTable: Map[GlobalQName, MStackOf[VariableInstance]])
val varQName = vrd.globalQName
val stack = vTable.get(varQName)
Assert.invariant(stack.isDefined)
- stack.get.push(vrd.newVariableInstance)
+ stack.get.push(vrd.createVariableInstance)
}
def removeVariableInstance(vrd: VariableRuntimeData) {
diff --git a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/PState.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/PState.scala
index ea85fca..73e7e92 100644
--- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/PState.scala
+++ b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/PState.scala
@@ -47,6 +47,7 @@ import org.apache.daffodil.processors.ParseOrUnparseState
import org.apache.daffodil.processors.ProcessorResult
import org.apache.daffodil.processors.TermRuntimeData
import org.apache.daffodil.processors.VariableMap
+import org.apache.daffodil.processors.VariableRead
import org.apache.daffodil.processors.VariableRuntimeData
import org.apache.daffodil.processors.dfa
import org.apache.daffodil.processors.dfa.DFADelimiter
@@ -220,6 +221,13 @@ final class PState private (
if (variable.isDefined)
variable.get.reset
}}
+ /* When parsing choices or unordered sequences it is necessary to clear the
+ * list at the top of the stack because it is possible for multiple
+ * PState.reset calls to occur withing the same point of uncertainty. If we
+ * do not clear the list, the changes made in failed branches of the choice
+ * will accumulate even though their effects have already been reset.
+ */
+ changedVariablesStack.top.clear
}
def discard(m: PState.Mark) {
@@ -288,7 +296,7 @@ final class PState private (
* Note that this function does not actually read the variable, it is used
* just to track that the variable was read in case we need to backtrack.
*/
- def variableRead(vrd: VariableRuntimeData) {
+ def markVariableRead(vrd: VariableRuntimeData) {
changedVariablesStack.top += vrd.globalQName
}
diff --git a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/SequenceParserBases.scala b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/SequenceParserBases.scala
index 2d692e8..fe8ae71 100644
--- a/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/SequenceParserBases.scala
+++ b/daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/SequenceParserBases.scala
@@ -289,7 +289,6 @@ abstract class SequenceParserBase(
val currentPos = pstate.bitPos0b
val wasDiscriminatorSet = pstate.discriminator
- if (hasPoU) pstate.popPointOfUncertainty
//
// Now we handle the result of the parse attempt.
//
@@ -332,6 +331,8 @@ abstract class SequenceParserBase(
case other => Assert.invariantFailed("Unexpected parse attempt status: " + other)
}
+ if (hasPoU) pstate.popPointOfUncertainty
+
} catch {
// Similar try/catch/finally logic for returning marks is also used in
// the Choice parser. The logic isn't
diff --git a/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml b/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
index 185871a..515ee3f 100644
--- a/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
+++ b/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
@@ -606,6 +606,51 @@
</xs:complexType>
</xs:element>
+ <dfdl:defineVariable name="s_with_default" type="xs:string" defaultValue="default" />
+ <dfdl:defineVariable name="s_no_default" type="xs:string" />
+ <xs:element name="e10" dfdl:lengthKind="implicit">
+ <xs:complexType>
+ <xs:sequence dfdl:separator=",">
+ <xs:choice>
+ <xs:element name="branch1" type="xs:string"
+ dfdl:lengthKind="delimited">
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/">
+ <dfdl:setVariable ref="ex:s_no_default">{ "variableSet_branch1" }</dfdl:setVariable>
+ <dfdl:assert><![CDATA[{ $s_with_default eq "branch1" }]]></dfdl:assert>
+ </xs:appinfo>
+ </xs:annotation>
+ </xs:element>
+ <xs:element name="branch2" type="xs:string"
+ dfdl:lengthKind="delimited">
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/">
+ <dfdl:setVariable ref="ex:s_no_default">{ "s_no_branch2" }</dfdl:setVariable>
+ <dfdl:setVariable ref="ex:s_with_default">{ "s_def_branch2" }</dfdl:setVariable>
+ <dfdl:assert><![CDATA[{ xs:string($ex:s_with_default) eq "s_def_branch2" }]]></dfdl:assert>
+ <dfdl:assert><![CDATA[{ xs:string($ex:s_with_default) eq "s_def_branch2" }]]></dfdl:assert>
+ <dfdl:assert><![CDATA[{ xs:string(.) eq "branch2" }]]></dfdl:assert>
+ </xs:appinfo>
+ </xs:annotation>
+ </xs:element>
+ <xs:element name="branch3" type="xs:string"
+ dfdl:lengthKind="delimited">
+ <xs:annotation>
+ <xs:appinfo source="http://www.ogf.org/dfdl/">
+ <dfdl:setVariable ref="ex:s_no_default">{ "s_no_branch3" }</dfdl:setVariable>
+ <dfdl:assert><![CDATA[{ xs:string(.) eq "branch3" }]]></dfdl:assert>
+ </xs:appinfo>
+ </xs:annotation>
+ </xs:element>
+ </xs:choice>
+ <xsd:element name="s_no_default" type="xs:string"
+ dfdl:inputValueCalc="{ $ex:s_no_default }" />
+ <xsd:element name="s_with_default" type="xs:string"
+ dfdl:inputValueCalc="{ $ex:s_with_default }" />
+ </xs:sequence>
+ </xs:complexType>
+ </xs:element>
+
<dfdl:defineVariable name="badscope" type="xs:int" defaultValue="70"/>
<dfdl:defineVariable name="unsignedLen" type="xs:unsignedInt" defaultValue="2"/>
@@ -991,6 +1036,36 @@
</tdml:infoset>
</tdml:parserTestCase>
+ <tdml:parserTestCase name="resetVar_01" root="e10"
+ model="v"
+ description="Demonstrates that a variables state is properly reset after a read">
+ <tdml:document><![CDATA[branch2]]></tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e10 xmlns="http://example.com">
+ <branch2>branch2</branch2>
+ <s_no_default>s_no_branch2</s_no_default>
+ <s_with_default>s_def_branch2</s_with_default>
+ </e10>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
+ <tdml:parserTestCase name="resetVar_02" root="e10"
+ model="v"
+ description="Demonstrates that a variables state is properly reset after a set and read">
+ <tdml:document><![CDATA[branch3]]></tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <e10 xmlns="http://example.com">
+ <branch3>branch3</branch3>
+ <s_no_default>s_no_branch3</s_no_default>
+ <s_with_default>default</s_with_default>
+ </e10>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
+
<tdml:parserTestCase name="unsignedIntVarCast" root="u1"
model="v"
description="Variable of type unsignedInt used to indicate element length">
diff --git a/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala b/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
index 4dbefa5..b38ed18 100644
--- a/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
+++ b/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
@@ -78,6 +78,9 @@ class TestVariables {
@Test def test_varAsSeparator2() { runner.runOneTest("varAsSeparator2") }
@Test def test_setVarBadScope2() { runner.runOneTest("setVarBadScope2") }
+ @Test def test_resetVar_01() { runner.runOneTest("resetVar_01") }
+ @Test def test_resetVar_02() { runner.runOneTest("resetVar_02") }
+
@Test def test_doubleEmptyDefault() { runner.runOneTest("doubleEmptyDefault") }
@Test def test_emptyDefault() { runner.runOneTest("emptyDefault") }
@Test def test_emptyDefault2() { runner.runOneTest("emptyDefault2") }