Skip to content

Fix indexing and destructuring of unions of tuple types #27587

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 14 commits into from
Oct 11, 2018

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Oct 7, 2018

This PR fixes several issues relating to indexing and destructuring of unions of tuple types. In particular, we now reflect that elements beyond the end of a fixed length tuple are known to be undefined, and we properly account for rest elements in unions of typle types. Some examples:

type T = [boolean] | [string, number];
type T0 = T[0];  // string | boolean
type T1 = T[1];  // number | undefined
type T2 = T[2];  // *Error*
type TX = T[number];  // string | number | boolean

type U = [boolean] | [string, ...number[]];
type U0 = U[0];  // string | boolean
type U1 = U[1];  // number | undefined
type U2 = U[2];  // number | undefined
type UX = U[number];  // string | number | boolean

Note that when indexing with a constant (thus selecting an element at a known index), we properly account for the possibility of undefined, but when indexing with a non-constant (as in T[number]), we assume that the index value is within range.

Fixes #27543.

}

function pollingInterval(watchPriority: PollingInterval): number {
return pollingIntervalsForPriority[watchPriority];
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 line above causes an error with this PR because we now correctly include undefined in the type because watchPriority is one of the values 250, 500, or 2000, all of which are out of range of the tuple type. This code is basically bogus, but also never used.

return restType;
}
if (everyType(objectType, isTupleType) && isNumericLiteralName(propName) && +propName >= 0) {
return mapType(objectType, t => getRestTypeOfTupleType(<TupleTypeReference>t) || undefinedType);
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this isn't using a undefinedWideningType which means outside of strict mode, the new types are going to be strictly less permissive than they were before. Are we OK with that?

Copy link
Member Author

@ahejlsberg ahejlsberg Oct 7, 2018

Choose a reason for hiding this comment

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

They're going to be less permissive no matter which undefined we use because previously, when the object type was a union type, we'd just fall back to the numeric index signature. For example:

type T = [boolean] | [string, number];
type T0 = T[0];  // Unchanged, string | boolean
type T1 = T[1];  // Now number | undefined, previously number | boolean
type T2 = T[2];  // Now undefined, previously string | number | boolean

I would argue that the non-widening undefined is the right choice above. It only matters in non-strict mode and the only time you'd ever observe a difference is when it is not in a union with something else, i.e. T2 above. I don't think we'd help anybody if T2 was magically going to widen to any.

@ajafff
Copy link
Contributor

ajafff commented Oct 7, 2018

Why is type T2 = T[2]; not an error? It will never be defined, so why allow access to that element using a constant?

@ahejlsberg
Copy link
Member Author

Why is type T2 = T[2]; not an error? It will never be defined, so why allow access to that element using a constant?

Indeed. It is already an error in an expression position, but inconsistently so (and with a different message from what we use elsewhere). With the latest commits we now have common error reporting for expression and type positions.

@ahejlsberg
Copy link
Member Author

@RyanCavanaugh You want to give this a look?

return restType;
if (everyType(objectType, isTupleType) && isNumericLiteralName(propName) && +propName >= 0) {
const restType = mapType(objectType, t => getRestTypeOfTupleType(<TupleTypeReference>t) || undefinedType);
if (restType === undefinedType && accessNode) {
Copy link
Member

Choose a reason for hiding this comment

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

type UndefinedTuple = [string, ...undefined[]]; // ???

Copy link
Member Author

Choose a reason for hiding this comment

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

Now fixed.

Copy link
Member

Choose a reason for hiding this comment

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

getTupleElementType still handles undefinedType incorrectly.

}
if (everyType(type, isTupleType)) {
const restType = mapType(type, t => getRestTypeOfTupleType(<TupleTypeReference>t) || undefinedType);
if (restType !== undefinedType) {
Copy link
Member

@weswigham weswigham Oct 10, 2018

Choose a reason for hiding this comment

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

Yeah, same remark - this needs to use a unique marker and not the undefinedType to signal the lack of a restType return, as it prevents a legitimate undefined rest type from functioning.

@ahejlsberg ahejlsberg merged commit 5a126e2 into master Oct 11, 2018
@ahejlsberg ahejlsberg deleted the fixUnionOfTupleIndexing branch October 11, 2018 19:01
@weswigham
Copy link
Member

@ahejlsberg we might not want to return undefined in all cases and might prefer the passed in missingType instead, since always using undefined gives follow-on errors like this:

                    noticeModel.attachments[0].content.buttons[1] = secondButton;
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2322: Type '{ readonly title: string; readonly actionUri: string; readonly actionTarget?: "" | "browser" | "inappbrowser" | undefined; } & { title: string; }' is not assignable to type 'undefined'.
                                                               ~
!!! error TS2339: Property '1' does not exist on type '[{ readonly title: string; readonly actionUri: string; readonly actionTarget?: "" | "browser" | "inappbrowser" | undefined; }]'.

(snipped from RWC)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Union of tuple types destructures incorrectly
3 participants