Merge pull request #3550 from line-o/fix/3474

dizzzz · web-flow · commit 99d278f39f55 · 2020-12-03T22:17:24.000+01:00
[hotfix] add location info to element content errors
diff --git a/exist-core/src/main/antlr/org/exist/xquery/parser/XQuery.g b/exist-core/src/main/antlr/org/exist/xquery/parser/XQuery.g
@@ -1663,13 +1663,22 @@ compElemConstructor throws XPathException
 { String eq; }
 :
 	( "element" LCURLY ) =>
-	"element"! LCURLY! expr RCURLY! LCURLY! (expr)? RCURLY!
+	"element"! LCURLY! expr RCURLY! compElemConstructorContent
 	{ #compElemConstructor = #(#[COMP_ELEM_CONSTRUCTOR], #compElemConstructor); }
 	|
-	"element"! eq=eqName LCURLY! (e3:expr)? RCURLY!
-	{ #compElemConstructor = #(#[COMP_ELEM_CONSTRUCTOR, eq], #[STRING_LITERAL, eq], #e3); }
+	"element"! eq=eqName v:compElemConstructorContent
+	{ #compElemConstructor = #(#[COMP_ELEM_CONSTRUCTOR, eq], #[STRING_LITERAL, eq], #v); }
 	;
 
+compElemConstructorContent throws XPathException
+:
+    ( LCURLY RCURLY ) => LCURLY! RCURLY!
+    { #compElemConstructorContent= #(#[PARENTHESIZED, "Parenthesized"], null); }
+    | LCURLY! e:expr RCURLY!
+    { #compElemConstructorContent.copyLexInfo(#e); }
+    ;
+
+
 compAttrConstructor throws XPathException
 { String eq; }
 :
diff --git a/exist-core/src/main/antlr/org/exist/xquery/parser/XQueryTree.g b/exist-core/src/main/antlr/org/exist/xquery/parser/XQueryTree.g
@@ -3157,8 +3157,11 @@ throws PermissionDeniedException, EXistException, XPathException
 		qnameExpr=expr [qnamePathExpr]
 		(
 			{ elementContent = new PathExpr(context); }
-			contentExpr=expr[elementContent]
-			{ construct.addPath(elementContent); }
+			contentExpr=ec:expr[elementContent]
+			{
+			  elementContent.setASTNode(ec);
+			  construct.addPathIfNotFunction(elementContent);
+			}
 		)*
 	)
 	|
diff --git a/exist-core/src/main/java/org/exist/xquery/EnclosedExpr.java b/exist-core/src/main/java/org/exist/xquery/EnclosedExpr.java
@@ -98,7 +98,7 @@ public Sequence eval(Sequence contextSequence, Item contextItem) throws XPathExc
                 while (next != null) {
                     context.proceed(this, builder);
                     if (Type.subTypeOf(next.getType(), Type.FUNCTION_REFERENCE)) {
-                        throw new XPathException(ErrorCodes.XQTY0105, "Enclosed expression contains function item");
+                        throw new XPathException(this, ErrorCodes.XQTY0105, "Enclosed expression contains function item");
 
                         // if item is an atomic value, collect the string values of all
                         // following atomic values and separate them by a space.
diff --git a/exist-core/src/main/java/org/exist/xquery/SequenceConstructor.java b/exist-core/src/main/java/org/exist/xquery/SequenceConstructor.java
@@ -35,11 +35,12 @@
  */
 public class SequenceConstructor extends PathExpr {
 
-    public SequenceConstructor(XQueryContext context) {
+    public SequenceConstructor(final XQueryContext context) {
         super(context);
     }
 
-    public void analyze(AnalyzeContextInfo contextInfo) throws XPathException {
+    @Override
+    public void analyze(final AnalyzeContextInfo contextInfo) throws XPathException {
         contextInfo.setParent(this);
         inPredicate = (contextInfo.getFlags() & IN_PREDICATE) > 0;
         unordered = (contextInfo.getFlags() & UNORDERED) > 0;
@@ -58,20 +59,20 @@ public void analyze(AnalyzeContextInfo contextInfo) throws XPathException {
         contextInfo.setStaticReturnType(staticType);
     }
 
-    /* (non-Javadoc)
-     * @see org.exist.xquery.Expression#eval(org.exist.dom.persistent.DocumentSet, org.exist.xquery.value.Sequence, org.exist.xquery.value.Item)
-     */
-    public Sequence eval(Sequence contextSequence, Item contextItem) throws XPathException {
+    @Override
+    public Sequence eval(final Sequence contextSequence, final Item contextItem) throws XPathException {
         if (context.getProfiler().isEnabled()) {
             context.getProfiler().start(this);
             context.getProfiler().message(this, Profiler.DEPENDENCIES,
                 "DEPENDENCIES", Dependency.getDependenciesName(this.getDependencies()));
-            if (contextSequence != null)
-                {context.getProfiler().message(this, Profiler.START_SEQUENCES,
-                    "CONTEXT SEQUENCE", contextSequence);}
-            if (contextItem != null)
-                {context.getProfiler().message(this, Profiler.START_SEQUENCES,
-                    "CONTEXT ITEM", contextItem.toSequence());}
+            if (contextSequence != null) {
+                context.getProfiler().message(this, Profiler.START_SEQUENCES,
+                    "CONTEXT SEQUENCE", contextSequence);
+            }
+            if (contextItem != null) {
+                context.getProfiler().message(this, Profiler.START_SEQUENCES,
+                    "CONTEXT ITEM", contextItem.toSequence());
+            }
         }
         final ValueSequence result = new ValueSequence();
         result.keepUnOrdered(unordered);
@@ -86,45 +87,61 @@ public Sequence eval(Sequence contextSequence, Item contextItem) throws XPathExc
                 context.popDocumentContext();
             }
         }
-        if (context.getProfiler().isEnabled()) 
-            {context.getProfiler().end(this, "", result);}
+        if (context.getProfiler().isEnabled()) {
+            context.getProfiler().end(this, "", result);
+        }
         return result;
     }
 
-    /* (non-Javadoc)
-     * @see org.exist.xquery.PathExpr#dump(org.exist.xquery.util.ExpressionDumper)
-     */
-    public void dump(ExpressionDumper dumper) {
+    @Override
+    public void dump(final ExpressionDumper dumper) {
         dumper.display("(");
         dumper.startIndent();
         boolean moreThanOne = false;
         for(final Expression step : steps) {
-            if (moreThanOne)
-                {dumper.display(", ");}
+            if (moreThanOne) {
+                dumper.display(", ");
+            }
             moreThanOne = true;
             step.dump(dumper);
         }
         dumper.endIndent();
         dumper.nl().display(")");
     }
 
+    @Override
     public String toString() {
         final StringBuilder result = new StringBuilder();
         result.append("( ");
         boolean moreThanOne = false;
         for (final Expression step : steps) {
-            if (moreThanOne)
-                {result.append(", ");}
+            if (moreThanOne) {
+                result.append(", ");
+            }
             moreThanOne = true;
             result.append(step.toString());
         }
         result.append(" )");
         return result.toString();
     }
 
-    /* (non-Javadoc)
-     * @see org.exist.xquery.Expression#returnsType()
+    /**
+     * Add another PathExpr to this object's expression list.
+     * Performs a static check, if return type is a function type other than array.
+     * see #id-content section 1.e.i and 1.e.ii
+     *
+     * @throws XPathException when Path returns a function type
+     * @param path A path to add to this path
      */
+    public void addPathIfNotFunction(final PathExpr path) throws XPathException {
+        final int retType = path.returnsType();
+        if (Type.subTypeOf(retType, Type.FUNCTION_REFERENCE) && retType != Type.ARRAY) {
+            throw new XPathException(path, ErrorCodes.XQTY0105, "Function types are not allowed in element content. Got " + Type.getTypeName(retType));
+        }
+        super.addPath(path);
+    }
+
+    @Override
     public int returnsType() {
         return Type.ITEM;
     }
@@ -139,10 +156,8 @@ public boolean allowMixedNodesInReturn() {
         return true;
     }
 
-    /* (non-Javadoc)
-     * @see org.exist.xquery.AbstractExpression#resetState()
-     */
-    public void resetState(boolean postOptimization) {
+    @Override
+    public void resetState(final boolean postOptimization) {
         super.resetState(postOptimization);
     }
 }
diff --git a/exist-core/src/test/java/org/exist/xquery/FunctionTypeInElementContentTest.java b/exist-core/src/test/java/org/exist/xquery/FunctionTypeInElementContentTest.java
@@ -0,0 +1,143 @@
+/*
+ * eXist-db Open Source Native XML Database
+ * Copyright (C) 2001 The eXist-db Authors
+ *
+ * info@exist-db.org
+ * http://www.exist-db.org
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+package org.exist.xquery;
+
+import org.exist.EXistException;
+import org.exist.security.PermissionDeniedException;
+import org.exist.test.XQueryCompilationTest;
+import org.junit.Test;
+
+import static org.exist.test.DiffMatcher.elemSource;
+import static org.exist.test.XQueryAssertions.*;
+
+/**
+ * Ensure function types returned in element content throws at compile time and
+ * has location information.
+ * See issue :<a href="https://github.com/eXist-db/exist/issues/3474">#3474</a>.
+ *
+ * @author <a href="mailto:juri@existsolutions.com">Juri Leino</a>
+ */
+public class FunctionTypeInElementContentTest extends XQueryCompilationTest {
+
+    @Test
+    public void arrayLiteral() throws EXistException, PermissionDeniedException {
+        final String query = "element test { [] }";
+        assertXQResultSimilar(elemSource("<test/>"), executeQuery(query));
+    }
+
+    // TODO(JL): array content could be removed after https://github.com/eXist-db/exist/issues/3472 is fixed
+    @Test
+    public void arrayConstructor() throws EXistException, PermissionDeniedException {
+        final String query = "element test { array { () } }";
+        assertXQResultSimilar(elemSource("<test/>"), executeQuery(query));
+    }
+
+    @Test
+    public void sequenceOfItems() throws EXistException, PermissionDeniedException {
+        final String query = "element test { (1, map {})[1] }";
+        assertXQResultSimilar(elemSource("<test>1</test>"), executeQuery(query));
+    }
+
+    @Test
+    public void partialBuiltIn() throws EXistException, PermissionDeniedException {
+        final String query = "element test { sum(?) }";
+        final String error = "Function types are not allowed in element content. Got function(*)";
+        assertXQStaticError(ErrorCodes.XQTY0105, 1, 16, error, compileQuery(query));
+    }
+
+    // TODO(JL): Does still throw without location info
+    @Test
+    public void functionReference() throws EXistException, PermissionDeniedException {
+        final String query = "element test { sum#0 }";
+        final String error = "Function types are not allowed in element content. Got function(*)";
+        assertXQStaticError(ErrorCodes.XQTY0105, 0, 0, error, compileQuery(query));
+    }
+
+    // TODO(JL): Does not throw at compile time
+    @Test
+    public void functionVariable() throws EXistException, PermissionDeniedException {
+        final String query = "let $f := function () {} return element test { $f }";
+        final String error = "Enclosed expression contains function item";
+        assertXQDynamicError(ErrorCodes.XQTY0105, 1, 49, error, executeQuery(query));
+    }
+
+    // TODO(JL): user defined function has its location offset to a weird location
+    @Test
+    public void userDefinedFunction() throws EXistException, PermissionDeniedException {
+        final String query = "element test { function () {} }";
+        final String error = "Function types are not allowed in element content. Got function(*)";
+        assertXQStaticError(ErrorCodes.XQTY0105, 1, 25, error, compileQuery(query));
+    }
+
+    @Test
+    public void mapConstructor() throws EXistException, PermissionDeniedException {
+        final String query = "element test { map {} }";
+        final String error = "Function types are not allowed in element content. Got map(*)";
+        assertXQStaticError(ErrorCodes.XQTY0105, 1, 16, error, compileQuery(query));
+    }
+
+    @Test
+    public void mapConstructorLookup() throws EXistException, PermissionDeniedException {
+        final String query = "element test { map {1:1}?1 }";
+        assertXQResultSimilar(elemSource("<test>1</test>"), executeQuery(query));
+    }
+
+    /**
+     * sequence in enclosed expression with only a function type
+     */
+    @Test
+    public void sequenceOfMaps() throws EXistException, PermissionDeniedException {
+        final String query = "element test { (map {}) }";
+        final String error = "Function types are not allowed in element content. Got map(*)";
+        assertXQDynamicError(ErrorCodes.XQTY0105, 0, 0, error, executeQuery(query));
+    }
+
+    // TODO(JL): add (sub-expression) location
+    /**
+     * This is an edge case, which would evaluate to empty sequence
+     * but should arguably still throw.
+     */
+    @Test
+    public void sequenceOfMapsEdgeCase() throws EXistException, PermissionDeniedException {
+        final String query = "element test { (map {})[2] }";
+        final String error = "Function types are not allowed in element content. Got map(*)";
+        assertXQStaticError(ErrorCodes.XQTY0105, 0, 0, error, compileQuery(query));
+    }
+
+    // TODO(JL): add (sub-expression) location
+    // TODO(JL): this could throw at compile time
+    @Test
+    public void arrayOfMaps() throws EXistException, PermissionDeniedException {
+        final String query = "element test { [map {}] }";
+        final String error = "Enclosed expression contains function item";
+        assertXQDynamicError(ErrorCodes.XQTY0105, 1, 16, error, executeQuery(query));
+    };
+
+    // TODO(JL): add (sub-expression) location
+    // TODO(JL): This should throw at compile time, but does not
+    @Test
+    public void mapConstructorInSubExpression() throws EXistException, PermissionDeniedException {
+        final String query = "element test { \"a\", map {} }";
+        final String error = "Enclosed expression contains function item";
+        assertXQDynamicError(ErrorCodes.XQTY0105, 0, 0, error, executeQuery(query));
+    }
+}
diff --git a/exist-core/src/test/xquery/errors.xql b/exist-core/src/test/xquery/errors.xql
@@ -325,14 +325,50 @@ function et:issue3473c() {
     }
 };
 
-(: https://github.com/eXist-db/exist/issues/3474 :)
+(:~
+ : function type item returned in element content in sequence at runtime
+ : related to https://github.com/eXist-db/exist/issues/3474
+ :)
 declare
-    %test:pending("to be addressed in another PR")
-    %test:assertEquals(333)
-function et:issue3474() {
+    %test:assertTrue
+    %test:pending("location info still missing")
+function et:subexpression-in-enclosed-expression-evaluates-to-map() {
     try {
-        element foo { map { "x": "y" } }
-    } catch * {
-        $err:line-number
+        element test { 1, map {} }
+    }
+    catch err:XQTY0105 {
+        exists($err:line-number) and
+        $err:line-number > 0 and
+        exists($err:column-number) and
+        $err:column-number > 0
+    }
+};
+
+declare
+    %test:assertTrue
+    %test:pending("location info still missing")
+function et:enclosed-expression-evaluates-to-map() {
+    try {
+        element test { ( "a", map {} )[2] }
+    }
+    catch err:XQTY0105 {
+        exists($err:line-number) and
+        $err:line-number > 0 and
+        exists($err:column-number) and
+        $err:column-number > 0
     }
-};
+};
+
+declare
+    %test:assertTrue
+function et:array-in-enclosed-expression-evaluates-to-map() {
+    try {
+        element test { [map {}] }
+    }
+    catch err:XQTY0105 {
+        exists($err:line-number) and
+        $err:line-number > 0 and
+        exists($err:column-number) and
+        $err:column-number > 0
+    }
+};

Original file line number	Diff line number	Diff line change
`@@ -3157,8 +3157,11 @@ throws PermissionDeniedException, EXistException, XPathException`
`3157`	`3157`	`qnameExpr=expr [qnamePathExpr]`
`3158`	`3158`	`(`
`3159`	`3159`	`{ elementContent = new PathExpr(context); }`
`3160`		`- contentExpr=expr[elementContent]`
`3161`		`- { construct.addPath(elementContent); }`
	`3160`	`+ contentExpr=ec:expr[elementContent]`
	`3161`	`+ {`
	`3162`	`+ elementContent.setASTNode(ec);`
	`3163`	`+ construct.addPathIfNotFunction(elementContent);`
	`3164`	`+ }`
`3162`	`3165`	`)*`
`3163`	`3166`	`)`
`3164`	`3167`	`\|`