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

Hovers are not shown correctly for duplicate variables in a block #16632

Closed
sandy081 opened this issue Dec 6, 2016 · 8 comments
Closed

Hovers are not shown correctly for duplicate variables in a block #16632

sandy081 opened this issue Dec 6, 2016 · 8 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verified Verification succeeded
Milestone

Comments

@sandy081
Copy link
Member

sandy081 commented Dec 6, 2016

Testing #15359

See the following snippet

router.get('/', function (req, res, next) {
  let object;
  {
    let object = '5';
    object = 7;
  }
  object = someFunction();
  res.render('index', { title: 'Express' });
});


function someFunction() {
  return [1, 2, 3];
}

While debugging inside the block where object is defined again. Hovering on it is not giving correct value. After coming out of the block, all object hovers are showing the value returned by someFunction(). See the video

dec-06-2016 12-13-58

@sandy081 sandy081 added the debug Debug viewlet, configurations, breakpoints, adapter issues label Dec 6, 2016
@sandy081 sandy081 changed the title Hovers are not shown for duplicate variables in a block Hovers are not shown correctly for duplicate variables in a block Dec 6, 2016
@isidorn
Copy link
Contributor

isidorn commented Dec 6, 2016

This is coming from the node2 debug adapter. Assigning first to @roblourens to investigate

@isidorn isidorn assigned roblourens and unassigned isidorn Dec 6, 2016
@kieferrm
Copy link
Member

kieferrm commented Dec 7, 2016

Here is a different variation of @sandy081's issue. Before the shadowing variable is introduced the line shows the outer-scope value. That makes sense since the debugger doesn't really look ahead. Once the shadowing variable is introduced no value is show for it.

n2elnekzgv

@roblourens
Copy link
Member

Two problems, first the scope locations are not sourcemapped, which is my mistake. I'll fix it today, tracked here - microsoft/vscode-chrome-debug-core#141

But even when I fix that, this line causes an issue - https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/electron-browser/debugHover.ts#L216 - it finds both variables, but they have different values, so the hover isn't shown. I'm not sure why that check exists? I think the first one should always take precedence if the scopes are in the right order.

@isidorn
Copy link
Contributor

isidorn commented Dec 8, 2016

@roblourens that check exists for all the other debug adapters that do not have the scope range information. Shouldn't this line take care of duplicate scopes? Thus only the scope with the correct range will be left?

First one taking precedence would potentially show the wrong debug hover value for other adapters, thus we decided it is better to now show any value at all.

@roblourens
Copy link
Member

The scopes are nested, so all the scopes contain the expression.

@isidorn
Copy link
Contributor

isidorn commented Dec 8, 2016

Then that line needs to be improved in order to find the most specific scope. E.g smallest scope that containes the current line.
Nothing I would change today but we can improve this in january - unless you want to makret this feature in the release notes in that ces we should improve this right now.

@roblourens
Copy link
Member

January is fine with me

@isidorn isidorn added this to the January 2017 milestone Dec 8, 2016
@isidorn isidorn added the bug Issue identified by VS Code Team member as probable bug label Dec 22, 2016
@roblourens
Copy link
Member

roblourens commented Jan 27, 2017

The original issue is verified. There's still a related problem which I think we can't do anything about.

image

This is even easier to see with inline debug values. On console.log(bar), all we know is that it's in the current scope, so we use this scope's bar. And if the redefinition of bar was removed, this would be correct.

@roblourens roblourens added the verified Verification succeeded label Jan 27, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants