From 3c1eef3d241471338d9eef35024b8b27ef4cea86 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Fri, 17 May 2019 17:23:14 -0700 Subject: [PATCH 1/4] Fix conditional type type parameter leak --- src/compiler/checker.ts | 26 +++++++++++++-- ...LeakUninstantiatedTypeParameter.errors.txt | 14 ++++++++ ...alDoesntLeakUninstantiatedTypeParameter.js | 13 ++++++++ ...sntLeakUninstantiatedTypeParameter.symbols | 32 +++++++++++++++++++ ...oesntLeakUninstantiatedTypeParameter.types | 18 +++++++++++ ...alDoesntLeakUninstantiatedTypeParameter.ts | 7 ++++ 6 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.errors.txt create mode 100644 tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.js create mode 100644 tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.symbols create mode 100644 tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.types create mode 100644 tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 9f5673af3eee9..95a99563d4c7b 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -10406,14 +10406,34 @@ namespace ts { const checkTypeInstantiable = maybeTypeOfKind(checkType, TypeFlags.Instantiable | TypeFlags.GenericMappedType); let combinedMapper: TypeMapper | undefined; if (root.inferTypeParameters) { - const context = createInferenceContext(root.inferTypeParameters, /*signature*/ undefined, InferenceFlags.None); + const freshParams = map(root.inferTypeParameters, cloneTypeParameter); + const freshMapper = createTypeMapper(root.inferTypeParameters, freshParams); + const context = createInferenceContext(freshParams, /*signature*/ undefined, InferenceFlags.None); + // We have three mappers that need applying: + // * The original `mapper` used to create this conditional + // * The mapper that maps the old root type parameter to the clone (`freshMapper`) + // * The mapper that maps the clone to its inference result (`context.mapper`) + // When we're looking at making an inference for a clone, when we get its constraint, it's autmagically be + // instantiated with the context, so it doesn't need that one - however the constraint may refer to another _root_, _uncloned_ + // `infer` type parameter. + // Eg, if we have `Foo` and `Foo` - `B` is constrained to `T`, which, in turn, has been instantiated + // as `number` + // Conversely, if we have `Foo`, `B` is still constrained to `T` and `T` is instantiated as `A` + // So we need to: + // * Clone the type parameters so their constraints can be instantiated in the context of `mapper` + // * Set the clones to both map the context and the original params + // * instantiate the extends type with the clones + // * incorporate all of the component mappers into the combined mapper for the members + for (const p of freshParams) { + p.mapper = combineTypeMappers(mapper, freshMapper); + } if (!checkTypeInstantiable) { // We don't want inferences from constraints as they may cause us to eagerly resolve the // conditional type instead of deferring resolution. Also, we always want strict function // types rules (i.e. proper contravariance) for inferences. - inferTypes(context.inferences, checkType, extendsType, InferencePriority.NoConstraints | InferencePriority.AlwaysStrict); + inferTypes(context.inferences, checkType, instantiateType(extendsType, freshMapper), InferencePriority.NoConstraints | InferencePriority.AlwaysStrict); } - combinedMapper = combineTypeMappers(mapper, context.mapper); + combinedMapper = combineTypeMappers(combineTypeMappers(freshMapper, context.mapper), mapper); } // Instantiate the extends type including inferences for 'infer T' type parameters const inferredExtendsType = combinedMapper ? instantiateType(root.extendsType, combinedMapper) : extendsType; diff --git a/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.errors.txt b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.errors.txt new file mode 100644 index 0000000000000..3a59247a60350 --- /dev/null +++ b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.errors.txt @@ -0,0 +1,14 @@ +tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts(7,7): error TS2322: Type '"3"' is not assignable to type 'number'. + + +==== tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts (1 errors) ==== + interface Synthetic {} + type SyntheticDestination = U extends Synthetic ? V : never; + type TestSynthetic = // Resolved to T, should be `number` or an inference failure (`unknown`) + SyntheticDestination>; + + const y: TestSynthetic = 3; // Type '3' is not assignable to type 'T'. (shouldn't error) + const z: TestSynthetic = '3'; // Type '"3""' is not assignable to type 'T'. (should not mention T) + ~ +!!! error TS2322: Type '"3"' is not assignable to type 'number'. + \ No newline at end of file diff --git a/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.js b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.js new file mode 100644 index 0000000000000..8813585f62bee --- /dev/null +++ b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.js @@ -0,0 +1,13 @@ +//// [conditionalDoesntLeakUninstantiatedTypeParameter.ts] +interface Synthetic {} +type SyntheticDestination = U extends Synthetic ? V : never; +type TestSynthetic = // Resolved to T, should be `number` or an inference failure (`unknown`) + SyntheticDestination>; + +const y: TestSynthetic = 3; // Type '3' is not assignable to type 'T'. (shouldn't error) +const z: TestSynthetic = '3'; // Type '"3""' is not assignable to type 'T'. (should not mention T) + + +//// [conditionalDoesntLeakUninstantiatedTypeParameter.js] +var y = 3; // Type '3' is not assignable to type 'T'. (shouldn't error) +var z = '3'; // Type '"3""' is not assignable to type 'T'. (should not mention T) diff --git a/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.symbols b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.symbols new file mode 100644 index 0000000000000..7a66712e5feb7 --- /dev/null +++ b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.symbols @@ -0,0 +1,32 @@ +=== tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts === +interface Synthetic {} +>Synthetic : Symbol(Synthetic, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 0)) +>A : Symbol(A, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 20)) +>B : Symbol(B, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 22)) +>A : Symbol(A, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 20)) + +type SyntheticDestination = U extends Synthetic ? V : never; +>SyntheticDestination : Symbol(SyntheticDestination, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 38)) +>T : Symbol(T, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 26)) +>U : Symbol(U, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 28)) +>U : Symbol(U, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 28)) +>Synthetic : Symbol(Synthetic, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 0)) +>T : Symbol(T, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 26)) +>V : Symbol(V, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 62)) +>V : Symbol(V, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 62)) + +type TestSynthetic = // Resolved to T, should be `number` or an inference failure (`unknown`) +>TestSynthetic : Symbol(TestSynthetic, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 78)) + + SyntheticDestination>; +>SyntheticDestination : Symbol(SyntheticDestination, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 38)) +>Synthetic : Symbol(Synthetic, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 0, 0)) + +const y: TestSynthetic = 3; // Type '3' is not assignable to type 'T'. (shouldn't error) +>y : Symbol(y, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 5, 5)) +>TestSynthetic : Symbol(TestSynthetic, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 78)) + +const z: TestSynthetic = '3'; // Type '"3""' is not assignable to type 'T'. (should not mention T) +>z : Symbol(z, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 6, 5)) +>TestSynthetic : Symbol(TestSynthetic, Decl(conditionalDoesntLeakUninstantiatedTypeParameter.ts, 1, 78)) + diff --git a/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.types b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.types new file mode 100644 index 0000000000000..4b08f80d3f521 --- /dev/null +++ b/tests/baselines/reference/conditionalDoesntLeakUninstantiatedTypeParameter.types @@ -0,0 +1,18 @@ +=== tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts === +interface Synthetic {} +type SyntheticDestination = U extends Synthetic ? V : never; +>SyntheticDestination : SyntheticDestination + +type TestSynthetic = // Resolved to T, should be `number` or an inference failure (`unknown`) +>TestSynthetic : number + + SyntheticDestination>; + +const y: TestSynthetic = 3; // Type '3' is not assignable to type 'T'. (shouldn't error) +>y : number +>3 : 3 + +const z: TestSynthetic = '3'; // Type '"3""' is not assignable to type 'T'. (should not mention T) +>z : number +>'3' : "3" + diff --git a/tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts b/tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts new file mode 100644 index 0000000000000..439b53bc71baa --- /dev/null +++ b/tests/cases/compiler/conditionalDoesntLeakUninstantiatedTypeParameter.ts @@ -0,0 +1,7 @@ +interface Synthetic {} +type SyntheticDestination = U extends Synthetic ? V : never; +type TestSynthetic = // Resolved to T, should be `number` or an inference failure (`unknown`) + SyntheticDestination>; + +const y: TestSynthetic = 3; // Type '3' is not assignable to type 'T'. (shouldn't error) +const z: TestSynthetic = '3'; // Type '"3""' is not assignable to type 'T'. (should not mention T) From 83b226a3ff835f12acc9356660a86127fb36d134 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Mon, 20 May 2019 16:56:55 -0700 Subject: [PATCH 2/4] Monkey with comment text per code review --- src/compiler/checker.ts | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 95a99563d4c7b..beb2ccecf849c 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -10406,24 +10406,26 @@ namespace ts { const checkTypeInstantiable = maybeTypeOfKind(checkType, TypeFlags.Instantiable | TypeFlags.GenericMappedType); let combinedMapper: TypeMapper | undefined; if (root.inferTypeParameters) { - const freshParams = map(root.inferTypeParameters, cloneTypeParameter); - const freshMapper = createTypeMapper(root.inferTypeParameters, freshParams); - const context = createInferenceContext(freshParams, /*signature*/ undefined, InferenceFlags.None); - // We have three mappers that need applying: - // * The original `mapper` used to create this conditional - // * The mapper that maps the old root type parameter to the clone (`freshMapper`) - // * The mapper that maps the clone to its inference result (`context.mapper`) - // When we're looking at making an inference for a clone, when we get its constraint, it's autmagically be - // instantiated with the context, so it doesn't need that one - however the constraint may refer to another _root_, _uncloned_ - // `infer` type parameter. - // Eg, if we have `Foo` and `Foo` - `B` is constrained to `T`, which, in turn, has been instantiated + // When we're looking at making an inference for an infer type, when we get its constraint, it'll automagically be + // instantiated with the context, so it doesn't need the mapper for the inference contex - however the constraint + // may refer to another _root_, _uncloned_ `infer` type parameter [1], or to something mapped by `mapper` [2]. + // [1] Eg, if we have `Foo` and `Foo` - `B` is constrained to `T`, which, in turn, has been instantiated // as `number` // Conversely, if we have `Foo`, `B` is still constrained to `T` and `T` is instantiated as `A` + // [2] Eg, if we have `Foo` and `Foo` where `Q` is mapped by `mapper` into `number` - `B` is constrained to `T` + // which is in turn instantiated as `Q`, which is in turn instantiated as `number`. // So we need to: - // * Clone the type parameters so their constraints can be instantiated in the context of `mapper` - // * Set the clones to both map the context and the original params + // * Clone the type parameters so their constraints can be instantiated in the context of `mapper` (otherwise theyd only get inference context information) + // * Set the clones to both map the conditional's enclosing `mapper` and the original params // * instantiate the extends type with the clones - // * incorporate all of the component mappers into the combined mapper for the members + // * incorporate all of the component mappers into the combined mapper for the true and false members + // This means we have three mappers that need applying: + // * The original `mapper` used to create this conditional + // * The mapper that maps the old root type parameter to the clone (`freshMapper`) + // * The mapper that maps the clone to its inference result (`context.mapper`) + const freshParams = map(root.inferTypeParameters, cloneTypeParameter); + const freshMapper = createTypeMapper(root.inferTypeParameters, freshParams); + const context = createInferenceContext(freshParams, /*signature*/ undefined, InferenceFlags.None); for (const p of freshParams) { p.mapper = combineTypeMappers(mapper, freshMapper); } From 9133ec5a32a5a75727612fdeed5b8bb0af882679 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Thu, 23 May 2019 13:03:43 -0700 Subject: [PATCH 3/4] Conditionally clone type param --- src/compiler/checker.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index beb2ccecf849c..d389736e45d85 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -10397,6 +10397,11 @@ namespace ts { return type; } + function maybeCloneTypeParameter(p: TypeParameter) { + const constraint = getConstraintOfTypeParameter(p); + return constraint && maybeTypeOfKind(constraint, TypeFlags.Instantiable | TypeFlags.GenericMappedType) ? cloneTypeParameter(p) : p; + } + function getConditionalType(root: ConditionalRoot, mapper: TypeMapper | undefined): Type { const checkType = instantiateType(root.checkType, mapper); const extendsType = instantiateType(root.extendsType, mapper); @@ -10423,11 +10428,14 @@ namespace ts { // * The original `mapper` used to create this conditional // * The mapper that maps the old root type parameter to the clone (`freshMapper`) // * The mapper that maps the clone to its inference result (`context.mapper`) - const freshParams = map(root.inferTypeParameters, cloneTypeParameter); + const freshParams = map(root.inferTypeParameters, maybeCloneTypeParameter); const freshMapper = createTypeMapper(root.inferTypeParameters, freshParams); const context = createInferenceContext(freshParams, /*signature*/ undefined, InferenceFlags.None); + const freshCombinedMapper = combineTypeMappers(mapper, freshMapper); for (const p of freshParams) { - p.mapper = combineTypeMappers(mapper, freshMapper); + if (root.inferTypeParameters.indexOf(p) === -1) { + p.mapper = freshCombinedMapper; + } } if (!checkTypeInstantiable) { // We don't want inferences from constraints as they may cause us to eagerly resolve the From 4d8ad93d271c6df97cef4d486579a0641602432c Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Mon, 19 Aug 2019 13:16:04 -0700 Subject: [PATCH 4/4] Reuse input array and avoid making mapper where possible --- src/compiler/checker.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index e2aab33af69aa..ed5826c76481b 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -10693,13 +10693,15 @@ namespace ts { // * The original `mapper` used to create this conditional // * The mapper that maps the old root type parameter to the clone (`freshMapper`) // * The mapper that maps the clone to its inference result (`context.mapper`) - const freshParams = map(root.inferTypeParameters, maybeCloneTypeParameter); - const freshMapper = createTypeMapper(root.inferTypeParameters, freshParams); + const freshParams = sameMap(root.inferTypeParameters, maybeCloneTypeParameter); + const freshMapper = freshParams !== root.inferTypeParameters ? createTypeMapper(root.inferTypeParameters, freshParams) : undefined; const context = createInferenceContext(freshParams, /*signature*/ undefined, InferenceFlags.None); - const freshCombinedMapper = combineTypeMappers(mapper, freshMapper); - for (const p of freshParams) { - if (root.inferTypeParameters.indexOf(p) === -1) { - p.mapper = freshCombinedMapper; + if (freshMapper) { + const freshCombinedMapper = combineTypeMappers(mapper, freshMapper); + for (const p of freshParams) { + if (root.inferTypeParameters.indexOf(p) === -1) { + p.mapper = freshCombinedMapper; + } } } if (!checkTypeInstantiable) {