Parse to extract all scopes and bindings. #3852

yurydelendik · 2017-09-01T21:17:22Z

Associated Issue: #3817

Summary of Changes

allows to send source text independent from parsing of the scopes
parsing scopes to extract positions and its scope of every symbol in the source code

codehag

Phew! great work here. a few comments. let me know if you have questions

codehag · 2017-09-06T18:29:09Z

src/utils/parser/getScopes.js

+  parent: TempScope | null,
+  loc: BabelLocation
+): TempScope {
+  let result = {


this should be const

codehag · 2017-09-06T18:34:23Z

src/utils/parser/getScopes.js

+    parent,
+    scopes: [],
+    loc: loc,
+    names: Object.create(null)


why do we want to use Object.create here?

codehag · 2017-09-06T18:35:19Z

src/utils/parser/getScopes.js

+function _parseDeclarator(
+  declaratorId: Node,
+  targetScope: TempScope,
+  kind: string


type might be a bit clearer here

codehag · 2017-09-06T18:35:31Z

src/utils/parser/getScopes.js

+  let result = {
+    type,
+    parent,
+    scopes: [],


this might be renamed children?

codehag · 2017-09-06T18:48:20Z

src/utils/parser/getScopes.js

+  name: string,
+  loc: BabelLocation
+) {
+  for (let s = scope; s; s = s.parent) {


it is hard to tell what is going on here. if i am guessing i would say we are traversing the temp scope s via the parents, as long as we have a truthy s object. and within this loop we are taking... the first? name from the array names, and pushing the start location. probably to add up the locations until we have a line number value.

I gave it a try myself... not sure it is any better

traverseTempScope(tempScope, callback) { function _recurse(s) { if (!s) return; if (name in s.names) { callback(s.names[name].refs) } return _recurse(s.parent, callback); } } function _recordIdentifierPosition(scope, name, loc) { _traverseTempScope( scope, (nameRefs) => nameRefs.push(loc.start) ) }

The recursive functions (even with tails call) are not really efficient in comparison with just a loop. I recommend to not introduce any unneeded recursion for simple algorithms, and in this case simple loop easier to read.

codehag · 2017-09-06T20:21:13Z

src/utils/parser/getScopes.js

+  let search = parsedScopes;
+  let found = [];
+  while (search) {
+    let foundOne = search.some(s => {


search.some stops execution after one is found; but we are modifying the found array here and the search object. perhaps what we want here again is reduce?

const found = search.reduce((found, s) => { if ( _compareLocations(s.start, location) <= 0 && _compareLocations(location, s.end) < 0 ) { return [s, ...found]; } return found; })

you can also factor that out into an independant function and do (imagining that the reduce is its own function, and the map is another):

return search.reduce(findLocations).map(createScope);

codehag · 2017-09-06T20:23:17Z

src/utils/parser/sources.js

+
+import type { Source, SourceId } from "debugger-html";
+
+var cachedSources = new Map();


why do we want to use var here? would let work?

codehag · 2017-09-06T20:23:45Z

src/utils/parser/getScopes.js

+  return a.line == b.line ? a.column - b.column : a.line - b.line;
+}
+
+var parsedScopesCache = new Map();


we are using var here, is there a particular reason? i would expect let to be used in this case

codehag · 2017-09-06T20:24:17Z

src/utils/parser/sources.js

+  if (!cachedSources.has(sourceId)) {
+    throw new Error(`${sourceId} was not provided.`);
+  }
+  // $FlowIgnore


why are we ignoring here?

Even if I did the check above, flow is convinced cacheSources.get can be undefined. Changing to ((x: any): Source) cast.

codehag · 2017-09-06T20:26:47Z

src/utils/parser/tests/getScopes.spec.js

+
+describe("Parser.getScopes", () => {
+  it("parses simple generated/minified es5 source", () => {
+    const source = {


nit: we can use the parser's test helper and save this in its own file i think

its used via

import { getSource } from "../helpers"; //... const source = getSource("getScopes/source1");

codehag · 2017-09-06T21:39:55Z

src/utils/parser/getScopes.js

+  });
+}
+
+function _parseJSScopes(source: Source) {


what do the _ signify in these function names?

probably private

thats what i thought, but wouldn't it be private by default, since it isnt exported?

yurydelendik · 2017-09-08T20:06:55Z

Moved scope parsing related code to the firefox-devtools/devtools-core#656

jasonLaster

Looks like a good start.

I don’t see where getScopes is used yet.

I mentioned this in a comment, but I think we can open a separate issue / PR for switching to a sources cache. I like it, but it is tough to review both changes at once

jasonLaster · 2017-09-08T20:48:39Z

src/utils/parser/sources.js

+
+export function clearSources() {
+  cachedSources = new Map();
+}


almost all of our APIs now expect to get a source.

I agree that this is a performance issue, but I think we should make the fix in a separate PR, that touches all of the spots.

https://github.com/devtools-html/debugger.html/blob/master/src/utils/parser/steps.js#L6-L8
https://github.com/devtools-html/debugger.html/blob/master/src/utils/parser/getSymbols.js#L335-L336

#3953

codecov · 2017-09-13T22:46:11Z

Codecov Report

Merging #3852 into master will decrease coverage by 0.04%.
The diff coverage is 25%.

@@            Coverage Diff            @@
##           master   #3852      +/-   ##
=========================================
- Coverage   55.05%     55%   -0.05%     
=========================================
  Files         132     132              
  Lines        5115    5123       +8     
  Branches     1051    1052       +1     
=========================================
+ Hits         2816    2818       +2     
- Misses       2299    2305       +6

Impacted Files	Coverage Δ
src/actions/sources.js	`76.92% <0%> (-3.08%)`	⬇️
src/actions/navigation.js	`10% <0%> (-0.53%)`	⬇️
src/utils/parser/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b63f2e...ba8877d. Read the comment docs.

yurydelendik · 2017-09-13T22:48:51Z

Depends on #4008

jasonLaster

👍

codehag reviewed Sep 6, 2017

View reviewed changes

yurydelendik force-pushed the getScopes2 branch from e0f9e89 to 827424c Compare September 7, 2017 21:19

yurydelendik mentioned this pull request Sep 8, 2017

Implements getScope parsing in the devtools-map-bindings firefox-devtools/devtools-core#656

Merged

yurydelendik force-pushed the getScopes2 branch from 827424c to 797f6a4 Compare September 8, 2017 20:06

jasonLaster reviewed Sep 8, 2017

View reviewed changes

yurydelendik force-pushed the getScopes2 branch from 797f6a4 to b476c20 Compare September 13, 2017 22:46

jasonLaster force-pushed the getScopes2 branch from d61366c to ff305fb Compare September 14, 2017 20:55

Parse to extract all scopes and bindings.

ba8877d

jasonLaster force-pushed the getScopes2 branch from ff305fb to ba8877d Compare September 14, 2017 21:23

jasonLaster approved these changes Sep 14, 2017

View reviewed changes

jasonLaster merged commit 07107b7 into firefox-devtools:master Sep 14, 2017


		import type { Source, SourceId } from "debugger-html";

		var cachedSources = new Map();

Parse to extract all scopes and bindings. #3852

Parse to extract all scopes and bindings. #3852

Uh oh!

Conversation

yurydelendik commented Sep 1, 2017

Summary of Changes

Uh oh!

codehag 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!

codehag Sep 6, 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!

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!

yurydelendik commented Sep 8, 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!

codecov bot commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yurydelendik commented Sep 13, 2017

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codehag Sep 6, 2017 •

edited

Loading

codecov bot commented Sep 13, 2017 •

edited

Loading