Skip to content

Commit 1b2abdf

Browse files
JaroslavTulachJaroslavTulach
authored
Improving debugging "step over" experience (#14312)
Greatly improving _"stepping over"_ for `if_then_else` and `case_of` control flow constructs. The condition and the branches are no longer treated as separate functions. Thus _step over_ takes one into the selected branch rather than over. Also removing the need to _step into_ method body. This fix probably also solves the problem with stepping into a function where the breakpoint was incorrectly placed at the beginning of a function comment ([highlighted by the red box](https://github.com/user-attachments/assets/db244cc3-e578-4127-b8f4-7b1c3c9d408c)).
1 parent 6df87ec commit 1b2abdf

File tree

6 files changed

+260
-45
lines changed

6 files changed

+260
-45
lines changed

‎engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/DebuggingControlFlowTest.java‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
package org.enso.interpreter.test;
2+
3+
import static org.hamcrest.Matchers.*;
4+
import static org.junit.Assert.assertEquals;
5+
6+
import com.oracle.truffle.api.debug.Debugger;
7+
import com.oracle.truffle.api.debug.StepConfig;
8+
import java.io.ByteArrayOutputStream;
9+
import java.io.IOException;
10+
import java.util.ArrayList;
11+
import org.enso.test.utils.ContextUtils;
12+
import org.junit.AfterClass;
13+
import org.junit.BeforeClass;
14+
import org.junit.ClassRule;
15+
import org.junit.Test;
16+
17+
public class DebuggingControlFlowTest {
18+
@ClassRule
19+
public static final ContextUtils ctx = ContextUtils.newBuilder("enso").assertGC(false).build();
20+
21+
private static Debugger debugger;
22+
private static final ByteArrayOutputStream out = new ByteArrayOutputStream();
23+
24+
@BeforeClass
25+
public static void initContext() {
26+
debugger = Debugger.find(ctx.getEngine());
27+
}
28+
29+
@AfterClass
30+
public static void disposeContext() throws IOException {
31+
debugger = null;
32+
}
33+
34+
@Test
35+
public void stepsOverBlock() {
36+
var prelude =
37+
"""
38+
from Standard.Base import Integer
39+
40+
main a:Integer=7 b:Integer=5 c:Integer=3 =
41+
""";
42+
43+
var body =
44+
"""
45+
bc = b*c
46+
ac = a*c
47+
ab = b*a
48+
plus = ab + ac + bc
49+
plus
50+
""";
51+
52+
var code = prelude + body;
53+
54+
var events = new ArrayList<String>();
55+
try (com.oracle.truffle.api.debug.DebuggerSession session =
56+
debugger.startSession(
57+
(event) -> {
58+
events.add(event.getSourceSection().getCharacters().toString());
59+
event.prepareStepOver(StepConfig.newBuilder().build());
60+
})) {
61+
session.suspendNextExecution();
62+
var res = ctx.evalModule(code);
63+
assertEquals(71, res.asInt());
64+
}
65+
assertSuspendedEvents(events, 5, body.split("\n"));
66+
}
67+
68+
@Test
69+
public void stepsOverIf() {
70+
var prelude =
71+
"""
72+
from Standard.Base import Integer
73+
74+
main a:Integer=7 b:Integer=5 c:Integer=3 =
75+
""";
76+
77+
var bc =
78+
"""
79+
bc = b*c
80+
""";
81+
var cond =
82+
"""
83+
if bc < 10 then 0 else
84+
ac = a*c
85+
ab = b*a
86+
plus = ab + ac + bc
87+
plus
88+
""";
89+
90+
var body = bc + cond;
91+
var code = prelude + body;
92+
93+
var events = new ArrayList<String>();
94+
try (com.oracle.truffle.api.debug.DebuggerSession session =
95+
debugger.startSession(
96+
(event) -> {
97+
events.add(event.getSourceSection().getCharacters().toString());
98+
event.prepareStepOver(StepConfig.newBuilder().build());
99+
})) {
100+
session.suspendNextExecution();
101+
var res = ctx.evalModule(code);
102+
assertEquals(71, res.asInt());
103+
}
104+
var bodyLines = body.split("\n");
105+
bodyLines[1] = cond; // if statement section is full `cond`
106+
assertSuspendedEvents(events, 6, bodyLines);
107+
}
108+
109+
@Test
110+
public void stepsOverCase() {
111+
var prelude =
112+
"""
113+
from Standard.Base import Integer
114+
115+
main a:Integer=7 b:Integer=5 c:Integer=3 =
116+
""";
117+
118+
var bc =
119+
"""
120+
bc = b*c
121+
""";
122+
var caseBegin =
123+
"""
124+
case bc of
125+
5 ->
126+
bc
127+
15 ->
128+
""";
129+
130+
var caseBody =
131+
"""
132+
ac = a*c
133+
ab = b*a
134+
plus = ab + ac + bc
135+
plus
136+
""";
137+
var caseTail =
138+
"""
139+
_ ->
140+
-bc
141+
""";
142+
143+
var body = bc + "case of line placeholder\n" + caseBody;
144+
var code = prelude + bc + caseBegin + caseBody + caseTail;
145+
146+
var events = new ArrayList<String>();
147+
try (com.oracle.truffle.api.debug.DebuggerSession session =
148+
debugger.startSession(
149+
(event) -> {
150+
events.add(event.getSourceSection().getCharacters().toString());
151+
event.prepareStepOver(StepConfig.newBuilder().build());
152+
})) {
153+
session.suspendNextExecution();
154+
var res = ctx.evalModule(code);
155+
assertEquals(71, res.asInt());
156+
}
157+
var bodyLines = body.split("\n");
158+
bodyLines[1] = caseBegin + caseBody + caseTail; // whole case statement section
159+
assertSuspendedEvents(events, 6, bodyLines);
160+
}
161+
162+
private static void assertSuspendedEvents(
163+
ArrayList<String> events, final int expectedStops, String... bodyLines) {
164+
if (events.size() != expectedStops) {
165+
var sb = new StringBuilder("Events:");
166+
var cnt = 0;
167+
for (var e : events) {
168+
sb.append("\n#").append(cnt++).append(": ").append(e);
169+
}
170+
assertEquals(sb.toString(), expectedStops, events.size());
171+
}
172+
for (var i = 0; i < events.size(); i++) {
173+
if (bodyLines[i] == null) {
174+
continue;
175+
}
176+
assertEquals("At " + i, bodyLines[i].trim(), events.get(i));
177+
}
178+
}
179+
}

‎engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/DebuggingEnsoTest.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -784,7 +784,6 @@ private void testStepping(
784784
*/
785785
private static Queue<SuspendedCallback> createStepOverEvents(int numSteps) {
786786
Queue<SuspendedCallback> steps = new ArrayDeque<>();
787-
steps.add((event) -> event.prepareStepInto(1));
788787
for (int i = 0; i < numSteps - 1; i++) {
789788
steps.add((event) -> event.prepareStepOver(1));
790789
}
@@ -821,7 +820,7 @@ public void testSteppingOver() {
821820
bar 42 # 6
822821
end = 0 # 7
823822
""");
824-
List<Integer> expectedLineNumbers = List.of(5, 6, 7);
823+
List<Integer> expectedLineNumbers = List.of(6, 7);
825824
Queue<SuspendedCallback> steps = createStepOverEvents(expectedLineNumbers.size());
826825
testStepping(src, "foo", new Object[] {0}, steps, expectedLineNumbers);
827826
}
@@ -851,7 +850,6 @@ public void testSteppingOverUseStdLib() {
851850

852851
List<String> expectedLines =
853852
List.of(
854-
"foo x =",
855853
"vec_builder = Builder.new",
856854
"vec_builder.append 1",
857855
"vec_builder.append 2",
@@ -873,7 +871,7 @@ public void testSteppingInto() {
873871
bar 42 # 4
874872
end = 0 # 5
875873
""");
876-
List<Integer> expectedLineNumbers = List.of(3, 4, 2, 1, 2, 4, 5);
874+
List<Integer> expectedLineNumbers = List.of(4, 2, 1, 2, 4, 5);
877875
Queue<SuspendedCallback> steps =
878876
new ArrayDeque<>(
879877
Collections.nCopies(expectedLineNumbers.size(), (event) -> event.prepareStepInto(1)));
@@ -892,7 +890,7 @@ public void testSteppingIntoMoreExpressionsOneLine() {
892890
bar (baz x) # 4
893891
end = 0 # 5
894892
""");
895-
List<Integer> expectedLineNumbers = List.of(3, 4, 1, 4, 2, 4, 5);
893+
List<Integer> expectedLineNumbers = List.of(4, 1, 4, 2, 4, 5);
896894
Queue<SuspendedCallback> steps =
897895
new ArrayDeque<>(
898896
Collections.nCopies(expectedLineNumbers.size(), (event) -> event.prepareStepInto(1)));

‎engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/RootNamesTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,13 +86,12 @@ public void computeFactorial() throws Exception {
8686
.map(l -> l.substring(7))
8787
.collect(Collectors.toSet());
8888

89-
assertEquals("Few closures: " + closures, 3, closures.size());
89+
assertEquals("Few closures: " + closures, 2, closures.size());
9090
assertTrue(
9191
"Fully qualified name for method: " + closures,
9292
closures.contains("factorial::factorial::fac"));
9393
assertTrue(
9494
"Name with dots for local method: " + closures, closures.contains("factorial.fac.acc"));
95-
assertTrue("if/then/else: " + closures, closures.contains("if_then_else"));
9695
}
9796

9897
@Test

‎engine/runtime/src/main/java/org/enso/interpreter/node/callable/function/BlockNode.java‎

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ private BlockNode(ExpressionNode[] expressions, ExpressionNode returnExpr) {
3434
}
3535

3636
/**
37-
* Creates an "root tagged" instance of block node.
37+
* Creates a "root tagged" instance of block node.
3838
*
3939
* @param expressions the function body
4040
* @param returnExpr the return expression from the function
@@ -45,13 +45,26 @@ public static BlockNode buildRoot(ExpressionNode[] expressions, ExpressionNode r
4545
}
4646

4747
/**
48-
* Creates a non-instrumented instance of block node.
48+
* Creates a block node with statements. When instrumented, the statements get wrapped in a into a
49+
* {@link StatementNode} so one can step over them.
4950
*
5051
* @param expressions the function body
5152
* @param returnExpr the return expression from the function
5253
* @return a node representing a block expression
5354
*/
54-
public static BlockNode buildSilent(ExpressionNode[] expressions, ExpressionNode returnExpr) {
55+
public static BlockNode buildStatements(ExpressionNode[] expressions, ExpressionNode returnExpr) {
56+
return new Statements(expressions, returnExpr);
57+
}
58+
59+
/**
60+
* Creates a non-instrumented instance of block node. Used by {@code case of} and {@code if then
61+
* else} control flow constructs where the block itself shall be invisible to tooling.
62+
*
63+
* @param expressions the function body
64+
* @param returnExpr the return expression from the function
65+
* @return a node representing a block expression
66+
*/
67+
public static BlockNode buildInvisible(ExpressionNode[] expressions, ExpressionNode returnExpr) {
5568
return new BlockNode(expressions, returnExpr);
5669
}
5770

@@ -78,32 +91,6 @@ public Object executeGeneric(VirtualFrame frame) {
7891
return returnExpr.executeGeneric(frame);
7992
}
8093

81-
/**
82-
* Wrap all the statements inside this block node in {@link StatementNode}. Care is taken not for
83-
* wrapping expression twice.
84-
*
85-
* @return This BlockNode with all the statements wrapped.
86-
*/
87-
@Override
88-
public InstrumentableNode materializeInstrumentableNodes(
89-
Set<Class<? extends Tag>> materializedTags) {
90-
if (materializedTags.contains(StandardTags.StatementTag.class)) {
91-
for (int i = 0; i < statements.length; i++) {
92-
if (!isNodeWrapped(statements[i])) {
93-
statements[i] = insert(StatementNode.wrap(statements[i]));
94-
}
95-
}
96-
if (!isNodeWrapped(returnExpr)) {
97-
returnExpr = insert(StatementNode.wrap(returnExpr));
98-
}
99-
}
100-
return this;
101-
}
102-
103-
private static boolean isNodeWrapped(ExpressionNode node) {
104-
return node instanceof StatementNode || ExpressionNode.isWrapper(node);
105-
}
106-
10794
@Override
10895
public SourceSection getSourceSection() {
10996
var ss = super.getSourceSection();
@@ -123,4 +110,36 @@ public boolean hasTag(Class<? extends Tag> tag) {
123110
return tag == StandardTags.RootBodyTag.class || tag == StandardTags.RootTag.class;
124111
}
125112
}
113+
114+
private static final class Statements extends BlockNode {
115+
Statements(ExpressionNode[] expressions, ExpressionNode returnExpr) {
116+
super(expressions, returnExpr);
117+
}
118+
119+
/**
120+
* Wrap all the statements inside this block node in {@link StatementNode}. Care is taken not
121+
* for wrapping expression twice.
122+
*
123+
* @return This BlockNode with all the statements wrapped.
124+
*/
125+
@Override
126+
public InstrumentableNode materializeInstrumentableNodes(
127+
Set<Class<? extends Tag>> materializedTags) {
128+
if (materializedTags.contains(StandardTags.StatementTag.class)) {
129+
for (int i = 0; i < super.statements.length; i++) {
130+
if (!isNodeWrapped(super.statements[i])) {
131+
super.statements[i] = insert(StatementNode.wrap(super.statements[i]));
132+
}
133+
}
134+
if (!isNodeWrapped(super.returnExpr)) {
135+
super.returnExpr = insert(StatementNode.wrap(super.returnExpr));
136+
}
137+
}
138+
return this;
139+
}
140+
141+
private static boolean isNodeWrapped(ExpressionNode node) {
142+
return node instanceof StatementNode || ExpressionNode.isWrapper(node);
143+
}
144+
}
126145
}

‎engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedConstructor.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ static DirectCallNode buildApplication(UnresolvedConstructor prototype) {
213213
expr.setSourceLocation(section.getCharIndex(), section.getCharLength());
214214
}
215215
var lang = EnsoLanguage.get(null);
216-
var body = BlockNode.buildSilent(new ExpressionNode[0], expr);
216+
var body = BlockNode.buildInvisible(new ExpressionNode[0], expr);
217217
body.adoptChildren();
218218
var loc =
219219
section == null ? null : new Location(section.getCharIndex(), section.getCharEndIndex());

0 commit comments

Comments
 (0)