mbeckerle commented on a change in pull request #473:
URL: https://github.com/apache/incubator-daffodil/pull/473#discussion_r562027522
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
Review comment:
The right behavior is that suppose a DFDL implementation did not do
streaming unparsing at all. There would be no-suspending when reaching forward
for element values.
The behavior one gets in that situation is the correct answer. Streaming
unparsing has to achieve this same behavior, or it is a bug.
The correct behavior in the above example in steve lawrence's comment is a
runtime SDE due to setVariable variable1 occuring after the variable1 has
already been read for its default value.
In Daffodil's streaming implementation, the forward referencing expression
causing a suspension should make a copy/clone of all variable state, so such
time as it does read variable1, it will get the default value of 1.
So far so good.
As unparsing continues, the setVariable statement will be encountered, and
it will set variable1's value to 2. It does not know that the expression has
suspended but will eventually read variable1. This is the bug.
A new state on variable1 indicating it is pending a read by a suspension
won't help. This doesn't work in general, because expressions can have
if-then-else logic such that some conditional branches evaluate certain
variables, not others. So until we evaluate an expression we can't know what
variables it would read.
I believe we have a situation close to a classic compiler anti-dependence
here.
Fact is, this setVariable should be itself suspended until after the
variable-read in the expression for variable2.
The current daffodil expression implementation won't even attempt to read
variable1's value until the suspension completes; hence, variable1 wouldn't
even get marked as 'pending read' state until the forward reference has been
resolved.
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
Review comment:
Doing the full dependency analysis of every read/write of a variable
across the DFDL schema is one way to solve this, albeit a complex one.
I think a "workaround" fix might be this: in variable direction both or
unparseOnly, disallow setVariable after a NVI with a default value. This
requires the user to either remove the default value from the NVI, or remove
the setVariable. This requires analysis of whether there are setVariable
statements in scope of an unparse-direction NVI having default value, and
that's itself somewhat tricky, but it is far easier than a full dependency
analysis.
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
Review comment:
An easier check could be if a given variable is used anywhere in the
schema by a NVI statement having default value, and the variable has
unparse-direction, that no setVariable is allowed anywhere for that variable.
This eliminates the need for a precise roll-up of which setVariable statements
are in scope of a particular NVI or not.
Such a variable is what we might call "bindable only" or non-settable. I
think this fixes many things, and DFDL schemas that actually try to use
setVariable with such variables can split a variable into two variables. Use
one for binding via NVI with defaults, and the other for the setVariable
purposes, it's NVIs would not have any default values.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala
##########
@@ -344,6 +346,16 @@ class VariableMap private(vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance]
*/
def readVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE,
maybeState: Maybe[ParseOrUnparseState]): DataValuePrimitive = {
val varQName = vrd.globalQName
+ if (maybeState.isDefined) {
+ vrd.direction match {
+ case VariableDirection.ParseOnly if
(!maybeState.get.isInstanceOf[PState]) =>
+ maybeState.get.SDW("Attempting to read variable %s which is marked
as parseOnly during unparsing".format(varQName))
+ case VariableDirection.UnparseOnly if
(!maybeState.get.isInstanceOf[UState]) =>
+ maybeState.get.SDW("Attempting to read variable %s which is marked
as unparseOnly during parsing".format(varQName))
+ case _ => // Do nothing
Review comment:
Definitely make them SDE. We can always weaken them to SDW in the future
if we find SDE too restrictive. But I think SDE is right.
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
Review comment:
As this is a dfdlx experimental feature, I think we need a wiki page
description of it, and that would have a section detailing these
implementation-specific restrictions on how to use variables at unparse time
due to the streaming unparser behavior.
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
Review comment:
Lastly this scenario SL came up with needs a test that either shows the
problem, or shows it not occurring.
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
// When implemented this almost certainly wants to be a combinator
// Not two separate unparsers.
-class NewVariableInstanceStartUnparser(override val context: RuntimeData)
+class NewVariableInstanceStartUnparser(override val context:
VariableRuntimeData)
extends PrimUnparserNoData {
override lazy val runtimeDependencies = Vector()
override lazy val childProcessors = Vector()
+ def suspendableExpression = {
+ Assert.invariant(context.maybeDefaultValueExpr.isDefined)
+ new
NewVariableInstanceSuspendableExpression(context.maybeDefaultValueExpr.get,
context)
+ }
+
override def unparse(state: UState) = {
- val vrd = context.asInstanceOf[VariableRuntimeData]
- state.newVariableInstance(vrd)
+ if (context.maybeDefaultValueExpr.isDefined) {
+ suspendableExpression.run(state)
+ }
+ else {
+ state.variableMap.newVariableInstance(context)
+ }
}
}
-class NewVariableInstanceEndUnparser(override val context: RuntimeData)
+class NewVariableInstanceEndUnparser(override val context: VariableRuntimeData)
extends PrimUnparserNoData {
override lazy val runtimeDependencies = Vector()
override lazy val childProcessors = Vector()
override def unparse(state: UState) = {
- state.removeVariableInstance(context.asInstanceOf[VariableRuntimeData])
+ if (state.variableMap.find(context.globalQName).isDefined)
Review comment:
I think SL is correct. This isn't right. We can't suspend creation of
the variable instance, we have to suspend the calculation of its value
sometimes, and we can't read it until that calculation is complete, but the
instance has to be there so that the expressions can try to read and determine
that they've blocked on a pending variable value.
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ExpressionEvaluatingUnparsers.scala
##########
@@ -77,30 +76,52 @@ final class SetVariableUnparser(
}
+final class NewVariableInstanceSuspendableExpression(
+ override val expr: CompiledExpression[AnyRef],
+ override val rd: VariableRuntimeData)
+ extends SuspendableExpression {
+
+ override protected def processExpressionResult(ustate: UState, v:
DataValuePrimitive): Unit = {
+ ustate.variableMap.newVariableInstance(rd, v)
+ }
+
+ override protected def maybeKnownLengthInBits(ustate: UState) = MaybeULong(0)
+}
+
Review comment:
This may ultimately just be a flaw in the DFDL language notion of
variables. They're supposed to be single-assignment variables, but this mixture
of defaultValue and setVariable where a read locks down the default value was
always a sketchy design point because things become very order dependent, and
since the unparser does things out of order due to suspensions and forward
reference, the tenuous interactions of these things become problematic.
A way to fix this is to say that for any variable that has unparse direction
(so both or unparseOnly) you have to pick whether you use *only* defaultValue
expressions to bind values to it, or whether you will use setVariable
statements, in which case the variable cannot have any default value in the
base defineVariable nor in any newVariableInstance statements. This I think
fixes all these issues. A variable is either defaultable, and not settable, or
it is settable and not defaultable. I think these restrictions are very
livable.
----------------------------------------------------------------
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]
