[Preview] mis-judge in scope inner locations #4699

Fischer-L · 2017-11-15T14:36:37Z

Associated Issue: #4459

Screenshots/Videos

Fischer-L · 2017-11-15T14:36:56Z

Waiting the test running...

Fischer-L · 2017-11-16T16:20:16Z

@jasonLaster
I need to investigate some mochitest error but didn't find how to run it locally at [1].
That would be great you could show me how to do that at local, thanks

[1] https://github.com/devtools-html/debugger.html/blob/master/docs/local-development.md#testing

jasonLaster · 2017-11-16T16:21:42Z

check out our docs https://github.com/devtools-html/debugger.html/blob/master/docs/mochitests.md

ryanjduffy · 2017-11-16T21:03:19Z

@Fischer-L I don't think including inner scopes of the current scope is the right approach here. By doing so, we've brought the inner function body into scope when it isn't. I'd prefer to convert our out of scope calculation to respect column and line rather than just line. We're already receiving this information from the parser but it's been simplified to lines by the {{linesInScope}} selector.

@jasonLaster - Thoughts?

jasonLaster · 2017-11-16T21:05:06Z

@ryanjduffy i agree that it would be nicer to out of scope lines & columns. I have not looked at this solution though :) so can't speak to what is going on here

Fischer-L · 2017-11-18T08:41:21Z

@Fischer-L I don't think including inner scopes of the current scope is the right approach here. By doing so, we've brought the inner function body into scope when it isn't. I'd prefer to convert our out of scope calculation to respect column and line rather than just line. We're already receiving this information from the parser but it's been simplified to lines by the {{linesInScope}} selector.

OK, this leads questions to discuss:

Is it ok to display the closure variable referenced both from outer and inner scope? With the current approach, will be able to see the closure variable both referenced but unable to see local ones (as undefined) Chrome devtool is doing the similar, please see the below gifs. It is, for sure, wrong to see the local variable's value, which at least should be undefined to the outside.

To allow developers to peek into these closure variables is like providing a convenient shortcut during debugging. Yes, we don't necessarily go like other browsers and could be strict on this but just would lose some friendly ability for developers.

function doA(obj) {
   let result = obj.array && obj.array.map( v => {    // bp
        // convenient to be able to preview `a_closure_var_a_bit_away` on mouse over
        return v + a_closure_var_a_bit_away.base;
   });
   return result
}

Respecting the column is correct I think so too. For doing that, we have to rethink the current approach deciding what is in-scope. A bp only tells us which line we are stop at. We have to decide in/out scope on mouse over so for the below case during counting our-of-scope locations, we only see 2 locations: Line10 -> 14 and Line 11 -> 12 plus one bp at Line 13
And we should not rule out Line 11 -> 12 since there are in-scope codes on Line 11.
Line 11 is potentially in-scope.

function doB(a, b) {                                       // Line 10
   let v = a > 0 ? a : nums.find(v => (v <= b &&           // Line 11
                                 v >= 0 && v % 2 == 0));   // Line 12
   return v * 2;                                           // Line 13, bp
}                                                          // Line 14

Combine (1) and (2). Assume we now respect column, do we still want to provide the convenient shortcut to closure variables to developers?

jasonLaster · 2017-11-18T17:53:56Z

Thanks for the write up! Great points, and i agree that hovering on an inner scope should work. Even though it technically is out of scope...

A bp only tells us which line we are stop at.

Fortunately pause data has the currently line & column. I suggest looking at the selected frame because this has to work regardless of which frame is selected.

Thanks for the hard work here, it's a super interesting problem!

ryanjduffy · 2017-11-21T20:08:10Z

Is the line/column discussion a red herring here, at least partially? The question seems to be "Is the hovered token in scope?" rather than "Is the location in scope?". To your point:

To allow developers to peek into these closure variables is like providing a convenient shortcut during debugging.

It make sense to be able to preview a variable that is in the current scope even if it resides in a different scope. So, in the following example, if I pause on return c(), I should be able to preview b on either line 2 or line 5 but not d even though it's also on line 5.

function a () {
  let b = 1;
  let c = () => {
    let d = 2;
    return b + d;
  }
  return c();
}

Fischer-L · 2017-11-26T09:18:22Z

src/workers/parser/getOutOfScopeLocations.js

  }
-
-  const contains =
-    locations.filter(a => containsLocation(a, location)).length > 0;


This is O(n^2) complexity.
Consider there are 10 locations to test: [ L1, L2, ..., L10 ] and only L7 contains L8 ~ L10.
Loop 1: we get [ L1 ]
Loop 2: we test L2 against [ L1 ]
Loop 3: we test L3 against [ L1, L2 ]
... ...
Loop 8: we test L8 against [ L1, .., L7 ]
Loop 9: we test L9 against [ L1, .., L7 ]
Loop 10: we test L10 against [ L1, .., L7 ]

Fischer-L · 2017-11-26T09:23:41Z

src/workers/parser/getOutOfScopeLocations.js

-    .sort(sortByStart);
+    .sort(sortByStart)
+    .filter(loc => !containsPosition(loc, position));
+  return removeOverlaps(locations);


sort should be O(nlogn). The old removeOverlaps is O(n^2) so the old way was doing O(n^2) then O(nlogn).
Try to improve that, just do once O(nlogn) then the following operations can do O(n)

Fischer-L · 2017-11-26T09:27:55Z

src/workers/parser/tests/__snapshots__/getOutOfScopeLocations.spec.js.snap

-(12, 22) -> (14, 3)
-(16, 16) -> (18, 3)
-(20, 27) -> (22, 3)
-(24, 9) -> (26, 3)


These are in fact the inner locations under the location enclosing the bp so shouldn't be out of scope

Fischer-L · 2017-11-26T09:30:39Z

src/workers/parser/utils/contains.js

 import type { Node } from "babel-traverse";

-function startsBefore(a: AstLocation, b: AstPosition) {
+export function startsBefore(a: AstLocation, b: AstPosition) {


Reuse the function we did in PR #4381 we can do some optimization in removeInnerLocations

Fischer-L · 2017-11-26T09:32:51Z

src/workers/parser/getOutOfScopeLocations.js

  }

-  const contains =
-    locations.filter(a => containsLocation(a, location)).length > 0;


This is O(n^2) complexity.
Consider there are 10 locations to test: [ L1, L2, ..., L10 ] and only L7 contains L8 ~ L10.
Loop 1: we get [ L1 ]
Loop 2: we test L2 against [ L1 ]
Loop 3: we test L3 against [ L1, L2 ]
... ...
Loop 8: we test L8 against [ L1, .., L7 ]
Loop 9: we test L9 against [ L1, .., L7 ]
Loop 10: we test L10 against [ L1, .., L7 ]

Fischer-L · 2017-11-26T09:52:56Z

@jasonLaster Would you have a look the patch, thanks
Now we can handle inner locations under the bp-enclosing location.

In the below test, I also try to step into the inner location. We can see the outer this which should be unknown to the inner is not revealed (thanks to the correct resolution on call stack)

More to discuss:

This patch should help Hover on a variable #1574 somehow (I guess) and the regression on [Previews] some lines are incorrectly marked as out of scope #4369 because now we start handling inner locations
The frame info. Plus the frame info we should be able to do early return at [1] and not even show undefined. That requires some thoughts on the current structure and handling on locations as well so I chose to do it in the future. With this approach at least we are improved and not worse-off.

[1] https://github.com/devtools-html/debugger.html/blob/e12186b556563e67ee8c1dd37094a1c1be5042bb/src/utils/editor/expression.js#L61

Fischer-L · 2017-11-26T09:54:38Z

src/workers/parser/getOutOfScopeLocations.js

+  locations = removeInnerLocations(locations, position).filter(
+    loc => !containsPosition(loc, position)
+  );
+  return removeOverlaps(locations);


sort should be O(nlogn). The old removeOverlaps is O(n^2) so the old way was doing O(n^2) then O(nlogn).
Try to improve that, just do once O(nlogn) then the following operations can do O(n)

jasonLaster · 2017-11-27T00:30:13Z

So... this is a bit hard to follow, looking at our test coverage and the fixture: https://github.com/Fischer-L/debugger.html/blob/4459-mis-judge-in-scope-inner-locations/src/workers/parser/tests/fixtures/outOfScope.js#L1

I wonder if it would help to have new tests that help cover these cases

jasonLaster · 2017-11-27T00:39:44Z

I think it is a bit strange to talk about bp-enclosing functions as opposed to just enclosing function. We want this to be generic and sometimes you're stepping and the original BP is far far away :)

jasonLaster · 2017-11-27T00:46:42Z

src/workers/parser/getOutOfScopeLocations.js

+function removeInnerLocations(locations: AstLocation[], position: AstPosition) {
+  // First, let's find the nearest bp-enclosing function location,
+  // which is to loop locations to find the last location enclosing the bp.
+  let nearestPos = -1;


I think we can replace this logic w/ a lodash helper:

this attempt is likely flawed, but you should see what I have in mind :P

const firstInnerLocation = findIndex(locations, loc => containsPosition(loc, position)

@jasonLaster I was trying to import and utilize lodash. It works fine on the local page but while running the mochi test it fails because

JavaScript error: , line 0: uncaught exception: TypeError: _lodash is undefined JS Error: "uncaught exception: TypeError: _lodash is undefined"]

Tried to search "lodash" under the mochi test folder but found nothing to set. Did I miss some settings?

oh yeah :) that's tricky. I forgot that this is running in a worker

so the typical way to use lodash is import {x} from "lodash" in the worker you need to do import x from "lodash/x" because we can't require lodash at runtime in the worker...

Thanks, now with a lodash findLastIndex

jasonLaster · 2017-11-27T00:46:42Z

src/workers/parser/getOutOfScopeLocations.js

+        break;
+      }
+    }
+  }


can we replace this with a lodash helper?

Now with a lodash findIndex

jasonLaster · 2017-11-27T00:46:42Z

src/workers/parser/getOutOfScopeLocations.js

+  locations = removeInnerLocations(locations, position).filter(
+    loc => !containsPosition(loc, position)
+  );
+  return removeOverlaps(locations);


Fischer-L · 2017-11-27T15:55:43Z

So... this is a bit hard to follow, looking at our test coverage and the fixture: https://github.com/Fischer-L/debugger.html/blob/4459-mis-judge-in-scope-inner-locations/src/workers/parser/tests/fixtures/outOfScope.js#L1
I wonder if it would help to have new tests that help cover these cases

Originally wanted to reuse the outer function. Later because I wanted to make some explicit and might-be-seen case, added another test function for that.

I think it is a bit strange to talk about bp-enclosing functions as opposed to just enclosing function. We want this to be generic and sometimes you're stepping and the original BP is far far away :)

Removed the bp wording in comments

p.s: during the local mochi test, saw the below error. At the 1st look this shouldn't relates to us here, wanna see the result on the server

 TEST-UNEXPECTED-FAIL browser_dbg-chrome-debugging.js:23 
 TypeError: DebuggerServer.addBrowserActors is not a function
     initDebuggerClient browser/devtools/client/debugger/new/test/mochitest/browser_dbg-chrome-debugging.js 23:5
      browser/devtools/client/debugger/new/test/mochitest/browser_dbg-chrome-debugging.js 72:13
     Async*Tester_execTest/< browser-test.js 1065:21
     Tester_execTest browser-test.js 1056:9
     Tester.prototype.nextTest</< browser-test.js 956:9
     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/< tests/SimpleTest/SimpleTest.js 795:59

Fischer-L · 2017-11-28T03:18:29Z

@jasonLaster The test looks fine. Would you look at the patch, thanks?

jasonLaster

Looking great. I’m being cautious because i think it’s important to make this code as readable as possible.

Ideally, any one interested can visualize our algorithms and use it in their own projects :)

basically trying to make it so good it stands on its own merits and could be its own open source project!!!

jasonLaster · 2017-11-30T05:34:04Z

src/workers/parser/getOutOfScopeLocations.js

+        newLocs.push(loc);
+        parentIdx = i;
+      }
+    }


can we use findIndex here too?

jasonLaster · 2017-11-30T05:34:04Z

src/workers/parser/tests/__snapshots__/getOutOfScopeLocations.spec.js.snap

+(29, 16) -> (33, 1)
+(35, 20) -> (37, 1)
+(39, 26) -> (41, 1)
+(43, 20) -> (45, 1)


This might be a crazy idea, but i think it would be fun to write a helper function for our tests so that we can visualize what is in scope and what is out of scope.

for instance, we could write expect(highlightInScopeLocations(..)).toMatchSnapshot() and the locations in the code that are inscope are bolded with https://github.com/chalk/chalk

This would make it really fun to debug this code because you could quickly visualize what the algorithm outputs!

function parentFunc() { let MAX = 3; let nums = [0, 1, 2, 3]; let x = 1; let y = nums.find(function(n) { return n == x; }); function innerFunc(a) { return Math.max(a, MAX); } return innerFunc(y); }

@jasonLaster

Do you mean printing out in-scope function in tests/fixtures/outOfScope.js like this?

// This parentFunc is in-scope so from here codes are in green function parentFunc() { let MAX = 3; let nums = [0, 1, 2, 3]; let x = 1; let y = nums.find(function(n) { return n == x; }); function innerFunc(a) { return Math.max(a, MAX); } return innerFunc(y); } // to here codes are in green

Yep, i might hack on this now that it has landed :)

jasonLaster · 2017-11-30T05:34:04Z

src/workers/parser/getOutOfScopeLocations.js

+      nearestPos + 1
+    );
+    lastInnerPos =
+      firstNotContained < 0 ? locations.length - 1 : firstNotContained - 1;


lets move this to a helper function... that returns lastInnerPos

jasonLaster · 2017-11-30T05:34:05Z

src/workers/parser/getOutOfScopeLocations.js

+  // which is to loop locations to find the last location enclosing the bp.
+  let nearestPos = findLastIndex(locations, loc =>
+    containsPosition(loc, position)
+  );


lets move this out to a helper function too with the comments above the function

codehag · 2017-12-04T15:07:06Z

src/workers/parser/getOutOfScopeLocations.js

+  const newLocs = [];
+  const length = locations.length;
+  if (length) {
+    newLocs.push(locations[0]);


this is easier to read as a reduce:

function deduplicateNode(nodes, location) { const parent = nodes[nodes.length - 1]; if (containsLocation(parent, location)) { return [ ...nodes, location ]; } return nodes; } function removeOverlappingNodes(locations: AstLocation[]) { const firstParent = locations[0]; return locations.reduce(deduplicateNode, [ firstParent ]) }

codehag · 2017-12-04T15:19:31Z

src/workers/parser/getOutOfScopeLocations.js

-  if (!contains) {
-    locations.push(location);
+  // Third, remove those inner functions
+  const newLocs = locations.slice();


why does locations need to be duplicated?

codehag

some comments to make it easier to read, let me know if you need any help or have questions

codehag · 2017-12-04T15:38:00Z

src/workers/parser/getOutOfScopeLocations.js

+  let lastInnerPos = -1;
+  if (nearestPos >= 0) {
+    let parentLoc = locations[nearestPos];
+    let firstNotContained = findIndex(


the naming here is confusing. maybe something like outerBoundaryIndex?

codehag · 2017-12-04T15:38:45Z

src/workers/parser/getOutOfScopeLocations.js

+function removeInnerLocations(locations: AstLocation[], position: AstPosition) {
+  // First, let's find the nearest position-enclosing function location,
+  // which is to loop locations to find the last location enclosing the bp.
+  let nearestPos = findLastIndex(locations, loc =>


this naming is a bit confusing, how about parentIndex or outerStartIndex (because nearest can also be adjacent, and Pos suggests Position similar to the ASTPosition, but this is an index. more specifically it is a boundary)

also we use it twice with +1; it makes sense to have that as a variable, maybe

const outerStartIndex = findLastIndex(locations, loc => containsPosition(loc, position)); const innerStartIndex = outerStartIndex + 1;

codehag · 2017-12-04T15:47:25Z

src/workers/parser/getOutOfScopeLocations.js

+  // once seeing the 1st location not enclosed by the nearest location
+  // to find the last inner locations inside the nearest location.
+  let lastInnerPos = -1;
+  if (nearestPos >= 0) {


Sometimes doing a positive check is making the code harder to read and adds unnecessary complexity. For this reason we usually prefer doing negative checks, because then we can return early. it also saves some work, in this case the memory allocation that is happening on 72 and also the other checks. You also wouldnt need to reassign lastInnerPos which is making this a bit more confusing to read.

Usually you want to use if statements sparingly. they force a reader to remember things, which is always extra mental load.

So maybe something like this would be a good idea:

if (nearestPos < 0) { return locations; }

codehag · 2017-12-04T15:52:17Z

src/workers/parser/getOutOfScopeLocations.js

+      loc => !containsLocation(parentLoc, loc),
+      nearestPos + 1
+    );
+    lastInnerPos =


since this isnt a position but an index. maybe innerBoundaryIndex is clearer?

codehag · 2017-12-04T16:00:55Z

src/workers/parser/getOutOfScopeLocations.js

-    locations.push(location);
+  // Third, remove those inner functions
+  const newLocs = locations.slice();
+  if (nearestPos >= 0 && lastInnerPos >= 0) {


i dont think it is possible for lastInnerPos to be -1, since you have a check on line 68

… complexity

Fischer-L · 2017-12-05T14:58:03Z

src/workers/parser/getOutOfScopeLocations.js

+function removeInnerLocations(locations: AstLocation[], position: AstPosition) {
+  // First, let's find the nearest position-enclosing function location,
+  // which is to loop locations to find the last location enclosing the position.
+  const newLocs = locations.slice();


The reason returning new locations is because:

Both removeOverlaps and Array filter return new locations. Since removeInnerLocations also does removal operation, that is for consistency, not to bring surprise.

Although this is not an exported function, for the removal operation probably less surprise without mutating the input arg. This is not necessary, however, consider the reason 1, consistency is good for less error.

Fischer-L · 2017-12-05T15:00:03Z

src/workers/parser/getOutOfScopeLocations.js

+ *                  so that we can do linear time complexity operation.
+ */
+function removeOverlaps(locations: AstLocation[]) {
+  if (locations.length == 0) {


Important to take care of an empty array. Otherwise

// firstParent will be undefined if locations are empty const firstParent = locations[0];

… locations

Fischer-L · 2017-12-05T16:19:29Z

src/workers/parser/getOutOfScopeLocations.js

+function removeInnerLocations(locations: AstLocation[], position: AstPosition) {
+  // First, let's find the nearest position-enclosing function location,
+  // which is to find the last location enclosing the position.
+  const newLocs = locations.slice();


The reasons returning new locations are because:

Both removeOverlaps and Array filter return new locations. Since removeInnerLocations also does removal operation, that is for consistency, not to bring surprise.

Although this is not an exported function, for the removal operation probably less surprise without mutating the input arg. This is not necessary, however, consider the reason 1, consistency is good for less error.

Fischer-L · 2017-12-05T16:20:20Z

@codehag Updated per comments, thanks

codehag

looks good, thanks!

jasonLaster changed the title ~~4459 - mis-judge in scope inner locations~~ [Preview] mis-judge in scope inner locations Nov 16, 2017

Fischer-L force-pushed the 4459-mis-judge-in-scope-inner-locations branch from a2ad00d to fba6467 Compare November 16, 2017 16:15

Fischer-L force-pushed the 4459-mis-judge-in-scope-inner-locations branch 2 times, most recently from 4f9d2b3 to 9689ebc Compare November 26, 2017 08:50

Fischer-L commented Nov 26, 2017

View reviewed changes

jasonLaster reviewed Nov 27, 2017

View reviewed changes

Fischer-L force-pushed the 4459-mis-judge-in-scope-inner-locations branch from 9689ebc to 28844c4 Compare November 27, 2017 15:48

Fischer-L force-pushed the 4459-mis-judge-in-scope-inner-locations branch from 28844c4 to 42ec620 Compare November 27, 2017 16:06

jasonLaster reviewed Nov 30, 2017

View reviewed changes

codehag reviewed Dec 4, 2017

View reviewed changes

4459 - Part 1: Improve the removeOverlaps function to run linear time…

9b09c94

… complexity

Fischer-L force-pushed the 4459-mis-judge-in-scope-inner-locations branch from 42ec620 to 238b568 Compare December 5, 2017 14:44

Fischer-L commented Dec 5, 2017

View reviewed changes

Fischer-L force-pushed the 4459-mis-judge-in-scope-inner-locations branch from 238b568 to 388fc26 Compare December 5, 2017 15:02

4459 - Part 2: Add the removeInnerLocations function to exclude inner…

c04a93a

… locations

Fischer-L force-pushed the 4459-mis-judge-in-scope-inner-locations branch from 388fc26 to c04a93a Compare December 5, 2017 15:04

Fischer-L commented Dec 5, 2017

View reviewed changes

codehag approved these changes Dec 6, 2017

View reviewed changes

codehag merged commit 2cc6bb7 into firefox-devtools:master Dec 6, 2017

[Preview] mis-judge in scope inner locations #4699

[Preview] mis-judge in scope inner locations #4699

Uh oh!

Conversation

Fischer-L commented Nov 15, 2017

Screenshots/Videos

Uh oh!

Fischer-L commented Nov 15, 2017

Uh oh!

Fischer-L commented Nov 16, 2017

Uh oh!

jasonLaster commented Nov 16, 2017

Uh oh!

ryanjduffy commented Nov 16, 2017

Uh oh!

jasonLaster commented Nov 16, 2017

Uh oh!

Fischer-L commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonLaster commented Nov 18, 2017

Uh oh!

ryanjduffy commented Nov 21, 2017

Uh oh!

Fischer-L Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fischer-L commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

More to discuss:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster commented Nov 27, 2017

Uh oh!

jasonLaster commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fischer-L Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fischer-L commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fischer-L commented Nov 28, 2017

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Fischer-L commented Nov 18, 2017 •

edited

Loading

Fischer-L Nov 26, 2017 •

edited

Loading

Fischer-L commented Nov 26, 2017 •

edited

Loading

jasonLaster commented Nov 27, 2017 •

edited

Loading

Fischer-L Nov 27, 2017 •

edited

Loading

Fischer-L commented Nov 27, 2017 •

edited

Loading

codehag Dec 4, 2017 •

edited

Loading