Skip to content

Fix circular mapped type instantiations for arrays and tuples #27911

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.

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
merged 8 commits into from
Oct 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 26 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10365,7 +10365,15 @@ namespace ts {
if (typeVariable) {
const mappedTypeVariable = instantiateType(typeVariable, mapper);
if (typeVariable !== mappedTypeVariable) {
return mapType(mappedTypeVariable, t => {
// If we are already in the process of creating an instantiation of this mapped type,
// return the error type. This situation only arises if we are instantiating the mapped
// type for an array or tuple type, as we then need to eagerly resolve the (possibly
// circular) element type(s).
if (type.instantiating) {
return errorType;
}
type.instantiating = true;
const result = mapType(mappedTypeVariable, t => {
if (t.flags & (TypeFlags.AnyOrUnknown | TypeFlags.InstantiableNonPrimitive | TypeFlags.Object | TypeFlags.Intersection) && t !== wildcardType) {
const replacementMapper = createReplacementMapper(typeVariable, t, mapper);
return isArrayType(t) ? createArrayType(instantiateMappedTypeTemplate(type, numberType, /*isOptional*/ true, replacementMapper)) :
Expand All @@ -10375,6 +10383,8 @@ namespace ts {
}
return t;
});
type.instantiating = false;
return result;
}
}
return instantiateAnonymousType(type, mapper);
Expand Down Expand Up @@ -14212,12 +14222,26 @@ namespace ts {
return undefined;
}

function isDiscriminantType(type: Type): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

this seems unrelated to the bug fix in the PR description

if (type.flags & TypeFlags.Union) {
if (type.flags & (TypeFlags.Boolean | TypeFlags.EnumLiteral)) {
return true;
}
let combined = 0;
for (const t of (<UnionType>type).types) combined |= t.flags;
if (combined & TypeFlags.Unit && !(combined & TypeFlags.Instantiable)) {
Copy link
Member

Choose a reason for hiding this comment

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

what happens if you have two two non-unit types? Should that still be discriminable?

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 pretty sure this change isn't supposed to be here, since it's #27695

return true;
}
}
return false;
}

function isDiscriminantProperty(type: Type | undefined, name: __String) {
if (type && type.flags & TypeFlags.Union) {
const prop = getUnionOrIntersectionProperty(<UnionType>type, name);
if (prop && getCheckFlags(prop) & CheckFlags.SyntheticProperty) {
if ((<TransientSymbol>prop).isDiscriminantProperty === undefined) {
(<TransientSymbol>prop).isDiscriminantProperty = !!((<TransientSymbol>prop).checkFlags & CheckFlags.HasNonUniformType) && isLiteralType(getTypeOfSymbol(prop));
(<TransientSymbol>prop).isDiscriminantProperty = !!((<TransientSymbol>prop).checkFlags & CheckFlags.HasNonUniformType) && isDiscriminantType(getTypeOfSymbol(prop));
}
return !!(<TransientSymbol>prop).isDiscriminantProperty;
}
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4074,6 +4074,7 @@ namespace ts {
templateType?: Type;
modifiersType?: Type;
resolvedApparentType?: Type;
instantiating?: boolean;
}

export interface EvolvingArrayType extends ObjectType {
Expand Down
86 changes: 86 additions & 0 deletions tests/baselines/reference/discriminatedUnionTypes2.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
tests/cases/conformance/types/union/discriminatedUnionTypes2.ts(27,30): error TS2322: Type '{ a: null; b: string; c: number; }' is not assignable to type '{ a: null; b: string; } | { a: string; c: number; }'.
Object literal may only specify known properties, and 'c' does not exist in type '{ a: null; b: string; }'.
tests/cases/conformance/types/union/discriminatedUnionTypes2.ts(32,11): error TS2339: Property 'b' does not exist on type '{ a: 0; b: string; } | { a: T; c: number; }'.
Property 'b' does not exist on type '{ a: T; c: number; }'.


==== tests/cases/conformance/types/union/discriminatedUnionTypes2.ts (2 errors) ====
function f10(x : { kind: false, a: string } | { kind: true, b: string } | { kind: string, c: string }) {
if (x.kind === false) {
x.a;
}
else if (x.kind === true) {
x.b;
}
else {
x.c;
}
}

function f11(x : { kind: false, a: string } | { kind: true, b: string } | { kind: string, c: string }) {
switch (x.kind) {
case false:
x.a;
break;
case true:
x.b;
break;
default:
x.c;
}
}

function f13(x: { a: null; b: string } | { a: string, c: number }) {
x = { a: null, b: "foo", c: 4}; // Error
~~~~
!!! error TS2322: Type '{ a: null; b: string; c: number; }' is not assignable to type '{ a: null; b: string; } | { a: string; c: number; }'.
!!! error TS2322: Object literal may only specify known properties, and 'c' does not exist in type '{ a: null; b: string; }'.
}

function f14<T>(x: { a: 0; b: string } | { a: T, c: number }) {
if (x.a === 0) {
x.b; // Error
~
!!! error TS2339: Property 'b' does not exist on type '{ a: 0; b: string; } | { a: T; c: number; }'.
!!! error TS2339: Property 'b' does not exist on type '{ a: T; c: number; }'.
}
}

type Result<T> = { error?: undefined, value: T } | { error: Error };

function f15(x: Result<number>) {
if (!x.error) {
x.value;
}
else {
x.error.message;
}
}

f15({ value: 10 });
f15({ error: new Error("boom") });

// Repro from #24193

interface WithError {
error: Error
data: null
}

interface WithoutError<Data> {
error: null
data: Data
}

type DataCarrier<Data> = WithError | WithoutError<Data>

function f20<Data>(carrier: DataCarrier<Data>) {
if (carrier.error === null) {
const error: null = carrier.error
const data: Data = carrier.data
} else {
const error: Error = carrier.error
const data: null = carrier.data
}
}

128 changes: 128 additions & 0 deletions tests/baselines/reference/discriminatedUnionTypes2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
//// [discriminatedUnionTypes2.ts]
function f10(x : { kind: false, a: string } | { kind: true, b: string } | { kind: string, c: string }) {
if (x.kind === false) {
x.a;
}
else if (x.kind === true) {
x.b;
}
else {
x.c;
}
}

function f11(x : { kind: false, a: string } | { kind: true, b: string } | { kind: string, c: string }) {
switch (x.kind) {
case false:
x.a;
break;
case true:
x.b;
break;
default:
x.c;
}
}

function f13(x: { a: null; b: string } | { a: string, c: number }) {
x = { a: null, b: "foo", c: 4}; // Error
}

function f14<T>(x: { a: 0; b: string } | { a: T, c: number }) {
if (x.a === 0) {
x.b; // Error
}
}

type Result<T> = { error?: undefined, value: T } | { error: Error };

function f15(x: Result<number>) {
if (!x.error) {
x.value;
}
else {
x.error.message;
}
}

f15({ value: 10 });
f15({ error: new Error("boom") });

// Repro from #24193

interface WithError {
error: Error
data: null
}

interface WithoutError<Data> {
error: null
data: Data
}

type DataCarrier<Data> = WithError | WithoutError<Data>

function f20<Data>(carrier: DataCarrier<Data>) {
if (carrier.error === null) {
const error: null = carrier.error
const data: Data = carrier.data
} else {
const error: Error = carrier.error
const data: null = carrier.data
}
}


//// [discriminatedUnionTypes2.js]
"use strict";
function f10(x) {
if (x.kind === false) {
x.a;
}
else if (x.kind === true) {
x.b;
}
else {
x.c;
}
}
function f11(x) {
switch (x.kind) {
case false:
x.a;
break;
case true:
x.b;
break;
default:
x.c;
}
}
function f13(x) {
x = { a: null, b: "foo", c: 4 }; // Error
}
function f14(x) {
if (x.a === 0) {
x.b; // Error
}
}
function f15(x) {
if (!x.error) {
x.value;
}
else {
x.error.message;
}
}
f15({ value: 10 });
f15({ error: new Error("boom") });
function f20(carrier) {
if (carrier.error === null) {
var error = carrier.error;
var data = carrier.data;
}
else {
var error = carrier.error;
var data = carrier.data;
}
}
Loading