This is an automated email from the ASF dual-hosted git repository.
henrib pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-jexl.git
The following commit(s) were added to refs/heads/master by this push:
new 669ecfe JEXL-278: added logic to better capture ambiguous statements
and a method to attempt cleaning them from the source
669ecfe is described below
commit 669ecfed0b22031d6e617c408e168a859f802628
Author: henrib <hen...@apache.org>
AuthorDate: Thu Oct 25 22:15:12 2018 +0200
JEXL-278: added logic to better capture ambiguous statements and a method
to attempt cleaning them from the source
---
RELEASE-NOTES.txt | 1 +
.../java/org/apache/commons/jexl3/JexlEngine.java | 22 +-----
.../org/apache/commons/jexl3/JexlException.java | 75 ++++++++++++++++++-
.../java/org/apache/commons/jexl3/JexlInfo.java | 32 +++++++-
.../org/apache/commons/jexl3/internal/Scope.java | 22 ++++--
.../apache/commons/jexl3/parser/ASTAmbiguous.java | 34 +++++++++
.../apache/commons/jexl3/parser/JexlParser.java | 87 ++++++++++++++++------
.../org/apache/commons/jexl3/parser/Parser.jjt | 4 +-
src/site/xdoc/changes.xml | 3 +
.../org/apache/commons/jexl3/Issues200Test.java | 35 ++++++++-
.../java/org/apache/commons/jexl3/LambdaTest.java | 12 ++-
.../apache/commons/jexl3/parser/ParserTest.java | 2 +
12 files changed, 272 insertions(+), 57 deletions(-)
diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
index 334cab7..9ef3f8b 100644
--- a/RELEASE-NOTES.txt
+++ b/RELEASE-NOTES.txt
@@ -60,6 +60,7 @@ New Features in 3.2:
Bugs Fixed in 3.2:
==================
+* JEXL-278: Ambiguous exceptions should point to actual statement
ambiguity
* JEXL-272: Dereferencing null property not reported on method call
* JEXL-271: Hoisted variable is lost when currying lambda
* JEXL-270: Wrong Script$Curried creation when script.curry() method is
called inside script
diff --git a/src/main/java/org/apache/commons/jexl3/JexlEngine.java
b/src/main/java/org/apache/commons/jexl3/JexlEngine.java
index 06202f0..b796bb8 100644
--- a/src/main/java/org/apache/commons/jexl3/JexlEngine.java
+++ b/src/main/java/org/apache/commons/jexl3/JexlEngine.java
@@ -579,27 +579,7 @@ public abstract class JexlEngine {
* @return a JexlInfo instance
*/
public JexlInfo createInfo() {
- JexlInfo info = null;
- StackTraceElement[] stack = new Throwable().getStackTrace();
- StackTraceElement se = null;
- String name = getClass().getName();
- for (int s = 1; s < stack.length; ++s) {
- se = stack[s];
- String className = se.getClassName();
- if (!className.equals(name)) {
- // go deeper if called from jexl implementation classes
- if (className.startsWith("org.apache.commons.jexl3.internal.")
- || className.startsWith("org.apache.commons.jexl3.J")) {
- name = className;
- } else {
- break;
- }
- }
- }
- if (se != null) {
- info = createInfo(se.getClassName() + "." + se.getMethodName(),
se.getLineNumber(), 0);
- }
- return info;
+ return new JexlInfo();
}
/**
diff --git a/src/main/java/org/apache/commons/jexl3/JexlException.java
b/src/main/java/org/apache/commons/jexl3/JexlException.java
index 856d154..f82d9cf 100644
--- a/src/main/java/org/apache/commons/jexl3/JexlException.java
+++ b/src/main/java/org/apache/commons/jexl3/JexlException.java
@@ -29,6 +29,9 @@ import java.lang.reflect.UndeclaredThrowableException;
import java.util.ArrayList;
import java.util.List;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.StringReader;
/**
* Wraps any error that might occur during interpretation of a script or
expression.
*
@@ -232,6 +235,14 @@ public class JexlException extends RuntimeException {
+ expr.substring(begin, end > length ? length : end) + "
...'";
}
}
+
+ /**
+ * Pleasing checkstyle.
+ * @return the info
+ */
+ protected JexlInfo info() {
+ return info;
+ }
/**
* Thrown when tokenization fails.
@@ -306,19 +317,79 @@ public class JexlException extends RuntimeException {
* @since 3.0
*/
public static class Ambiguous extends Parsing {
+ /** The mark at which ambiguity might stop and recover. */
+ private JexlInfo recover = null;
/**
* Creates a new Ambiguous statement exception instance.
* @param info the location information
* @param expr the source expression line
*/
public Ambiguous(JexlInfo info, String expr) {
- super(info, expr);
+ this(info, null, expr);
}
-
+
+ /**
+ * Creates a new Ambiguous statement exception instance.
+ * @param begin the start location information
+ * @param end the end location information
+ * @param expr the source expression line
+ */
+ public Ambiguous(JexlInfo begin, JexlInfo end, String expr) {
+ super(begin, expr);
+ recover = end;
+ }
+
@Override
protected String detailedMessage() {
return parserError("ambiguous statement", getDetail());
}
+
+ /**
+ * Tries to remove this ambiguity in the source.
+ * @param src the source that triggered this exception
+ * @return the source with the ambiguous statement removed
+ * or null if no recovery was possible
+ */
+ public String tryCleanSource(String src) {
+ JexlInfo ji = info();
+ return ji == null || recover == null
+ ? src
+ : sliceSource(src, ji.getLine(), ji.getColumn(),
recover.getLine(), recover.getColumn());
+ }
+ }
+
+ /**
+ * Removes a slice from a source.
+ * @param src the source
+ * @param froml the begin line
+ * @param fromc the begin column
+ * @param tol the to line
+ * @param toc the to column
+ * @return the source with the (begin) to (to) zone removed
+ */
+ public static String sliceSource(String src, int froml, int fromc, int
tol, int toc) {
+ BufferedReader reader = new BufferedReader(new StringReader(src));
+ StringBuilder buffer = new StringBuilder();
+ String line;
+ int cl = 1;
+ try {
+ while ((line = reader.readLine()) != null) {
+ if (cl < froml || cl > tol) {
+ buffer.append(line).append('\n');
+ } else {
+ if (cl == froml) {
+ buffer.append(line.substring(0, fromc - 1));
+ }
+ if (cl == tol) {
+ buffer.append(line.substring(toc));
+ }
+ } // else ignore line
+ cl += 1;
+ }
+ } catch (IOException xignore) {
+ //damn the checked exceptions :-)
+ }
+ return buffer.toString();
}
/**
diff --git a/src/main/java/org/apache/commons/jexl3/JexlInfo.java
b/src/main/java/org/apache/commons/jexl3/JexlInfo.java
index b105c98..c0abe07 100644
--- a/src/main/java/org/apache/commons/jexl3/JexlInfo.java
+++ b/src/main/java/org/apache/commons/jexl3/JexlInfo.java
@@ -22,7 +22,7 @@ package org.apache.commons.jexl3;
* debugging information reporting.
*/
public class JexlInfo {
-
+
/** line number. */
private final int line;
@@ -73,6 +73,36 @@ public class JexlInfo {
line = l;
column = c;
}
+
+ /**
+ * Create an information structure for dynamic set/get/invoke/new.
+ * <p>This gathers the class, method and line number of the first calling
method
+ * outside of o.a.c.jexl3.</p>
+ */
+ public JexlInfo() {
+ StackTraceElement[] stack = new Throwable().getStackTrace();
+ String cname = getClass().getName();
+ String pkgname = getClass().getPackage().getName();
+ StackTraceElement se = null;
+ for (int s = 1; s < stack.length; ++s) {
+ se = stack[s];
+ String className = se.getClassName();
+ if (!className.equals(cname)) {
+ // go deeper if called from jexl implementation classes
+ if (className.startsWith(pkgname + ".internal.")
+ || className.startsWith(pkgname + ".Jexl")) {
+ cname = className;
+ } else {
+ break;
+ }
+ }
+ }
+ this.name = se != null
+ ? se.getClassName() + "." + se.getMethodName() +":" +
se.getLineNumber()
+ : "?";
+ this.line = 0;
+ this.column = 0;
+ }
/**
* Creates info reusing the name.
diff --git a/src/main/java/org/apache/commons/jexl3/internal/Scope.java
b/src/main/java/org/apache/commons/jexl3/internal/Scope.java
index a0d52ab..380cd4c 100644
--- a/src/main/java/org/apache/commons/jexl3/internal/Scope.java
+++ b/src/main/java/org/apache/commons/jexl3/internal/Scope.java
@@ -121,7 +121,7 @@ public final class Scope {
if (namedVariables == null) {
namedVariables = new LinkedHashMap<String, Integer>();
}
- register = Integer.valueOf(namedVariables.size());
+ register = namedVariables.size();
namedVariables.put(name, register);
hoistedVariables.put(register, pr);
}
@@ -153,7 +153,7 @@ public final class Scope {
}
Integer register = namedVariables.get(name);
if (register == null) {
- register = Integer.valueOf(namedVariables.size());
+ register = namedVariables.size();
namedVariables.put(name, register);
parms += 1;
}
@@ -173,9 +173,19 @@ public final class Scope {
}
Integer register = namedVariables.get(name);
if (register == null) {
- register = Integer.valueOf(namedVariables.size());
+ register = namedVariables.size();
namedVariables.put(name, register);
vars += 1;
+// // check if local is redefining hoisted
+// if (parent != null) {
+// Integer pr = parent.getSymbol(name, true);
+// if (pr != null) {
+// if (hoistedVariables == null) {
+// hoistedVariables = new LinkedHashMap<Integer,
Integer>();
+// }
+// hoistedVariables.put(register, pr);
+// }
+// }
}
return register;
}
@@ -193,8 +203,8 @@ public final class Scope {
for (Map.Entry<Integer, Integer> hoist :
hoistedVariables.entrySet()) {
Integer target = hoist.getKey();
Integer source = hoist.getValue();
- Object arg = frame.get(source.intValue());
- arguments[target.intValue()] = arg;
+ Object arg = frame.get(source);
+ arguments[target] = arg;
}
}
return new Frame(this, arguments, 0);
@@ -275,7 +285,7 @@ public final class Scope {
String[] pa = new String[parms - (hoistedVariables == null? 0 :
hoistedVariables.size())];
int p = 0;
for (Map.Entry<String, Integer> entry : namedVariables.entrySet())
{
- int symnum = entry.getValue().intValue();
+ int symnum = entry.getValue();
if (symnum >= parms && (hoistedVariables == null ||
!hoistedVariables.containsKey(symnum))) {
pa[p++] = entry.getKey();
}
diff --git a/src/main/java/org/apache/commons/jexl3/parser/ASTAmbiguous.java
b/src/main/java/org/apache/commons/jexl3/parser/ASTAmbiguous.java
new file mode 100644
index 0000000..7bbbb54
--- /dev/null
+++ b/src/main/java/org/apache/commons/jexl3/parser/ASTAmbiguous.java
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.jexl3.parser;
+
+public final class ASTAmbiguous extends JexlNode {
+
+ ASTAmbiguous(int id) {
+ super(id);
+ }
+
+ ASTAmbiguous(Parser p, int id) {
+ super(p, id);
+ }
+
+ @Override
+ public Object jjtAccept(ParserVisitor visitor, Object data) {
+ return visitor.visit(this, data);
+ }
+}
+
diff --git a/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java
b/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java
index 188626e..5c02a70 100644
--- a/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java
+++ b/src/main/java/org/apache/commons/jexl3/parser/JexlParser.java
@@ -33,7 +33,8 @@ import java.util.Map;
import java.util.Set;
import java.util.Stack;
import java.util.TreeMap;
-
+import static org.apache.commons.jexl3.parser.ParserConstants.EOF;
+import static org.apache.commons.jexl3.parser.ParserConstants.SEMICOL;
/**
* The base class for parsing, manages the parameter/local variable frame.
@@ -44,6 +45,10 @@ public abstract class JexlParser extends StringParser {
*/
protected final FeatureController featureController = new
FeatureController(JexlEngine.DEFAULT_FEATURES);
/**
+ * The basic source info.
+ */
+ protected JexlInfo info = null;
+ /**
* The source being processed.
*/
protected String source = null;
@@ -82,8 +87,8 @@ public abstract class JexlParser extends StringParser {
}
/**
- * Reading a given source line
- * @parma src the source
+ * Read a given source line.
+ * @param src the source
* @param lineno the line number
* @return the line
*/
@@ -101,9 +106,10 @@ public abstract class JexlParser extends StringParser {
}
return msg;
}
-
+
/**
* Internal, for debug purpose only.
+ * @param registers whether register syntax is recognized by this parser
*/
public void allowRegisters(boolean registers) {
featureController.setFeatures(new
JexlFeatures(featureController.getFeatures()).register(registers));
@@ -260,9 +266,37 @@ public abstract class JexlParser extends StringParser {
*/
protected abstract Token getToken(int index);
+ /**
+ * Overridden in actual parser to access tokens stack.
+ * @return the next token on the stack
+ */
+ protected abstract Token getNextToken();
+
+ /**
+ * Throws Ambiguous exception.
+ * <p>It seeks a ';' (or EOF) to recover.
+ * @param begin the first token in ambiguous expression
+ */
+ protected void throwAmbiguousException(Token begin) {
+ Token pt = null, end = null;
+ while((end = getNextToken()) != null) {
+ if (end.kind == EOF || end.kind == SEMICOL) {
+ if (pt != null) {
+ end = pt;
+ }
+ break;
+ }
+ pt = end;
+ }
+ JexlInfo infob = info.at(begin.beginLine, begin.beginColumn);
+ JexlInfo infoe = end != null? info.at(end.endLine, end.endColumn) :
null;
+ String msg = readSourceLine(source, begin.beginLine);
+ throw new JexlException.Ambiguous(infob, infoe, msg);
+ }
+
protected void jjtreeOpenNodeScope(JexlNode node) {
if (node instanceof ASTAmbiguous) {
- throwParsingException(JexlException.Ambiguous.class, node);
+ throwAmbiguousException(getToken(1));
}
}
@@ -304,7 +338,7 @@ public abstract class JexlParser extends StringParser {
} else if (ASSIGN_NODES.contains(node.getClass())) {
JexlNode lv = node.jjtGetChild(0);
if (!lv.isLeftValue()) {
- throwParsingException(JexlException.Assignment.class, lv);
+ throwParsingException(JexlException.Assignment.class, null);
}
}
// heavy check
@@ -334,8 +368,8 @@ public abstract class JexlParser extends StringParser {
throw new JexlException.Parsing(null,
JexlFeatures.stringify(feature));
}
}
- JexlInfo info = new JexlInfo(token.image, token.beginLine,
token.beginColumn);
- throwFeatureException(feature, info);
+ JexlInfo xinfo = info.at(token.beginLine, token.beginColumn);
+ throwFeatureException(feature, xinfo);
}
/**
@@ -343,30 +377,35 @@ public abstract class JexlParser extends StringParser {
* @param node the node that caused it
*/
protected void throwParsingException(JexlNode node) {
- throwParsingException(null, node);
+ throwParsingException(null, null);
}
/**
- * Throws a parsing exception.
+ * Creates a parsing exception.
* @param xclazz the class of exception
- * @param node the node that caused it
+ * @param tok the token to report
+ * @param <T> the parsing exception subclass
*/
- protected void throwParsingException(Class<? extends JexlException>
xclazz, JexlNode node) {
- Token tok = this.getToken(0);
+ protected <T extends JexlException.Parsing> void
throwParsingException(Class<T> xclazz, Token tok) {
+ JexlInfo xinfo = null;
+ String msg = "unrecoverable state";
+ JexlException.Parsing xparse = null;
if (tok == null) {
- throw new JexlException.Parsing(null, "unrecoverable state");
+ tok = this.getToken(0);
}
- JexlInfo dbgInfo = new JexlInfo(tok.image, tok.beginLine,
tok.beginColumn);
- String msg = readSourceLine(source, tok.beginLine);
- JexlException xjexl = null;
- if (xclazz != null) {
- try {
- Constructor<? extends JexlException> ctor =
xclazz.getConstructor(JexlInfo.class, String.class);
- xjexl = ctor.newInstance(dbgInfo, msg);
- } catch (Exception xany) {
- // ignore, very unlikely but then again..
+ if (tok != null) {
+ xinfo = info.at(tok.beginLine, tok.beginColumn);
+ msg = readSourceLine(source, tok.beginLine);
+ if (xclazz != null) {
+ try {
+ Constructor<T> ctor =
xclazz.getConstructor(JexlInfo.class, String.class);
+ xparse = ctor.newInstance(xinfo, msg);
+ } catch (Exception xany) {
+ // ignore, very unlikely but then again..
+ }
}
}
- throw xjexl != null ? xjexl : new JexlException.Parsing(dbgInfo, msg);
+ // unlikely but safe
+ throw xparse != null ? xparse : new JexlException.Parsing(xinfo, msg);
}
}
diff --git a/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt
b/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt
index b7bfd27..1ec783c 100644
--- a/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt
+++ b/src/main/java/org/apache/commons/jexl3/parser/Parser.jjt
@@ -46,7 +46,7 @@ public final class Parser extends JexlParser
{
private int loopCount = 0;
- public ASTJexlScript parse(JexlInfo info, JexlFeatures jexlFeatures,
String jexlSrc, Scope scope) {
+ public ASTJexlScript parse(JexlInfo jexlInfo, JexlFeatures jexlFeatures,
String jexlSrc, Scope scope) {
JexlFeatures previous = getFeatures();
try {
setFeatures(jexlFeatures);
@@ -55,6 +55,7 @@ public final class Parser extends JexlParser
token_source.defaultLexState = REGISTERS;
}
// lets do the 'Unique Init' in here to be safe - it's a pain to
remember
+ info = jexlInfo != null? jexlInfo : new JexlInfo();
source = jexlSrc;
pragmas = null;
frame = scope;
@@ -71,6 +72,7 @@ public final class Parser extends JexlParser
} catch (ParseException xparse) {
throw new JexlException.Parsing(info, xparse).clean();
} finally {
+ info = null;
source = null;
frame = null;
token_source.defaultLexState = DEFAULT;
diff --git a/src/site/xdoc/changes.xml b/src/site/xdoc/changes.xml
index 7156f73..e2316d4 100644
--- a/src/site/xdoc/changes.xml
+++ b/src/site/xdoc/changes.xml
@@ -26,6 +26,9 @@
</properties>
<body>
<release version="3.2" date="unreleased">
+ <action dev="henrib" type="fix" issue="JEXL-278">
+ Ambiguous exceptions should point to actual statement ambiguity
+ </action>
<action dev="henrib" type="add" issue="JEXL-275">
Allow safe navigation as option
</action>
diff --git a/src/test/java/org/apache/commons/jexl3/Issues200Test.java
b/src/test/java/org/apache/commons/jexl3/Issues200Test.java
index ea5a84e..63a33bf 100644
--- a/src/test/java/org/apache/commons/jexl3/Issues200Test.java
+++ b/src/test/java/org/apache/commons/jexl3/Issues200Test.java
@@ -16,6 +16,7 @@
*/
package org.apache.commons.jexl3;
+
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@@ -384,7 +385,7 @@ public class Issues200Test extends JexlTestCase {
result = script.execute(ctx);
Assert.assertEquals(10, result);
}
-
+
@Test
public void test230() throws Exception {
JexlEngine jexl = new JexlBuilder().cache(4).create();
@@ -595,5 +596,37 @@ public class Issues200Test extends JexlTestCase {
String sxs = xstack.toString();
Assert.assertTrue(sxs.contains("jvm"));
}
+ }
+
+ @Test
+ public void test278() throws Exception {
+ String[] srcs = new String[]{
+ "return union x('arg',5,6) ",
+ "return union 143('arg',5,6) ;",
+ "return union\n 143('arg',5,6) ;",
+ "var f =()->{ return union 143; } foo[0]"
+ };
+ Object[] ctls = new Object[]{
+ "42","42","42", 42
+ };
+ JexlEngine jexl = new JexlBuilder().cache(4).create();
+ JexlContext ctxt = new MapContext();
+ int[] foo = {42};
+ ctxt.set("foo", foo);
+ ctxt.set("union", "42");
+ Object value;
+ JexlScript jc;
+ for(int i = 0; i < srcs.length; ++i) {
+ String src = srcs[i];
+ try {
+ jc = jexl.createScript(src);
+ Assert.fail("should have failed, " + (jc != null));
+ } catch(JexlException.Ambiguous xa) {
+ src = xa.tryCleanSource(src);
+ }
+ jc = jexl.createScript(src);
+ value = jc.execute(ctxt);
+ Assert.assertEquals(src, ctls[i], value);
+ }
}
}
diff --git a/src/test/java/org/apache/commons/jexl3/LambdaTest.java
b/src/test/java/org/apache/commons/jexl3/LambdaTest.java
index 78bb49d..930377c 100644
--- a/src/test/java/org/apache/commons/jexl3/LambdaTest.java
+++ b/src/test/java/org/apache/commons/jexl3/LambdaTest.java
@@ -146,7 +146,7 @@ public class LambdaTest extends JexlTestCase {
}
@Test
- public void testHoistLambada() throws Exception {
+ public void testHoistLambda() throws Exception {
JexlEngine jexl = createEngine();
JexlContext ctx = null;
JexlScript s42;
@@ -343,4 +343,14 @@ public class LambdaTest extends JexlTestCase {
Object result = y.execute(null, 2, 3);
Assert.assertEquals(8, result);
}
+
+ // redefining an hoisted var is not resolved correctly in left hand side;
+ // declare the var in local frame, resolved in local frame instead of
parent
+// @Test
+// public void test271e() throws Exception {
+// JexlEngine jexl = createEngine();
+// JexlScript base = jexl.createScript("var base = 1000; var f = (x,
y)->{ var base = x + y + (base?:-1000); base; }; f(100, 20)");
+// Object result = base.execute(null);
+// Assert.assertEquals(-880, result);
+// }
}
diff --git a/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java
b/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java
index 8a038fa..0c66c16 100644
--- a/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java
+++ b/src/test/java/org/apache/commons/jexl3/parser/ParserTest.java
@@ -17,9 +17,11 @@
package org.apache.commons.jexl3.parser;
import java.io.StringReader;
+import org.apache.commons.jexl3.JexlEngine;
import org.apache.commons.jexl3.JexlException;
import org.apache.commons.jexl3.JexlFeatures;
+import org.apache.commons.logging.LogFactory;
import org.junit.Assert;
import org.junit.Test;
