1448166 - Consolidate function scopes

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Jason Laster [:jlast]]

Originally filed: http://github.com/devtools-html/debugger.html/issues/5061

STR:

1. Navigate to  [es6](https://yurydelendik.github.io/old-man-sandbox/sorted-es6/test.es6.html) example
2. Set breakpoint at line 94 of bundle.js
3. Inspect environments/scopes

```js
function comparer(a, b) {
  const ma = /.(\d+)\W*$/.exec(a);
  const mb = /.(\d+)\W*$/.exec(b);
  if (ma == null || mb == null || ma[1] == mb[1]) {
  	return a < b ? -1 : a > b ? 1 : 0;
  } else {
    const na = +ma[1], nb = +mb[1];
    return na < nb ? -1 : na > nb ? 1 : 0; // set breakpoint here
  }
}
```

The function at the specified location contains 3 environments: 

1. function
2. function body
3. if-else.

https://user-images.githubusercontent.com/1523410/34700489-0b22cbca-f4a9-11e7-9bac-7940f7e5aaba.png

Chrome has only 2 environments: function, if-else.

https://user-images.githubusercontent.com/1523410/34724749-a661664e-f514-11e7-97b2-5ae5ec9301fe.png

Does it make sense to consolidate the function & function body scopes into a single scope? 

Scope content will change during stepping through since new environments may appear/disappear. In other words, it's expected to see scopes less granular on the debugger side, and grouped within function boundary.

/cc @jimblandy

Comment 1

[Jason Laster [:jlast]]

Some other cases include

* (`for` might have `let`)
* `catch` creates a scopes
* all breaks with `with()`

Comment 2

[Jim Blandy :jimb]

The consolidation of the function and function body scopes makes perfect sense. Debugger is just exposing what the engine and spec say ought to be there; we should absolutely feel free to merge/omit/re-interpret things however we please.

The only concern is to make sure you've handled all the cases, so that we don't omit necessary information. For example, there can be a definition in the function body with the same name as the function itself. Simply listing them both in a single block would be unclear; the display should reflect the fact that only the body's definition is visible. Etc.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: WIP

This patch works but I'm not very happy with it. The main problem is that there currently isn't a way to distinguish between the top level function lexical scope and inner lexical scopes. The top level function lexical scope isn't created if it's empty, so there isn't a way to tell apart this case:

function foo(a) {
  let x = a;
}

from this one:

function foo(a) {
  if (a) {
    let x = a;
  }
}

This patch deals with this by modifying spidermonkey to give a different scope kind to the top level function lexical scope, which the debugger conveys to the devtools server/client. There are already some special scope kinds like this for try/catch lexical scopes and so forth, but adding more is painful and error prone and it seems best to avoid. This same thing would have to be done for other lexical scopes we want to collapse, like the following:

for (let x in a) {
  let y = x;
}

As an alternative, would it make sense to just collapse all the lexical scopes in a script into a single entry in the scopes pane? The UI would need to distinguish between variables that are shadowed vs. those that aren't, but that seems like it would be a good thing in general, e.g. when stopped in 'bar' below it would be nice to indicate that the 'a' in foo is shadowed.

function foo(a) {
  bar(a);
  function bar(a) {
  }
}

This would be easier to do with the information provided by spidermonkey and seems like it would provide a better UX: users wouldn't have to expand all the various identically-named block scopes in a script to find the variable they are interested in.

Comment 4

[Jason Laster [:jlast]]

As an alternative, would it make sense to just collapse all the lexical scopes in a script into a single entry in the scopes pane? The UI would need to distinguish between variables that are shadowed vs. those that aren't, but that seems like it would be a good thing in general, e.g. when stopped in 'bar' below it would be nice to indicate that the 'a' in foo is shadowed.

I think it makes sense to try and collapse the scopes in the UI or server. It's more presentational this way and we can still support shadowing.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: Bug 1448166 - Consolidate blocks into their enclosing scope, and indicate shadowed variables, r=jlast.

Comment 7

[Jason Laster [:jlast]]

Actually, after looking into it a bit more, I think I'm a fan of what chrome is doing.

Here is a small test case dbg-shadow.

In general, I think it is helpful in this UI to visualize the scopes because they could be meaningful to the users. In this case, top-level const are just not helpful and should be a separate case.

I think your initial patch was good in that it differentiates function lexicals and lets the UI do the merging.

--

We're currently working on inline variable preview bug, doc, which i hope will do a better job of helping users see the most relevant values. But either way, I think the scopes pane should show most block scopes, while somewhere else we help users see the most relevant local values

Comment 8

[Jason Laster [:jlast]]

Brian, if you're fine with direction, would you mind if I ran with your C++ patch? I think it would be a good opportunity to learn more about the engine.

Comment 9

[Jason Laster [:jlast]]

try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=982d2043dc5f7c4be4ae2f77b470e17524d8d5fa

one failing debugger mochitest
devtools/client/debugger/test/mochitest/browser_dbg-scopes-mutations.js

but that is likely because we're looking in the wrong scope now

Comment 10

[Brian Hackett [Laid off!]]

Sure, that's fine.

Comment 11

[Jason Laster [:jlast]]

and some jit-test failures

basic/bug688939.js
basic/bug832203.js
basic/inflate-oom.js
coverage/bug1214548.js
gc/bug-1143706.js
gc/bug-1303015.js
gc/bug-1462337.js
gc/bug-1543589.js
gc/oomInFindPath.js
gc/oomInGetJumpLabelForBranch.js
gc/weak-marking-varying.js

Comment 12

[Jason Laster [:jlast]]

It looks like the jit failures were only local and not on try.

Here is a quick fix for the scopes-mutations failure.

It looks like the try run is green, so there isn't much for me to do here :) I'll hand it back to you so you can get a proper phab review.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Bug 1448166 Part 1 - Add a kind for top level function lexical scopes, r=tcampbell.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: Bug 1448166 Part 2 - Add Debugger.Environment.prototype.scopeKind, r=jorendorff.

Depends on D35043

Comment 15

[Brian Hackett [Laid off!]]

Attachment: Bug 1448166 Part 3 - Merge function body and function lexical scopes in scopes pane, r=jlast.

Depends on D35044

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1059c1d9019
Part 1 - Add a kind for top level function lexical scopes, r=tcampbell.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43145570ca5
Part 2 - Add Debugger.Environment.prototype.scopeKind, r=jimb.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b69083f7aba
Part 3 - Merge function body and function lexical scopes in scopes pane, r=jlast.

Comment 17

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/a1059c1d9019
https://hg.mozilla.org/mozilla-central/rev/f43145570ca5
https://hg.mozilla.org/mozilla-central/rev/2b69083f7aba