[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #385: Fix initiatedContent="yes" with empty initiator

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #385:
URL: https://github.com/apache/incubator-daffodil/pull/385#discussion_r436764216



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/dsom/CompiledExpression1.scala
##########
@@ -143,7 +143,7 @@ final case class ConstantExpression[+T <: AnyRef](
 
   lazy val sourceType: NodeInfo.Kind = NodeInfo.fromObject(value)
 
-  def isKnownNonEmpty = value != ""
+  def isKnownNonEmpty = value != "" && value != "%ES;" && !(value == "%WSP*;" && qn.toQNameString == "dfdl:initiator")

Review comment:
       Sorry about the delay.
   
   I guess my concern here is that ``isKnownNonEmpty`` now means something different. Previously, it essentially meant the initiator was constant and was equal to the empty string, i.e. ``dfdl:initiator=""``. And it was used to test exactly this condition.  For example:
   ```scala
     lazy val hasInitiator = {
       val hasOne = initiatorExpr.isKnownNonEmpty
       hasOne
     }
   ```
   hasInitiator should only be true if this condition is true (contstant with empty string).
   
   But With this change, if we have ``dfdl:initiator="%WSP*;``, then ``isKnownNonEmpty`` will now evaluate to false, and so will ``hasInitiator``. This feels off to me. Perhaps this doesn't matter based on the order things are called, but it feels fragile. It feels to me like ``isKnownNonEmpty`` should keep the logic it currently has (perhaps with a rename like isConstantEmptyString to make it more clear what it's testing), and something else needs to have the logic to fix this issue, e.g a function call canMatchEmptyString on the DFADelimiter.
   
   This change also prevents other properties from being able to use this. Although only delimiters use this at the momement, it seems reasonable that other properties might want to know if the property is a constant empty string, but do not care about whether the value can actually consume empty no data or not. Seems like this is sort of mixing logic of two different things.
   
   Another thought, this restriction related to initiatedContent also applies when the initiator property is runtime evaluated, for example:
   ```xml
   <xs:element name="bar" dfdl:initiator="{ if (../foo eq 1) 'foo' else '%WSP*;' }" ... />
   ```
   Because isKnownNonEmpty only applies to constant expressions, the initiated content check won't apply for this runtime expression.
   
   The right fix is probably to have some logic described above for DFADelimters to determine if it can possible match the empty string, and then have the ``InitiatorParseEv`` check that logic. This would make it so the logic would be tested on both constant and runtime expressions, and would be specific to initiators.
   
   That's probably a somewhat significant change though...




----------------------------------------------------------------
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:
users@infra.apache.org