stevedlawrence commented on a change in pull request #450:
URL: https://github.com/apache/incubator-daffodil/pull/450#discussion_r539280896
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/util/MStack.scala
##########
@@ -182,7 +182,7 @@ object MStackOfAnyRef {
* things.
*/
protected abstract class MStack[@specialized T] private[util] (
- arrayAllocator: (Int) => Array[T], nullValue: T) extends Serializable {
+ arrayAllocator: (Int) => Array[T], nullValue: T) {
Review comment:
Is this intentional? Is there a problem with serializing an MStack?
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -87,7 +87,8 @@ class NewVariableInstanceStartUnparser(override val context:
RuntimeData)
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.newVariableInstance(context.asInstanceOf[VariableRuntimeData])
+ val vrd = context.asInstanceOf[VariableRuntimeData]
+ state.newVariableInstance(vrd, vrd, state)
Review comment:
It looks like in all cases where we call newVariableInstance, we pass in
the same thing as the first and second paramter, and the same state as the
third parameter. Can newVariableInstance just only accept one parameter and
always use vrd when it needs VariableRuntimeData, and always use ``this`` when
it needs state?
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala
##########
@@ -435,6 +435,11 @@ class DataProcessor private (
state.notifyDebugging(true)
}
state.dataProc.get.init(ssrd.parser)
+
+ // Force the evaluation of any defineVariable's with non-constant default
+ // value expressions
+ state.variableMap.forceExpressionEvaluations(state)
+
Review comment:
You mention there's an issue with the TDML runner not seeing this
diagnostic
([DAFFODIL-2443](https://issues.apache.org/jira/browse/DAFFODIL-2443)). I think
this change might be the issue.
When you force the expression evaluate for defaults, that could fail and
throw an SDE exception. There's nothing here to catch it, so it exception will
bubble up to the TDML Runner. But the TDML Runner does (and shouldn't) handle
SDE exceptions. Instead SDE exceptions are supposed to be caught by Daffodil
and turned into a failure diagnostic. That catch happens in the doParse method
(along with a bunch of other exceptions that might be thrown that implied some
sort of error that should become a diagnostic).
So I think the fix is to just move this forceExpressionEvaluations call into
the try-block of the doParse method. That way if this function does throw an
SDE, then it will be caught and correctly turned into a diagnostic. I think it
can be moved to right before the startElement call and be funcionally
equivalent to what you have now, just with the SDE handled correctly.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -188,24 +283,42 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
* VariableInstances are mutable and cannot safely be shared across threads
*/
def copy(): VariableMap = {
- val table = Map[GlobalQName, MStackOf[VariableInstance]]()
- vTable.foreach { case (k: GlobalQName, s: MStackOf[VariableInstance]) => {
- val newStack= new MStackOf[VariableInstance]()
+ val table = Map[GlobalQName, ArrayBuffer[VariableInstance]]()
+ vTable.foreach { case (k: GlobalQName, abuf:
ArrayBuffer[VariableInstance]) => {
+ val newBuf = new ArrayBuffer[VariableInstance]()
// toList provides a list in LIFO order, so we need to reverse it to
// maintain order
- s.toList.reverse.foreach { case v: VariableInstance =>
newStack.push(v.copy()) }
- table(k) = newStack
+ abuf.foreach { case v: VariableInstance => { newBuf += v.copy() }}
Review comment:
The comment above about toList seems to no longer apply. I think it can
be removed. Also, I think the foreach's can be replaced with maps. Should make
things ab it cleaner, e.g.:
```scala
val table = vTable.map { case (k: GlobalQName, abuf:
ArrayBuffer[VariableInstance]) =>
val newBuf = abuf.map { _.copy() }
(k, newBuf)
}
new VariableMap(table)
```
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -188,24 +283,42 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
* VariableInstances are mutable and cannot safely be shared across threads
*/
def copy(): VariableMap = {
- val table = Map[GlobalQName, MStackOf[VariableInstance]]()
- vTable.foreach { case (k: GlobalQName, s: MStackOf[VariableInstance]) => {
- val newStack= new MStackOf[VariableInstance]()
+ val table = Map[GlobalQName, ArrayBuffer[VariableInstance]]()
+ vTable.foreach { case (k: GlobalQName, abuf:
ArrayBuffer[VariableInstance]) => {
+ val newBuf = new ArrayBuffer[VariableInstance]()
// toList provides a list in LIFO order, so we need to reverse it to
// maintain order
- s.toList.reverse.foreach { case v: VariableInstance =>
newStack.push(v.copy()) }
- table(k) = newStack
+ abuf.foreach { case v: VariableInstance => { newBuf += v.copy() }}
+ table(k) = newBuf
}}
new VariableMap(table)
}
+ // For defineVariable's with non-constant expressions for default values, it
+ // is necessary to force the evaluation of the expressions after the
+ // VariableMap has been created and initialized, but before parsing begins.
We
+ // must also ensure that the expressions only reference other variables that
+ // have default value expressions or are defined externally.
+ def forceExpressionEvaluations(state: ParseOrUnparseState): Unit = {
+ vTable.map { case (_, abuf) => { abuf.foreach { inst => {
Review comment:
This map can be a foreach. No need to allocate a new Map since we're
mutate the abuf's inside it.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -232,19 +345,33 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE,
maybePstate: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
- val stack = vTable.get(varQName)
- if (stack.isDefined) {
- val variable = stack.get.top
- variable.state match {
- case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ val abuf = vTable.get(varQName)
+ if (abuf.isDefined) {
+ val variable = abuf.get.last
+ (variable.state, variable.value.isDefined,
variable.defaultValueExpr.isDefined) match {
+ case (VariableRead, true, _) => variable.value.getNonNullable
+ case (VariableDefined | VariableSet, true, _) => {
if (maybePstate.isDefined && maybePstate.get.isInstanceOf[PState])
maybePstate.get.asInstanceOf[PState].markVariableRead(vrd)
variable.setState(VariableRead)
variable.value.getNonNullable
}
- case _ => throw new VariableHasNoValue(varQName, vrd)
+ case (VariableDefined, false, true) => {
+ Assert.invariant(maybePstate.isDefined)
+ variable.setState(VariableInProcess)
+ val pstate = maybePstate.get
+ val res =
DataValue.unsafeFromAnyRef(variable.defaultValueExpr.get.evaluate(pstate))
+
+ // Need to update the variable's value with the result of the
+ // expression
+ variable.setState(VariableRead)
+ variable.setValue(res)
+
+ res
Review comment:
Is this correct? This is evaluating the defaultValue and setting the
state when the variable is read. But the spec says this about
newVariableInstance:
> If a default value is specified this initial value of the instance will be
created when the instance is created
So it seems this evaluation should not happen during read, but when the
instance of the variable is created. And then that default could be overridden
with a setVariableCal.
Looks like you do evaluate the default expression below in
newVariableInstance, so maybe this just needs to use what that sets?
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -286,23 +413,28 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
/**
* Creates a new instance of a variable
*/
- def newVariableInstance(vrd: VariableRuntimeData) = {
+ def newVariableInstance(vrd: VariableRuntimeData, referringContext:
ThrowsSDE, state: ParseOrUnparseState) = {
val varQName = vrd.globalQName
- val stack = vTable.get(varQName)
- Assert.invariant(stack.isDefined)
- stack.get.push(vrd.createVariableInstance)
+ val abuf = vTable.get(varQName)
+ Assert.invariant(abuf.isDefined)
+
+ if (vrd.maybeDefaultValueExpr.isDefined) {
+ val defaultValue =
DataValue.unsafeFromAnyRef(vrd.maybeDefaultValueExpr.get.evaluate(state))
+ abuf.get +=
vrd.createVariableInstance(VariableUtils.convert(defaultValue.getAnyRef.toString,
vrd, referringContext))
Review comment:
This ``VariableUtils.convert`` seems incorrect to me. I would expect
that evaluating the defaultValue expression, or any setVariable expression as
well, would know the target type and the expression evaluation would handle the
conversion, just like it does with all other expression evaluations. Looks like
this isn't something you added, so probably out of scope for this change. But
we should create a bug to fix this.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -232,19 +345,33 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE,
maybePstate: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
- val stack = vTable.get(varQName)
- if (stack.isDefined) {
- val variable = stack.get.top
- variable.state match {
- case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ val abuf = vTable.get(varQName)
+ if (abuf.isDefined) {
+ val variable = abuf.get.last
+ (variable.state, variable.value.isDefined,
variable.defaultValueExpr.isDefined) match {
+ case (VariableRead, true, _) => variable.value.getNonNullable
+ case (VariableDefined | VariableSet, true, _) => {
if (maybePstate.isDefined && maybePstate.get.isInstanceOf[PState])
maybePstate.get.asInstanceOf[PState].markVariableRead(vrd)
variable.setState(VariableRead)
variable.value.getNonNullable
}
- case _ => throw new VariableHasNoValue(varQName, vrd)
+ case (VariableDefined, false, true) => {
+ Assert.invariant(maybePstate.isDefined)
+ variable.setState(VariableInProcess)
+ val pstate = maybePstate.get
+ val res =
DataValue.unsafeFromAnyRef(variable.defaultValueExpr.get.evaluate(pstate))
+
+ // Need to update the variable's value with the result of the
+ // expression
+ variable.setState(VariableRead)
+ variable.setValue(res)
+
+ res
+ }
+ case (VariableInProcess, _, _) => throw new
VariableCircularDefinition(varQName, vrd)
+ case (_, _, _) => throw new VariableHasNoValue(varQName, vrd)
Review comment:
Should the test we have add coverage for this, but it's currently
failing due to DAFFODIL-2444? This seems like a really important path to cover.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -166,17 +257,21 @@ final class VariableBox(initialVMap: VariableMap) {
*
* The DPath implementation must be made to implement the
* no-set-after-default-value-has-been-read behavior. This requires that
reading the variables causes a state transition.
+ *
+ * The implementaiton of the VariabeMap uses ArrayBuffers essentially as a
Review comment:
Typo: implementation
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -232,19 +345,33 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE,
maybePstate: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
- val stack = vTable.get(varQName)
- if (stack.isDefined) {
- val variable = stack.get.top
- variable.state match {
- case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ val abuf = vTable.get(varQName)
+ if (abuf.isDefined) {
+ val variable = abuf.get.last
+ (variable.state, variable.value.isDefined,
variable.defaultValueExpr.isDefined) match {
+ case (VariableRead, true, _) => variable.value.getNonNullable
+ case (VariableDefined | VariableSet, true, _) => {
if (maybePstate.isDefined && maybePstate.get.isInstanceOf[PState])
Review comment:
While we're renaming variables, can we change maybePstate to just
maybeState, since it seems it can be either parse or unparse? I thought this
was specifically for parse, and was confused about what happens during unparse,
but that doesn't seem to be the case. Or should this always be PState, and
maybe the type of maybePState should be Maybe[PState] in which case you only
need th isDefined check?
##########
File path:
daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
##########
@@ -1884,4 +1976,63 @@
</tdml:infoset>
</tdml:parserTestCase>
+ <tdml:defineSchema name="circular_defineVariable">
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
+
+ <dfdl:defineVariable name="circular1" type="xs:int"
+ defaultValue="{ $ex:circular2 }" />
+ <dfdl:defineVariable name="circular2" type="xs:int"
+ defaultValue="{ $ex:circular1 }" />
+ <xs:element name="root" type="xs:int" dfdl:inputValueCalc="{ $ex:circular1
}" />
+
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="circular_defineVariable_err" root="root"
+ model="circular_defineVariable" description="Section 7 - ">
+ <tdml:document/>
+ <tdml:errors>
+ <tdml:error>Runtime Schema Definition Error</tdml:error>
+ <tdml:error>part of circular definition</tdml:error>
+ </tdml:errors>
+ </tdml:parserTestCase>
+
+ <tdml:defineSchema name="defineVariable_ref_infoset">
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
+
+ <dfdl:defineVariable name="badRef" type="xs:int"
+ defaultValue="{ ex:root }" />
+ <xs:element name="root" type="xs:int" dfdl:inputValueCalc="{ 42 }" />
+
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="defineVariable_ref_infoset_err" root="root"
+ model="defineVariable_ref_infoset" description="Section 7 - ">
+ <tdml:document/>
+ <tdml:errors>
+ <tdml:error>????</tdml:error>
+ </tdml:errors>
+ </tdml:parserTestCase>
+
+ <tdml:defineSchema name="defineVariable_ref_noDefault">
+ <xs:include
schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
+
+ <dfdl:defineVariable name="noDefault" type="xs:int" />
+ <dfdl:defineVariable name="badRef" type="xs:int"
+ defaultValue="{ $ex:noDefault }" />
+ <xs:element name="root" type="xs:int" dfdl:inputValueCalc="{ $ex:badRef }"
/>
+
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="defineVariable_ref_noDefault_err" root="root"
+ model="defineVariable_ref_noDefault" description="Section 7 - ">
+ <tdml:document/>
+ <tdml:errors>
+ <tdml:error>Runtime Schema Definition Error</tdml:error>
+ <tdml:error>has no value. It was not set</tdml:error>
+ </tdml:errors>
+ </tdml:parserTestCase>
+
Review comment:
An intersting test case that I'm not sure is covered but would be worth
having: What if you have two variables where they both have defaults, and one
refers to the other, like what you have above:
```xml
<dfdl:defineVariable name="var1" type="xs:int" defaultValue="5" />
 <dfdl:defineVariable name="var2" type="xs:int" defaultValue="{ $var1 +
1 }" />
```
Now what if during parse we have a dfdl:setVariable that sets the value of
both of these variables? The spec says this:
> If a defaultValue expression references another variable then the
single-assignment nature of variables prevents the referenced variable's value
from ever changing, that is, it is considered to be a read of the variable's
value, and once read, a variable's value cannot be changed.
So we shoudl be able to setVariable var2, but not setVariable var1. I think
having a test that checks for this would be useful.
Another test I think would be useful, if it doesn't already exist, would be
to allow var1 and var2 to be externally set, and make sure that those values
override the default values. With the change with the forceEvaluations and
delay evaluates of things, I'm not 100% confident that works as expected.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -232,19 +345,33 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE,
maybePstate: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
- val stack = vTable.get(varQName)
- if (stack.isDefined) {
- val variable = stack.get.top
- variable.state match {
- case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ val abuf = vTable.get(varQName)
+ if (abuf.isDefined) {
+ val variable = abuf.get.last
+ (variable.state, variable.value.isDefined,
variable.defaultValueExpr.isDefined) match {
+ case (VariableRead, true, _) => variable.value.getNonNullable
+ case (VariableDefined | VariableSet, true, _) => {
if (maybePstate.isDefined && maybePstate.get.isInstanceOf[PState])
maybePstate.get.asInstanceOf[PState].markVariableRead(vrd)
variable.setState(VariableRead)
variable.value.getNonNullable
}
- case _ => throw new VariableHasNoValue(varQName, vrd)
+ case (VariableDefined, false, true) => {
+ Assert.invariant(maybePstate.isDefined)
+ variable.setState(VariableInProcess)
+ val pstate = maybePstate.get
+ val res =
DataValue.unsafeFromAnyRef(variable.defaultValueExpr.get.evaluate(pstate))
Review comment:
I guess this means maybePState really could be a UState or a PState?
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -232,19 +345,33 @@ class VariableMap private(vTable: Map[GlobalQName,
MStackOf[VariableInstance]])
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE,
maybePstate: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
- val stack = vTable.get(varQName)
- if (stack.isDefined) {
- val variable = stack.get.top
- variable.state match {
- case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ val abuf = vTable.get(varQName)
+ if (abuf.isDefined) {
+ val variable = abuf.get.last
+ (variable.state, variable.value.isDefined,
variable.defaultValueExpr.isDefined) match {
Review comment:
This is maybe personal preference, but I prefer the previous code where
we had ``case Foo if ...``. It's easier to see what a particular case is
checking for when you can see the condition, rather than just seeing true/false
and having to refer back to the match statement. There also might be some
overhead with having to allocate a tuple (not sure if Scala will do that behind
the scenes).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
