Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

Bugfix: find name uses within classes #28

Merged
merged 5 commits into from
Jul 23, 2019
Merged

Bugfix: find name uses within classes #28

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ecd981
Enable flatMap
joyceerhl Jul 23, 2019
f1c877f
Find name uses within classes
joyceerhl Jul 23, 2019
f85780b
Amend comment
joyceerhl Jul 23, 2019
e01fe8c
Don't use flatMap for browser compatibility
joyceerhl Jul 23, 2019
f2359e6
Add unit tests
joyceerhl Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/analysis/slice/data-flow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
andrewhead marked this conversation as resolved.
Show resolved Hide resolved
const usesArr = statement.code
.flatMap(classStatement => this.getUses(classStatement, _));
andrewhead marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A potential fix is to just factor out flatMap. How about...

Suggested change
.flatMap(classStatement => this.getUses(classStatement, _));
.forEach(classStatement => uses.add(...this.getUses(classStatement, _).items);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also lets you remove the line below (uses = ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! Dropped the assignment to usesArr too.

// Don't reduce an empty array
uses = usesArr.length > 0 ? usesArr.reduce((prev, curr) => prev.union(curr)) : uses;
break;
default: {
const usedNames = gatherNames(statement);
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"moduleResolution": "node",
"target": "es6",
"outDir": "./lib",
"lib": ["es6", "dom"],
"lib": ["es6", "dom", "esnext.array"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this extra lib in tsconfig.json get us?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this thread, the flatMap function.

Without this, TypeScript complains that Property 'flatMap' does not exist on type 'ISyntaxNode[]'.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don't even need flatMap? Want to check out the suggested fix in data-flow.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I removed the extra lib from the tsconfig!

"allowJs": true,
"allowSyntheticDefaultImports": true,
"typeRoots": ["./node_modules/@types"]
Expand Down