Proposal for new StackFrame.presentationHint hint "externalCode"

[DavidKarlas]

I'm having issue with "[External Code]" in .NET Core and includeAll in StackTrace request https://github.com/Microsoft/vscode-debugadapter-node/issues/117

I have option in CallStack pad where user can on the fly(while program is paused) check or uncheck "Show External Code" which shows or hides external code... Problem is if user enables showing external code and switches to frame that is external and after that switches back to hide external frames indexes of stack frames are all kind of wrong(total number of frames changes, index of current frame changes, frame doesn't exist anymore...)

So what I think should happen is... https://github.com/Microsoft/vscode-debugadapter-node/blob/7fb3cf8/protocol/src/debugProtocol.ts#L1216 should get additional hint(could be "externalCode") this will allow me to always send "includeAll=true" in request and do filtering myself based on if (presentationHint.Has(externalCode)).

/cc: @gregg-miskelly @andrewcrawley

[gregg-miskelly]

Calculating a call stack with includeAll=true is much more expensive than calculating a call stack with external code collapsed. So unless you expect to mostly have JMC filtering off, you don't want to try and do the filtering on the IDE side.

This is how we solve this problem in VS: every frame has a 'frame base' and we search for a frame with the same frame base when switching the setting.

So I would suggest adding:

export interface StackFrame {
...
   // starting stack range of the frame. Can be used to compare stack frames across formatting operations (such as toggling showing external code).
   // can either be a number (for addresses that fit within a json number) or a string (for addresses that are too large)
   frameBase?: number | string;

[andrewcrawley]

How about just giving stack frames an "id", like many other items in the protocol? "frameBase" isn't a very meaningful name for languages / runtimes that don't deal directly with addresses, and it also avoids the "number | string" confusion.

[gregg-miskelly]

There are two possible reasons not to call it id. Neither are super fundamental, but here they are:

1. Pine Zorro would likely want to treat it as a number so we can fill in m_addrMin. So it isn't totally opaque. We could probably just address this through a comment saying that for VS, this should be a numeric 64-bit unsigned integer.
2. Its not as strong as a normal 'id' -- in some future break state it is quite likely that we will provide another frame with the same 'id' which is different.

Another thought -- we could call it something like 'stackLevel' and leave it as an implementation detail if it means depth, stack pointer values, or whatever. And keep the note that its value is finding the same frame after applying different filtering, loading symbols, etc.

[andrewcrawley]

Hang on, a StackFrame already has an integer ID property (used for requesting scopes, etc). It sounds like all we need to do here is have the debugger ensure that that id remains consistent across formatting changes within the same break state.

[gregg-miskelly]

That would be kind of a pain in the but for debug adapters to implement, but I suppose it could be done.

[gregg-miskelly]

I opened https://github.com/OmniSharp/omnisharp-vscode/issues/1865 to try Andrew's idea for the C# extension. Assuming this works okay, I think we should just open the documentation for includeAll to indicate this behavior.

[weinand]

What do you propose to add to the includeAll comment?

[gregg-miskelly]

How does this sound --

Debug adapters that support includeAll should reuse the same stackFrame.Id value for equivalent frames within the same break state session. This allows the UI to preserve the current stack frame when the setting is toggled.

[gregg-miskelly]

FYI I was able to implement Andrew's suggestion of keeping the frame id the same. So I think we can proceed with the plan of just updating the comment. @DavidKarlas we should have a build you can try soon.