Improving debugging "step over" experience #14312

JaroslavTulach · 2025-11-18T15:23:28Z

Pull Request Description

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).

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Java,
Unit tests have been written where possible.

JaroslavTulach · 2025-11-18T15:25:36Z

@kazcw, I am not really happy about using fun.getBody()
body may not include function arguments, right?
however I haven't found any better element to anchor to
and possibly body is good enough for debugger

JaroslavTulach · 2025-11-20T14:39:27Z

I am getting test failures like

sbt:enso> runtime-integration-tests/testOnly *NonStrictModeTests
[error] Test org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversion failed: java.lang.AssertionError: Cannot find Unnamed:7:1: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module. in enso.org.enso.compiler.Compiler
[error] lis 20, 2025 3:37:44 ODP. enso.org.enso.compiler.Compiler
[error] WARNING: Unnamed:7:23: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module.
[error]     7 | Foo.from (that:Bar) = Foo.Mk_Foo that.x+1000
[error]       |                       ^~~~~~~~~~~~~~~~~~~~~~
[error] , took 0.252 sec
[error]     at org.enso.interpreter.test.MockLogHandler.assertMessage(MockLogHandler.java:46)
[error]     at org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversion(NonStrictModeTests.java:51)
[error]     at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
[error]     at java.lang.reflect.Method.invoke(Method.java:565)
[error]     ...
[error] Test org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversionUsage failed: java.lang.AssertionError: Cannot find Unnamed:9:1: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module. in enso.org.enso.compiler.Compiler
[error] lis 20, 2025 3:37:47 ODP. enso.org.enso.compiler.Compiler
[error] WARNING: Unnamed:9:23: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module.
[error]     9 | Foo.from (that:Bar) = Foo.Mk_Foo that.x+1000
[error]       |                       ^~~~~~~~~~~~~~~~~~~~~~
[error] , took 3.089 sec
[error]     at org.enso.interpreter.test.MockLogHandler.assertMessage(MockLogHandler.java:46)
[error]     at org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversionUsage(NonStrictModeTests.java:80)
[error]     at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
[error]     at java.lang.reflect.Method.invoke(Method.java:565)
[error]     ...
[info] Test run org.enso.interpreter.test.NonStrictModeTests finished: 2 failed, 0 ignored, 3 total, 4.435s
[error] Failed: Total 3, Failed 2, Errors 0, Passed 1
[error] Failed tests:
[error]         org.enso.interpreter.test.NonStrictModeTests
[error] (runtime-integration-tests / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 38 s, completed 20. 11. 2025 15:37:47

I believe the test wants the whole function to be reported, not just the body
this test is fixed by 1209119

…body

…ition

…omment

JaroslavTulach · 2025-11-21T10:31:07Z

Another problem in IGV. The root cause is the same. Comment shouldn't be part of executable location:

JaroslavTulach · 2025-11-21T10:32:44Z

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java

+  private IdentifiedLocation getIdentifiedLocation(
+      Tree.Function fn, int b, int e, Option<UUID> someId) {
+    var orig = getIdentifiedLocation((Tree) fn, b, e, someId);
+    var start = castToInt(fn.getName().getStartCode()) + b;


I am not really happy about the tricks that I have to do to remove comment from the location of Tree.Function, but unless @kazcw suggest easier way to do the same, I go with this code. (Multi line) comment just cannot be part of executable IdentifiedLocation as also demonstrated by the IGV problem.

kazcw · 2025-11-24T11:55:04Z

I am getting test failures like

sbt:enso> runtime-integration-tests/testOnly *NonStrictModeTests
[error] Test org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversion failed: java.lang.AssertionError: Cannot find Unnamed:7:1: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module. in enso.org.enso.compiler.Compiler
[error] lis 20, 2025 3:37:44 ODP. enso.org.enso.compiler.Compiler
[error] WARNING: Unnamed:7:23: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module.
[error]     7 | Foo.from (that:Bar) = Foo.Mk_Foo that.x+1000
[error]       |                       ^~~~~~~~~~~~~~~~~~~~~~
[error] , took 0.252 sec
[error]     at org.enso.interpreter.test.MockLogHandler.assertMessage(MockLogHandler.java:46)
[error]     at org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversion(NonStrictModeTests.java:51)
[error]     at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
[error]     at java.lang.reflect.Method.invoke(Method.java:565)
[error]     ...
[error] Test org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversionUsage failed: java.lang.AssertionError: Cannot find Unnamed:9:1: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module. in enso.org.enso.compiler.Compiler
[error] lis 20, 2025 3:37:47 ODP. enso.org.enso.compiler.Compiler
[error] WARNING: Unnamed:9:23: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module.
[error]     9 | Foo.from (that:Bar) = Foo.Mk_Foo that.x+1000
[error]       |                       ^~~~~~~~~~~~~~~~~~~~~~
[error] , took 3.089 sec
[error]     at org.enso.interpreter.test.MockLogHandler.assertMessage(MockLogHandler.java:46)
[error]     at org.enso.interpreter.test.NonStrictModeTests.testAmbiguousConversionUsage(NonStrictModeTests.java:80)
[error]     at jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
[error]     at java.lang.reflect.Method.invoke(Method.java:565)
[error]     ...
[info] Test run org.enso.interpreter.test.NonStrictModeTests finished: 2 failed, 0 ignored, 3 total, 4.435s
[error] Failed: Total 3, Failed 2, Errors 0, Passed 1
[error] Failed tests:
[error]         org.enso.interpreter.test.NonStrictModeTests
[error] (runtime-integration-tests / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 38 s, completed 20. 11. 2025 15:37:47

* I believe the test wants the **whole function** to be reported, not just the body

* this test is fixed by [1209119](https://github.com/enso-org/enso/commit/12091190a20369b15cd5938ff41b36265bbbadc6)

The test was more correct before. The error does not originate from the body, it depends only on the name and arguments.

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java

Akirathan

Testing: I think that it would be better, and more correct, to test this behavior in DebuggingEnsoTest. You can, e.g., get a stack trace and see if the source section of a function contains the documentation.

Implementation: I don't like that the location of a function is changed in TreeToIr. This feels dangerous. Why don't you just modify the places that are responsible for communicating with the debugger? Maybe ExpressionNode.getSourceSection?

JaroslavTulach · 2025-11-24T19:40:13Z

Testing: I think that it would be better, and more correct, to test this behavior in DebuggingEnsoTest.

OK, I give it a try. Update on Dec 3:

yes, it is way better to have a test for the expected behavior
created new "control flow" test with the goal to provide reasonable step over behavior for if/then/else and case of control flow structures
check 355cb1db7bd7594e4e0f4221d58fa8267b9cec0f, @Akirathan

Implementation: I don't like that the location of a function is changed in TreeToIr. This feels dangerous. Why don't you just modify the places that are responsible for communicating with the debugger? Maybe ExpressionNode.getSourceSection?

No, that would be weird. Locations set in TreeToIr and just represented as SourceSection in the Truffle node. This would be a first case when SourceSection is different to location.

…omment

…ol flow code constructs.

JaroslavTulach · 2025-12-03T15:20:43Z

changing locations of Tree.Function so that frontend and backend don't agree on the locations is dangerous
removing in 908290e for now
the debugging experience should be improved by the other changes in this PR
the problem with function comments still manifests itself
- when reporting errors (demonstrated by the NonStrictModeTests) removed in 908290e
- as well as while using IGV
stripping of comments from the the Location of a Tree.Function is still going to be needed
the safest approach seems to me to split Tree.Function into two parts:
- Tree.Function = Tree.FunctionComment & Tree.FunctionDefinition
- location of Tree.FunctionDefinition would then be exactly what's needed instead of 908290e

JaroslavTulach · 2025-12-03T15:23:27Z

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

   */
  private static Queue<SuspendedCallback> createStepOverEvents(int numSteps) {
    Queue<SuspendedCallback> steps = new ArrayDeque<>();
-    steps.add((event) -> event.prepareStepInto(1));


Take a look @Akirathan, removal of this line means the debugger will behave well more naturally!

How so? If numSteps is 1, an empty Queue is returned. How is that more "natural"?

these are supposed to be step over steps

is it natural that first such a step is step into?

these are supposed to be step over steps

Right, my mistake.

is it natural that first such a step is step into?

Not at all. I have misread it.

JaroslavTulach · 2025-12-03T15:27:09Z

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

        closures.contains("factorial::factorial::fac"));
    assertTrue(
        "Name with dots for local method: " + closures, closures.contains("factorial.fac.acc"));
-    assertTrue("if/then/else: " + closures, closures.contains("if_then_else"));


this change returns the test back prior to IfThenElse IR instead of if_then_else function invocation #11365

if_then_else is again not visible to instrumentation ...

...but this time it is not a function call, but a control flow construct

Akirathan

few minor comments

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala

Akirathan · 2025-12-03T15:43:28Z

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

   */
  private static Queue<SuspendedCallback> createStepOverEvents(int numSteps) {
    Queue<SuspendedCallback> steps = new ArrayDeque<>();
-    steps.add((event) -> event.prepareStepInto(1));


How so? If numSteps is 1, an empty Queue is returned. How is that more "natural"?

...time-integration-tests/src/test/java/org/enso/interpreter/test/DebuggingControlFlowTest.java

Co-authored-by: Pavel Marek <pavel.marek@enso.org>

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)). (cherry picked from commit 1b2abdf)

Comment isn't part of function

f5e842a

JaroslavTulach requested a review from kazcw November 18, 2025 15:23

JaroslavTulach self-assigned this Nov 18, 2025

JaroslavTulach requested review from 4e6, Akirathan and hubertp as code owners November 18, 2025 15:23

enso-bot bot mentioned this pull request Nov 19, 2025

Use Arrow format for LongStorage to make exchange between JVMs and Python possible #13851

Closed

JaroslavTulach added 3 commits November 20, 2025 15:55

getIdentifiedLocation for function needs to include name, not just a …

1209119

…body

Test errors don't include function documentation, just function defin…

3586ba7

…ition

Merge remote-tracking branch 'origin/develop' into wip/jtulach/AvoidC…

f904f34

…omment

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 21, 2025

JaroslavTulach commented Nov 21, 2025

View reviewed changes

JaroslavTulach changed the title ~~Debugging painful as comment is treated as part of function~~ Extract only executable location from Tree.Function to simplify debugging & co. Nov 21, 2025

kazcw reviewed Nov 24, 2025

View reviewed changes

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java Outdated Show resolved Hide resolved

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java Outdated Show resolved Hide resolved

kazcw reviewed Nov 24, 2025

View reviewed changes

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java Outdated Show resolved Hide resolved

4e6 reviewed Nov 24, 2025

View reviewed changes

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java Outdated Show resolved Hide resolved

Akirathan reviewed Nov 24, 2025

View reviewed changes

JaroslavTulach added 4 commits December 2, 2025 19:07

Merge remote-tracking branch 'origin/develop' into wip/jtulach/AvoidC…

e0741f7

…omment

Step over if_then_else and case_of control flow structures

355cb1d

Dump all events when the count doesn't match

6d66212

BlockNode can be root, statements or invisible. Helps debugging contr…

54b2e92

…ol flow code constructs.

JaroslavTulach changed the title ~~Extract only executable location from Tree.Function to simplify debugging & co.~~ Improving debugging "step over" experience Dec 3, 2025

JaroslavTulach added 3 commits December 3, 2025 12:56

No need to step into body of a function anymore

7698b8a

if_then_else is no longer an instrumentable root

89aa8cb

Always specify defineRoot as named argument

c7ccdbc

JaroslavTulach force-pushed the wip/jtulach/AvoidComment branch from 192c0b0 to c7ccdbc Compare December 3, 2025 12:52

JaroslavTulach requested a review from Akirathan December 3, 2025 13:23

Removing Tree.Function IdentifiedLocation tricks out of this PR

908290e

JaroslavTulach commented Dec 3, 2025

View reviewed changes

JaroslavTulach requested a review from 4e6 December 3, 2025 15:29

Akirathan approved these changes Dec 3, 2025

View reviewed changes

argsExpr._1 is said to already be an Array

2067e93

Co-authored-by: Pavel Marek <pavel.marek@enso.org>

JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Dec 3, 2025

mergify bot merged commit 1b2abdf into develop Dec 3, 2025
77 of 78 checks passed

mergify bot deleted the wip/jtulach/AvoidComment branch December 3, 2025 22:46

enso-bot bot mentioned this pull request Dec 5, 2025

Using validity bitmap as Arrow does #14409

Merged

3 tasks

jdunkerley added this to the 2025.3 Release milestone Dec 5, 2025

Improving debugging "step over" experience #14312

Improving debugging "step over" experience #14312

Conversation

JaroslavTulach commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

Checklist

Uh oh!

JaroslavTulach commented Nov 18, 2025

Uh oh!

JaroslavTulach commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaroslavTulach commented Nov 21, 2025

Uh oh!

JaroslavTulach Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazcw commented Nov 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Akirathan left a comment

Choose a reason for hiding this comment

Uh oh!

JaroslavTulach commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaroslavTulach commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaroslavTulach Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Akirathan Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

JaroslavTulach Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Akirathan Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

JaroslavTulach Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Akirathan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Akirathan Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JaroslavTulach commented Nov 18, 2025 •

edited

Loading

JaroslavTulach commented Nov 20, 2025 •

edited

Loading

JaroslavTulach Nov 21, 2025 •

edited

Loading

JaroslavTulach commented Nov 24, 2025 •

edited

Loading

JaroslavTulach commented Dec 3, 2025 •

edited

Loading