[incubator-daffodil] 01/01: Fix variables getting reset incorrectly

[ja...@apache.org]

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

commit 779e9803ba7708d700c0205cbd690a179841b084
Author: Josh Adams <ja...@tresys.com>
AuthorDate: Mon Jul 27 15:27:50 2020 -0400

    Fix variables getting reset incorrectly
    
    When the variables infrastructure was changed to be mutable, a
    changedVariablesStack was introduced to track what variables had been
    changed in the current scope. These variables would be reset when the
    PState was reset. The problem was that new lists of variables would only
    be pushed when entering a point of uncertainty and popped when leaving,
    however the call to PState.reset() can happen outside of points of
    uncertainty, such as with nillable elements.
    
    This change makes it so that whenever a PState.Mark is created, a new
    list of changed variables is pushed on the stack. When a mark is
    discarded or the PState is reset, we pop the stack.
    
    DAFFODIL-2374
---
 .../daffodil/processors/parsers/PState.scala       | 15 ++++----
 .../daffodil/section07/variables/variables.tdml    | 41 ++++++++++++++++++++++
 .../section07/variables/TestVariables.scala        |  3 ++
 3 files changed, 51 insertions(+), 8 deletions(-)

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 1ce5002..8b3d8d7 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
@@ -169,7 +169,10 @@ final class PState private (
   /**
    * This stack tracks variables that have changed within the current point of
    * uncertainty. This tracking is necessary to revert changes made to variables
-   * when the parser needs to backtrack.
+   * when the parser needs to backtrack. This stack needs to be pushed when a
+   * new mark is created and popped when it is discarded or reset. It is
+   * necessary for every mark to either be discarded or reset to inorder for
+   * this stack to funciton correctly.
    */
   private val changedVariablesStack = new MStackOf[mutable.MutableList[GlobalQName]]()
   changedVariablesStack.push(mutable.MutableList[GlobalQName]())
@@ -197,6 +200,7 @@ final class PState private (
 
   def mark(requestorID: String): PState.Mark = {
     // threadCheck()
+    changedVariablesStack.push(mutable.MutableList[GlobalQName]())
     val m = markPool.getFromPool(requestorID)
     m.captureFrom(this, requestorID)
     m
@@ -218,19 +222,14 @@ 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
+    changedVariablesStack.pop
   }
 
   def discard(m: PState.Mark): Unit = {
     dataInputStream.discard(m.disMark)
     m.clear()
     markPool.returnToPool(m)
+    changedVariablesStack.pop
   }
 
   override def toString() = {
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 35eb691..d7fc1c9 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
@@ -1754,4 +1754,45 @@
     </tdml:infoset>
   </tdml:parserTestCase>
 
+  <tdml:defineSchema name="variables_nilled_element">
+    <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+    <dfdl:format ref="ex:GeneralFormat"
+      nilKind="literalValue" nilValue="%ES;" useNilForDefault="no" nilValueDelimiterPolicy="none" />
+
+    <dfdl:defineVariable name="var1" type="xs:string" defaultValue="false" />
+    <xs:element name="root">
+      <xs:complexType>
+        <xs:sequence dfdl:separator=",">
+          <xs:element name="id" type="xs:unsignedShort" dfdl:lengthKind="explicit" dfdl:length="1">
+            <xs:annotation>
+              <xs:appinfo source="http://www.ogf.org/dfdl/">
+                <dfdl:setVariable ref="var1" value="true" />
+              </xs:appinfo>
+            </xs:annotation>
+          </xs:element>
+          <xs:element name="nillable_but_present" type="xs:string" nillable="true" dfdl:lengthKind="explicit" dfdl:length="4" />
+          <xs:choice dfdl:choiceDispatchKey="{ $ex:var1 }">
+            <xs:element name="var1_true" type="xs:string" dfdl:choiceBranchKey="true" dfdl:lengthKind="explicit" dfdl:length="7" />
+            <xs:element name="var1_false" type="xs:string" dfdl:choiceBranchKey="false" dfdl:lengthKind="explicit" dfdl:length="7" />
+          </xs:choice>
+        </xs:sequence>
+      </xs:complexType>
+    </xs:element>
+
+  </tdml:defineSchema>
+
+  <tdml:parserTestCase name="variables_nilled_element" root="root"
+    model="variables_nilled_element" description="Section 7 - setVariable - verify that variables aren't getting reset incorrectly - DAFFODIL-2374">
+    <tdml:document>1,test,is set?</tdml:document>
+    <tdml:infoset>
+      <tdml:dfdlInfoset>
+        <root>
+          <id>1</id>
+          <nillable_but_present>test</nillable_but_present>
+          <var1_true>is set?</var1_true>
+        </root>
+      </tdml:dfdlInfoset>
+    </tdml:infoset>
+  </tdml:parserTestCase>
+
 </tdml:testSuite>
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 79d62b8..0a330af 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
@@ -93,6 +93,9 @@ class TestVariables {
 
   @Test def test_unsignedIntVarCast(): Unit = { runner.runOneTest("unsignedIntVarCast") }
 
+  // DFDL-2374
+  @Test def test_variables_nilled_element(): Unit = { runner.runOneTest("variables_nilled_element") }
+
   // DFDL-2375
   //@Test def test_choiceBranchVariables(): Unit = { runner.runOneTest("choiceBranchVariables") }