4369-[Previews] some lines are incorrectly marked as out of scope #4381

Fischer-L · 2017-10-16T15:07:21Z

Associated Issue: #4369

Screenshots/Videos

Fischer-L · 2017-10-16T15:10:18Z

src/workers/parser/utils/contains.js

+    // We do the same check as the startsBefore case for the endsAfter case.
+    endsAfter =
+      a.end.column >= 0 && b.column >= 0 ? a.end.column >= b.column : true;
+  }


@jasonLaster Thanks for the gist hint.

Before proceeding to the test, would like to check with you if this solution is the right direction since this touches the util function. Below is my analysis, please have a look, thank you:

While getting out-of-scope locations, it uses the parsed source locations and the bp position to filter out those not contained within scope [1]
The bp position only has the line criterion [2] and the source locations have one location as [3].
During filtering, the containsPosition doesn’t consider the case without the column criterion.
As a result [4]

// This evaluates to false although a.start.line === b.line === 46, // because a.start.column <= b.column is false (b.column === undefined) startsBefore = (a.start.line === b.line && a.start.column <= b.column);

then the line 46 where the bp exists is judged as out of scope.

So the solution is to take the case without column criterion into consideration.

[1] https://github.com/devtools-html/debugger.html/blob/c5177581a8af4b797f9aa56f82214216de0176c6/src/workers/parser/getOutOfScopeLocations.js#L86

[2] Object { sourceId: "server1.conn26.child2/source32", line: 46 }

[3] {
end: Object { line: 48, column: 3 }
start: Object { line: 46, column: 30 }
}

[4] https://github.com/devtools-html/debugger.html/blob/c5177581a8af4b797f9aa56f82214216de0176c6/src/workers/parser/utils/contains.js#L9

jasonLaster · 2017-10-16T15:24:45Z

Thanks @Fischer-L This looks really good! I think it will help document the fix if you could add some unit tests.

parser/tests/contains.spec.js
some tests that document the basic behavior and some others that show the edge case that you fixed

Fischer-L · 2017-10-19T13:50:45Z

@jasonLaster The test is added. Please have a look thanks.

jasonLaster · 2017-10-19T14:04:58Z

@Fischer-L looks like there are some small test issues

jasonLaster

Great job!

jasonLaster · 2017-10-19T14:12:33Z

src/workers/parser/utils/contains.js

+    // If the start line is equal to the target line
+    // and there is no column criterion, we judge it as true.
+    // Otherwise we have to check if the start column is really before the target colimn.
+    startsBefore =


perhaps we need two smaller functions:

function startsBefore(a, b) { if (!(a.start.column > 0 && b.column)) { return true } return a.start.column <= b.column; }

Definitely, will update

Fischer-L · 2017-10-20T05:10:17Z

src/workers/parser/utils/contains.js

+  let endsAfter = a.end.line > b.line;
+  if (!endsAfter && a.end.line === b.line) {
+    endsAfter = endsAfterColumn(a, b);
+  }


@jasonLaster I was thinking the following as well (but seems not really different from the current one):

function startsBefore(a: AstLocation, b: AstPosition) { let startsBefore = a.start.line < b.line; if (a.start.line === b.line && a.start.column && b.column) { startsBefore = a.start.column <= b.column; } return startsBefore; } function endsAfter(a: AstLocation, b: AstPosition) { // The similar codes as startsBefore } export function containsPosition(a: AstLocation, b: AstPosition) { return startsBefore(a, b) && endsAfter(a, b); }

Please let me know which is better you think, thanks

I like it better :)

function startsBefore(a: AstLocation, b: AstPosition) { if (a.start.line === b.line && a.start.column && b.column) { return a.start.column <= b.column; } return a.start.line < b.line; }

it's easier for me to read

amitzur · 2017-10-20T13:32:12Z

I think this is not fully accurate. In the example above line 47 is actually out of scope. With this code it is considered in scope, so when you hover over todo in line 47 you see undefined in the preview, which is not good. It's not defined since it's out of scope.

Fischer-L · 2017-10-21T09:26:34Z

I think this is not fully accurate. In the example above line 47 is actually out of scope. With this code it is considered in scope, so when you hover over todo in line 47 you see undefined in the preview, which is not good. It's not defined since it's out of scope.

I would say Line 47 is in scope and todo is not initialized and not visible from outside. When entering toggleAll function, that function scope is on. For some not-yet-initialized variable, undefined for them should be expected.

See another case:

go to todomvc-react
add a bp in TodoModel.js#32 (the 1st line inside app.TodoModel.prototype.addTodo )
Add one todo in todomvc-react
Observe the moment we stop at the bp in TodoModel.js#32
We can see the variables inside that function scope are observable and no-yet-initialized variables are undefined.

Consider another case

function doA(n) {
   let result = doSomeJob({ id: n + 1  }); // Set a bp A here
   return result;
}
function doB(n) {
   let result = doSomeJob( // Set a bp B here
      { id: n + 1  }
   );
   return result;
}

doA and doB are totally the same just different in line breaks. When stopping at the bp A, { id: n + 1 } is in scope but undefined because it is not yet initialized. What about stopping at the bp B? The situation should be the same for the bp B.

Chrome is doing the same thing and just not displaying it

Fischer-L · 2017-10-21T09:57:15Z

@jasonLaster Updated to use the 2nd approach. Please look thank you

Fischer-L · 2017-10-21T10:47:28Z

@jasonLaster and @amitzur
Elaborate why go updating contains.js to fix this issue.
This is a fix without affecting the total architecture plus improving contains.js.

For the whole issue, there are 2 aspects to consider:

Currently, containsPosition may face the condition without the column criterion but it didn't handle that so the issue happened. Maybe we could fix this issue using another approach. However, containsPosition still should be improved in case the similar issue happens again. As a result, no matter what, we should improve containsPosition anyway. Go for it to fix this issue is more reasonable.
What if try to fix this issue from the in-scope angle? The thing will get complicated. The current architecture takes out-of-scope angle, which is to remove those not in scope given by containsPosition. There are some options for the in-scope way, for instances:
- Touch getInScopeLines[1] to consider the case with the same line but out-of-scope column. This approach is to re-translate the job of getInScopeLines, which is like saying getInScopeLines only cares the same line and column doesn't matter. To some degree that's acceptable(but maybe still debatable). However we now are facing a problem that we don't have the bp info.
  [1] https://github.com/devtools-html/debugger.html/blob/085c871b1dd6b84956525cca1b08ed0bac8b4ace/src/selectors/linesInScope.js#L22
- Do in-scope from the ground up. OK, then we have to touch actions and reducers [2] and more architecture, which is too risky and requires deeper and broader analysis.
  [2] https://github.com/devtools-html/debugger.html/search?utf8=%E2%9C%93&q=OUT_OF_SCOPE_LOCATIONS&type=
- Add fix in other places, I can foresee some side-effect or impact to the architecture.

Fischer-L · 2017-10-21T13:32:08Z

I think this is not fully accurate. In the example above line 47 is actually out of scope. With this code it is considered in scope, so when you hover over todo in line 47 you see undefined in the preview, which is not good. It's not defined since it's out of scope.

I would say Line 47 is in scope and todo is just not yet initialized. When entering toggleAll function, that function scope is on. For some not-yet-initialized variable, undefined for them should be expected.

Correct myself: Precisely speaking todo is not initialized and not visible from outside. But for the closure variables Utils.extend and checked should be visible.

jasonLaster · 2017-10-24T03:07:12Z

@Fischer-L could you fix up the tests so we can land this fix. 👍

Fischer-L · 2017-10-24T13:34:09Z

src/actions/tests/sources.spec.js


    const locations = getOutOfScopeLocations(getState());
-    expect(locations.length).toEqual(1);
+    expect(locations.length).toEqual(0);


This tests loads a fake "foo1" source, which is [1] and set a bp at Line 1. Originally it judged this was out of scope but that contradicts what we wanna fix in this bug. In this bug this case in fact should be ruled as in-scope, otherwise the similar case [2] would be out of scope too, so update the expected test result.

[1] "function foo1() {\n return 5;\n}": https://github.com/devtools-html/debugger.html/blob/9a2977bdb5bbb3a85effd63a315d716d9656fda3/src/actions/tests/helpers/threadClient.js#L71

[2]

let obj = { bar: "bar", foo: function foo1() { return 5; } };

No longer have to update sources.spec.js test because it has been modified by PR #4143 to include the column criterion.

Fischer-L · 2017-10-24T13:36:10Z

src/workers/parser/utils/contains.js

+  let startsBefore = a.start.line < b.line;
+  if (a.start.line === b.line) {
+    startsBefore =
+      a.start.column >= 0 && b.column >= 0 ? a.start.column <= b.column : true;


The getOutOfScopeLocations.spec.js test shows "0"[1] matters so >= 0 is required.

[1] https://github.com/devtools-html/debugger.html/blob/c5177581a8af4b797f9aa56f82214216de0176c6/src/workers/parser/tests/getOutOfScopeLocations.spec.js#L29

Fischer-L · 2017-10-24T13:37:08Z

@Fischer-L could you fix up the tests so we can land this fix.

Definitely, updated.

jasonLaster · 2017-10-24T13:39:03Z

@Fischer-L still seeing some errors

Fischer-L · 2017-10-24T13:47:59Z

@Fischer-L still seeing some errors

Sorry. This was embarrassing, didn't notice some eslint error out there... Fixed

Fischer-L · 2017-10-24T14:44:06Z

@jasonLaster The tests are fine. Please have a look, thanks

jasonLaster · 2017-10-25T03:44:51Z

Hey @Fischer-L I pushed some opinionated test refactorings. The goal was to make them more readable from the outside, ideally show all the permutations in code as well as the overview.

Let me know what you think. By the way, the tests really are great :) thanks for the coverage.

Tests 💃

Fischer-L · 2017-10-25T11:57:35Z

@jasonLaster Thank you for the update. Looks nice.

Fischer-L · 2017-10-25T11:59:38Z

src/workers/parser/tests/contains.spec.js

+
+describe("containsPosition", () => {
+  describe("location and postion both with the column criteria", () => {
+    it("should contain position within the location range", () =>


Was prettier moving the test description up again? I didn't touch the line break...
Only made testContains(startPos(-1, 0), false)) wrapped inside the arrow function.

Fischer-L · 2017-10-25T12:08:26Z

src/workers/parser/tests/contains.spec.js

+    it("should not contain position out of the end column", () =>
+      testContains(endPos(0, 1), false));
+
+    it("should contain position on the same end line and within the end column", () =>


@jasonLaster the eslint complains about a over-80-length line but looks like it was prettier which formatted this line...

jasonLaster · 2017-10-25T12:56:56Z

hmm, yeah... i'll see what we can do about that :)

Fischer-L commented Oct 16, 2017

View reviewed changes

jasonLaster mentioned this pull request Oct 18, 2017

[Previews] Fix computation of out of scope locations #4428

Closed

Fischer-L force-pushed the 4369-incorrect-line-out-of-scope branch 3 times, most recently from ff580f4 to 001e9eb Compare October 19, 2017 13:46

jasonLaster reviewed Oct 19, 2017

View reviewed changes

Fischer-L force-pushed the 4369-incorrect-line-out-of-scope branch 2 times, most recently from e2e2160 to e1de587 Compare October 20, 2017 05:09

Fischer-L commented Oct 20, 2017

View reviewed changes

Fischer-L force-pushed the 4369-incorrect-line-out-of-scope branch from e1de587 to 7c15683 Compare October 21, 2017 09:52

Fischer-L mentioned this pull request Oct 21, 2017

[Previews] some lines are incorrectly marked as out of scope #4369

Closed

Fischer-L force-pushed the 4369-incorrect-line-out-of-scope branch from 7c15683 to 9668043 Compare October 24, 2017 13:21

Fischer-L commented Oct 24, 2017

View reviewed changes

Fischer-L force-pushed the 4369-incorrect-line-out-of-scope branch from 9668043 to e81763b Compare October 24, 2017 13:46

jasonLaster force-pushed the 4369-incorrect-line-out-of-scope branch 2 times, most recently from 451a47a to 2a401ce Compare October 25, 2017 03:43

jasonLaster force-pushed the 4369-incorrect-line-out-of-scope branch 3 times, most recently from da7fdd0 to f57e1f5 Compare October 25, 2017 04:28

Fischer-L force-pushed the 4369-incorrect-line-out-of-scope branch from f57e1f5 to 84dca80 Compare October 25, 2017 11:55

Fischer-L commented Oct 25, 2017

View reviewed changes

4369-[Previews] some lines are incorrectly marked as out of scope

397ea02

jasonLaster force-pushed the 4369-incorrect-line-out-of-scope branch from 84dca80 to 397ea02 Compare October 26, 2017 15:45

jasonLaster merged commit 3f579fb into firefox-devtools:master Oct 26, 2017

Fischer-L mentioned this pull request Nov 26, 2017

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

Merged

4369-[Previews] some lines are incorrectly marked as out of scope #4381

4369-[Previews] some lines are incorrectly marked as out of scope #4381

Uh oh!

Conversation

Fischer-L commented Oct 16, 2017

Screenshots/Videos

Uh oh!

Fischer-L Oct 16, 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!

jasonLaster commented Oct 16, 2017

Uh oh!

Fischer-L commented Oct 19, 2017

Uh oh!

jasonLaster commented Oct 19, 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!

amitzur commented Oct 20, 2017

Uh oh!

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

Uh oh!

Uh oh!

Fischer-L commented Oct 21, 2017

Uh oh!

Fischer-L commented Oct 21, 2017

Uh oh!

Fischer-L commented Oct 21, 2017

Uh oh!

jasonLaster commented Oct 24, 2017

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 Oct 24, 2017

Uh oh!

jasonLaster commented Oct 24, 2017

Uh oh!

Fischer-L commented Oct 24, 2017

Uh oh!

Fischer-L commented Oct 24, 2017

Uh oh!

jasonLaster commented Oct 25, 2017

Tests 💃

Uh oh!

Fischer-L commented Oct 25, 2017

Uh oh!

Fischer-L Oct 25, 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!

jasonLaster commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fischer-L Oct 16, 2017 •

edited

Loading

Fischer-L commented Oct 21, 2017 •

edited

Loading

Fischer-L Oct 25, 2017 •

edited

Loading