Local variables are collapsed in annoying ways

[jrieken]

I have a launch config that attaches to an extension host and often (this seems somewhat random) the variable nodes are collapsed in super annoying ways (hello paper cuts). The first screen shot shows the view after hitting the first breakpoint. Not sure what/if something goes wrong but I think in no universe it makes sense to show all variables nodes as collapsed. The second screen shot is after stepping a bit and the variables view doesn't "follow" - I am already three blocks further down but nothing is reveal/expanded which makes this whole thing painful to work with.

[isidorn]

@jrieken thanks for providing great feedback

What we do:

1. If every scope is collapsed and the first one is not expensive we automatically expand it
2. between steps we always preserve the expansions state

I believe that 1) we can improve that we auto expand all non expensive scopes. Global scope is for example marked as expensive.
2) should work always, @jrieken let me know if this is not working sometimes for you

For some smarter auto expensions we could look at the Scope Source and for example automatically expand all scopes that match the currently active file (which have not been explicltly collapsed by the user). And this approach I like the best. Not sure thuogh how much debug adapters provide exact Scope Source information

@weinand @roblourens @connor4312 let me know what you think

[jrieken]

between steps we always preserve the expansions state

But that doesn't work when hitting the same breakpoint repeatedly, right?

[weinand]

@isidorn we are preserving expansion state between steps based on the "sourceReference" returned from the DA, correct? If these "sourceReference" are not reused between steps, we might lose the expansion state.

@jrieken are you using preview js-debug?

[jrieken]

yes - this is insiders and believe js-debug the default there

[isidorn]

@jrieken should work when hitting the same breakpoint repeatedly
@weinand we preserve the state based on scope name and it's index in the Scopes array which we get from the stack frame. We could also use the sourceReference instead of the index but that proved less stable in practice than the index. Code pointer

And yes, js-debug is the default in insiders.

[weinand]

@isidorn "scope name and its index in the Scopes array" sounds much better than sourceReference.
But could it be that @jrieken runs into the issue that the "scope index" changes?

[isidorn]

It could be looking at the attached picture, there are 3 new block scopes added between pictures.

[jrieken]

Attached video shows the randomness that I am observing

Mar-23-2020 15-45-48.mp4.zip

[connor4312]

js-debug is still behind a flag as far as I know on Insiders. The call stack looks like the old debugger. But for either one trace:true will let you verify whether expensive is being set.

[isidorn]

Thanks for sharing the video this show exactly that a block scope came in first and screwed up with the order, thus changing the ids. I have an idea how to fix this.
Use the index of the name as the id, not the overall index. Thus the ids for your screenshot will be
Block:0, Block:1, Block:2, Block:3, Local:0, Closure(execute call):0, Closure:0, Global:0

[isidorn]

Pushed a fix using the id computation from my previous comment.
I have also changed our inital expansion strategy: to expand a first non expensive scope, and not to only check the first scope.

Bottom line @jrieken let me know if you still see weirdness, and we can look into those cases in more detail. Thanks

[jrieken]

Bottom line @jrieken let me know if you still see weirdness, and we can look into those cases in more detail. Thanks

Yes, still see weirdness. Nodes for locals or blocks are randomly collapsed, interleaved, the first, the last. idk - I am the only one seeing this? I feel like I am running into this too often for that to be true

[isidorn]

Then let's reopen. Assigning to April to investigate more then.

[jrieken]

@isidorn I am assuming that you also see this when self-hosting and that no more input from me is needed, right? It's hard to distill steps here but I'd say that locals or block variables are always collapsed except for one or two containers

[isidorn]

@jrieken when I self host I personally hit two issues:

• When stepping in and when most of the scopes get changed the expansion expectedly gets lost
• When I manually collapse all scopes next step the first scope reopens

Though I am not sure if we can do anything about these two cases.
I will keep my eyes open for this one in the next week, and if you see some other pattern than what I described above please let me know.

If you see the same scope with the same name lossing expansions state that would be very weird and I would be interested in more details (as I do not see that).

[jrieken]

Dumping some sample here, not sure if this is all the same root cause or not but it definitely makes me think negatively about our debug

Debugging stopped at line 46 (no stepping, first stop)

• Why is there at the top a scope called "Local" with only this
• Why is "Closure" collapsed (manually expanded in 2nd screen shot) and why isn't it atop?

[jrieken]

do you have any thoughts on heuristics we should use to determine whether to auto-expand?

Hard to say because I feel a sting when using debug bug things that I am confident about

1. Never collapse the top stack element
2. Always expand all block scopes and the containing function scope (for-of inside for-in inside function)
3. Expand function scopes up to containing class-function/method (foreach-callback inside call-function)

[jrieken]

Oh and the obvious, which I guess is more on the UI than on the adapter: never collapse something that I have manually expanded.

[isidorn]

Makes sense. The first three would have to handled on the extensions side.
The last one would have to handled on the VS Code side, and that I believe we already do nicely. If this still happens I would be interested in details. The problem for this is establishing scope identity, and as mentioned I use scope name and index for that. So if the scope order remained somewhat stable across steps this should be fine.

[jrieken]

A sample of unwanted (out of the box) collapsing

• use https://github.com/microsoft/vscode-github-issue-notebooks/blob/40fcef8ef4d639fe6b630abe16b153937eb2d335/src/notebookProvider.ts#L124
• attach, run query so that a break point is hit
• step three times
• 🐛 three block scopes, all collapsed, not expanded

[isidorn]

Thanks for steps, so looking at this, all three are block scopes which should be auto expanded by the debug adapter.
Fyi @weinand for adding an autoExpand field to the Scope object in the DAP.

[jrieken]

another case

• breakpoint here https://github.com/microsoft/vscode-github-issue-notebooks/blob/1cf3f313b515525c3bde62300a28e1b4a224f945/src/notebookProvider.ts#L153
• make GH querying throw an errors (e.g query private (now fixed) or non-existent repo)
• top scope collapsed

[connor4312]

Thanks, Joh. Not sure I'll get to this for endgame but I'll aim for a fix early next week 🙂

[isidorn]

Seems like most of the issues are on the js-debug side, thus unassigning myself.
@connor4312 please assign me if something needed.

As for an autoExpand field in the scopes I have created this feature reuqest against the debug adapter protocol microsoft/debug-adapter-protocol#116

[weinand]

Before we start introducing new "UI hints" on the DAP side (which is problematic), could we just try to fully use the existing mechanism? Could we just use the scope's "expensive" attribute to control what's gets expanded, e.g.:

• initial expand state is driven by the "expensive" attribute: only if value is true, scope starts collapsed. If this results in too many expanded scopes, debug extension will have to adapt by adding more "expensive" attributes.
• if user has changed the expand state of a scope, we try to preserve that (by ignoring the "expensive" attribute).

[isidorn]

@weinand we can do that, however that might introduce a lot of breaking behavior. Since too many scopes may be expanded intially. Which would lead to a bad user experience.

[connor4312]

I don't think there's more I can do for js-debug wrt expansion in the current state. For example, the "locals" block that does expand:

{
  "name": "Block: doBusyWork",
  "presentationHint": "locals",
  "expensive": false,
  "variablesReference": 12,
  "line": 8,
  "column": 9,
  "source": {
    "name": "main.js",
    "path": "c:\\Users\\Connor\\Documents\\Github\\vscode-js-debug\\demos\\node\\main.js",
    "sourceReference": 0
  },
  "endLine": 10,
  "endColumn": 6
}

and the catch block that doesn't:

{
  "name": "Catch Block: doBusyWork",
  "presentationHint": "locals",
  "expensive": false,
  "variablesReference": 8,
  "line": 15,
  "column": 11,
  "source": {
    "name": "main.js",
    "path": "c:\\Users\\Connor\\Documents\\Github\\vscode-js-debug\\demos\\node\\main.js",
    "sourceReference": 0
  },
  "endLine": 17,
  "endColumn": 4
}

We send expensive: false on both of those and everything except the location is the same, so it looks like it's vscode's heuristics is causing the collapsing.

[jrieken]

Back to @isidorn based on #93230 (comment) and based on the fact that it is still majorly annoying.

[isidorn]

I can change our heuristic that we expand all non expansive scopes. However I would only do that start of milestone to have a long time to gather feedback since I am not optimistic about that change.
Thus pushing to June when I will change that.

[connor4312]

Maybe always expand the first scope (or the first N) if it's not expensive.

[isidorn]

@connor4312 that's what we do currently. We always expand the first scope if not expensive.

[isidorn]

I have pushed improvements to the Variables view that the view state should be better preserved.
In more detail:

• we now preserve view state per stack frame, so if you have a expansion state you switch to another stack frame and switch back the state should be preserved
• we no longer use the frameId getting from the debugAdapter in computation of the stackFrame id. This id was changing in every step and was not allowing us to correlate stack frames between step events
• we always expand the first non expensive stack frame if all scopes are collapsed
• we always expand the first non expensive stack frame if we never expanded it before

Having all those togethers the behavior is now much improved. There might still be some corner cases, thus still not closing this issue.
@jrieken it would be great if you give some feedback when you try this out starting from tomorrow's vscode insiders.
And sorry for not tackling this sooner!

[isidorn]

No negative feedback thus far, so closing.
If there are issues please let me know and I will reopen. Thanks