Turn 2 type flags into properties by sandersn · Pull Request #11212 · microsoft/TypeScript

sandersn · 2016-09-28T16:55:22Z

This is needed for object spread and rest, which will each need a type
flag.

There are 4-5 other likely targets for removal, and I may remove those
later.

1. Instantiated (only modifies anonymous types) 2. ObjectLiteralWithComputedProperties (only modifies [resolved] object types) 3. ThisType (only modifies type parameters) This is needed for object spread and rest, which will each need a type flag. There are 4-5 other likely targets for removal, and I may remove those later.

sandersn · 2016-09-28T16:55:47Z

@ahejlsberg and @mhegazy can you take a look at this?

ahejlsberg · 2016-09-28T22:23:15Z

        Union                   = 1 << 19,  // Union (T | U)
        Intersection            = 1 << 20,  // Intersection (T & U)
        Anonymous               = 1 << 21,  // Anonymous
-        Instantiated            = 1 << 22,  // Instantiated anonymous type


I would preserve this flag unless you really need to free it up. In generic code we create lots of instantiated anonymous types and it would be nice not to incur the overhead of another property.

I only need one flag right now (Spread), but I'll need another for Rest and Ryan will need one for Subset. Maybe ObjectLiteral is a better target?

FreshLiteral seems pretty easy to excise as well, but there are a lot of literals in any given program.

Instantiated is back in. I'm looking at ObjectLiteral instead.

I'd leave ObjectLiteral alone. Just free up the other two for now and then we can do more if and when it is needed. I'd prefer not incurring overhead unless we have to.

ahejlsberg · 2016-09-28T22:25:20Z

-            if (hasComputedProperties) {
-                result.flags |= TypeFlags.ObjectLiteralPatternWithComputedProperties;
-            }
+            result.inObjectLiteralPatternWithComputedProperties = hasComputedProperties;


Only create this property if hasComputedProperties is true.

Hm, turns out I was wrong when I thought that x.prop = undefined is a no-op. Fixed.

ahejlsberg · 2016-09-28T22:26:52Z

         * type itself. Note that the apparent type of a union type is the union type itself.
         */
-        function getApparentType(type: Type): Type {
+        function getApparentType(type: Type): ObjectType {


Why this change? This function isn't guaranteed to return an ObjectType.

All the branches do return ObjectType, but I forgot about the else branch. Reverted.

ahejlsberg · 2016-09-28T22:31:25Z


            function hasExcessProperties(source: FreshObjectLiteralType, target: Type, reportErrors: boolean): boolean {
-                if (!(target.flags & TypeFlags.ObjectLiteralPatternWithComputedProperties) && maybeTypeOfKind(target, TypeFlags.ObjectType)) {
+                if (maybeTypeOfKind(target, TypeFlags.ObjectType) && !(target as ObjectType).inObjectLiteralPatternWithComputedProperties) {


The maybeTypeOfKind function could return true when target is a union or intersection containing an object type, so the second check needs to first ensure target is actually an object type.

I was relying on unions and intersections not having inObjectLiteralPatternWithComputedProperties and having the check come back with undefined. Adding the check is the right thing, though.

ahejlsberg · 2016-09-28T22:33:21Z

        // Return the contextual type for a given expression node. During overload resolution, a contextual type may temporarily
        // be "pushed" onto a node using the contextualType property.
-        function getApparentTypeOfContextualType(node: Expression): Type {
+        function getApparentTypeOfContextualType(node: Expression): ObjectType {


The returned type could be something other than an ObjectType so this change isn't correct.

ahejlsberg · 2016-09-28T22:34:17Z

            const freshObjectLiteralFlag = compilerOptions.suppressExcessPropertyErrors ? 0 : TypeFlags.FreshLiteral;
-            result.flags |= TypeFlags.ObjectLiteral | TypeFlags.ContainsObjectLiteral | freshObjectLiteralFlag | (typeFlags & TypeFlags.PropagatingFlags) | (patternWithComputedProperties ? TypeFlags.ObjectLiteralPatternWithComputedProperties : 0);
+            result.flags |= TypeFlags.ObjectLiteral | TypeFlags.ContainsObjectLiteral | freshObjectLiteralFlag | (typeFlags & TypeFlags.PropagatingFlags);
+            result.inObjectLiteralPatternWithComputedProperties = patternWithComputedProperties;


Only set the property if patternWithComputedProperties is true.

sandersn · 2016-09-29T17:36:17Z

I put Instantiated back in and removed ObjectLiteral instead. This keeps 3 flags open for use with Spread, Rest and Subset.

sandersn · 2016-09-29T18:27:18Z

Oops, I didn't see your new comments, @ahejlsberg. I put ObjectLiteral back as well.

Igorbek · 2016-09-29T18:46:07Z

Why didn't you just introduce a new flags2 ?

ahejlsberg · 2016-10-03T22:43:43Z

            }
            if (hasComputedProperties) {
-                result.flags |= TypeFlags.ObjectLiteralPatternWithComputedProperties;
+                result.inObjectLiteralPatternWithComputedProperties = hasComputedProperties;


Just say = true;

Also, shouldn't it be isObjectBlaBlaBla...? In other words, "is" instead of "in".

ahejlsberg · 2016-10-03T22:46:20Z

                (contextualType.pattern.kind === SyntaxKind.ObjectBindingPattern || contextualType.pattern.kind === SyntaxKind.ObjectLiteralExpression);
            let typeFlags: TypeFlags = 0;
-            let patternWithComputedProperties = false;
+            let patternWithComputedProperties: boolean | undefined;


No need for this change.

ahejlsberg · 2016-10-03T22:46:35Z

+        function getTypeFromObjectBindingPattern(pattern: ObjectBindingPattern, includePatternInType: boolean, reportErrors: boolean): ResolvedType {
            const members = createMap<Symbol>();
-            let hasComputedProperties = false;
+            let hasComputedProperties: boolean;


No need for this change.

ahejlsberg · 2016-10-03T23:04:37Z


        // Return the type implied by an object binding pattern
-        function getTypeFromObjectBindingPattern(pattern: ObjectBindingPattern, includePatternInType: boolean, reportErrors: boolean): Type {
+        function getTypeFromObjectBindingPattern(pattern: ObjectBindingPattern, includePatternInType: boolean, reportErrors: boolean): ResolvedType {


One more change that isn't necessary.

reverted to Type

msftclas added the cla-not-required label Sep 28, 2016

ahejlsberg requested changes Sep 28, 2016

View reviewed changes

Remove TypeFlags.ObjectLiteral, not Instantiated

bc028a3

sandersn added 2 commits September 29, 2016 10:46

Make isObjectLiteral required

561168c

Put TypeFlags.ObjectLiteral back

9e1b530

sandersn changed the title ~~Turn 3 type flags into properties~~ Turn 2 type flags into properties Oct 3, 2016

ahejlsberg reviewed Oct 3, 2016

View reviewed changes

sandersn added 3 commits October 3, 2016 15:54

Minor style cleanup

84e319e

Merge branch 'master' into cleanup-TypeFlags

99cbd1f

Rename isObjectLiteralPatternWithComputedProperties

663ee44

ahejlsberg reviewed Oct 3, 2016

View reviewed changes

sandersn added 2 commits October 3, 2016 16:06

getTypeFromObjectBindingPattern just returns Type again

45af47a

Just assign true inside an if where a boolean was just checked

f852696

ahejlsberg approved these changes Oct 3, 2016

View reviewed changes

sandersn merged commit 4b8e8b7 into master Oct 3, 2016

sandersn deleted the cleanup-TypeFlags branch October 3, 2016 23:25

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

sandersn commented Sep 28, 2016

Uh oh!

sandersn commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Sep 29, 2016

Uh oh!

sandersn commented Sep 29, 2016

Uh oh!

Igorbek commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sandersn Sep 29, 2016 •

edited

Loading