Bugfix: find name uses within classes #28
Conversation
There was a problem hiding this comment.
@joyceerhl fantastic! What a sleek fix.
Can you add a unit test for getUses to verify the fix on a minimal class? Can you also run a test to verify that params used in an instance function body aren't registered as "uses" of that param? (Or should they be?)
https://github.com/microsoft/gather/blob/master/src/test/analysis.test.ts#L350
Also, if you haven't already, can you verify the regression tests run fine (jlpm run test), and prettify the source (jlpm run format:all)? If you've already done that, 👍 and thanks ;-)
Edit: and as always, let me know if you want to remote chat about any of this. You rock as a remote chat collaborator, but wanted to make known the offer stands
tsconfig.json
Outdated
| @@ -6,7 +6,7 @@ | |||
| "moduleResolution": "node", | |||
| "target": "es6", | |||
| "outDir": "./lib", | |||
| "lib": ["es6", "dom"], | |||
| "lib": ["es6", "dom", "esnext.array"], | |||
There was a problem hiding this comment.
What does this extra lib in tsconfig.json get us?
There was a problem hiding this comment.
According to this thread, the flatMap function.
Without this, TypeScript complains that Property 'flatMap' does not exist on type 'ISyntaxNode[]'.
There was a problem hiding this comment.
Got it! Possibly stupid question: does this mean this code won't work for browsers that don't support flatMap on array (e.g., Chrome pre-69, Firefox pre-62 according to this MDN page)? Or will flatMap get cross-browser compatible JavaScript by the TypeScript compiler?
If this breaks compatibility with older browsers, we probably want to consider an alternate implementation that doesn't use the flatMap method.
There was a problem hiding this comment.
You're right, I forgot this needs to run in a browser. I don't know much about browser compatibility, would this implementation be sufficiently compatible?
There was a problem hiding this comment.
Maybe we don't even need flatMap? Want to check out the suggested fix in data-flow.ts?
There was a problem hiding this comment.
Yep, I removed the extra lib from the tsconfig!
src/analysis/slice/data-flow.ts
Outdated
| @@ -337,6 +337,11 @@ export class DataflowAnalyzer { | |||
| uses = undefinedRefs.filter(r => r.level == ReferenceType.USE); | |||
| break; | |||
| case ast.CLASS: | |||
| // For each class function, call getUses | |||
| const usesArr = statement.code | |||
| .flatMap(classStatement => this.getUses(classStatement, _)); | |||
There was a problem hiding this comment.
A potential fix is to just factor out flatMap. How about...
| .flatMap(classStatement => this.getUses(classStatement, _)); | |
| .forEach(classStatement => uses.add(...this.getUses(classStatement, _).items); |
There was a problem hiding this comment.
Also lets you remove the line below (uses = ...)
There was a problem hiding this comment.
Great point! Dropped the assignment to usesArr too.
tsconfig.json
Outdated
| @@ -6,7 +6,7 @@ | |||
| "moduleResolution": "node", | |||
| "target": "es6", | |||
| "outDir": "./lib", | |||
| "lib": ["es6", "dom"], | |||
| "lib": ["es6", "dom", "esnext.array"], | |||
There was a problem hiding this comment.
Maybe we don't even need flatMap? Want to check out the suggested fix in data-flow.ts?
https://github.com/microsoft/gather/pull/28/files#diff-570ebd4142e7b14c265ba52bd048c158R379-R404
I might have misunderstood your request--is this what you had in mind?
Done and done--thanks for the reminder! |
For #23
With this PR, the fixed notebook looks like this:
@andrewhead feedback is much appreciated! :-) And thank you for your patience and help so far!