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

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

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


##########
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:
   > I added an additional test to ensure there is an error when a string literal is used as the first argument. That causes a ClassCastException. I wasn't sure how to restrict that from happening using the type system and I just hacked in a type check to instead throw an SDE. I need some suggestions on how to implement this fix properly.
   
   Is this the place you mentioned where you want a better suggestion how to avoid the ClassCastException from the asInstanceOf[PathExpression] on line 2835?  Your type check looks fine to me but any thoughts, Steve?  The only suggestion I have is to call schemaDefinitionUnless instead which collapses the if and SDE into a single function call.  This file already has 27 SDE calls and 9 schemaDefinitionUnless calls, so your type check will be in good company.



##########
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'm not familiar with the TypeNode constructor, so Steve please take a look too.



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

Review Comment:
   Please find the comment on the next line (not shown in the diff) and update it so it no longer refers to `Exists` but refers to `Scalar` instead.



##########
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:
   I notice Exists is listed in allAbstractTypes (above at line 969) while Scalar is listed in allTypes here.  I'm sure Scalar is more analogous to Complex than Exists, but I thought I should mention it.



-- 
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