Skip to content

Allowed tuples to have both named and anonymous members #53356

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 18 commits into from
Jun 26, 2023
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
74 changes: 37 additions & 37 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6876,21 +6876,21 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const arity = getTypeReferenceArity(type);
const tupleConstituentNodes = mapToTypeNodes(typeArguments.slice(0, arity), context);
if (tupleConstituentNodes) {
if ((type.target as TupleType).labeledElementDeclarations) {
for (let i = 0; i < tupleConstituentNodes.length; i++) {
const flags = (type.target as TupleType).elementFlags[i];
const { labeledElementDeclarations } = type.target as TupleType;
for (let i = 0; i < tupleConstituentNodes.length; i++) {
const flags = (type.target as TupleType).elementFlags[i];
const labeledElementDeclaration = labeledElementDeclarations?.[i];

if (labeledElementDeclaration) {
tupleConstituentNodes[i] = factory.createNamedTupleMember(
flags & ElementFlags.Variable ? factory.createToken(SyntaxKind.DotDotDotToken) : undefined,
factory.createIdentifier(unescapeLeadingUnderscores(getTupleElementLabel((type.target as TupleType).labeledElementDeclarations![i]))),
factory.createIdentifier(unescapeLeadingUnderscores(getTupleElementLabel(labeledElementDeclaration))),
flags & ElementFlags.Optional ? factory.createToken(SyntaxKind.QuestionToken) : undefined,
flags & ElementFlags.Rest ? factory.createArrayTypeNode(tupleConstituentNodes[i]) :
tupleConstituentNodes[i]
);
}
}
else {
for (let i = 0; i < Math.min(arity, tupleConstituentNodes.length); i++) {
const flags = (type.target as TupleType).elementFlags[i];
else {
tupleConstituentNodes[i] =
flags & ElementFlags.Variable ? factory.createRestTypeNode(flags & ElementFlags.Rest ? factory.createArrayTypeNode(tupleConstituentNodes[i]) : tupleConstituentNodes[i]) :
flags & ElementFlags.Optional ? factory.createOptionalTypeNode(tupleConstituentNodes[i]) :
Expand Down Expand Up @@ -12636,19 +12636,20 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
function getExpandedParameters(sig: Signature, skipUnionExpanding?: boolean): readonly (readonly Symbol[])[] {
if (signatureHasRestParameter(sig)) {
const restIndex = sig.parameters.length - 1;
const restName = sig.parameters[restIndex].escapedName;
const restType = getTypeOfSymbol(sig.parameters[restIndex]);
if (isTupleType(restType)) {
return [expandSignatureParametersWithTupleMembers(restType, restIndex)];
return [expandSignatureParametersWithTupleMembers(restType, restIndex, restName)];
}
else if (!skipUnionExpanding && restType.flags & TypeFlags.Union && every((restType as UnionType).types, isTupleType)) {
return map((restType as UnionType).types, t => expandSignatureParametersWithTupleMembers(t as TupleTypeReference, restIndex));
return map((restType as UnionType).types, t => expandSignatureParametersWithTupleMembers(t as TupleTypeReference, restIndex, restName));
}
}
return [sig.parameters];

function expandSignatureParametersWithTupleMembers(restType: TupleTypeReference, restIndex: number) {
const elementTypes = getElementTypes(restType);
const associatedNames = getUniqAssociatedNamesFromTupleType(restType);
function expandSignatureParametersWithTupleMembers(restType: TupleTypeReference, restIndex: number, restName: __String) {
const elementTypes = getTypeArguments(restType);
const associatedNames = getUniqAssociatedNamesFromTupleType(restType, restName);
const restParams = map(elementTypes, (t, i) => {
// Lookup the label from the individual tuple passed in before falling back to the signature `rest` parameter name
const name = associatedNames && associatedNames[i] ? associatedNames[i] :
Expand All @@ -12663,10 +12664,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return concatenate(sig.parameters.slice(0, restIndex), restParams);
}

function getUniqAssociatedNamesFromTupleType(type: TupleTypeReference) {
function getUniqAssociatedNamesFromTupleType(type: TupleTypeReference, restName: __String) {
const associatedNamesMap = new Map<__String, number>();
return map(type.target.labeledElementDeclarations, labeledElement => {
const name = getTupleElementLabel(labeledElement);
return map(type.target.labeledElementDeclarations, (labeledElement, i) => {
const name = getTupleElementLabel(labeledElement, i, restName);
const prevCounter = associatedNamesMap.get(name);
if (prevCounter === undefined) {
associatedNamesMap.set(name, 1);
Expand Down Expand Up @@ -15934,8 +15935,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return readonly ? globalReadonlyArrayType : globalArrayType;
}
const elementFlags = map((node as TupleTypeNode).elements, getTupleElementFlags);
const missingName = some((node as TupleTypeNode).elements, e => e.kind !== SyntaxKind.NamedTupleMember);
return getTupleTargetType(elementFlags, readonly, /*associatedNames*/ missingName ? undefined : (node as TupleTypeNode).elements as readonly NamedTupleMember[]);
return getTupleTargetType(elementFlags, readonly, map((node as TupleTypeNode).elements, memberIfLabeledElementDeclaration));
}

function memberIfLabeledElementDeclaration(member: Node): NamedTupleMember | ParameterDeclaration | undefined {
return isNamedTupleMember(member) || isParameter(member) ? member : undefined;
}

// Return true if the given type reference node is directly aliased or if it needs to be deferred
Expand Down Expand Up @@ -16025,21 +16029,22 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return isTypeOperatorNode(node) && node.operator === SyntaxKind.ReadonlyKeyword;
}

function createTupleType(elementTypes: readonly Type[], elementFlags?: readonly ElementFlags[], readonly = false, namedMemberDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[]) {
function createTupleType(elementTypes: readonly Type[], elementFlags?: readonly ElementFlags[], readonly = false, namedMemberDeclarations: readonly (NamedTupleMember | ParameterDeclaration | undefined)[] = []) {
const tupleTarget = getTupleTargetType(elementFlags || map(elementTypes, _ => ElementFlags.Required), readonly, namedMemberDeclarations);
return tupleTarget === emptyGenericType ? emptyObjectType :
elementTypes.length ? createNormalizedTypeReference(tupleTarget, elementTypes) :
tupleTarget;
}

function getTupleTargetType(elementFlags: readonly ElementFlags[], readonly: boolean, namedMemberDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[]): GenericType {
function getTupleTargetType(elementFlags: readonly ElementFlags[], readonly: boolean, namedMemberDeclarations: readonly (NamedTupleMember | ParameterDeclaration | undefined)[]): GenericType {
if (elementFlags.length === 1 && elementFlags[0] & ElementFlags.Rest) {
// [...X[]] is equivalent to just X[]
return readonly ? globalReadonlyArrayType : globalArrayType;
}
const memberIds = mapDefined(namedMemberDeclarations, node => node ? getNodeId(node) : undefined);
const key = map(elementFlags, f => f & ElementFlags.Required ? "#" : f & ElementFlags.Optional ? "?" : f & ElementFlags.Rest ? "." : "*").join() +
(readonly ? "R" : "") +
(namedMemberDeclarations && namedMemberDeclarations.length ? "," + map(namedMemberDeclarations, getNodeId).join(",") : "");
(memberIds.length ? "," + memberIds.join(",") : "");
let type = tupleTypes.get(key);
if (!type) {
tupleTypes.set(key, type = createTupleTargetType(elementFlags, readonly, namedMemberDeclarations));
Expand All @@ -16054,7 +16059,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
//
// Note that the generic type created by this function has no symbol associated with it. The same
// is true for each of the synthesized type parameters.
function createTupleTargetType(elementFlags: readonly ElementFlags[], readonly: boolean, namedMemberDeclarations: readonly (NamedTupleMember | ParameterDeclaration)[] | undefined): TupleType {
function createTupleTargetType(elementFlags: readonly ElementFlags[], readonly: boolean, namedMemberDeclarations: readonly (NamedTupleMember | ParameterDeclaration | undefined)[]): TupleType {
const arity = elementFlags.length;
const minLength = countWhere(elementFlags, f => !!(f & (ElementFlags.Required | ElementFlags.Variadic)));
let typeParameters: TypeParameter[] | undefined;
Expand Down Expand Up @@ -16136,7 +16141,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// In either layout, zero or more generic variadic elements may be present at any location.
const expandedTypes: Type[] = [];
const expandedFlags: ElementFlags[] = [];
let expandedDeclarations: (NamedTupleMember | ParameterDeclaration)[] | undefined = [];
const expandedDeclarations: (NamedTupleMember | ParameterDeclaration | undefined)[] = [];
let lastRequiredIndex = -1;
let firstRestIndex = -1;
let lastOptionalOrRestIndex = -1;
Expand Down Expand Up @@ -16179,7 +16184,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
(t, i) => expandedFlags[firstRestIndex + i] & ElementFlags.Variadic ? getIndexedAccessType(t, numberType) : t));
expandedTypes.splice(firstRestIndex + 1, lastOptionalOrRestIndex - firstRestIndex);
expandedFlags.splice(firstRestIndex + 1, lastOptionalOrRestIndex - firstRestIndex);
expandedDeclarations?.splice(firstRestIndex + 1, lastOptionalOrRestIndex - firstRestIndex);
expandedDeclarations.splice(firstRestIndex + 1, lastOptionalOrRestIndex - firstRestIndex);
}
const tupleTarget = getTupleTargetType(expandedFlags, target.readonly, expandedDeclarations);
return tupleTarget === emptyGenericType ? emptyObjectType :
Expand All @@ -16198,12 +16203,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
expandedTypes.push(flags & ElementFlags.Optional ? addOptionality(type, /*isProperty*/ true) : type);
expandedFlags.push(flags);
if (expandedDeclarations && declaration) {
expandedDeclarations.push(declaration);
}
else {
expandedDeclarations = undefined;
}
expandedDeclarations.push(declaration);
}
}

Expand Down Expand Up @@ -34793,7 +34793,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return type;
}

function getTupleElementLabel(d: ParameterDeclaration | NamedTupleMember) {
function getTupleElementLabel(d: ParameterDeclaration | NamedTupleMember): __String;
function getTupleElementLabel(d: ParameterDeclaration | NamedTupleMember | undefined, index: number, restParameterName?: __String): __String;
function getTupleElementLabel(d: ParameterDeclaration | NamedTupleMember | undefined, index?: number, restParameterName = "arg" as __String) {
if (!d) {
return `${restParameterName}_${index}` as __String;
}
Debug.assert(isIdentifier(d.name)); // Parameter declarations could be binding patterns, but we only allow identifier names
return d.name.escapedText;
}
Expand All @@ -34808,7 +34813,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (isTupleType(restType)) {
const associatedNames = ((restType as TypeReference).target as TupleType).labeledElementDeclarations;
const index = pos - paramCount;
return associatedNames && getTupleElementLabel(associatedNames[index]) || restParameter.escapedName + "_" + index as __String;
return getTupleElementLabel(associatedNames?.[index], index, restParameter.escapedName);
}
return restParameter.escapedName;
}
Expand Down Expand Up @@ -38821,12 +38826,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const elementTypes = node.elements;
let seenOptionalElement = false;
let seenRestElement = false;
const hasNamedElement = some(elementTypes, isNamedTupleMember);
for (const e of elementTypes) {
if (e.kind !== SyntaxKind.NamedTupleMember && hasNamedElement) {
grammarErrorOnNode(e, Diagnostics.Tuple_members_must_all_have_names_or_all_not_have_names);
break;
}
const flags = getTupleElementFlags(e);
if (flags & ElementFlags.Variadic) {
const type = getTypeFromTypeNode((e as RestTypeNode | NamedTupleMember).type);
Expand Down
4 changes: 0 additions & 4 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4237,10 +4237,6 @@
"category": "Error",
"code": 5083
},
"Tuple members must all have names or all not have names.": {
"category": "Error",
"code": 5084
},
"A tuple member cannot be both optional and rest.": {
"category": "Error",
"code": 5085
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6430,7 +6430,7 @@ export interface TupleType extends GenericType {
hasRestElement: boolean;
combinedFlags: ElementFlags;
readonly: boolean;
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[];
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration | undefined)[];
Copy link
Contributor Author

@JoshuaKGoldberg JoshuaKGoldberg Mar 19, 2023

Choose a reason for hiding this comment

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

[Breaking API Change] Is this the right choice to make? One alternative could be to rename it to elementDeclarations with a type like readonly (ParameterDeclaration | TypeNode). My hunch is that that would be preferable, but I'm going with this smaller change for now to make reviewing easier.

Copy link
Member

Choose a reason for hiding this comment

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

Renaming it doesn't feel like a good idea compared to this; I assume that this array is indexed such that there are empty spaces where there are no names?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that this array is indexed such that there are empty spaces where there are no names?

To the best of my knowledge - yes. There are a couple of places in this PR where it seems to be handled this way

Copy link
Member

Choose a reason for hiding this comment

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

What would the alternative be if not an array with missing names?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that most implementations that would include keeping labeledElementDeclarations with partial labels could be considered a breaking change. Even if you somehow omit undefined positions you still end up with a .length mismatch between those and other properties and that would be rather disastrous.

I think the current structure of this object makes the most sense but if you consider it to be too big of a breaking change then I think a different property should be added when mixed labels are involved and this one should be kept as undefined in such situations. That will be inconvenient to work with but if maintaining a backward compatibility is the goal then I don't see any other straightforward way of achieving this.

Copy link
Member

Choose a reason for hiding this comment

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

As in, this array seems to be the only one in the interface which actually describes each element; if we were to keep this array without making elements undefined, then the only thing I can think is that all of the named members gets smooshed up together, except now their indexes are lost.

So, it seems to me that the change in the PR is the least breaky option when we're now saying that something can potentially be undefined, because otherwise someone may iterate over this list thinking they're seeing everything, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I mean "what would the alternative be if not an array which can contain undefined?"

I was responding to exactly that but perhaps I wasn't super clear. Should I try to rephrase my original answer?

if we were to keep this array without making elements undefined, then the only thing I can think is that all of the named members gets smooshed up together, except now their indexes are lost.

yeah, that would be IMHO bad - I mentioned that here: "Even if you somehow omit undefined positions you still end up with a .length mismatch between those and other properties and that would be rather disastrous.". But right now one alternative option comes to my mind - the missing labels could be prefilled with some defaults. You already generate such things for function parameters and stuff at times, like arg_0, etc, or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

I know we generate synthetic nodes for names for stuff like declaration emit, but do we commonly store them in the types? (I genuinely don't know the answer!)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that either and perhaps you don't ever do that right now. I prefer to leave those optional members as undefined, that's just an idea that would help you to minimize the impact of this change if you think that this is a strong requirement for this PR to land.

Copy link
Member

Choose a reason for hiding this comment

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

I think we're agreeing? I think I too prefer to make them undefined just like this PR does; I just wanted to make sure that was the best choice given the break. Reasonably no matter what, if downstream users care about this particular bit, they're going to have to fix their code.

}

export interface TupleTypeReference extends TypeReference {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6948,7 +6948,7 @@ declare namespace ts {
hasRestElement: boolean;
combinedFlags: ElementFlags;
readonly: boolean;
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[];
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration | undefined)[];
}
interface TupleTypeReference extends TypeReference {
target: TupleType;
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2895,7 +2895,7 @@ declare namespace ts {
hasRestElement: boolean;
combinedFlags: ElementFlags;
readonly: boolean;
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration)[];
labeledElementDeclarations?: readonly (NamedTupleMember | ParameterDeclaration | undefined)[];
}
interface TupleTypeReference extends TypeReference {
target: TupleType;
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/genericRestParameters2.types
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ type T05<T extends any[]> = Parameters<(x: string, ...args: T) => void>;
>args : T

type T06 = T05<[number, ...boolean[]]>;
>T06 : [string, number, ...boolean[]]
>T06 : [x: string, number, ...boolean[]]

type P1<T extends Function> = T extends (head: infer A, ...tail: infer B) => any ? { head: A, tail: B } : any[];
>P1 : P1<T>
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/genericRestParameters3.types
Original file line number Diff line number Diff line change
Expand Up @@ -264,19 +264,19 @@ ff2 = ff1;
function ff3<A extends unknown[]>(s1: (...args: [x: string, ...rest: A | [number]]) => void, s2: (x: string, ...rest: A | [number]) => void) {
>ff3 : <A extends unknown[]>(s1: (...args: [x: string, ...rest: A | [number]]) => void, s2: (x: string, ...rest: A | [number]) => void) => void
>s1 : (...args: [x: string, ...rest: A | [number]]) => void
>args : [string, number] | [x: string, ...rest: A]
>args : [x: string, number] | [x: string, ...rest: A]
>s2 : (x: string, ...rest: A | [number]) => void
>x : string
>rest : [number] | A

s1 = s2;
>s1 = s2 : (x: string, ...rest: [number] | A) => void
>s1 : (...args: [string, number] | [x: string, ...rest: A]) => void
>s1 : (...args: [x: string, number] | [x: string, ...rest: A]) => void
>s2 : (x: string, ...rest: [number] | A) => void

s2 = s1;
>s2 = s1 : (...args: [string, number] | [x: string, ...rest: A]) => void
>s2 = s1 : (...args: [x: string, number] | [x: string, ...rest: A]) => void
>s2 : (x: string, ...rest: [number] | A) => void
>s1 : (...args: [string, number] | [x: string, ...rest: A]) => void
>s1 : (...args: [x: string, number] | [x: string, ...rest: A]) => void
}

Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type SubTup2VariadicAndRest<T extends unknown[]> = T extends [
: never;

type SubTup2VariadicAndRestTest = SubTup2VariadicAndRest<[a: 0, b: 1, ...c: 2[]]>;
>SubTup2VariadicAndRestTest : [0, 1, 2]
>SubTup2VariadicAndRestTest : [a: 0, b: 1, 2]

type SubTup2TrailingVariadic<T extends unknown[]> = T extends [
>SubTup2TrailingVariadic : SubTup2TrailingVariadic<T>
Expand All @@ -67,7 +67,7 @@ type SubTup2RestAndTrailingVariadic2<T extends unknown[]> = T extends [
: never;

type SubTup2RestAndTrailingVariadic2Test = SubTup2RestAndTrailingVariadic2<[...a: 0[], b: 1, c: 2]>;
>SubTup2RestAndTrailingVariadic2Test : [0, 1, 2]
>SubTup2RestAndTrailingVariadic2Test : [0, b: 1, c: 2]

type SubTup2VariadicWithLeadingFixedElements<T extends unknown[]> = T extends [
>SubTup2VariadicWithLeadingFixedElements : SubTup2VariadicWithLeadingFixedElements<T>
Expand Down
Loading