Skip to content

Commit 7259411

Browse files
ghadishaybanstuarthalloway
ghadishayban
authored and
stuarthalloway
committed
CLJ-1793 - Clear 'this' before calls in tail position
The criteria for when a tail call is a safe point to clear 'this': 1) Must be in return position 2) Not in a try block (might need 'this' during catch/finally) 3) When not direct linked Signed-off-by: Stuart Halloway <[email protected]>
1 parent 21a7d51 commit 7259411

File tree

3 files changed

+108
-48
lines changed

3 files changed

+108
-48
lines changed

‎src/jvm/clojure/lang/Compiler.java‎

Lines changed: 77 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,8 @@ public class Compiler implements Opcodes{
226226
//null or not
227227
static final public Var IN_CATCH_FINALLY = Var.create(null).setDynamic();
228228

229+
static final public Var METHOD_RETURN_CONTEXT = Var.create(null).setDynamic();
230+
229231
static final public Var NO_RECUR = Var.create(null).setDynamic();
230232

231233
//DynamicClassLoader
@@ -373,6 +375,10 @@ static boolean isSpecial(Object sym){
373375
return specials.containsKey(sym);
374376
}
375377

378+
static boolean inTailCall(C context) {
379+
return (context == C.RETURN) && (METHOD_RETURN_CONTEXT.deref() != null) && (IN_CATCH_FINALLY.deref() == null);
380+
}
381+
376382
static Symbol resolveSymbol(Symbol sym){
377383
//already qualified or classname?
378384
if(sym.name.indexOf('.') > 0)
@@ -1005,12 +1011,13 @@ else if(instance != null && instance.hasJavaClass() && instance.getJavaClass() !
10051011
Symbol sym = (Symbol) RT.first(call);
10061012
Symbol tag = tagOf(form);
10071013
PersistentVector args = PersistentVector.EMPTY;
1014+
boolean tailPosition = inTailCall(context);
10081015
for(ISeq s = RT.next(call); s != null; s = s.next())
10091016
args = args.cons(analyze(context == C.EVAL ? context : C.EXPRESSION, s.first()));
10101017
if(c != null)
1011-
return new StaticMethodExpr(source, line, column, tag, c, munge(sym.name), args);
1018+
return new StaticMethodExpr(source, line, column, tag, c, munge(sym.name), args, tailPosition);
10121019
else
1013-
return new InstanceMethodExpr(source, line, column, tag, instance, munge(sym.name), args);
1020+
return new InstanceMethodExpr(source, line, column, tag, instance, munge(sym.name), args, tailPosition);
10141021
}
10151022
}
10161023
}
@@ -1440,13 +1447,15 @@ static class InstanceMethodExpr extends MethodExpr{
14401447
public final int line;
14411448
public final int column;
14421449
public final Symbol tag;
1450+
public final boolean tailPosition;
14431451
public final java.lang.reflect.Method method;
14441452

14451453
final static Method invokeInstanceMethodMethod =
14461454
Method.getMethod("Object invokeInstanceMethod(Object,String,Object[])");
14471455

14481456

1449-
public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr target, String methodName, IPersistentVector args)
1457+
public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr target,
1458+
String methodName, IPersistentVector args, boolean tailPosition)
14501459
{
14511460
this.source = source;
14521461
this.line = line;
@@ -1455,6 +1464,7 @@ public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr
14551464
this.methodName = methodName;
14561465
this.target = target;
14571466
this.tag = tag;
1467+
this.tailPosition = tailPosition;
14581468
if(target.hasJavaClass() && target.getJavaClass() != null)
14591469
{
14601470
List methods = Reflector.getMethods(target.getJavaClass(), args.count(), methodName, false);
@@ -1548,10 +1558,10 @@ public void emitUnboxed(C context, ObjExpr objx, GeneratorAdapter gen){
15481558
gen.checkCast(type);
15491559
MethodExpr.emitTypedArgs(objx, gen, method.getParameterTypes(), args);
15501560
gen.visitLineNumber(line, gen.mark());
1551-
if(context == C.RETURN)
1561+
if(tailPosition && !objx.canBeDirect)
15521562
{
15531563
ObjMethod method = (ObjMethod) METHOD.deref();
1554-
method.emitClearLocals(gen);
1564+
method.emitClearThis(gen);
15551565
}
15561566
Method m = new Method(methodName, Type.getReturnType(method), Type.getArgumentTypes(method));
15571567
if(method.getDeclaringClass().isInterface())
@@ -1622,12 +1632,14 @@ static class StaticMethodExpr extends MethodExpr{
16221632
public final int column;
16231633
public final java.lang.reflect.Method method;
16241634
public final Symbol tag;
1635+
public final boolean tailPosition;
16251636
final static Method forNameMethod = Method.getMethod("Class classForName(String)");
16261637
final static Method invokeStaticMethodMethod =
16271638
Method.getMethod("Object invokeStaticMethod(Class,String,Object[])");
16281639
final static Keyword warnOnBoxedKeyword = Keyword.intern("warn-on-boxed");
16291640

1630-
public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c, String methodName, IPersistentVector args)
1641+
public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c,
1642+
String methodName, IPersistentVector args, boolean tailPosition)
16311643
{
16321644
this.c = c;
16331645
this.methodName = methodName;
@@ -1636,6 +1648,7 @@ public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c
16361648
this.line = line;
16371649
this.column = column;
16381650
this.tag = tag;
1651+
this.tailPosition = tailPosition;
16391652

16401653
List methods = Reflector.getMethods(c, args.count(), methodName, true);
16411654
if(methods.isEmpty())
@@ -1774,10 +1787,10 @@ public void emit(C context, ObjExpr objx, GeneratorAdapter gen){
17741787
MethodExpr.emitTypedArgs(objx, gen, method.getParameterTypes(), args);
17751788
gen.visitLineNumber(line, gen.mark());
17761789
//Type type = Type.getObjectType(className.replace('.', '/'));
1777-
if(context == C.RETURN)
1790+
if(tailPosition && !objx.canBeDirect)
17781791
{
17791792
ObjMethod method = (ObjMethod) METHOD.deref();
1780-
method.emitClearLocals(gen);
1793+
method.emitClearThis(gen);
17811794
}
17821795
Type type = Type.getType(c);
17831796
Method m = new Method(methodName, Type.getReturnType(method), Type.getArgumentTypes(method));
@@ -2271,13 +2284,14 @@ public Expr parse(C context, Object frm) {
22712284
}
22722285
else
22732286
{
2274-
if(bodyExpr == null)
2275-
try {
2276-
Var.pushThreadBindings(RT.map(NO_RECUR, true));
2277-
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
2278-
} finally {
2279-
Var.popThreadBindings();
2280-
}
2287+
if(bodyExpr == null)
2288+
try {
2289+
Var.pushThreadBindings(RT.map(NO_RECUR, true, METHOD_RETURN_CONTEXT, null));
2290+
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
2291+
} finally {
2292+
Var.popThreadBindings();
2293+
}
2294+
22812295
if(Util.equals(op, CATCH))
22822296
{
22832297
Class c = HostExpr.maybeClass(RT.second(f), false);
@@ -2325,17 +2339,21 @@ public Expr parse(C context, Object frm) {
23252339
}
23262340
}
23272341
}
2328-
if(bodyExpr == null) {
2329-
try
2330-
{
2331-
Var.pushThreadBindings(RT.map(NO_RECUR, true));
2332-
bodyExpr = (new BodyExpr.Parser()).parse(C.EXPRESSION, RT.seq(body));
2333-
}
2334-
finally
2335-
{
2336-
Var.popThreadBindings();
2337-
}
2338-
}
2342+
if(bodyExpr == null)
2343+
{
2344+
// this codepath is hit when there is neither catch or finally, e.g. (try (expr))
2345+
// return a body expr directly
2346+
try
2347+
{
2348+
Var.pushThreadBindings(RT.map(NO_RECUR, true));
2349+
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
2350+
}
2351+
finally
2352+
{
2353+
Var.popThreadBindings();
2354+
}
2355+
return bodyExpr;
2356+
}
23392357

23402358
return new TryExpr(bodyExpr, catches, finallyExpr, retLocal,
23412359
finallyLocal);
@@ -2587,23 +2605,13 @@ public void emit(C context, ObjExpr objx, GeneratorAdapter gen){
25872605
gen.newInstance(type);
25882606
gen.dup();
25892607
MethodExpr.emitTypedArgs(objx, gen, ctor.getParameterTypes(), args);
2590-
if(context == C.RETURN)
2591-
{
2592-
ObjMethod method = (ObjMethod) METHOD.deref();
2593-
method.emitClearLocals(gen);
2594-
}
25952608
gen.invokeConstructor(type, new Method("<init>", Type.getConstructorDescriptor(ctor)));
25962609
}
25972610
else
25982611
{
25992612
gen.push(destubClassName(c.getName()));
26002613
gen.invokeStatic(RT_TYPE, forNameMethod);
26012614
MethodExpr.emitArgsAsArray(args, objx, gen);
2602-
if(context == C.RETURN)
2603-
{
2604-
ObjMethod method = (ObjMethod) METHOD.deref();
2605-
method.emitClearLocals(gen);
2606-
}
26072615
gen.invokeStatic(REFLECTOR_TYPE, invokeConstructorMethod);
26082616
}
26092617
if(context == C.STATEMENT)
@@ -3431,16 +3439,18 @@ static class StaticInvokeExpr implements Expr, MaybePrimitiveExpr{
34313439
public final Type[] paramtypes;
34323440
public final IPersistentVector args;
34333441
public final boolean variadic;
3442+
public final boolean tailPosition;
34343443
public final Object tag;
34353444

34363445
StaticInvokeExpr(Type target, Class retClass, Class[] paramclasses, Type[] paramtypes, boolean variadic,
3437-
IPersistentVector args,Object tag){
3446+
IPersistentVector args,Object tag, boolean tailPosition){
34383447
this.target = target;
34393448
this.retClass = retClass;
34403449
this.paramclasses = paramclasses;
34413450
this.paramtypes = paramtypes;
34423451
this.args = args;
34433452
this.variadic = variadic;
3453+
this.tailPosition = tailPosition;
34443454
this.tag = tag;
34453455
}
34463456

@@ -3497,14 +3507,20 @@ public void emitUnboxed(C context, ObjExpr objx, GeneratorAdapter gen){
34973507
else
34983508
MethodExpr.emitTypedArgs(objx, gen, paramclasses, args);
34993509

3510+
if(tailPosition && !objx.canBeDirect)
3511+
{
3512+
ObjMethod method = (ObjMethod) METHOD.deref();
3513+
method.emitClearThis(gen);
3514+
}
3515+
35003516
gen.invokeStatic(target, ms);
35013517
}
35023518

35033519
private Type getReturnType(){
35043520
return Type.getType(retClass);
35053521
}
35063522

3507-
public static Expr parse(Var v, ISeq args, Object tag) {
3523+
public static Expr parse(Var v, ISeq args, Object tag, boolean tailPosition) {
35083524
if(!v.isBound() || v.get() == null)
35093525
{
35103526
// System.out.println("Not bound: " + v);
@@ -3560,7 +3576,7 @@ else if(argcount > params.length
35603576
for(ISeq s = RT.seq(args); s != null; s = s.next())
35613577
argv = argv.cons(analyze(C.EXPRESSION, s.first()));
35623578

3563-
return new StaticInvokeExpr(target,retClass,paramClasses, paramTypes,variadic, argv, tag);
3579+
return new StaticInvokeExpr(target,retClass,paramClasses, paramTypes,variadic, argv, tag, tailPosition);
35643580
}
35653581

35663582
}
@@ -3571,6 +3587,7 @@ static class InvokeExpr implements Expr{
35713587
public final IPersistentVector args;
35723588
public final int line;
35733589
public final int column;
3590+
public final boolean tailPosition;
35743591
public final String source;
35753592
public boolean isProtocol = false;
35763593
public boolean isDirect = false;
@@ -3593,12 +3610,14 @@ static Object sigTag(int argcount, Var v){
35933610
return null;
35943611
}
35953612

3596-
public InvokeExpr(String source, int line, int column, Symbol tag, Expr fexpr, IPersistentVector args) {
3613+
public InvokeExpr(String source, int line, int column, Symbol tag, Expr fexpr, IPersistentVector args, boolean tailPosition) {
35973614
this.source = source;
35983615
this.fexpr = fexpr;
35993616
this.args = args;
36003617
this.line = line;
36013618
this.column = column;
3619+
this.tailPosition = tailPosition;
3620+
36023621
if(fexpr instanceof VarExpr)
36033622
{
36043623
Var fvar = ((VarExpr)fexpr).var;
@@ -3743,10 +3762,10 @@ void emitArgsAndCall(int firstArgToEmit, C context, ObjExpr objx, GeneratorAdapt
37433762
}
37443763
gen.visitLineNumber(line, gen.mark());
37453764

3746-
if(context == C.RETURN)
3765+
if(tailPosition && !objx.canBeDirect)
37473766
{
37483767
ObjMethod method = (ObjMethod) METHOD.deref();
3749-
method.emitClearLocals(gen);
3768+
method.emitClearThis(gen);
37503769
}
37513770

37523771
gen.invokeInterface(IFN_TYPE, new Method("invoke", OBJECT_TYPE, ARG_TYPES[Math.min(MAX_POSITIONAL_ARITY + 1,
@@ -3762,6 +3781,7 @@ public Class getJavaClass() {
37623781
}
37633782

37643783
static public Expr parse(C context, ISeq form) {
3784+
boolean tailPosition = inTailCall(context);
37653785
if(context != C.EVAL)
37663786
context = C.EXPRESSION;
37673787
Expr fexpr = analyze(context, form.first());
@@ -3791,7 +3811,7 @@ static public Expr parse(C context, ISeq form) {
37913811
Object sigtag = sigTag(arity, v);
37923812
Object vtag = RT.get(RT.meta(v), RT.TAG_KEY);
37933813
Expr ret = StaticInvokeExpr
3794-
.parse(v, RT.next(form), formtag != null ? formtag : sigtag != null ? sigtag : vtag);
3814+
.parse(v, RT.next(form), formtag != null ? formtag : sigtag != null ? sigtag : vtag, tailPosition);
37953815
if(ret != null)
37963816
{
37973817
// System.out.println("invoke direct: " + v);
@@ -3838,7 +3858,7 @@ static public Expr parse(C context, ISeq form) {
38383858
// throw new IllegalArgumentException(
38393859
// String.format("No more than %d args supported", MAX_POSITIONAL_ARITY));
38403860

3841-
return new InvokeExpr((String) SOURCE.deref(), lineDeref(), columnDeref(), tagOf(form), fexpr, args);
3861+
return new InvokeExpr((String) SOURCE.deref(), lineDeref(), columnDeref(), tagOf(form), fexpr, args, tailPosition);
38423862
}
38433863
}
38443864

@@ -5296,6 +5316,7 @@ static FnMethod parse(ObjExpr objx, ISeq form, Object rettag) {
52965316
,CLEAR_PATH, pnode
52975317
,CLEAR_ROOT, pnode
52985318
,CLEAR_SITES, PersistentHashMap.EMPTY
5319+
,METHOD_RETURN_CONTEXT, RT.T
52995320
));
53005321

53015322
method.prim = primInterface(parms);
@@ -5873,6 +5894,11 @@ void emitClearLocalsOld(GeneratorAdapter gen){
58735894
}
58745895
}
58755896
}
5897+
5898+
void emitClearThis(GeneratorAdapter gen) {
5899+
gen.visitInsn(Opcodes.ACONST_NULL);
5900+
gen.visitVarInsn(Opcodes.ASTORE, 0);
5901+
}
58765902
}
58775903

58785904
public static class LocalBinding{
@@ -6300,14 +6326,14 @@ public Expr parse(C context, Object frm) {
63006326
{
63016327
if(recurMismatches != null && RT.booleanCast(recurMismatches.nth(i/2)))
63026328
{
6303-
init = new StaticMethodExpr("", 0, 0, null, RT.class, "box", RT.vector(init));
6329+
init = new StaticMethodExpr("", 0, 0, null, RT.class, "box", RT.vector(init), false);
63046330
if(RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))
63056331
RT.errPrintWriter().println("Auto-boxing loop arg: " + sym);
63066332
}
63076333
else if(maybePrimitiveType(init) == int.class)
6308-
init = new StaticMethodExpr("", 0, 0, null, RT.class, "longCast", RT.vector(init));
6334+
init = new StaticMethodExpr("", 0, 0, null, RT.class, "longCast", RT.vector(init), false);
63096335
else if(maybePrimitiveType(init) == float.class)
6310-
init = new StaticMethodExpr("", 0, 0, null, RT.class, "doubleCast", RT.vector(init));
6336+
init = new StaticMethodExpr("", 0, 0, null, RT.class, "doubleCast", RT.vector(init), false);
63116337
}
63126338
//sequential enhancement of env (like Lisp let*)
63136339
try
@@ -6339,10 +6365,12 @@ else if(maybePrimitiveType(init) == float.class)
63396365
try {
63406366
if(isLoop)
63416367
{
6368+
Object methodReturnContext = context == C.RETURN ? METHOD_RETURN_CONTEXT.deref() : null;
63426369
Var.pushThreadBindings(
63436370
RT.map(CLEAR_PATH, clearpath,
63446371
CLEAR_ROOT, clearroot,
6345-
NO_RECUR, null));
6372+
NO_RECUR, null,
6373+
METHOD_RETURN_CONTEXT, methodReturnContext));
63466374

63476375
}
63486376
bodyExpr = (new BodyExpr.Parser()).parse(isLoop ? C.RETURN : context, body);
@@ -8247,6 +8275,7 @@ static NewInstanceMethod parse(ObjExpr objx, ISeq form, Symbol thistag,
82478275
,CLEAR_PATH, pnode
82488276
,CLEAR_ROOT, pnode
82498277
,CLEAR_SITES, PersistentHashMap.EMPTY
8278+
,METHOD_RETURN_CONTEXT, RT.T
82508279
));
82518280

82528281
//register 'this' as local 0

‎test/clojure/test_clojure/compilation.clj‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,33 @@
354354
;; throws an exception on failure
355355
(is (eval `(fn [] ~(CLJ1399. 1)))))
356356

357+
(deftest CLJ-1250-this-clearing
358+
(testing "clearing during try/catch/finally"
359+
(let [closed-over-in-catch (let [x :foo]
360+
(fn []
361+
(try
362+
(throw (Exception. "boom"))
363+
(catch Exception e
364+
x)))) ;; x should remain accessible to the fn
365+
366+
a (atom nil)
367+
closed-over-in-finally (fn []
368+
(try
369+
:ret
370+
(finally
371+
(reset! a :run))))]
372+
(is (= :foo (closed-over-in-catch)))
373+
(is (= :ret (closed-over-in-finally)))
374+
(is (= :run @a))))
375+
(testing "no clearing when loop not in return context"
376+
(let [x (atom 5)
377+
bad (fn []
378+
(loop [] (System/getProperties))
379+
(swap! x dec)
380+
(when (pos? @x)
381+
(recur)))]
382+
(is (nil? (bad))))))
383+
357384
(deftest CLJ-1586-lazyseq-literals-preserve-metadata
358385
(should-not-reflect (eval (list '.substring (with-meta (concat '(identity) '("foo")) {:tag 'String}) 0))))
359386

0 commit comments

Comments
 (0)