Create type node by aozgaa · Pull Request #14709 · microsoft/TypeScript

aozgaa · 2017-03-17T19:27:19Z

Migrating away from typeToString in codefixes, with the eventual hope of eliminating typeToString.

While there is still work to be done, I'm putting this up now to solicit feedback.
Known issues:

we can't represent some forms emitted by typeToString in the AST. This is most notable with type references that have type arguments in the middle of their qualified names.
I'm unsure if the method getSynthesizeDeepClone is necessary (https://github.com/Microsoft/TypeScript/compare/master...aozgaa:createTypeNode?expand=1#diff-9c914fcc05c72a72db62c0a023b15f3fR71).
I've only roughly implemented error handling. Do we want to return a more informative value than undefined if we enocunter an error?
In buildSymbolDisplay, we could register a callback to track symbols. Do we want analogous functionality for symbols used in creating type nodes (callback would be at https://github.com/Microsoft/TypeScript/compare/master...aozgaa:createTypeNode?expand=1#diff-c3ed224e4daa84352f7f1abcd23e8ccaR2576). This would be useful for checking whether the resulting identifiers are accessible under the enclosingDeclaration argument.
in visit question token in parameter #14642, we discussed adding a tokenVisitor so that token's don't need to be visited except by callers who want to see them. We are such a caller as we use the textChanges API, which requires that we synthesize clones of all children, including tokens. (added)
the types for createSignatureDeclaration and updateSignatureDeclaration are wonky (https://github.com/Microsoft/TypeScript/compare/master...aozgaa:createTypeNode?expand=1#diff-9c914fcc05c72a72db62c0a023b15f3fR392)

…existing node factory, formatter and printer

This reverts commit fd65966. Accidentally pushed to the wrong branch.

…eateTypeNode

Fix assigning positions to node array

* create type reference node from type parameters * expose index signature synthesis * widen types in helpers * format unions * use deep cloning

rbuckton · 2017-03-18T00:41:10Z

                visitNodes(node.modifiers, visitor, isModifier),
                node.asteriskToken,
                node.name,
+                node.questionToken,


questionToken is TypeScript syntax. You might as well just pass undefined here, as this property should always be undefined by the time the es2017 transform runs. We do the same for decorators, typeParameters, and type here as well.

rbuckton · 2017-03-18T00:42:18Z

                    /*modifiers*/ undefined,
                    node.dotDotDotToken,
                    getGeneratedNameForNode(node),
+                    node.questionToken,


Just pass undefined here.

rbuckton · 2017-03-18T00:42:29Z

                    ? undefined
                    : node.asteriskToken,
                visitNode(node.name, visitor, isPropertyName),
+                visitNode(node.questionToken, visitor, isToken),


Just pass undefined here.

rbuckton · 2017-03-18T00:43:47Z

                visitNodes(node.modifiers, modifierVisitor, isModifier),
                node.asteriskToken,
                visitPropertyNameOfClassElement(node),
+                node.questionToken,


You should pass undefined here.

rbuckton · 2017-03-18T00:50:01Z

            || kind === SyntaxKind.BooleanKeyword
            || kind === SyntaxKind.StringKeyword
            || kind === SyntaxKind.SymbolKeyword
+            || kind === SyntaxKind.ThisKeyword


Keep in mind this and null can also be expressions. I would check to be sure that isTypeNode (which calls isTypeNodeKind) isn't used to skip visiting a TypeNode that could also be an expression.

resolved offline: we decided that conflating this as a type node and expression isn't a problem for now because it has no children and we would only rely on the distinction for now when aggregating modifier flags from children.

We may want to introduce a distinction as we have with StringLiteral and LiteralType in the future.

rbuckton · 2017-03-21T17:13:32Z

+    // Type Elements
+
+    export function createConstructSignature() {
+        throw new Error("not implemented.");


Why have this if it is not implemented?

rbuckton · 2017-03-21T17:13:50Z

+
+    export function createTypeReferenceNode(typeName: string | EntityName, typeArguments: NodeArray<TypeNode> | undefined) {
+        const typeReference = createSynthesizedNode(SyntaxKind.TypeReference) as TypeReferenceNode;
+        typeReference.typeName = isQualifiedName(<EntityName>typeName) ? <QualifiedName>typeName : asName(<string | Identifier>typeName);


isQualifiedName test is unnecessary, just add an overload to asName for EntityName just as we do for BindingName and PropertyName

mhegazy · 2017-03-21T18:49:36Z

        getNonNullableType(type: Type): Type;

+        /** Note that the resulting nodes cannot be checked. */
+        createTypeNode(type: Type, enclosingDeclaration: Node): TypeNode;


I would recommend calling these:
typeToTypeNode and signatureToSingatureDeclaration to match typeToString and signatureToString.

I would also remove createSignatureParts, possibly extract it to a helper if needed.

mhegazy · 2017-03-21T18:52:13Z

        }

-        let typeString = "any";
+        let typeNode: TypeNode = createKeywordTypeNode(SyntaxKind.AnyKeyword);


move this to use site, no need to create it all the time.

mhegazy · 2017-03-21T18:58:51Z

-        const visibility = getVisibilityPrefixWithSpace(getModifierFlags(declaration));
-
-        const type = checker.getTypeOfSymbolAtLocation(symbol, enclosingDeclaration);
+        const name = declaration.name ? getSynthesizedDeepClone(declaration.name) as PropertyName : undefined;


do not think you need a clone here, just use the same node

mhegazy · 2017-03-21T19:00:17Z

+                    /*decorators*/undefined,
+                    modifiers,
+                    name,
+                    /*questionToken*/ undefined,


you should be able to know if the parent property is optional or not, and then add the questiontoken as needed.

mhegazy · 2017-03-21T19:18:58Z

+
+            function createTypeNodeWorker(type: Type): TypeNode {
+                if (!type) {
+                    if (undefinedArgumentIsError) { encounteredError = true; }


i would make it always an error.

mhegazy · 2017-03-21T19:20:52Z

+                    return (<IntrinsicType>type).intrinsicName === "true" ? createTrue() : createFalse();
+                }
+                if (type.flags & TypeFlags.EnumLiteral) {
+                    throw new Error("enum literal not implemented");


this should work, e.g. var x: E.value

so i would remove this then.

mhegazy · 2017-03-21T19:26:07Z

+                    return createAnonymousTypeNode(<ObjectType>type);
+                }
+
+                // TODO (aozgaa): implement string and number literals here once there is a testable case.


these are already handled earlier.

rbuckton · 2017-03-24T21:07:59Z

Do we really need this check? Are we being defensive against third parties or ourselves? If ourselves, then this check shouldn't be strictly necessary.

rbuckton · 2017-03-24T21:08:58Z

Do we really need this closure? We just tear off the functions and add them to TypeChecker.

rbuckton · 2017-03-24T21:20:14Z

+                        return createKeywordTypeNode(SyntaxKind.NeverKeyword);
+                    }
+                    if (type.flags & TypeFlags.ESSymbol) {
+                        throw new Error("ESSymbol not implemented");


Should be createKeywordTypeNode(SyntaxKind.SymbolKeyword)?

rbuckton · 2017-03-24T21:20:33Z

+                        throw new Error("ESSymbol not implemented");
+                    }
+                    if (type.flags & TypeFlags.NonPrimitive) {
+                        throw new Error("Non primitive not implemented");


Should be createKeywordTypeNode(SyntaxKind.ObjectKeyword)?

rbuckton · 2017-03-24T21:34:37Z

+                }
+            };
+
+            function typeToTypeNodeHelper(type: Type, enclosingDeclaration: Node, flags: NodeBuilderFlags): TypeNode {


Consider moving the inObjectTypeLiteral, checkAlias, and symbolStack variables to a state object and call typeToTypeNodeHelper recursively instead.

rbuckton · 2017-03-25T00:15:29Z

+
+        function signatureToMethodDeclaration(signature: Signature, enclosingDeclaration: Node, body?: Block) {
+            const signatureDeclaration = <MethodDeclaration>checker.signatureToSignatureDeclaration(signature, SyntaxKind.MethodDeclaration, enclosingDeclaration);
+            if (signatureDeclaration) {


May introduce new hidden class? Ensure decorators is assigned to undefined either here or in signatureToSignatureDeclaration.

rbuckton · 2017-03-25T00:16:20Z

-        newSignatureDeclaration.parent = enclosingDeclaration;
-        newSignatureDeclaration.name = signatures[0].getDeclaration().name;
+    function createMethodImplementingSignatures(signatures: Signature[], name: PropertyName, optional: boolean, modifiers: Modifier[] | undefined): MethodDeclaration {
+        Debug.assert(signatures && signatures.length > 0);


Do you need this assert?

rbuckton · 2017-03-25T00:17:54Z

-            if (nonRestLength > maxNonRestArgs) {
-                maxNonRestArgs = nonRestLength;
-                maxArgsIndex = i;
+            someSigHasRestParameter = someSigHasRestParameter || sig.hasRestParameter;


Unecessary assignments, consider:

if (sig.hasRestParameter) someSigHasRestParameter = true;

rbuckton · 2017-03-25T00:20:54Z

-         * for them separators are treated as the part of the node.
+         * This function should be used to insert nodes in lists when nodes don't carry separators as the part of the node range,
+         * i.e. arguments in arguments lists, parameters in parameter lists etc.
+         * Note that separators are art of the node in statements and class elements.


rbuckton · 2017-03-25T00:21:15Z

            // order changes by start position
            const normalized = stableSort(changes, (a, b) => a.range.pos - b.range.pos);
-            // verify that end position of the change is less than start position of the next change
+            // verify that change intervals to not overlap, except possible at end points.


vladima and others added 30 commits March 3, 2017 13:50

initial revision of infrastructure to produce text changes that uses …

d7e62bb

…existing node factory, formatter and printer

Merge remote-tracking branch 'origin/master' into vladima/textChanges

d847e6a

temp

fd65966

Revert "temp"

c669398

This reverts commit fd65966. Accidentally pushed to the wrong branch.

more temp

fff531c

basic end-to-end building type nodes

bf2acf1

missing member fixes use createTypeNode

d6863be

snythesize methods in missing member fix

03511c0

Simplify Ranges

f5f01a1

Print new nodes in correct order

965d43c

get name for index info

93cd1dc

Add type parameters

0603177

Use consistent quotes

6c72283

Unions and intersections

6fe59f3

type literals, properties only

f0b4efe

Merge branch 'createTypeNode' of github.com:aozgaa/TypeScript into cr…

ef72c2e

…eateTypeNode

always create new node array when assigning positions

c46a2e7

Merge pull request microsoft#14633 from zhengbli/fixNodeArray

c07fb70

Fix assigning positions to node array

update visitor/factory

1b14ed4

type parameters and basic object literals

20f512c

Refactor

d298f96

Merge branch 'vladima/textChanges' into createTypeNode

059f5b7

Merge branch 'master' into createTypeNode

e02ac17

some missed token bugs

27fe2df

Various fixes

2fd3229

* create type reference node from type parameters * expose index signature synthesis * widen types in helpers * format unions * use deep cloning

Merge remote-tracking branch 'origin/master' into vladima/textChanges

e7e1ac9

Fix deep clone

92b3e17

add insertNodeInListAfter functionality

d4bb267

visit type literal and dotDotDottoken

79a4557

don't add any typenode for signature return type

e381ffa

Arthur Ozga added 2 commits March 17, 2017 16:50

Merge branch 'master' into createTypeNode

57cd9ea

Merge branch 'master' into createTypeNode

67587cb

rbuckton suggested changes Mar 21, 2017

View reviewed changes

mhegazy reviewed Mar 21, 2017

View reviewed changes

Arthur Ozga and others added 15 commits March 21, 2017 22:54

respond to comments

5b739cf

Respond to more comments

c4ddc5b

Enum and enum literal

2958649

remove trivia

43e01f2

temp

abd14ef

error handling

1f3029d

move undefined handling to caller

dfb4df9

Split signature factory methods

8915cb9

handle todo's

ee7e9fc

Merge branch 'createTypeNode2' into createTypeNode

bfb1bb4

Merge branch 'master' into createTypeNode

82bd05b

build builder

5421812

optional signature

a7c166c

rename helper

797f6da

dont check isPartOfTypeNode

85986dd

rbuckton suggested changes Mar 25, 2017

View reviewed changes

Arthur Ozga and others added 6 commits March 25, 2017 14:14

use NodeBuilderContext

a94f874

remove NodeBuilder

e2b3c9c

essymbol and object

fb4e3d8

midway through response to ron's comments

a39bb0a

type predicate support

7340c4c

Cleanup

4329b45

aozgaa merged commit 5e4b8d6 into microsoft:master Mar 27, 2017

aozgaa mentioned this pull request Aug 11, 2017

Deduplicate Type Emit #17746

Closed

microsoft locked and limited conversation to collaborators Jun 21, 2018

Conversation

aozgaa commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

5 participants

aozgaa commented Mar 17, 2017 •

edited

Loading