Consolidate function scopes

RESOLVED FIXED in Firefox 70

Status

defect
P1
normal
RESOLVED FIXED
Last year
6 days ago

People

(Reporter: jlast, Assigned: bhackett)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 70
Dependency tree / graph

Firefox Tracking Flags

(firefox70 fixed)

Details

(Whiteboard: [debugger-mvp])

Attachments

(4 attachments, 1 obsolete attachment)

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
Some other cases include

* (`for` might have `let`)
* `catch` creates a scopes
* all breaks with `with()`
Blocks: js-devtools
Priority: -- → P3
Product: Firefox → DevTools
Blocks: dbg-scopes
Blocks: dbg-stepping
No longer blocks: dbg-stepping
Priority: P3 → P2
Blocks: dbg-68
No longer blocks: dbg-68

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.

Assignee: nobody → bhackett1024
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [debugger-mvp]
Posted patch WIPSplinter Review

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.

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.

Duplicate of this bug: 910696
Assignee: bhackett1024 → jlaster

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

Assignee: jlaster → bhackett1024

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.

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

Sure, that's fine.

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

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.

Attachment #9069146 - Attachment is obsolete: true
Blocks: dbg-70
No longer blocks: dbg-69
Pushed by bhackett@mozilla.com:
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.
Status: ASSIGNED → RESOLVED
Closed: 6 days ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
You need to log in before you can comment on or make changes to this bug.