Skip to content

Minor cleanup to symbolWalker #18549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
2 commits merged into from
Sep 22, 2017
Merged

Minor cleanup to symbolWalker #18549

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91211a1
Minor cleanup to symbolWalker
Sep 18, 2017
dc99140
Undo inlining
Sep 18, 2017
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
16 changes: 11 additions & 5 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,11 +994,6 @@ namespace ts {

/**
* Gets the owned, enumerable property keys of a map-like.
*
* NOTE: This is intended for use with MapLike<T> objects. For Map<T> objects, use
Copy link
Author

Choose a reason for hiding this comment

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

Comment outdated; Map<T> is now a real ES6 map, so we would use the .keys() method.

* Object.keys instead as it offers better performance.
*
* @param map A map-like.
*/
export function getOwnKeys<T>(map: MapLike<T>): string[] {
const keys: string[] = [];
Expand All @@ -1011,6 +1006,17 @@ namespace ts {
return keys;
}

export function getOwnValues<T>(sparseArray: T[]): T[] {
const values: T[] = [];
for (const key in sparseArray) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure I'm missing something, but how is this different from a for-of loop?

Copy link
Author

Choose a reason for hiding this comment

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

A for-in loop iterates over the keys of an object, while a for-of loop only works on an object implementing the iterator protocol. See MDN

Copy link
Member

@weswigham weswigham Sep 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's also supposedly identical to Object.values, which, much like hasOwnProperty, which we should probably check for the existence of and use if available.

if (hasOwnProperty.call(sparseArray, key)) {
values.push(sparseArray[key]);
}
}

return values;
}

/** Shims `Array.from`. */
export function arrayFrom<T, U>(iterator: Iterator<T>, map: (t: T) => U): U[];
export function arrayFrom<T>(iterator: Iterator<T>): T[];
Expand Down
74 changes: 30 additions & 44 deletions src/compiler/symbolWalker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,29 @@ namespace ts {
return getSymbolWalker;

function getSymbolWalker(accept: (symbol: Symbol) => boolean = () => true): SymbolWalker {
const visitedTypes = createMap<Type>(); // Key is id as string
const visitedSymbols = createMap<Symbol>(); // Key is id as string
const visitedTypes: Type[] = []; // Sparse array from id to type
const visitedSymbols: Symbol[] = []; // Sparse array from id to symbol

return {
walkType: type => {
visitedTypes.clear();
visitedSymbols.clear();
visitType(type);
return { visitedTypes: arrayFrom(visitedTypes.values()), visitedSymbols: arrayFrom(visitedSymbols.values()) };
try {
visitType(type);
return { visitedTypes: getOwnValues(visitedTypes), visitedSymbols: getOwnValues(visitedSymbols) };
}
finally {
clear(visitedTypes);
clear(visitedSymbols);
}
},
walkSymbol: symbol => {
visitedTypes.clear();
visitedSymbols.clear();
visitSymbol(symbol);
return { visitedTypes: arrayFrom(visitedTypes.values()), visitedSymbols: arrayFrom(visitedSymbols.values()) };
try {
visitSymbol(symbol);
return { visitedTypes: getOwnValues(visitedTypes), visitedSymbols: getOwnValues(visitedSymbols) };
}
finally {
clear(visitedTypes);
clear(visitedSymbols);
}
},
};

Expand All @@ -37,11 +45,10 @@ namespace ts {
return;
}

const typeIdString = type.id.toString();
if (visitedTypes.has(typeIdString)) {
if (visitedTypes[type.id]) {
return;
}
visitedTypes.set(typeIdString, type);
visitedTypes[type.id] = type;

// Reuse visitSymbol to visit the type's symbol,
// but be sure to bail on recuring into the type if accept declines the symbol.
Expand All @@ -66,43 +73,22 @@ namespace ts {
}
}
if (type.flags & TypeFlags.TypeParameter) {
visitTypeParameter(type as TypeParameter);
visitType(getConstraintFromTypeParameter(type as TypeParameter));
Copy link
Member

Choose a reason for hiding this comment

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

I do not really like the inlining of these short functions - it removes the ability to grep for the symbol handling that case, and makes the block less uniform (and potentially makes the outer function more polymorphic). TBQH, if TypeFlags wasn't a flags enum, I'd just have stored these visitors in a map and dispatched on kind; but you can't so 🤷‍♂️ .

}
if (type.flags & TypeFlags.UnionOrIntersection) {
visitUnionOrIntersectionType(type as UnionOrIntersectionType);
forEach((type as UnionOrIntersectionType).types, visitType);
}
if (type.flags & TypeFlags.Index) {
visitIndexType(type as IndexType);
visitType((type as IndexType).type);
}
if (type.flags & TypeFlags.IndexedAccess) {
visitIndexedAccessType(type as IndexedAccessType);
}
}

function visitTypeList(types: Type[]): void {
if (!types) {
return;
}
for (let i = 0; i < types.length; i++) {
visitType(types[i]);
}
}

function visitTypeReference(type: TypeReference): void {
visitType(type.target);
visitTypeList(type.typeArguments);
}

function visitTypeParameter(type: TypeParameter): void {
visitType(getConstraintFromTypeParameter(type));
}

function visitUnionOrIntersectionType(type: UnionOrIntersectionType): void {
visitTypeList(type.types);
}

function visitIndexType(type: IndexType): void {
visitType(type.type);
forEach(type.typeArguments, visitType);
}

function visitIndexedAccessType(type: IndexedAccessType): void {
Expand All @@ -122,7 +108,7 @@ namespace ts {
if (signature.typePredicate) {
visitType(signature.typePredicate.type);
}
visitTypeList(signature.typeParameters);
forEach(signature.typeParameters, visitType);

for (const parameter of signature.parameters){
visitSymbol(parameter);
Expand All @@ -133,8 +119,8 @@ namespace ts {

function visitInterfaceType(interfaceT: InterfaceType): void {
visitObjectType(interfaceT);
visitTypeList(interfaceT.typeParameters);
visitTypeList(getBaseTypes(interfaceT));
forEach(interfaceT.typeParameters, visitType);
forEach(getBaseTypes(interfaceT), visitType);
visitType(interfaceT.thisType);
}

Expand All @@ -161,11 +147,11 @@ namespace ts {
if (!symbol) {
return;
}
const symbolIdString = getSymbolId(symbol).toString();
if (visitedSymbols.has(symbolIdString)) {
const symbolId = getSymbolId(symbol);
if (visitedSymbols[symbolId]) {
return;
}
visitedSymbols.set(symbolIdString, symbol);
visitedSymbols[symbolId] = symbol;
if (!accept(symbol)) {
return true;
}
Expand Down