Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug Variables View does not show multiple variables from Debug Adapter when names are not unique #107506

Closed
justarandomgeek opened this issue Sep 26, 2020 · 18 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@justarandomgeek
Copy link

Issue Type: Bug

When I provide a DAP Variables response from my debug adapter extension with the following content, reflecting multiple local vars named a, only one of them is shown in the Variables view.

{"variablesReference":1,"seq":11,"vars":[{"name":"<temporaries>","value":"<temporaries>","variablesReference":4},{"name":"a","value":"1","type":"number","variablesReference":0,"evaluateName":"a"},{"name":"a","value":"2","type":"number","variablesReference":0,"evaluateName":"a"},{"name":"a","value":"3","type":"number","variablesReference":0,"evaluateName":"a"},{"name":"a","value":"4","type":"number","variablesReference":0,"evaluateName":"a"}]}

image

VS Code version: Code - Insiders 1.50.0-insider (ddc98c3, 2020-09-25T05:35:02.163Z)
OS version: Windows_NT x64 10.0.18363

System Info
Item Value
CPUs Intel(R) Core(TM) i7-6850K CPU @ 3.60GHz (12 x 3598)
GPU Status 2d_canvas: enabled
flash_3d: enabled
flash_stage3d: enabled
flash_stage3d_baseline: enabled
gpu_compositing: enabled
multiple_raster_threads: enabled_on
oop_rasterization: disabled_off
opengl: enabled_on
protected_video_decode: unavailable_off
rasterization: enabled
skia_renderer: disabled_off_ok
video_decode: enabled
vulkan: disabled_off
webgl: enabled
webgl2: enabled
Load (avg) undefined
Memory (System) 63.91GB (27.54GB free)
Process Argv --crash-reporter-id 1d78045f-7d02-46d4-981d-e8dc1b7d9885
Screen Reader no
VM 0%
Extensions (10)
Extension Author (truncated) Version
bracket-pair-colorizer-2 Coe 0.2.0
vscode-eslint dba 2.1.8
gitlens eam 10.2.2
vscode-pull-request-github Git 0.20.0
git-graph mhu 1.26.0
remote-wsl ms- 0.44.5
indent-rainbow ode 7.4.0
vscode-zipexplorer sle 0.3.1
lua sum 0.20.8
factorio-lua-api-autocomplete svi 0.7.0
@justarandomgeek
Copy link
Author

As a workaround for now, I've started renaming the "shadowed" versions of these variables to alternate names (illegal identifiers to avoid creating more collisions) to differentiate them in the listing.

@isidorn
Copy link
Contributor

isidorn commented Sep 28, 2020

Correct. This is currently not supported. A name must be unique inside it's container - we dedupe the names.
This is a limitation of the vscode implementation since we also use the name for computing the id which has to be unique.

Leaving this open as a fair feature reuqest.

@isidorn isidorn added debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality labels Sep 28, 2020
@isidorn isidorn added this to the Backlog milestone Sep 28, 2020
@isidorn isidorn added the *out-of-scope Posted issue is not in scope of VS Code label Nov 2, 2020
@fabioz
Copy link
Contributor

fabioz commented Jan 22, 2021

I had reported this at: #114757 (it seems I hadn't searched with the proper terms previously).

If this won't be fixed, it'd be nice to document the limitation in the debug adapter protocol specification.

@isidorn
Copy link
Contributor

isidorn commented Jan 22, 2021

fyi @weinand for potentaily updating the doc. We might also accept PRs to update the docs
On second thought this might be just a VS Code limitation, other clients might support this...

@polinasok
Copy link

polinasok commented Mar 16, 2021

+1 I too did not find this issue (probably only searched the open ones) and filed a dup #119136.
If this cannot be supported, please document this limitation. I think it is fine to document this in the spec by saying that some clients (e.g. vscode) may chose to dedup the values. I understand that DAP is meant to be generic, but anybody implementing it would want their adapter to work with vscode as well. Limitations of the most popular client(s) usually become the least common denominator for any implementation and serve as the de-factor documentation anyway.

@isidorn
Copy link
Contributor

isidorn commented Mar 17, 2021

Let's see what @weinand thinks

@weinand
Copy link
Contributor

weinand commented Mar 17, 2021

@polinasok Yes, I will consider to document this issue in the DAP spec.

In general I think that variables with identical names in the same scope (container) are problematic and users will try to avoid them because they destroy the readability of their code. So what we are discussing here is probably a rare corner case.

Ideally VS Code could show them all (no deduping) in the same order they are received in the variables request (assuming that this order reflects the declaration order in the source). With this approach users can correlate what they see in the VARIABLES view with the variables in the source because the order is identical.
@isidorn would it be possible to append a serial number to the variable name when calculating the ID of a tree node?

VS Code's current deduplication behavior is unfortunate because it seems that the value of the deduped variable shows the value of the first variable instead of the value of the last received value. I think this should be fixed.

@isidorn
Copy link
Contributor

isidorn commented Mar 17, 2021

@weinand yes we could append a serial number to the variable. However the serial number would not make the id of the variable stable across steps. Thus resulting in unstable expand / collapse behavior. I would not change this.
We can change to take the last received value if that would improve this experience.

@weinand
Copy link
Contributor

weinand commented Mar 17, 2021

@isidorn with "serial number" I mean a number starting with 1 that gets appended only to the duplicated variable names. So in the example from above the four "a"s would have the unique names "a", "a:1", "a:2", "a:3". And if there is another duplicated set of "b"s they would get their own serial number starting again with 1.

IDs based on these names would be as stable as all other IDs. Basically VS Code would do the "name mangling" is a similar way a compiler/transpiler would do it...

@weinand
Copy link
Contributor

weinand commented Mar 17, 2021

We can change to take the last received value if that would improve this experience.

Yes, that would improve the experience because when stepping through the example from above the value of "a" would change on each step and it would correctly reflect the value of closest "a".

@isidorn
Copy link
Contributor

isidorn commented Mar 17, 2021

@weinand that approach for serial numbers I think makes sense.
However let's first change that when removing duplicates we keep the last one. I can do that change now.

@isidorn
Copy link
Contributor

isidorn commented Mar 17, 2021

Actualy I went with the serial number approach.
So we are no longer de-duping.
I did not test this out. So @fabioz @polinasok please try it out with VS Code insiders from tomorrow and let me know how it goes

isidorn added a commit that referenced this issue Mar 17, 2021
@isidorn isidorn removed the *out-of-scope Posted issue is not in scope of VS Code label Mar 17, 2021
@isidorn isidorn modified the milestones: Backlog, March 2021 Mar 17, 2021
@isidorn isidorn added the verification-needed Verification of issue is requested label Mar 17, 2021
@fabioz
Copy link
Contributor

fabioz commented Mar 17, 2021

I already did dedupe this on the debugger, so, don't really have a repro anymore...

@polinasok
Copy link

The fix did not work for me. I tried with insiders and saw no difference. I then used my fork and synced up to head and tried there with "Launch VS Code" and also saw no difference. And then I was trying to set a breakpoint at the location of the fix in of the debugModel.ts, just to see how it works in action, but now something has gone wrong. And I am getting this error

ERR vscode_proxy_agent_1.createProxyResolver is not a function: TypeError: vscode_proxy_agent_1.createProxyResolver is not a function
	at Object.connectProxyResolver (/Users/polina/vscode/out/vs/workbench/services/extensions/node/proxyResolver.js:12:51)
	at ExtHostExtensionService._beforeAlmostReadyToRunExtensions (/Users/polina/vscode/out/vs/workbench/api/node/extHostExtensionService.js:50:35)
	at processTicksAndRejections (internal/process/task_queues.js:97:5)
	at async ExtHostExtensionService.initialize (/Users/polina/vscode/out/vs/workbench/api/common/extHostExtensionService.js:83:17)

and the UI for my test program appears unusable without the LHS panel:
image
So I cannot provide more info.

If can try this yourself with any recent version of the Go extension and default launch configuration. Use the following Go program.

package main

import (
	"errors"
	"runtime"
)

func main() {
	m1 := map[error]int{
		errors.New("one"):   1,
		errors.New("two"):   2,
		errors.New("three"): 2,
	}
	m2 := map[string]bool{
		"veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery loooooooong string1": true,
		"veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery loooooooong string2": true,
		"short string": true,
	}
	runtime.Breakpoint()
	_, _ = m1, m2
}

Start debugging, hit built-in breakpoint, expand the vars and see the missing entries in the maps.
image

@isidorn
Copy link
Contributor

isidorn commented Mar 22, 2021

@polinasok thanks for trying this out, we also tried this out with mock-debug and it seems to work just fine.

What I suggest you can do as next steps:

  • Fork mock-debug and try to reproduce the issue you are seeing. Once you have a fork I can clone it and investigate more locally
  • Debug vscode as you already tried. I can not reproduce the proxy error you were seeing. But I do see that the activity bar is broken for you due to a const enum non compilation. I suggest to git clean -fxd the vscode repo and rebuild all again yarn && yarn watch

image

@weinand
Copy link
Contributor

weinand commented Mar 22, 2021

@polinasok yes, the fix works for me in mock-debug and I just verified that it works for structured variables too:

2021-03-22_09-59-49

@connor4312 connor4312 added the verified Verification succeeded label Mar 24, 2021
@connor4312
Copy link
Member

This works for me in js-debug as well, so marking as verified

@weinand weinand added the release-notes Release notes issues label Mar 25, 2021
@isidorn isidorn added the on-release-notes Issue/pull request mentioned in release notes label Mar 26, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes release-notes Release notes issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@fabioz @weinand @isidorn @connor4312 @justarandomgeek @polinasok and others