Skip to content

Commit

Permalink
Fixed issue #16.
Browse files Browse the repository at this point in the history
Made the prefix/postfix operator work correctly
  • Loading branch information
kohsuke committed Oct 28, 2014
1 parent 9247985 commit eec9c43
Show file tree
Hide file tree
Showing 3 changed files with 183 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.kohsuke.groovy.sandbox

import org.codehaus.groovy.ast.ASTNode
import org.codehaus.groovy.ast.ClassCodeExpressionTransformer
import org.codehaus.groovy.ast.ClassNode
import org.codehaus.groovy.ast.GroovyClassVisitor
Expand All @@ -14,6 +15,8 @@ import org.codehaus.groovy.ast.expr.Expression
import org.codehaus.groovy.ast.expr.ListExpression
import org.codehaus.groovy.ast.expr.MethodCallExpression
import org.codehaus.groovy.ast.expr.MethodPointerExpression
import org.codehaus.groovy.ast.expr.PostfixExpression
import org.codehaus.groovy.ast.expr.PrefixExpression
import org.codehaus.groovy.ast.expr.PropertyExpression
import org.codehaus.groovy.ast.expr.StaticMethodCallExpression
import org.codehaus.groovy.ast.expr.TupleExpression
Expand Down Expand Up @@ -396,9 +399,74 @@ class SandboxTransformer extends CompilationCustomizer {
}
}

if (exp instanceof PostfixExpression) {
return prefixPostfixExp(exp, exp.expression, exp.operation, "Postfix");
}
if (exp instanceof PrefixExpression) {
return prefixPostfixExp(exp, exp.expression, exp.operation, "Prefix");
}

return super.transform(exp)
}

private Expression prefixPostfixExp(Expression whole, Expression atom, Token opToken, String mode) {
String op = opToken.text=="++" ? "next" : "previous";

// a[b]++
if (atom instanceof BinaryExpression && atom.operation.type==Types.LEFT_SQUARE_BRACKET && interceptArray) {
return makeCheckedCall("checked${mode}Array", [
transform(atom.leftExpression),
transform(atom.rightExpression),
stringExp(op)
])
}

// a++
if (atom instanceof VariableExpression) {
if (isLocalVariable(atom.name)) {
if (mode=="Postfix") {
// a trick to rewrite a++ without introducing a new local variable
// a++ -> [a,a=a.next()][0]
return transform(new BinaryExpression(
new ListExpression([
atom,
new BinaryExpression(atom, ASSIGNMENT_OP,
new MethodCallExpression(atom,op,EMPTY_ARGUMENTS))
]),
new Token(Types.LEFT_SQUARE_BRACKET, "[", -1,-1),
new ConstantExpression(0)
));
} else {
// ++a -> a=a.next()
return transform(new BinaryExpression(atom,ASSIGNMENT_OP,
new MethodCallExpression(atom,op,EMPTY_ARGUMENTS))
);
}
} else {
// if the variable is not in-scope local variable, it gets treated as a property access with implicit this.
// see AsmClassGenerator.visitVariableExpression and processClassVariable.
PropertyExpression pexp = new PropertyExpression(VariableExpression.THIS_EXPRESSION, atom.name);
pexp.implicitThis = true;

atom = pexp;
// fall through to the "a.b++" case below
}
}

// a.b++
if (atom instanceof PropertyExpression && interceptProperty) {
return makeCheckedCall("checked${mode}Property", [
transformObjectExpression(atom),
atom.property,
boolExp(atom.safe),
boolExp(atom.spreadSafe),
stringExp(op)
]);
}

return whole;
}

/**
* See {@link #visitingClosureBody} for the details of what this method is about.
*/
Expand Down Expand Up @@ -433,6 +501,8 @@ class SandboxTransformer extends CompilationCustomizer {
}
}

static final Token ASSIGNMENT_OP = new Token(Types.ASSIGN, '=', -1, -1)

static final def checkerClass = new ClassNode(Checker.class)
static final def ScriptBytecodeAdapterClass = new ClassNode(ScriptBytecodeAdapter.class)

Expand Down
43 changes: 43 additions & 0 deletions src/main/java/org/kohsuke/groovy/sandbox/impl/Checker.java
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,49 @@ public Object call(Object receiver, String _, Object index, Object value) throws
}
}

/**
* a[i]++ / a[i]--
*
* @param op
* "next" for ++, "previous" for --. These names are defined by Groovy.
*/
public static Object checkedPostfixArray(Object r, Object i, String op) throws Throwable {
Object o = checkedGetArray(r, i);
Object n = checkedCall(o, false, false, op, new Object[0]);
checkedSetArray(r,i,Types.ASSIGN,n);
return o;
}

/**
* ++a[i] / --a[i]
*/
public static Object checkedPrefixArray(Object r, Object i, String op) throws Throwable {
Object o = checkedGetArray(r, i);
Object n = checkedCall(o, false, false, op, new Object[0]);
checkedSetArray(r,i,Types.ASSIGN,n);
return n;
}

/**
* a.x++ / a.x--
*/
public static Object checkedPostfixProperty(Object receiver, Object property, boolean safe, boolean spread, String op) throws Throwable {
Object o = checkedGetProperty(receiver, safe, spread, property);
Object n = checkedCall(o, false, false, op, new Object[0]);
checkedSetProperty(receiver, property, safe, spread, Types.ASSIGN, n);
return o;
}

/**
* ++a.x / --a.x
*/
public static Object checkedPrefixProperty(Object receiver, Object property, boolean safe, boolean spread, String op) throws Throwable {
Object o = checkedGetProperty(receiver, safe, spread, property);
Object n = checkedCall(o, false, false, op, new Object[0]);
checkedSetProperty(receiver, property, safe, spread, Types.ASSIGN, n);
return n;
}

/**
* Intercepts the binary expression of the form "lhs op rhs" like "lhs+rhs", "lhs>>rhs", etc.
*
Expand Down
70 changes: 70 additions & 0 deletions src/test/groovy/org/kohsuke/groovy/sandbox/TheTest.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -493,4 +493,74 @@ Script1\$_run_closure1.message
return x;
""")
}

// issue 16
void testPrePostfixLocalVariable() {
assertIntercept([
'Integer.next()',
'ArrayList[Integer]',
],[1,0],"""
def x = 0;
def y=x++;
return [x,y];
""")

assertIntercept([
'Integer.previous()'
],[2,2],"""
def x = 3;
def y=--x;
return [x,y];
""")
}

void testPrePostfixArray() {
assertIntercept([
'ArrayList[Integer]', // for reading x[1] before increment
'Integer.next()',
'ArrayList[Integer]=Integer', // for writing x[1] after increment
'ArrayList[Integer]', // for reading x[1] in the return statement
],[3,2],"""
def x = [1,2,3];
def y=x[1]++;
return [x[1],y];
""")

assertIntercept([
'ArrayList[Integer]', // for reading x[1] before increment
'Integer.previous()',
'ArrayList[Integer]=Integer', // for writing x[1] after increment
'ArrayList[Integer]', // for reading x[1] in the return statement
],[1,1],"""
def x = [1,2,3];
def y=--x[1];
return [x[1],y];
""")
}

void testPrePostfixProperty() {
assertIntercept([
'Script1.x=Integer', // x=3
'Script1.x',
'Integer.next()',
'Script1.x=Integer', // read, plus, then write back
'Script1.x' // final read for the return statement
],[4,3],"""
x = 3;
def y=x++;
return [x,y];
""")

assertIntercept([
'Script2.x=Integer', // x=3
'Script2.x',
'Integer.previous()',
'Script2.x=Integer', // read, plus, then write back
'Script2.x' // final read for the return statement
],[2,2],"""
x = 3;
def y=--x;
return [x,y];
""")
}
}

0 comments on commit eec9c43

Please sign in to comment.