stevedlawrence commented on code in PR #1097:
URL: https://github.com/apache/daffodil/pull/1097#discussion_r1367209492
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/VariableMap1.scala:
##########
@@ -384,41 +376,34 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
case _ => // Do nothing
}
- val variableInstances = vTable.get(varQName)
- if (variableInstances.isDefined) {
- val variable = variableInstances.get.last
- variable.state match {
- case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
- variable.setState(VariableRead)
- variable.value.getNonNullable
- }
- // This case is only hit for defineVariable's who's expression
reference
- // other defineVariables with expressions. It will be hit at the start
- // of parsing, before an infoset is generated, via the
- // forceExpressionEvaluations function after which all variables should
- // have a defined value
- case VariableUndefined if
(variable.rd.maybeDefaultValueExpr.isDefined) => {
- variable.setState(VariableBeingDefined)
- val res =
-
DataValue.unsafeFromAnyRef(variable.rd.maybeDefaultValueExpr.get.evaluate(state))
-
- // Need to update the variable's value with the result of the
- // expression
- variable.setState(VariableRead)
- variable.setValue(res)
-
- res
- }
- case VariableBeingDefined => throw new
VariableCircularDefinition(varQName, vrd)
- case VariableInProcess => throw new VariableSuspended(varQName, vrd)
- case _ => throw new VariableHasNoValue(varQName, vrd)
+ val variable = vTable(vrd.vmapIndex).head
+ variable.state match {
+ case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
+ case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ variable.setState(VariableRead)
+ variable.value.getNonNullable
+ }
+ // This case is only hit for defineVariable's who's expression reference
+ // other defineVariables with expressions. It will be hit at the start
+ // of parsing, before an infoset is generated, via the
+ // forceExpressionEvaluations function after which all variables should
+ // have a defined value
+ case VariableUndefined if (variable.rd.maybeDefaultValueExpr.isDefined)
=> {
+ variable.setState(VariableBeingDefined)
+ val res =
+
DataValue.unsafeFromAnyRef(variable.rd.maybeDefaultValueExpr.get.evaluate(state))
+
+ // Need to update the variable's value with the result of the
+ // expression
+ variable.setState(VariableRead)
+ variable.setValue(res)
Review Comment:
> Yet another reason to always use LinkedHashMap, not regular HashMap.
Deterministic behavior is your friend.
Agreed.
Turns out this was the issue. As the comment for this case in the code
suggests, this code path is used to support cases where the `defaultValue` of a
`defineVariable` references another variable that has a `defaultValue` that
hasn't been evaluated yet. So it effectively pauses and goes to evaluate that
other `defaultValue` real quick.
Due to the random nature of Maps, it was very easy for this to happen.
Randomly evaluating `defaultValues` (which is essentially what we did) will
very likely get into a case where one variable references some other variable
whose `defaultValue` hasn't been evaluated yet.
But with this change, we now evaluate variables in the exact order that they
are defined in the schema, and none of our tests happened to have a case where
variables were defined in backwards order that could cause this. I've added a
test that explicitly does this to make sure it works.
Note that the only other test where we fall into this block is with a
circular definition of `defaultValue`s, which is detected and an exception
thrown in the `evaluate()` function. So that explains why we had coverage just
above these two lines where `evaluate()` is called, but not these two lines.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
