Skip to content

Remove missing type from tuple type arguments under exactOptionalPropertyTypes #54718

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
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
46 changes: 29 additions & 17 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
@@ -6982,7 +6982,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function typeReferenceToTypeNode(type: TypeReference) {
let typeArguments: readonly Type[] = getTypeArguments(type);
const typeArguments = getTypeArguments(type);
if (type.target === globalArrayType || type.target === globalReadonlyArrayType) {
if (context.flags & NodeBuilderFlags.WriteArrayAsGenericType) {
const typeArgumentNode = typeToTypeNodeHelper(typeArguments[0], context);
@@ -6993,7 +6993,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return type.target === globalArrayType ? arrayType : factory.createTypeOperatorNode(SyntaxKind.ReadonlyKeyword, arrayType);
}
else if (type.target.objectFlags & ObjectFlags.Tuple) {
typeArguments = sameMap(typeArguments, (t, i) => removeMissingType(t, !!((type.target as TupleType).elementFlags[i] & ElementFlags.Optional)));
if (typeArguments.length > 0) {
const arity = getTypeReferenceArity(type);
const tupleConstituentNodes = mapToTypeNodes(typeArguments.slice(0, arity), context);
@@ -12863,7 +12862,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
else {
mapper = createTypeMapper(typeParameters, typeArguments);
members = createInstantiatedSymbolTable(source.declaredProperties, mapper, /*mappingThisOnly*/ typeParameters.length === 1);
const propertiesMapper = isTupleType(type)
? createTypeMapper(typeParameters, sameMap(typeArguments, (t, i) => addOptionality(t, /*isProperty*/ true, !!(type.target.elementFlags[i] & ElementFlags.Optional))))
: mapper;
Comment on lines +12865 to +12867
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make the overall change smaller in scope we could "flip" the situation here. The existing mapper could be used as propertiesMapper (without the current changes in this PR it would have the missingType in the type arguments) and we could removeMissingType from the "core" mapper here. This way T in Tuple<T> wouldn't contain the missingType but properties would still be instantiated with it.

Alternatively, it's also very likely that we could add the missingType to the tuple.target elements at optional elements. This way we could just have a single mapper here and the missingType would be added to tuple properties at the instantiation time of the .target's fixed elements. Currently, target is represented as smth like [?, ?, ?] - where ? are type parameters. It could be represented as [?, ?, ? | missingType] though. I'm still battling myself how to think about it - would it be a cool trick or an obfuscated solution? 😅 The trick is that the instantiated tuple still wouldn't have missingType in its type arguments but the properties would be automatically read from the instantiated .target - so there would be a tricky mismatch between the tuple's type arguments and its .target. I'm not even sure if that's even possible - I didn't actually try to do it this way, I distinctly recall having such an idea 2 days ago when analyzing this.

That said, I still very much like that the missingType gets removed from the type arguments of the tuple. It feels right to me. The optionality bit is added by the element flag - and it's not part of the "core" type at that position. This way, It would feel a lot closer to me to how it works with object properties where the optionality gets added here. It's part of the computed property type - but not part of the declaredType there.

members = createInstantiatedSymbolTable(source.declaredProperties, propertiesMapper, /*mappingThisOnly*/ typeParameters.length === 1);
callSignatures = instantiateSignatures(source.declaredCallSignatures, mapper);
constructSignatures = instantiateSignatures(source.declaredConstructSignatures, mapper);
indexInfos = instantiateIndexInfos(source.declaredIndexInfos, mapper);
@@ -16552,7 +16554,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (flags & (ElementFlags.Optional | ElementFlags.Rest)) {
lastOptionalOrRestIndex = expandedFlags.length;
}
expandedTypes.push(flags & ElementFlags.Optional ? addOptionality(type, /*isProperty*/ true) : type);
expandedTypes.push(flags & ElementFlags.Optional && !exactOptionalPropertyTypes ? addOptionality(type, /*isProperty*/ true) : type);
expandedFlags.push(flags);
expandedDeclarations.push(declaration);
}
@@ -16591,7 +16593,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function getTypeFromOptionalTypeNode(node: OptionalTypeNode): Type {
return addOptionality(getTypeFromTypeNode(node.type), /*isProperty*/ true);
const type = getTypeFromTypeNode(node.type);
return exactOptionalPropertyTypes ? type : addOptionality(type, /*isProperty*/ true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda feel that this could be taken even further. As I mentioned in the other comment - it feels to me that the type argument's type shouldn't include the optionality, even outside of EOPT. The optionality could always be added based on the combination of the type argument and the respective element flag.

I'd rather prefer to explore this as a follow-up change in another PR - if there would be an intereset for it.

}

function getTypeId(type: Type): TypeId {
@@ -18829,8 +18832,20 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

function getTypeFromNamedTupleTypeNode(node: NamedTupleMember): Type {
const links = getNodeLinks(node);
return links.resolvedType || (links.resolvedType = node.dotDotDotToken ? getTypeFromRestTypeNode(node) :
addOptionality(getTypeFromTypeNode(node.type), /*isProperty*/ true, !!node.questionToken));
if (links.resolvedType) {
return links.resolvedType;
}
let type: Type;
if (node.dotDotDotToken) {
type = getTypeFromRestTypeNode(node);
}
else {
type = getTypeFromTypeNode(node.type);
if (!exactOptionalPropertyTypes) {
type = addOptionality(type, /*isProperty*/ true, !!node.questionToken);
}
}
return links.resolvedType = type;
}

function getTypeFromTypeNode(node: TypeNode): Type {
@@ -22723,11 +22738,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}

const sourceType = removeMissingType(sourceTypeArguments[sourcePosition], !!(sourceFlags & targetFlags & ElementFlags.Optional));
const sourceType = sourceTypeArguments[sourcePosition];
const targetType = targetTypeArguments[targetPosition];
const targetCheckType = sourceFlags & ElementFlags.Variadic && targetFlags & ElementFlags.Rest ? createArrayType(targetType) : targetType;

const targetCheckType = sourceFlags & ElementFlags.Variadic && targetFlags & ElementFlags.Rest ? createArrayType(targetType) :
removeMissingType(targetType, !!(targetFlags & ElementFlags.Optional));
const related = isRelatedTo(sourceType, targetCheckType, RecursionFlags.Both, reportErrors, /*headMessage*/ undefined, intersectionState);
if (!related) {
if (reportErrors && (targetArity > 1 || sourceArity > 1)) {
@@ -29817,7 +29831,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// If index is before any spread element and within the fixed part of the contextual tuple type, return
// the type of the contextual tuple element.
if ((firstSpreadIndex === undefined || index < firstSpreadIndex) && index < t.target.fixedLength) {
return removeMissingType(getTypeArguments(t)[index], !!(t.target.elementFlags[index] && ElementFlags.Optional));
return getTypeArguments(t)[index];
}
// When the length is known and the index is after all spread elements we compute the offset from the element
// to the end and the number of ending fixed elements in the contextual tuple type.
@@ -30519,7 +30533,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const inConstContext = isConstContext(node);
const contextualType = getApparentTypeOfContextualType(node, /*contextFlags*/ undefined);
const inTupleContext = isSpreadIntoCallOrNew(node) || !!contextualType && someType(contextualType, isTupleLikeType);
let hasOmittedExpression = false;
for (let i = 0; i < elementCount; i++) {
const e = elements[i];
if (e.kind === SyntaxKind.SpreadElement) {
@@ -30556,14 +30569,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}
else if (exactOptionalPropertyTypes && e.kind === SyntaxKind.OmittedExpression) {
hasOmittedExpression = true;
elementTypes.push(undefinedOrMissingType);
elementFlags.push(ElementFlags.Optional);
elementTypes.push(undefinedType);
elementFlags.push(ElementFlags.Required);
}
else {
const type = checkExpressionForMutableLocation(e, checkMode, forceTuple);
elementTypes.push(addOptionality(type, /*isProperty*/ true, hasOmittedExpression));
elementFlags.push(hasOmittedExpression ? ElementFlags.Optional : ElementFlags.Required);
elementTypes.push(type);
elementFlags.push(ElementFlags.Required);
if (inTupleContext && checkMode && checkMode & CheckMode.Inferential && !(checkMode & CheckMode.SkipContextSensitive) && isContextSensitive(e)) {
const inferenceContext = getInferenceContext(node);
Debug.assert(inferenceContext); // In CheckMode.Inferential we should always have an inference context
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
forOfOptionalTupleMember.ts(10,3): error TS18048: 'item' is possibly 'undefined'.
forOfOptionalTupleMember.ts(16,3): error TS18048: 'item' is possibly 'undefined'.
forOfOptionalTupleMember.ts(21,5): error TS18048: 'num' is possibly 'undefined'.
forOfOptionalTupleMember.ts(27,5): error TS18048: 'num' is possibly 'undefined'.


==== forOfOptionalTupleMember.ts (4 errors) ====
// repro from https://github.com/microsoft/TypeScript/issues/54302

type Item = {
value: string;
};

type Foo = [Item?];
declare const foo: Foo;
for (let item of foo) {
item.value;
~~~~
!!! error TS18048: 'item' is possibly 'undefined'.
}

type Foo2 = [item?: Item];
declare const foo2: Foo2;
for (let item of foo2) {
item.value;
~~~~
!!! error TS18048: 'item' is possibly 'undefined'.
}

function fn1(t: [number, number?, number?]) {
for (let num of t) {
num.toString()
~~~
!!! error TS18048: 'num' is possibly 'undefined'.
}
}

function fn2(t: [a: number, b?: number, c?: number]) {
for (let num of t) {
num.toString()
~~~
!!! error TS18048: 'num' is possibly 'undefined'.
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
//// [tests/cases/compiler/forOfOptionalTupleMember.ts] ////

=== forOfOptionalTupleMember.ts ===
// repro from https://github.com/microsoft/TypeScript/issues/54302

type Item = {
>Item : Symbol(Item, Decl(forOfOptionalTupleMember.ts, 0, 0))

value: string;
>value : Symbol(value, Decl(forOfOptionalTupleMember.ts, 2, 13))

};

type Foo = [Item?];
>Foo : Symbol(Foo, Decl(forOfOptionalTupleMember.ts, 4, 2))
>Item : Symbol(Item, Decl(forOfOptionalTupleMember.ts, 0, 0))

declare const foo: Foo;
>foo : Symbol(foo, Decl(forOfOptionalTupleMember.ts, 7, 13))
>Foo : Symbol(Foo, Decl(forOfOptionalTupleMember.ts, 4, 2))

for (let item of foo) {
>item : Symbol(item, Decl(forOfOptionalTupleMember.ts, 8, 8))
>foo : Symbol(foo, Decl(forOfOptionalTupleMember.ts, 7, 13))

item.value;
>item.value : Symbol(value, Decl(forOfOptionalTupleMember.ts, 2, 13))
>item : Symbol(item, Decl(forOfOptionalTupleMember.ts, 8, 8))
>value : Symbol(value, Decl(forOfOptionalTupleMember.ts, 2, 13))
}

type Foo2 = [item?: Item];
>Foo2 : Symbol(Foo2, Decl(forOfOptionalTupleMember.ts, 10, 1))
>Item : Symbol(Item, Decl(forOfOptionalTupleMember.ts, 0, 0))

declare const foo2: Foo2;
>foo2 : Symbol(foo2, Decl(forOfOptionalTupleMember.ts, 13, 13))
>Foo2 : Symbol(Foo2, Decl(forOfOptionalTupleMember.ts, 10, 1))

for (let item of foo2) {
>item : Symbol(item, Decl(forOfOptionalTupleMember.ts, 14, 8))
>foo2 : Symbol(foo2, Decl(forOfOptionalTupleMember.ts, 13, 13))

item.value;
>item.value : Symbol(value, Decl(forOfOptionalTupleMember.ts, 2, 13))
>item : Symbol(item, Decl(forOfOptionalTupleMember.ts, 14, 8))
>value : Symbol(value, Decl(forOfOptionalTupleMember.ts, 2, 13))
}

function fn1(t: [number, number?, number?]) {
>fn1 : Symbol(fn1, Decl(forOfOptionalTupleMember.ts, 16, 1))
>t : Symbol(t, Decl(forOfOptionalTupleMember.ts, 18, 13))

for (let num of t) {
>num : Symbol(num, Decl(forOfOptionalTupleMember.ts, 19, 10))
>t : Symbol(t, Decl(forOfOptionalTupleMember.ts, 18, 13))

num.toString()
>num.toString : Symbol(Number.toString, Decl(lib.es5.d.ts, --, --))
>num : Symbol(num, Decl(forOfOptionalTupleMember.ts, 19, 10))
>toString : Symbol(Number.toString, Decl(lib.es5.d.ts, --, --))
}
}

function fn2(t: [a: number, b?: number, c?: number]) {
>fn2 : Symbol(fn2, Decl(forOfOptionalTupleMember.ts, 22, 1))
>t : Symbol(t, Decl(forOfOptionalTupleMember.ts, 24, 13))

for (let num of t) {
>num : Symbol(num, Decl(forOfOptionalTupleMember.ts, 25, 10))
>t : Symbol(t, Decl(forOfOptionalTupleMember.ts, 24, 13))

num.toString()
>num.toString : Symbol(Number.toString, Decl(lib.es5.d.ts, --, --))
>num : Symbol(num, Decl(forOfOptionalTupleMember.ts, 25, 10))
>toString : Symbol(Number.toString, Decl(lib.es5.d.ts, --, --))
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
//// [tests/cases/compiler/forOfOptionalTupleMember.ts] ////

=== forOfOptionalTupleMember.ts ===
// repro from https://github.com/microsoft/TypeScript/issues/54302

type Item = {
>Item : { value: string; }

value: string;
>value : string

};

type Foo = [Item?];
>Foo : [(Item | undefined)?]

declare const foo: Foo;
>foo : Foo

for (let item of foo) {
>item : Item | undefined
>foo : Foo

item.value;
>item.value : string
>item : Item | undefined
>value : string
}

type Foo2 = [item?: Item];
>Foo2 : [item?: Item | undefined]

declare const foo2: Foo2;
>foo2 : Foo2

for (let item of foo2) {
>item : Item | undefined
>foo2 : Foo2

item.value;
>item.value : string
>item : Item | undefined
>value : string
}

function fn1(t: [number, number?, number?]) {
>fn1 : (t: [number, number?, number?]) => void
>t : [number, (number | undefined)?, (number | undefined)?]

for (let num of t) {
>num : number | undefined
>t : [number, (number | undefined)?, (number | undefined)?]

num.toString()
>num.toString() : string
>num.toString : (radix?: number | undefined) => string
>num : number | undefined
>toString : (radix?: number | undefined) => string
}
}

function fn2(t: [a: number, b?: number, c?: number]) {
>fn2 : (t: [a: number, b?: number, c?: number]) => void
>t : [a: number, b?: number | undefined, c?: number | undefined]

for (let num of t) {
>num : number | undefined
>t : [a: number, b?: number | undefined, c?: number | undefined]

num.toString()
>num.toString() : string
>num.toString : (radix?: number | undefined) => string
>num : number | undefined
>toString : (radix?: number | undefined) => string
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
forOfOptionalTupleMember.ts(10,3): error TS18048: 'item' is possibly 'undefined'.
forOfOptionalTupleMember.ts(16,3): error TS18048: 'item' is possibly 'undefined'.
forOfOptionalTupleMember.ts(21,5): error TS18048: 'num' is possibly 'undefined'.
forOfOptionalTupleMember.ts(27,5): error TS18048: 'num' is possibly 'undefined'.


==== forOfOptionalTupleMember.ts (4 errors) ====
// repro from https://github.com/microsoft/TypeScript/issues/54302

type Item = {
value: string;
};

type Foo = [Item?];
declare const foo: Foo;
for (let item of foo) {
item.value;
~~~~
!!! error TS18048: 'item' is possibly 'undefined'.
}

type Foo2 = [item?: Item];
declare const foo2: Foo2;
for (let item of foo2) {
item.value;
~~~~
!!! error TS18048: 'item' is possibly 'undefined'.
}

function fn1(t: [number, number?, number?]) {
for (let num of t) {
num.toString()
~~~
!!! error TS18048: 'num' is possibly 'undefined'.
}
}

function fn2(t: [a: number, b?: number, c?: number]) {
for (let num of t) {
num.toString()
~~~
!!! error TS18048: 'num' is possibly 'undefined'.
}
}

Loading