[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #966: Improve errors with contentLength/valueLength (WIP)

["stevedlawrence (via GitHub)" <gi...@apache.org>]

stevedlawrence commented on code in PR #966:
URL: https://github.com/apache/daffodil/pull/966#discussion_r1109777236


##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2131,7 +2131,7 @@ case class FunctionCallExpression(functionQNameString: String, expressions: List
           functionQName,
           args,
           NodeInfo.Long,
-          NodeInfo.Exists,
+          NodeInfo.Scalar,

Review Comment:
   Right above this is a big comment about when "exists" makes sense. That needs to be removed/changed.



##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
   ) {
 
   protected final def leafReferencedElements = {
+    if (!args(0).isInstanceOf[PathExpression]) {
+      SDE(s"expected a path expression but got type ${args(0).inherentType.globalQName}")
+    }
     val arg = args(0).asInstanceOf[PathExpression]

Review Comment:
   It feels like our type system should be able to handle this, but I don't think it can. The probably is it doesn't really differentiate between a literal string and a path to a string. In both cases, the arg expression is going to have an `inerhentType` of "String".
   
   I wonder if our type systems needs the ability to differentiate that too? For example, we might have something like `NodeInfo.String` and `NodeInfo.ScalarString`. If we had this separate hiearchy of scalar-types and literal-types, paths would have an inherent type of the Scalar variant, and literals would have the inherent type of the non-Scalar variant.  When doing conversions, a NodeString can convert to a String, but not the other way around. This way the SDE would get caugt in the `conversionOps` function.
   
   If feels like maybe that's what Exists was accomplishing (since it's allow about requiring a node), but it does so without enough type information.
   
   Note that this approach might allow more complex things like this:
   ```
   dfdl:contentLength(if (foo) /path/to/one/thing else /path/to/another/thing, "bytes")
   ```
   
   But wouldn't allow something like this:
   ```
   dfdl:contentLength(if (foo) /path/to/one/thing else "bar", "bytes")
   ```
   This would fail because the inherent type of the if-expression would convert to a String but contentLength would need a ScalarString, and String can't convert to ScalarString.
   
   However, this idea feels pretty excessive for this problem, and even ignoring the fact that it doubles our types, I'm not sure it would actually work.
   
   All that to say this check seems like a reasonable approach.
   
   I'll add that since our type system doesn't help us with this problem, it probably means there are other functions where we allow literals where we shouldn't. For example, `fn:exists()` doesn't seem to check for this. There are likely others that we may need to look at, though that feels out of scope for this PR though. We might want to add an issue to add tests an fix those cases.
   
   



##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/Expression.scala:
##########
@@ -2829,13 +2829,15 @@ sealed abstract class LengthExprBase(
   ) {
 
   protected final def leafReferencedElements = {
+    if (!args(0).isInstanceOf[PathExpression]) {
+      SDE(s"expected a path expression but got type ${args(0).inherentType.globalQName}")

Review Comment:
   Suggest capitalizing "e" in expected..



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -382,18 +380,28 @@ object NodeInfo extends Enum {
    * All complex types are represented by this one type object.
    */
   protected sealed trait ComplexKind extends AnyType.Kind
-  case object Complex extends TypeNode('Complex, AnyType) with ComplexKind {
+  case object Complex extends TypeNode('Complex, Scalar) with ComplexKind {
     type Kind = ComplexKind
   }
 
   /**
-   * For things like fn:exists, fn:empty, dfdl:contenLength
+   * For things like fn:exists, fn:empty
    */
   protected sealed trait ExistsKind extends AnyType.Kind
   case object Exists extends TypeNode('Exists, AnyType) with AnyTypeKind {
     type Kind = ExistsKind
   }
 
+  /**
+   * For things like dfdl:contentLength, dfdl:valueLength
+   */
+  protected sealed trait ScalarKind extends AnyType.Kind
+  case object Scalar
+    extends TypeNode('Scalar, AnyType, Seq(AnySimpleType, Complex))
+    with AnyTypeKind {
+    type Kind = ScalarKind
+  }

Review Comment:
   I believe this is correct. TypeNode basically just sets up a type hierarchy. AnyType is the parent of Scalar, and it's children are simple types and complex types.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dpath/NodeInfo.scala:
##########
@@ -993,6 +1001,7 @@ object NodeInfo extends Enum {
   lazy val allTypes =
     allDFDLTypes ++ List(
       Complex,
+      Scalar,
       Array,

Review Comment:
   Good point. Scalar's children are Complex (here in `allTypes`) and AnySimple (above in allAbstractTypes), so there doesn't seem to be an obvious place for. I don't think it currently matters because nothing actually uses `allAbstractTypes`, but it feels like Scalar is more like Exists/AnySimpleType than Complex and does want to be in the abstract lis.



-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org