Skip to content

Commit db95c68

Browse files
aardvark179andrea.bergia
aardvark179
and
andrea.bergia
authored
Disambiguate local variable names to fix debugger issues
* Disambiguate local variable names to fix debugger issues. * Refactor debugger interface to have default implementations * Add javadoc and comments about when and why vars are referenced by name. --------- Co-authored-by: andrea.bergia <[email protected]>
1 parent 9eacb3a commit db95c68

File tree

7 files changed

+102
-22
lines changed

7 files changed

+102
-22
lines changed

‎rhino/src/main/java/org/mozilla/javascript/CodeGenUtils.java‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package org.mozilla.javascript;
22

3+
import java.util.HashMap;
4+
import java.util.concurrent.atomic.AtomicInteger;
35
import org.mozilla.javascript.ast.AstNode;
46
import org.mozilla.javascript.ast.AstRoot;
57
import org.mozilla.javascript.ast.Block;
@@ -101,7 +103,8 @@ private static void fillInTopLevelCommon(
101103
}
102104

103105
private static void fillInCommon(JSDescriptor.Builder builder, ScriptNode scriptOrFn) {
104-
builder.paramAndVarNames = scriptOrFn.getParamAndVarNames();
106+
builder.paramAndVarNames =
107+
disambiguateNames(scriptOrFn.getParamAndVarNames(), scriptOrFn.getParamCount());
105108
builder.paramCount = scriptOrFn.getParamCount();
106109
builder.paramIsConst = scriptOrFn.getParamAndVarConst();
107110
builder.paramAndVarCount = scriptOrFn.getParamAndVarCount();
@@ -130,4 +133,31 @@ public static <T extends ScriptOrFn<T>> void setConstructor(
130133
builder.constructor = new JSCode.NullBuilder<T>();
131134
}
132135
}
136+
137+
/**
138+
* Disambiguate all variable names after the parameters. We avoid disambiguating the parameters
139+
* themselves because those names can be repeated and are not unique even if an activation frame
140+
* is used.
141+
*
142+
* <p>This is only required when an interpreted function might have duplicate local variable
143+
* names and might be run under the debugger. This can only occur if the function has nested
144+
* blocks, uses `let` with a variable name in that nested block which is also used outside that
145+
* block, and does not require the use of an activation frame for any other reason. When running
146+
* under the debugger variables are accessed by name rather than by index, and hence those names
147+
* must not include duplicates.
148+
*/
149+
private static String[] disambiguateNames(String[] names, int paramCount) {
150+
String[] result = new String[names.length];
151+
var checkMap = new HashMap<String, AtomicInteger>();
152+
for (int i = 0; i < names.length; i++) {
153+
var counter = checkMap.computeIfAbsent(names[i], k -> new AtomicInteger());
154+
var count = counter.getAndIncrement();
155+
if (i >= paramCount && count > 0) {
156+
result[i] = String.format("%s(%d)", names[i], count);
157+
} else {
158+
result[i] = names[i];
159+
}
160+
}
161+
return result;
162+
}
133163
}

‎rhino/src/main/java/org/mozilla/javascript/Interpreter.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3787,6 +3787,9 @@ NewState execute(Context cx, CallFrame frame, InterpreterState state, int op) {
37873787
varDbls[state.indexReg] = frame.sDbl[state.stackTop];
37883788
}
37893789
} else {
3790+
// This only occurs if we are using the debugger, in
3791+
// which case `frame.scope` will be an activation
3792+
// frame for this function.
37903793
Object val = frame.stack[state.stackTop];
37913794
if (val == DOUBLE_MARK) val = ScriptRuntime.wrapNumber(frame.sDbl[state.stackTop]);
37923795
String stringReg =
@@ -3818,6 +3821,9 @@ NewState execute(Context cx, CallFrame frame, InterpreterState state, int op) {
38183821
varDbls[state.indexReg] = frame.sDbl[state.stackTop];
38193822
}
38203823
} else {
3824+
// This only occurs if we are using the debugger, in
3825+
// which case `frame.scope` will be an activation
3826+
// frame for this function.
38213827
Object val = frame.stack[state.stackTop];
38223828
if (val == DOUBLE_MARK) val = ScriptRuntime.wrapNumber(frame.sDbl[state.stackTop]);
38233829
String stringReg =
@@ -3844,6 +3850,9 @@ NewState execute(Context cx, CallFrame frame, InterpreterState state, int op) {
38443850
varDbls[state.indexReg] = frame.sDbl[state.stackTop];
38453851
}
38463852
} else {
3853+
// This only occurs if we are using the debugger, in
3854+
// which case `frame.scope` will be an activation
3855+
// frame for this function.
38473856
Object val = frame.stack[state.stackTop];
38483857
if (val == DOUBLE_MARK) val = ScriptRuntime.wrapNumber(frame.sDbl[state.stackTop]);
38493858
String stringReg =
@@ -3866,6 +3875,9 @@ NewState execute(Context cx, CallFrame frame, InterpreterState state, int op) {
38663875
varDbls[state.indexReg] = frame.sDbl[state.stackTop];
38673876
}
38683877
} else {
3878+
// This only occurs if we are using the debugger, in
3879+
// which case `frame.scope` will be an activation
3880+
// frame for this function.
38693881
Object val = frame.stack[state.stackTop];
38703882
if (val == DOUBLE_MARK) val = ScriptRuntime.wrapNumber(frame.sDbl[state.stackTop]);
38713883
String stringReg =
@@ -3887,6 +3899,9 @@ NewState execute(Context cx, CallFrame frame, InterpreterState state, int op) {
38873899
frame.stack[state.stackTop] = vars[state.indexReg];
38883900
frame.sDbl[state.stackTop] = varDbls[state.indexReg];
38893901
} else {
3902+
// This only occurs if we are using the debugger, in
3903+
// which case `frame.scope` will be an activation
3904+
// frame for this function.
38903905
String stringReg =
38913906
frame.fnOrScript.getDescriptor().getParamOrVarName(state.indexReg);
38923907
frame.stack[state.stackTop] = frame.scope.get(stringReg, frame.scope);
@@ -3905,6 +3920,9 @@ NewState execute(Context cx, CallFrame frame, InterpreterState state, int op) {
39053920
frame.stack[state.stackTop] = vars[state.indexReg];
39063921
frame.sDbl[state.stackTop] = varDbls[state.indexReg];
39073922
} else {
3923+
// This only occurs if we are using the debugger, in
3924+
// which case `frame.scope` will be an activation
3925+
// frame for this function.
39083926
String stringReg =
39093927
frame.fnOrScript.getDescriptor().getParamOrVarName(state.indexReg);
39103928
frame.stack[state.stackTop] = frame.scope.get(stringReg, frame.scope);
@@ -3977,6 +3995,9 @@ NewState execute(Context cx, CallFrame frame, InterpreterState state, int op) {
39773995
}
39783996
}
39793997
} else {
3998+
// This only occurs if we are using the debugger, in
3999+
// which case `frame.scope` will be an activation
4000+
// frame for this function.
39804001
String varName = frame.fnOrScript.getDescriptor().getParamOrVarName(state.indexReg);
39814002
frame.stack[state.stackTop] =
39824003
ScriptRuntime.nameIncrDecr(frame.scope, varName, cx, incrDecrMask);

‎rhino/src/main/java/org/mozilla/javascript/debug/DebugFrame.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,23 @@ public interface DebugFrame {
2626
* @param thisObj value of the JavaScript {@code this} object
2727
* @param args the array of arguments
2828
*/
29-
public void onEnter(Context cx, Scriptable activation, Scriptable thisObj, Object[] args);
29+
default void onEnter(Context cx, Scriptable activation, Scriptable thisObj, Object[] args) {}
3030

3131
/**
3232
* Called when executed code reaches new line in the source.
3333
*
3434
* @param cx current Context for this thread
3535
* @param lineNumber current line number in the script source
3636
*/
37-
public void onLineChange(Context cx, int lineNumber);
37+
default void onLineChange(Context cx, int lineNumber) {}
3838

3939
/**
4040
* Called when thrown exception is handled by the function or script.
4141
*
4242
* @param cx current Context for this thread
4343
* @param ex exception object
4444
*/
45-
public void onExceptionThrown(Context cx, Throwable ex);
45+
default void onExceptionThrown(Context cx, Throwable ex) {}
4646

4747
/**
4848
* Called when the function or script for this frame is about to return.
@@ -53,12 +53,12 @@ public interface DebugFrame {
5353
* @param resultOrException function result in case of normal return or exception object if
5454
* about to throw exception
5555
*/
56-
public void onExit(Context cx, boolean byThrow, Object resultOrException);
56+
default void onExit(Context cx, boolean byThrow, Object resultOrException) {}
5757

5858
/**
5959
* Called when the function or script executes a 'debugger' statement.
6060
*
6161
* @param cx current Context for this thread
6262
*/
63-
public void onDebuggerStatement(Context cx);
63+
default void onDebuggerStatement(Context cx) {}
6464
}

‎rhino/src/main/java/org/mozilla/javascript/debug/Debugger.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public interface Debugger {
2020
* @param fnOrScript object describing the function or script
2121
* @param source the function or script source
2222
*/
23-
void handleCompilationDone(Context cx, DebuggableScript fnOrScript, String source);
23+
default void handleCompilationDone(Context cx, DebuggableScript fnOrScript, String source) {}
2424

2525
/**
2626
* Called when execution entered a particular function or script.

‎rhino/src/test/java/org/mozilla/javascript/DebuggerTest.java‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,6 @@ public static synchronized MockDebugFrame getFrame() {
3333
return frame;
3434
}
3535

36-
@Override
37-
public void handleCompilationDone(Context cx, DebuggableScript fnOrScript, String source) {}
38-
3936
@Override
4037
public DebugFrame getFrame(Context cx, DebuggableScript fnOrScript) {
4138
return frame;
@@ -54,9 +51,6 @@ public synchronized void incrementBreakPointsHit() {
5451
++breakPointsHit;
5552
}
5653

57-
@Override
58-
public void onEnter(Context cx, Scriptable activation, Scriptable thisObj, Object[] args) {}
59-
6054
@Override
6155
public void onLineChange(Context cx, int line) {
6256
// decide to break on a line and evaluate a command similar to what GlideScriptDebugger
@@ -65,15 +59,6 @@ public void onLineChange(Context cx, int line) {
6559
incrementBreakPointsHit();
6660
}
6761
}
68-
69-
@Override
70-
public void onExceptionThrown(Context cx, Throwable ex) {}
71-
72-
@Override
73-
public void onExit(Context cx, boolean byThrow, Object resultOrException) {}
74-
75-
@Override
76-
public void onDebuggerStatement(Context cx) {}
7762
}
7863

7964
@Test

‎rhino/src/test/java/org/mozilla/javascript/debug/NameDisambiguateTest.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package org.mozilla.javascript.debug;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
5+
import org.junit.jupiter.api.Test;
6+
import org.mozilla.javascript.Context;
7+
8+
class NameDisambiguateTest {
9+
private static final String SOURCE =
10+
"(function g(x) {\n"
11+
+ " {\n"
12+
+ " let x = 'inner';\n"
13+
+ " }\n"
14+
+ " return x;\n"
15+
+ "})('outer');";
16+
17+
// Having a debugger causes an activation frame to be used, which is handled specially in
18+
// Interpreter, for the GETVAR icode.
19+
@Test
20+
void letVariablesAreHandledCorrectlyWithDebugger() {
21+
try (Context cx = Context.enter()) {
22+
cx.setDebugger(new NoOpDebugger(), null);
23+
cx.setGeneratingDebug(true);
24+
cx.setInterpretedMode(true);
25+
26+
var scope = cx.initStandardObjects();
27+
28+
Object value = cx.evaluateString(scope, SOURCE, "test", 1, null);
29+
assertEquals("outer", value);
30+
}
31+
}
32+
}

‎rhino/src/test/java/org/mozilla/javascript/debug/NoOpDebugger.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package org.mozilla.javascript.debug;
2+
3+
import org.mozilla.javascript.Context;
4+
5+
class NoOpDebugger implements Debugger {
6+
@Override
7+
public DebugFrame getFrame(Context cx, DebuggableScript fnOrScript) {
8+
return new NoOpDebugFrame();
9+
}
10+
11+
private static class NoOpDebugFrame implements DebugFrame {}
12+
}

0 commit comments

Comments
 (0)