Source location hover in CALLSTACK view shows bogus path for internal modules

[weinand]

• VSCode Version: 1.8.0 Insiders

The source path starts with the (internal) variable reference used in the debug protocol:

We should find a better path root for this.
Ideally we should use a 'magic world' that could be used in the 'Just my code' feature to filter those modules (see #16918).
But the VS Code frontend should not hardcode this.

[isidorn]

@weinand so how do you propose that this magic word is contributed to vscode? Should it come from the source response?
What should ideally appear on the hover? internal_module/module.js?

[Eli-Goldberg]

@weinand
I think this is a duplicate: #17301
This happens all the time when stepping in.
Thanks.

[weinand]

@isidorn the frontend should not have to know anything about this case. So the 'magic word' would be introduced by the backend (debug adapter) and for the debugger UI it would just appear as a regular path segment.
What is your special frontend logic around this? We should try to eliminate it completely.

[isidorn]

@weinand the frontend currently simply takes the fs path for each uri in the callstack. For internal modules this uri is fake and looks something like 'debug://internal/1056/module.js' so the fs path is 1056/module.js, namely the reference and the name.
Different internal modules need to have different uris so we decided in the past to include the reference when generting this fake uri.

Instead of using a fsPath for internal modules the frontend could use anything coming from the adapter. Should we add something to the protocol or is there already something we can use?

[roblourens]

It seems like the UI should only show module.js even if it uses the reference as a unique ID internally. Why do we need a magic word instead of only showing module.js?

[weinand]

We will continue to show only module.js in the CALLSTACK. But on hover we currently show the full path and the frontend manufactures a (fake) path by using the source ID. This is wrong.

Instead of doing this we should show something that makes clear that module.js is not a file the user will find in his workspace, but is an internal module.

If we do not want to extend the debug protocol, we could manufacture a 'better' fake path that says something like "core_modules/module.js". This would be shown on hover and we could use the core_modules in skipFiles glob patterns as well, e.g. "core_modules/**/*.js". This would be a nice consistent story.

Alternatively we could introduce some optional information attribute on the Source debug type and communicate 'internal core module' from the debug adapter to the hover. For skipFiles we would have to invent a corresponding mechanism so that the user could configure to skip internal core modules.

[isidorn]

As usual, I prefer the more pragmatic approach, the first one you proposed - manufacture a 'better' fake path.
Though I do not have strong feeling and the alternative approach would also work for me here.

[roblourens]

$/module.js, %/module.js, &/module.js? A symbol is simple but might not be clear what it means.

<node_internals>/module.js?

[weinand]

@isidorn as a first step towards a solution I suggest that we try to remove the path magic in the debugger UI.

• When the debug adapter returns a source object without a path, the UI manufactures a path from the sourceReference of the form /1000/module.js. Since sourceReference is an implementation detail of the debug adapter, it does not make sense to surface it to the user.
  -> I suggest to use the value of the name attribute for the hover if path is missing.
• When I return a source object with a path attribute <node_internals>/module.js it shows up in the UI as /<node_internals>/module.js
  -> I suggest to show the path as is, without any modification.
• If I click on a stackframe with a <node_internals>/module.js path, the debugger UI tries to open the path in the editor. This fails which is ok because the debug spec says: "It is not guaranteed that the source exists at this location". However, the UI should not show this as an error if a sourceReference is provided.
  -> I suggest that you try to load the source via the sourceReference if the load via the path fails. (Alternatively you could always try to give priority to the sourceReference, but that would require a protocol doc change).

@isidorn do you see any issues with this approach?

[isidorn]

@weinand thanks for the suggestion, however there are limitations to what we can customise in how our workbench displays a uri.

First thing we need to consider is what internal URI representation does an internal module have. Every resource in the workbench needs to have a unique uri. Once we know that everything flows automtically from there. Currently this uri for internal modules lookes like debug://internal/1002/module.js

How we display paths in the call stack view is fully up to us and we can customise that as we think is best. So yes, the hover in the callstack we can use the name if the path is missing
As for the last point I agree that it always makes sense to give priority to the source reference and this is fully in our control.

In short: all of your suggestions make sense and can be done in the call stack view.

[weinand]

@isidorn You can continue to use your current URI representation for internal modules (as long as you are not persisting soureReferences across sessions because that would be a violation of the protocol spec).
My proposal from above only talks about what the user should see, not how this is implemented.

[weinand]

@isidorn I've delivered feature #16918 for node-debug. Since this change will return a <node_internals>/module.js path for internal modules it will result in the errors explained above in #16913 (comment). So without your changes debugging of internal node modules will be broken with the latest version of node-debug.

[isidorn]

@weinand I tried out your change with my latest changes and it looks good.

We need to update the documentation as previously we would "declare" something an internal module if it does not have a path. Now this has changed, something is an internal module if it has sourceReference > 0 which makes sense IMHO

[weinand]

@isidorn perfect timing!
Yes, I've updated the protocol spec already (but need to push the change).

[isidorn]

@weinand great 😉

[weinand]

@isidorn the protocol spec never talked about 'internal modules'. It only alluded that the content of a source is loaded either via a path or a sourceReference (but left unspecified what happens if both are given).

I tried to make this more precise by tweaking the comments for path and sourceReference:

• path: "The path of the source to be shown in the UI. It is only used to locate and load the content of the source if no sourceReference is specified (or its vaule is 0)."
• sourceReference: "If sourceReference > 0 the contents of the source must be retrieved through the SourceRequest (even if a path is specified). A sourceReference is only valid for a session, so it must not be used to persist a source."

[isidorn]

Looks good to me