Skip to content

Spread & rest over intersection and unions #12552

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 12 commits into from
Nov 30, 2016

Conversation

mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Nov 29, 2016

Fixes #12520 , #12534 and #12460

//CC @sandersn

@mhegazy mhegazy changed the title Spread rest intersection and unions Spread & rest over intersection and unions Nov 29, 2016
@@ -0,0 +1,38 @@
=== tests/cases/compiler/restIntersectionOrIntersection.ts ===
Copy link
Member

Choose a reason for hiding this comment

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

should be named restIntersectionOrUnion.ts, right?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

You should copy some fourslash tests from the original spread PR, pre-object-only. In particular, rename was tricky, as I recall.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Couple of tiny updates, otherwise looks good.

return getUnionType(map(types, t => getSpreadType(left, t, isFromObjectLiteral)));
}
else {
right = emptyObjectType
Copy link
Member

Choose a reason for hiding this comment

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

missing semicolon

@@ -68,11 +66,7 @@ tests/cases/conformance/types/spread/objectSpreadNegative.ts(61,14): error TS269

// null, undefined and primitives are not allowed
Copy link
Member

Choose a reason for hiding this comment

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

can you update this comment?

@mhegazy
Copy link
Contributor Author

mhegazy commented Nov 29, 2016

@ahejlsberg can you take a look

if (left.flags & TypeFlags.Any || right.flags & TypeFlags.Any) {
return anyType;
}

if (left.flags & TypeFlags.Union) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these two parts of code for left and right looks quite similar, can they be extracted to a function?

if (left.flags & TypeFlags.Any || right.flags & TypeFlags.Any) {
return anyType;
}

if (left.flags & TypeFlags.Union) {
const types = filter((<UnionType>left).types, t => !(t.flags & TypeFlags.Nullable));
Copy link
Contributor

Choose a reason for hiding this comment

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

this pattern filter out nullable types is repeated at least 3 times

mhegazy added a commit to microsoft/tslib that referenced this pull request Nov 30, 2016
if (left.flags & TypeFlags.Any || right.flags & TypeFlags.Any) {
return anyType;
}

if (left.flags & TypeFlags.Union) {
Copy link
Member

Choose a reason for hiding this comment

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

You can replace all of these new lines with just this:

            left = filterType(left, t => !(t.flags & TypeFlags.Nullable));
            if (left.flags & TypeFlags.Never) {
                return right;
            }
            right = filterType(right, t => !(t.flags & TypeFlags.Nullable));
            if (right.flags & TypeFlags.Never) {
                return left;
            }
            if (left.flags & TypeFlags.Union) {
                return mapType(left, t => getSpreadType(t, right, isFromObjectLiteral));
            }
            if (right.flags & TypeFlags.Union) {
                return mapType(right, t => getSpreadType(left, t, isFromObjectLiteral));
            }

@@ -11516,6 +11552,24 @@ namespace ts {
}
}

function isValidSpreadType(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.

The functional style is a lot shorter:

function isValidSpreadType(type: Type): boolean {
    return !!(type.flags & (TypeFlags.Any | TypeFlags.Null | TypeFlags.Undefined) ||
        type.flags & TypeFlags.Object && !isGenericMappedType(type) ||
        type.flags & TypeFlags.UnionOrIntersection && !forEach((<UnionOrIntersectionType>type).types, t => !isValidSpreadType(t)));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. fixed.

@@ -6097,16 +6103,46 @@ namespace ts {
return symbol ? getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(symbol) : undefined;
}

function filterNulableTypes(union: UnionType): Type[] {
Copy link
Member

Choose a reason for hiding this comment

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

Spelling. Not sure you need to keep this function, but if you do, have it operate on any kind of type in the same way as mapType and filterType.

@@ -11516,6 +11538,24 @@ namespace ts {
}
}

function isValidSpreadType(type: Type): boolean {
if (type.flags & (TypeFlags.Any | TypeFlags.Null | TypeFlags.Undefined)) {
return true;

Choose a reason for hiding this comment

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

IIUC this should also fix #12460

mhegazy added a commit to microsoft/tslib that referenced this pull request Nov 30, 2016
@ahejlsberg
Copy link
Member

ahejlsberg commented Nov 30, 2016

👍

@mhegazy mhegazy merged commit 4c9bdb9 into master Nov 30, 2016
@mhegazy mhegazy deleted the spreadRestIntersectionAndUnions branch November 30, 2016 01:27
@mhegazy mhegazy mentioned this pull request Dec 1, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants