Skip to content

Skip unnecessary instatiation of anonymous types #7448

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
ahejlsberg merged 3 commits into from
Mar 9, 2016
Merged

Skip unnecessary instatiation of anonymous types #7448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf97250
Skip unnecessary instatiation of anonymous types
ahejlsberg Mar 9, 2016
1d18136
Adding tests
ahejlsberg Mar 9, 2016
4f441bd
Adding comment to test
ahejlsberg Mar 9, 2016
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
99 changes: 73 additions & 26 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5045,42 +5045,29 @@ namespace ts {
return t => t === source1 ? target1 : t === source2 ? target2 : t;
}

function createTypeMapper(sources: Type[], targets: Type[]): TypeMapper {
switch (sources.length) {
case 1: return createUnaryTypeMapper(sources[0], targets[0]);
case 2: return createBinaryTypeMapper(sources[0], targets[0], sources[1], targets[1]);
}
function createArrayTypeMapper(sources: Type[], targets: Type[]): TypeMapper {
return t => {
for (let i = 0; i < sources.length; i++) {
if (t === sources[i]) {
return targets[i];
return targets ? targets[i] : anyType;
}
}
return t;
};
}

function createUnaryTypeEraser(source: Type): TypeMapper {
return t => t === source ? anyType : t;
}

function createBinaryTypeEraser(source1: Type, source2: Type): TypeMapper {
return t => t === source1 || t === source2 ? anyType : t;
function createTypeMapper(sources: Type[], targets: Type[]): TypeMapper {
const count = sources.length;
const mapper: TypeMapper =
count == 1 ? createUnaryTypeMapper(sources[0], targets ? targets[0] : anyType) :
count == 2 ? createBinaryTypeMapper(sources[0], targets ? targets[0] : anyType, sources[1], targets ? targets[1] : anyType) :
createArrayTypeMapper(sources, targets);
mapper.mappedTypes = sources;
return mapper;
}

function createTypeEraser(sources: Type[]): TypeMapper {
switch (sources.length) {
case 1: return createUnaryTypeEraser(sources[0]);
case 2: return createBinaryTypeEraser(sources[0], sources[1]);
}
return t => {
for (const source of sources) {
if (t === source) {
return anyType;
}
}
return t;
};
return createTypeMapper(sources, undefined);
}

function getInferenceMapper(context: InferenceContext): TypeMapper {
Expand All @@ -5095,6 +5082,7 @@ namespace ts {
}
return t;
};
mapper.mappedTypes = context.typeParameters;
mapper.context = context;
context.mapper = mapper;
}
Expand All @@ -5106,7 +5094,9 @@ namespace ts {
}

function combineTypeMappers(mapper1: TypeMapper, mapper2: TypeMapper): TypeMapper {
return t => instantiateType(mapper1(t), mapper2);
const mapper: TypeMapper = t => instantiateType(mapper1(t), mapper2);
mapper.mappedTypes = mapper1.mappedTypes;

@sandersn sandersn Mar 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In #7138 I had the equivalent of mapper1.mappedTypes.concat(mapper2.mappedTypes) which I'm pretty sure is wrong. But it should really be mapper1.mappedTypes.intersect(mapper2.mappedTypes), I think.

Since this is an optimization, I don't think it will make a practical difference, because constructing a pathological case would be very difficult.

@ahejlsberg ahejlsberg Mar 9, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, from an outside point of view, a combined mapper only re-maps the type parameters of mapper1 (the one in front). The fact that more mapping happens below is really irrelevant. Either way, with the current analysis we should never actually see a combined mapper because that only happens when we're instantiating already instantiated types (and we don't analyse those now).

return mapper;
}

function cloneTypeParameter(typeParameter: TypeParameter): TypeParameter {
Expand Down Expand Up @@ -5201,13 +5191,70 @@ namespace ts {
return result;
}

function isSymbolInScopeOfMappedTypeParameter(symbol: Symbol, mapper: TypeMapper) {
const mappedTypes = mapper.mappedTypes;
// Starting with the parent of the symbol's declaration, check if the mapper maps any of
// the type parameters introduced by enclosing declarations. We just pick the first
// declaration since multiple declarations will all have the same parent anyway.
let node = symbol.declarations[0].parent;
while (node) {
switch (node.kind) {
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructorType:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.Constructor:
case SyntaxKind.CallSignature:
case SyntaxKind.ConstructSignature:
case SyntaxKind.IndexSignature:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction:
case SyntaxKind.ClassDeclaration:
case SyntaxKind.ClassExpression:
case SyntaxKind.InterfaceDeclaration:
case SyntaxKind.TypeAliasDeclaration:
const declaration = <DeclarationWithTypeParameters>node;
if (declaration.typeParameters) {
for (const d of declaration.typeParameters) {
if (contains(mappedTypes, getDeclaredTypeOfTypeParameter(d.symbol))) {

@sandersn sandersn Mar 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why does it work to say d.symbol directly when normally you have to say getSymbolOfNode(node)? (for example, in the thisType case below).

@ahejlsberg ahejlsberg Mar 9, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We know that type parameters are never subject to global symbol merging, so getSymbolOfNode(d) will always be the same as d.symbol. We do the same thing in a few other places in the compiler.

return true;
}
}
}
if (isClassLike(node) || node.kind === SyntaxKind.InterfaceDeclaration) {
const thisType = getDeclaredTypeOfClassOrInterface(getSymbolOfNode(node)).thisType;
if (thisType && contains(mappedTypes, thisType)) {
return true;
}
}
break;
case SyntaxKind.ModuleDeclaration:
case SyntaxKind.SourceFile:
return false;
}
node = node.parent;
}
return false;
}

function instantiateType(type: Type, mapper: TypeMapper): Type {
if (type && mapper !== identityMapper) {
if (type.flags & TypeFlags.TypeParameter) {
return mapper(<TypeParameter>type);
}
if (type.flags & TypeFlags.Anonymous) {
return type.symbol && type.symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.Class | SymbolFlags.TypeLiteral | SymbolFlags.ObjectLiteral) ?
// If the anonymous type originates in a declaration of a function, method, class, or
// interface, in an object type literal, or in an object literal expression, we may need
// to instantiate the type because it might reference a type parameter. We skip instantiation
// if none of the type parameters that are in scope in the type's declaration are mapped by
// the given mapper, however we can only do that analysis if the type isn't itself an
// instantiation.
return type.symbol &&
type.symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.Class | SymbolFlags.TypeLiteral | SymbolFlags.ObjectLiteral) &&
(type.flags & TypeFlags.Instantiated || isSymbolInScopeOfMappedTypeParameter(type.symbol, mapper)) ?
instantiateAnonymousType(<AnonymousType>type, mapper) : type;
}
if (type.flags & TypeFlags.Reference) {
Expand Down
3 changes: 3 additions & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1208,6 +1208,8 @@ namespace ts {
block: Block;
}

export type DeclarationWithTypeParameters = SignatureDeclaration | ClassLikeDeclaration | InterfaceDeclaration | TypeAliasDeclaration;

export interface ClassLikeDeclaration extends Declaration {
name?: Identifier;
typeParameters?: NodeArray<TypeParameterDeclaration>;
Expand Down Expand Up @@ -2286,6 +2288,7 @@ namespace ts {
/* @internal */
export interface TypeMapper {
(t: TypeParameter): Type;
mappedTypes?: Type[]; // Types mapped by this mapper
instantiations?: Type[]; // Cache of instantiations created using this type mapper.
context?: InferenceContext; // The inference context this mapper was created from.
// Only inference mappers have this set (in createInferenceMapper).
Expand Down
Loading