Breakpoints don't work on wrapped variable assignments

[DanTup]

A change was recently made to prevent breakpoints from jumping to different lines than the one where they were added if the location was invalid (#56820). While I think the idea is right, it seems to have impacted some kinds of code I hadn't considered and I'm not sure it's a good experience.

Consider this code:

I wanted to step into ElementLocator.locate so I put a breakpoint on line 78 expecting it to be the correct one (because I wanted to stop on this statement). However, line 78 is not a valid location to stop, so now we don't pause at all and it feels like a bug.

I'm not sure I could define rules that would fix this without reverting the change for #56820, but perhaps someone more familiar with the internals could.

@bkonyi @derekxu16

[dart-github-bot]

Summary: Breakpoints are not working on variable assignments wrapped in parentheses, preventing stepping into functions called within the assignment. This issue arises from a recent change intended to improve breakpoint behavior, but it seems to have unintended consequences for certain code patterns.

[a-siva]

@derekxu16 will follow up with @DanTup on two options

• we switch back to the old behavior
• we have VS code use the information that the VM provides about lines that are breakable and only allow the user to set breakpoints at those locations.

[derekxu16]

The debugger currently doesn't know the token range covered by any particular statement, and it would be a significant undertaking to support that, so it's not something we can promise in the foreseeable future. As Siva mentioned, our only options for now are to: 1) keep enforcing that breakpoints can only be resolved on the same line where they were requested (it would be nice to show users which lines contain debuggable code, but I don’t think it’s possible to show that when there’s no active debug session, right?), 2) revert the change and allow breakpoints to move any number of lines, or 3) enforce that breakpoints can only move up to some fixed number of lines. Please let me know what you think.

[DanTup]

we have VS code use the information that the VM provides about lines that are breakable and only allow the user to set breakpoints at those locations.

This would only work when there is a debug session, but breakpoints are often set before a debug session is starting. It might be useful to use this information (just so setting breakpoints when a session is active is better), but it probably doesn't solve this problem well enough.

or 3) enforce that breakpoints can only move up to some fixed number of lines

I think this might be the best experience, though I'm not sure that number should be. I feel like jumping one line is probably reasonable. Jumping 15 lines is probably not. 2? 3? I'm less sure. Maybe we should start with 1 though and see what the feedback is like. I think it will be better than both the old and the new/current behaviour.

WDYT?

[a-siva]

@DanTup jumping one line might sound reasonable but I am worried we may encounter scenarios where it might work better if we had picked two lines or three lines.

I see two scenarios here

1. The IDE is connected to a debug session, in this case querying the VM to figure out locations where breakpoints can be set and acting accordingly seems like a good experience (maybe even having a gutter with an indication that states where breakpoints can be set)
2. The IDE is not connected to a debug session, in this case it is harder as there is no information to validate where breakpoints can be set, I see two possible options

• we set breakpoints at lines that the user specifies and when the user connects to a debug session we try setting the breakpoints and just report back to the user about all the breakpoints that could not be set and have them adjust the lines
• the IDE tool is made smarter to try and adjust all the breakpoints that could not be set when a debug session is created by retrying at a different location using a policy (maybe +1 line or +2 line or scan until a semi colon is seen and keep trying +n until that line is reached)

[DanTup]

we set breakpoints at lines that the user specifies and when the user connects to a debug session we try setting the breakpoints and just report back to the user about all the breakpoints that could not be set and have them adjust the lines

We could certainly print some output whenever we fail to set a breakpoint in the debug console. Adding the reason as a tooltip to the reason mostly does the same thing, although if it's a short-lived script, they might never be able to hover, so having both could help.

I don't think that addresses the main issue above - it could be quite annoying to have to remember every time you want to set a breakpoint on an assignment like that (particularly if you don't see the text until after the breakpoint is missed, or you're debugging a CLI script that has no exited and you need to run again).

the IDE tool is made smarter to try and adjust all the breakpoints that could not be set when a debug session is created by retrying at a different location using a policy (maybe +1 line or +2 line or scan until a semi colon is seen and keep trying +n until that line is reached)

In the debug adapter (which is the only place we can execute code here, VS Code talks directly to the debug adapter) we don't have easy access to the source code, so I doubt we could do this reliably (probably we could get the source from the VM, but at that point the VM could probably do this itself better/faster?).

jumping one line might sound reasonable but I am worried we may encounter scenarios where it might work better if we had picked two lines or three lines.

It's definitely possible, but I think a variable assignment over 1 line is the only case I can come up with so far. That said, I also feel there's something inconsistent here.. why is the breakpoint on line 6 valid, but the one on line 8 should be 9? If I can stop at 6 I feel like I should be able to stop at 8?

[derekxu16]

I also feel there's something inconsistent here.. why is the breakpoint on line 6 valid, but the one on line 8 should be 9? If I can stop at 6 I feel like I should be able to stop at 8?

I just looked into this. For the assignment on line 6, we mark the = on line 6 as a location where breakpoints can be set, because we know that the RHS of the assignment does not already contain any locations where breakpoints can be set, because it is a constant. This is checked by the following code:

sdk/runtime/vm/compiler/frontend/kernel_to_il.cc

Line 2274 in 53bad05

if (definition->IsConstant() || definition->IsLoadStaticField() ||

For the assignment on line 8, we know that the RHS of the assignment is a call, and calls are already locations where breakpoints can be set, so we do not mark the = as a location where breakpoints can be set.

[DanTup]

@derekxu16 what's the reason for the difference? (why wasn't it just set on the RHS?)

Are you able to add synthetic locations to break at? Would it be possible for every assignment like this to just have a stop point at the start of the line?

[derekxu16]

@derekxu16 what's the reason for the difference? (why wasn't it just set on the RHS?)

Are you able to add synthetic locations to break at?

Yes, we are able to synthetically mark locations as "breakable". That's what we're doing to the = on line 6. I guess the original writers of this code just thought that it made more sense for the breakpoint end up on the = than on the constant.

Would it be possible for every assignment like this to just have a stop point at the start of the line?

That is possible, but I would personally like it more if the = of every assignment was made "breakable". Let me check with Siva to see if we need to have a meeting to come to a consensus on this.

[a-siva]

That is possible, but I would personally like it more if the = of every assignment was made "breakable". Let me check with Siva to see if we need to have a meeting to come to a consensus on this.

A statement like 'int j = foo();' would already have a breakable point at the start of foo correct, in this case an additional breakable at the '=' seems redundant.

[DanTup]

That's true, but I don't think if my debugger stopped at the start of the statement and then again on foo() I would be surprised. I feel like that might be the smaller drawback of all the possible solutions here?