Skip to content
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

Jump to bottom

Use objects instead of closures for type mappers #36576

Merged
merged 11 commits into from
Mar 11, 2020
Merged

Use objects instead of closures for type mappers #36576

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
63338f9
Use objects instead of closures for type mappers
ahejlsberg Feb 2, 2020
979bc6a
Flatten combined type mappers
ahejlsberg Feb 2, 2020
82cd7a1
Merge branch 'master' into typeMappersAsObjects
ahejlsberg Mar 5, 2020
8e6a2b1
Single point of creation for type mappers
ahejlsberg Mar 5, 2020
c7d3806
More optimizations
ahejlsberg Mar 5, 2020
163ba2f
Fix lint error
ahejlsberg Mar 5, 2020
61f0c7a
Fewer symbol instantiations / discard type mapper after instantiation
ahejlsberg Mar 5, 2020
50de2db
More optimizations
ahejlsberg Mar 6, 2020
ce9ddf3
Simplify mapper layout and cache in composite mappers
ahejlsberg Mar 8, 2020
30e7a18
Removing cache as it doesn't seem to matter much
ahejlsberg Mar 8, 2020
1cb54e1
Get rid of identityMapper
ahejlsberg Mar 10, 2020
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
118 changes: 73 additions & 45 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,6 @@ namespace ts {
let currentNode: Node | undefined;

const emptySymbols = createSymbolTable();
const identityMapper: (type: Type) => Type = identity;
const arrayVariances = [VarianceFlags.Covariant];

const compilerOptions = host.getCompilerOptions();
Expand Down Expand Up @@ -729,6 +728,10 @@ namespace ts {
const keyofConstraintType = keyofStringsOnly ? stringType : stringNumberSymbolType;
const numberOrBigIntType = getUnionType([numberType, bigintType]);

const identityMapper: TypeMapper = makeUnaryTypeMapper(anyType, anyType);
amcasey marked this conversation as resolved.
Show resolved Hide resolved
const restrictiveMapper: TypeMapper = makeFunctionTypeMapper(t => t.flags & TypeFlags.TypeParameter ? getRestrictiveTypeParameter(<TypeParameter>t) : t);
const permissiveMapper: TypeMapper = makeFunctionTypeMapper(t => t.flags & TypeFlags.TypeParameter ? wildcardType : t);

const emptyObjectType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined);
const emptyJsxObjectType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined);
emptyJsxObjectType.objectFlags |= ObjectFlags.JsxAttributes;
Expand Down Expand Up @@ -4957,7 +4960,7 @@ namespace ts {
const params = getTypeParametersOfClassOrInterface(
parentSymbol.flags & SymbolFlags.Alias ? resolveAlias(parentSymbol) : parentSymbol
);
typeParameterNodes = mapToTypeNodes(map(params, (nextSymbol as TransientSymbol).mapper!), context);
typeParameterNodes = mapToTypeNodes(map(params, t => getMappedType(t, (nextSymbol as TransientSymbol).mapper!)), context);
}
else {
typeParameterNodes = typeParametersToTypeParameterDeclarations(symbol, context);
Expand Down Expand Up @@ -9636,7 +9639,7 @@ namespace ts {
// Create a mapper from T to the current iteration type constituent. Then, if the
// mapped type is itself an instantiated type, combine the iteration mapper with the
// instantiation mapper.
const templateMapper = combineTypeMappers(type.mapper, createTypeMapper([typeParameter], [t]));
const templateMapper = addTypeMapping(type.mapper, typeParameter, t);
// If the current iteration type constituent is a string literal type, create a property.
// Otherwise, for type string create a string index signature.
if (isTypeUsableAsPropertyName(t)) {
Expand Down Expand Up @@ -9689,6 +9692,7 @@ namespace ts {
type = errorType;
}
symbol.type = type;
symbol.mapper = undefined!;
amcasey marked this conversation as resolved.
Show resolved Hide resolved
}
return symbol.type;
}
Expand Down Expand Up @@ -13538,30 +13542,50 @@ namespace ts {
return instantiateList<Signature>(signatures, mapper, instantiateSignature);
}

function makeUnaryTypeMapper(source: Type, target: Type) {
return (t: Type) => t === source ? target : t;
function createTypeMapper(sources: readonly TypeParameter[], targets: readonly Type[] | undefined): TypeMapper {
return sources.length === 1 ? makeUnaryTypeMapper(sources[0], targets ? targets[0] : anyType) :
amcasey marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I kinda-sorta wanna preemptively make this a loop rather than a recursive function to better optimize the recursive cases, but... it's not strictly required.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at that, but just makes the code more complex for no appreciable gain.

sources.length === 2 ? makeSimpleTypeMapper(sources[0], targets ? targets[0] : anyType, sources[1], targets ? targets[1] : anyType) :
makeArrayTypeMapper(sources, targets);
}

function makeBinaryTypeMapper(source1: Type, target1: Type, source2: Type, target2: Type) {
return (t: Type) => t === source1 ? target1 : t === source2 ? target2 : t;
function getMappedType(type: Type, map: TypeMapper): Type {
switch (map.kind) {
case TypeMapKind.Simple:
return type === map.source1 ? map.target1 : type === map.source2 ? map.target2 : type;
case TypeMapKind.Array:
const sources = map.sources;
const targets = map.targets;
for (let i = 0; i < sources.length; i++) {
if (type === sources[i]) {
return targets ? targets[i] : anyType;
}
}
return type;
case TypeMapKind.Function:
return map.func(type);
case TypeMapKind.Composite:
return instantiateType(getMappedType(type, map.mapper1), map.mapper2);
}
}

function makeArrayTypeMapper(sources: readonly Type[], targets: readonly Type[] | undefined) {
return (t: Type) => {
for (let i = 0; i < sources.length; i++) {
if (t === sources[i]) {
return targets ? targets[i] : anyType;
}
}
return t;
};
function makeUnaryTypeMapper(source: Type, target: Type): TypeMapper {
return makeSimpleTypeMapper(source, target, anyType, anyType);
}

function createTypeMapper(sources: readonly TypeParameter[], targets: readonly Type[] | undefined): TypeMapper {
Debug.assert(targets === undefined || sources.length === targets.length);
return sources.length === 1 ? makeUnaryTypeMapper(sources[0], targets ? targets[0] : anyType) :
sources.length === 2 ? makeBinaryTypeMapper(sources[0], targets ? targets[0] : anyType, sources[1], targets ? targets[1] : anyType) :
makeArrayTypeMapper(sources, targets);
function makeSimpleTypeMapper(source1: Type, target1: Type, source2: Type, target2: Type): TypeMapper {
return { kind: TypeMapKind.Simple, source1, target1, source2, target2 };
}

function makeArrayTypeMapper(sources: readonly TypeParameter[], targets: readonly Type[] | undefined): TypeMapper {
return { kind: TypeMapKind.Array, sources, targets };
}

function makeFunctionTypeMapper(func: (t: Type) => Type): TypeMapper {
return { kind: TypeMapKind.Function, func };
}

function makeCompositeTypeMapper(mapper1: TypeMapper, mapper2: TypeMapper): TypeMapper {
return { kind: TypeMapKind.Composite, mapper1, mapper2 };
}

function createTypeEraser(sources: readonly TypeParameter[]): TypeMapper {
Expand All @@ -13573,23 +13597,30 @@ namespace ts {
* This is used during inference when instantiating type parameter defaults.
*/
function createBackreferenceMapper(context: InferenceContext, index: number): TypeMapper {
return t => findIndex(context.inferences, info => info.typeParameter === t) >= index ? unknownType : t;
return makeFunctionTypeMapper(t => findIndex(context.inferences, info => info.typeParameter === t) >= index ? unknownType : t);
}

function combineTypeMappers(mapper1: TypeMapper | undefined, mapper2: TypeMapper): TypeMapper;
function combineTypeMappers(mapper1: TypeMapper, mapper2: TypeMapper | undefined): TypeMapper;
function combineTypeMappers(mapper1: TypeMapper, mapper2: TypeMapper): TypeMapper {
if (!mapper1) return mapper2;
if (!mapper2) return mapper1;
return t => instantiateType(mapper1(t), mapper2);
return !mapper1 ? mapper2 : !mapper2 ? mapper1 : makeCompositeTypeMapper(mapper1, mapper2);
}

function createReplacementMapper(source: Type, target: Type, baseMapper: TypeMapper): TypeMapper {
return t => t === source ? target : baseMapper(t);
function addTypeMapping(mapper: TypeMapper | undefined, source: TypeParameter, target: Type) {
return mapper && mapper.kind === TypeMapKind.Simple && mapper.source2 === mapper.target2 ?
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 getting mixed up. When you combine two maps, you look in the first map first and then, if you find something, do you stop looking or apply the second map to the first target? From the handling of composite maps in getMappedType it seems like it might be the latter, but this appears to do the former for unary maps?

Copy link
Member Author

Choose a reason for hiding this comment

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

The particular test here is to see if the second source/target pair in the simple mapper is unused (recall, when source and target are the same, we have a no-op). If so, we create a new simple mapper with both source/target pairs in use.

One added twist with composite mappers is that the first mapper may map to some type that the second mapper further maps. For example, the first mapper might map from T to U[] and the second mapper from U to string. This also explains why we directly call the getMappedType with the first mapper, but then call instantiateType with the second one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should add, in the addTypeMapping case, we actually don't care about the ability for the second mapping to affect the first mapping, which is why we can do the simple mapper optimization.

Copy link
Member

Choose a reason for hiding this comment

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

So maps are combined by composition, but we happen to know that this particular composition is equivalent to concatenation?

makeSimpleTypeMapper(mapper.source1, mapper.target1, source, target) :
combineTypeMappers(mapper, makeUnaryTypeMapper(source, target));
}

function permissiveMapper(type: Type) {
return type.flags & TypeFlags.TypeParameter ? wildcardType : type;
function createReplacementMapper(source: Type, target: Type, baseMapper: TypeMapper): TypeMapper {
switch (baseMapper.kind) {
case TypeMapKind.Simple:
return makeSimpleTypeMapper(baseMapper.source1, baseMapper.source1 === source ? target : baseMapper.target1,
baseMapper.source2, baseMapper.source2 === source ? target : baseMapper.target2);
case TypeMapKind.Array:
return makeArrayTypeMapper(baseMapper.sources, map(baseMapper.targets, (t, i) => baseMapper.sources[i] === source ? target : t));
}
return makeFunctionTypeMapper(t => t === source ? target : getMappedType(t, baseMapper));
Copy link
Member

Choose a reason for hiding this comment

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

Depending on how badly I've mixed up composite maps (see my question above), this seems like it might be equivalent to a composite map with source-to-target on the left hand side?

Copy link
Member Author

@ahejlsberg ahejlsberg Mar 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not quite. We don't want the second mapping to be applied to the result of the first. We basically just want to replace one of the mappings in the second mapper, which we can do by putting a check in front of the second mapper.

Copy link
Member

Choose a reason for hiding this comment

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

How come this transformation (concatenation?) wasn't interesting enough to become a TypeMapKind?

}

function getRestrictiveTypeParameter(tp: TypeParameter) {
Expand All @@ -13600,10 +13631,6 @@ namespace ts {
);
}

function restrictiveMapper(type: Type) {
return type.flags & TypeFlags.TypeParameter ? getRestrictiveTypeParameter(<TypeParameter>type) : type;
}

function cloneTypeParameter(typeParameter: TypeParameter): TypeParameter {
const result = createTypeParameter(typeParameter.symbol);
result.target = typeParameter;
Expand Down Expand Up @@ -13643,7 +13670,7 @@ namespace ts {

function instantiateSymbol(symbol: Symbol, mapper: TypeMapper): Symbol {
const links = getSymbolLinks(symbol);
if (links.type && !maybeTypeOfKind(links.type, TypeFlags.Object | TypeFlags.Instantiable)) {
if (links.type && !couldContainTypeVariables(links.type)) {
// If the type of the symbol is already resolved, and if that type could not possibly
// be affected by instantiation, simply return the symbol itself.
return symbol;
Expand Down Expand Up @@ -13710,7 +13737,8 @@ namespace ts {
// We are instantiating an anonymous type that has one or more type parameters in scope. Apply the
// mapper to the type parameters to produce the effective list of type arguments, and compute the
// instantiation cache key from the type IDs of the type arguments.
const typeArguments = map(typeParameters, combineTypeMappers(type.mapper, mapper));
const combinedMapper = combineTypeMappers(type.mapper, mapper);
const typeArguments = map(typeParameters, t => getMappedType(t, combinedMapper));
const id = getTypeListId(typeArguments);
let result = links.instantiations!.get(id);
if (!result) {
Expand Down Expand Up @@ -13823,7 +13851,7 @@ namespace ts {
}

function instantiateMappedTypeTemplate(type: MappedType, key: Type, isOptional: boolean, mapper: TypeMapper) {
const templateMapper = combineTypeMappers(mapper, createTypeMapper([getTypeParameterFromMappedType(type)], [key]));
const templateMapper = addTypeMapping(mapper, getTypeParameterFromMappedType(type), key);
const propType = instantiateType(getTemplateTypeFromMappedType(<MappedType>type.target || type), templateMapper);
const modifiers = getMappedTypeModifiers(type);
return strictNullChecks && modifiers & MappedTypeModifiers.IncludeOptional && !maybeTypeOfKind(propType, TypeFlags.Undefined | TypeFlags.Void) ? getOptionalType(propType) :
Expand Down Expand Up @@ -13855,7 +13883,7 @@ namespace ts {
// We are instantiating a conditional type that has one or more type parameters in scope. Apply the
// mapper to the type parameters to produce the effective list of type arguments, and compute the
// instantiation cache key from the type IDs of the type arguments.
const typeArguments = map(root.outerTypeParameters, mapper);
const typeArguments = map(root.outerTypeParameters, t => getMappedType(t, mapper));
const id = getTypeListId(typeArguments);
let result = root.instantiations!.get(id);
if (!result) {
Expand All @@ -13874,7 +13902,7 @@ namespace ts {
// type A | B, we produce (A extends U ? X : Y) | (B extends U ? X : Y).
if (root.isDistributive) {
const checkType = <TypeParameter>root.checkType;
const instantiatedType = mapper(checkType);
const instantiatedType = getMappedType(checkType, mapper);
if (checkType !== instantiatedType && instantiatedType.flags & (TypeFlags.Union | TypeFlags.Never)) {
return mapType(instantiatedType, t => getConditionalType(root, createReplacementMapper(checkType, t, mapper)));
}
Expand Down Expand Up @@ -13906,7 +13934,7 @@ namespace ts {
function instantiateTypeWorker(type: Type, mapper: TypeMapper): Type {
const flags = type.flags;
if (flags & TypeFlags.TypeParameter) {
return mapper(type);
return getMappedType(type, mapper);
}
if (flags & TypeFlags.Object) {
const objectFlags = (<ObjectType>type).objectFlags;
Expand Down Expand Up @@ -15705,10 +15733,10 @@ namespace ts {
// We're in the middle of variance checking - integrate any unmeasurable/unreliable flags from this cached component
const saved = entry & RelationComparisonResult.ReportsMask;
if (saved & RelationComparisonResult.ReportsUnmeasurable) {
instantiateType(source, reportUnmeasurableMarkers);
instantiateType(source, makeFunctionTypeMapper(reportUnmeasurableMarkers));
}
if (saved & RelationComparisonResult.ReportsUnreliable) {
instantiateType(source, reportUnreliableMarkers);
instantiateType(source, makeFunctionTypeMapper(reportUnreliableMarkers));
}
}
return entry & RelationComparisonResult.Succeeded ? Ternary.True : Ternary.False;
Expand Down Expand Up @@ -16153,7 +16181,7 @@ namespace ts {
if (modifiersRelated) {
let result: Ternary;
const targetConstraint = getConstraintTypeFromMappedType(target);
const sourceConstraint = instantiateType(getConstraintTypeFromMappedType(source), getCombinedMappedTypeOptionality(source) < 0 ? reportUnmeasurableMarkers : reportUnreliableMarkers);
const sourceConstraint = instantiateType(getConstraintTypeFromMappedType(source), makeFunctionTypeMapper(getCombinedMappedTypeOptionality(source) < 0 ? reportUnmeasurableMarkers : reportUnreliableMarkers));
if (result = isRelatedTo(targetConstraint, sourceConstraint, reportErrors)) {
const mapper = createTypeMapper([getTypeParameterFromMappedType(source)], [getTypeParameterFromMappedType(target)]);
return result & isRelatedTo(instantiateType(getTemplateTypeFromMappedType(source), mapper), getTemplateTypeFromMappedType(target), reportErrors);
Expand Down Expand Up @@ -16632,7 +16660,7 @@ namespace ts {
*/
function signatureRelatedTo(source: Signature, target: Signature, erase: boolean, reportErrors: boolean, incompatibleReporter: (source: Type, target: Type) => void): Ternary {
return compareSignaturesRelated(erase ? getErasedSignature(source) : source, erase ? getErasedSignature(target) : target,
relation === strictSubtypeRelation ? SignatureCheckMode.StrictArity : 0, reportErrors, reportError, incompatibleReporter, isRelatedTo, reportUnreliableMarkers);
relation === strictSubtypeRelation ? SignatureCheckMode.StrictArity : 0, reportErrors, reportError, incompatibleReporter, isRelatedTo, makeFunctionTypeMapper(reportUnreliableMarkers));
}

function signaturesIdenticalTo(source: Type, target: Type, kind: SignatureKind): Ternary {
Expand Down Expand Up @@ -17829,8 +17857,8 @@ namespace ts {
signature,
flags,
compareTypes,
mapper: t => mapToInferredType(context, t, /*fix*/ true),
nonFixingMapper: t => mapToInferredType(context, t, /*fix*/ false),
mapper: makeFunctionTypeMapper(t => mapToInferredType(context, t, /*fix*/ true)),
nonFixingMapper: makeFunctionTypeMapper(t => mapToInferredType(context, t, /*fix*/ false)),
};
return context;
}
Expand Down
14 changes: 13 additions & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4875,7 +4875,19 @@ namespace ts {
}

/* @internal */
export type TypeMapper = (t: TypeParameter) => Type;
export const enum TypeMapKind {
Simple,
Array,
Function,
Composite,
}

/* @internal */
export type TypeMapper =
| { kind: TypeMapKind.Simple, source1: Type, target1: Type, source2: Type, target2: Type }
| { kind: TypeMapKind.Array, sources: readonly Type[], targets: readonly Type[] | undefined }
| { kind: TypeMapKind.Function, func: (t: Type) => Type }
| { kind: TypeMapKind.Composite, mapper1: TypeMapper, mapper2: TypeMapper };

export const enum InferencePriority {
NakedTypeVariable = 1 << 0, // Naked type variable in union or intersection type
Expand Down