Type to string via type node by aozgaa · Pull Request #15790 · microsoft/TypeScript

aozgaa · 2017-05-12T02:00:26Z

Fixes #14444, and a number of miscellaneous issues with TypeNode creation, including:

parentheses creation
emit default type parameters
Better qualified name handling
formatting fixes
and moves us closer to consolidating printing infrastructure. Will still need to make an emitter building display parts to eliminate SymbolDisplayBuilder methods.

* pass context as argument in xToNode methods * make sourcefile optional in printer * start consolidating NodeBuilderFlags and TypeFormatFlags

rbuckton · 2017-05-12T21:09:06Z

        /*@internal*/ autoGenerateKind?: GeneratedIdentifierKind; // Specifies whether to auto-generate the text for an identifier.
        /*@internal*/ autoGenerateId?: number;         // Ensures unique generated identifiers get unique names, but clones get the same name.
        isInJSDocNamespace?: boolean;                  // if the node is a member in a JSDoc namespace
+        /*@internal*/ typeArguments?: NodeArray<TypeNode>;          // Only defined on synthesized nodes.Though not syntactically valid, used in emitting diagnostics.


nit: spacing for comments.

rbuckton · 2017-05-12T21:10:26Z

+        SuppressAnyReturnType                   = 1 << 8,   // If the return type is any-like, don't offer a return type.
+
+        // Error handling
+        allowThisInObjectLiteral                = 1 << 10,


Mixed case for first letter. Prefer one or the other.

rbuckton · 2017-05-12T21:13:10Z

    export const enum NewLineKind {
        CarriageReturnLineFeed = 0,
        LineFeed = 1,
+        None = 2


Prefer we use EmitFlags.SingleLine instead of adding this member.

rbuckton · 2017-05-12T21:13:29Z

    }

    export const enum EmitFlags {
+        None       = 0,


Do we need this?

rbuckton · 2017-05-12T21:14:27Z

         * collisions.
         */
-        printNode(hint: EmitHint, node: Node, sourceFile: SourceFile): string;
+        printNode(hint: EmitHint, node: Node, sourceFile: SourceFile | undefined): string;


Don't allow undefined for source file on public API.

rbuckton · 2017-05-12T21:36:39Z


            case SyntaxKind.PropertySignature:
                return updatePropertySignature((<PropertySignature>node),
+                    nodesVisitor((<PropertySignature>node).modifiers, visitor, isToken),


Add this to reduceEachChild as well.

rbuckton · 2017-05-12T22:17:30Z

            }

-            function indexInfoToIndexSignatureDeclarationHelper(indexInfo: IndexInfo, kind: IndexKind): IndexSignatureDeclaration {
+            function mapToTypeNodeArray(types: Type[], context: NodeBuilderContext, addInElementTypeFlag: boolean, addInFirstTypeArgumentFlag: boolean): TypeNode[] {


Consider adding parenthesization logic to the factory methods for types, similar to what we do for expressions. This would ensure we always handle parentheses consistently and would simplify much of the logic here.

rbuckton · 2017-05-12T22:23:08Z

        _incrementExpressionBrand: any;
        _unaryExpressionBrand: any;
        _expressionBrand: any;
+        typeArguments: any;


Give this the right type, but mark it /*@internal*/

rbuckton · 2017-05-12T22:26:01Z

 var x = "\u{abcd}\u{ef12}\u{3456}\u{7890}";
 >x : string
->"\u{abcd}\u{ef12}\u{3456}\u{7890}" : "ꯍ㑖碐"
+>"\u{abcd}\u{ef12}\u{3456}\u{7890}" : "\uABCD\uEF12\u3456\u7890"


We should probably fix these cases.

Consider adding support for textSourceNode from StringLiteral to Identifier so that you can just pull the raw text from the original source file. You would have to modify both getTextOfNode and getLiteralTextOfNode in emitter.ts to make a best effort to find the source file of the node if currentSourceFile is undefined, and otherwise just use the node's text.

rbuckton · 2017-05-12T22:26:48Z

+//// class C implements I {[| |]}
+
+verify.rangeAfterCodeFix(`
+    test(a: [string, number]): void {


Why can't we use the alias name?

Sometimes we really ought to instantiate the type reference. Consider:

interface I<T> { foo(t: T): void; } class C implements I<number> {}

Then we clearly ought to use the instantiated type for a.

It looks like it will be tricky to distinguish the above case and other variants from the test case.

The behavior in the test doesn't represent a regression.

Because the fix is complicated, I'll leave it for a separate PR.

rbuckton · 2017-05-12T22:41:25Z

        IntersectionTypeConstituents = AmpersandDelimited | SpaceBetweenSiblings | SingleLine,
-        ObjectBindingPatternElements = SingleLine | AllowTrailingComma | SpaceBetweenBraces | CommaDelimited | SpaceBetweenSiblings,
+        ObjectBindingPatternElements = SingleLine | CommaDelimited | SpaceBetweenSiblings,
+        ObjectBindingPatternElementsWithSpaceBetweenBraces = SingleLine | AllowTrailingComma | SpaceBetweenBraces | CommaDelimited | SpaceBetweenSiblings,


Consider reverting this in a separate PR, and simplify the names (especially for the general emit case).

rbuckton

Approved with a few minor comments.

rbuckton · 2017-05-16T21:02:04Z

+
+    export function parenthesizeElementTypeMembers(members: TypeNode[]) {
+        // TODO: does this lose `originalNode` ptr?
+        return createNodeArray(members.map(parenthesizeElementTypeMember));


Consider using sameMap. Also do you need to resolve the TODO here?

rbuckton · 2017-05-16T21:02:16Z

+    }
+
+    export function parenthesizeTypeParameters(typeParameters: TypeNode[]) {
+        if (typeParameters && typeParameters.length > 0) {


some(typeParameters)

rbuckton · 2017-05-16T21:05:03Z

+    export function parenthesizeTypeParameters(typeParameters: TypeNode[]) {
+        if (typeParameters && typeParameters.length > 0) {
+            const nodeArray = createNodeArray(typeParameters);
+            const firstEntry = nodeArray[0];


Create a fresh node array, createNodeArray actually just turns typeParameters into a node array, so you end up mutating an existing array.

rbuckton · 2017-05-16T21:05:54Z

        return false;
    }

+    export function isFunctionOrConstructor(node: Node): node is FunctionTypeNode | ConstructorTypeNode {


Maybe isFunctionOrConstructorTypeNode?

rbuckton · 2017-05-16T21:07:01Z

-        /*@internal*/ typeArguments?: NodeArray<TypeNode>;          // Only defined on synthesized nodes.Though not syntactically valid, used in emitting diagnostics.
+        /*@internal*/ autoGenerateId?: number;                    // Ensures unique generated identifiers get unique names, but clones get the same name.
+        isInJSDocNamespace?: boolean;                             // if the node is a member in a JSDoc namespace
+        /*@internal*/ typeArguments?: NodeArray<TypeNode>;        // Only defined on synthesized nodes.Though not syntactically valid, used in emitting diagnostics.


nit: spacing nodes.Though

rbuckton · 2017-05-16T21:12:03Z

-                const inFirstTypeArgument = context.flags & NodeBuilderFlags.InFirstTypeArgument;
                const inTypeAlias = context.flags & NodeBuilderFlags.InTypeAlias;
-                context.flags &= ~(NodeBuilderFlags.StateClearingFlags);
+                context.flags &= ~(NodeBuilderFlags.InTypeAlias);


Remove parens

rbuckton · 2017-05-16T21:13:56Z

                    isSymbolAccessible(type.aliasSymbol, context.enclosingDeclaration, SymbolFlags.Type, /*shouldComputeAliasesToMakeVisible*/ false).accessibility === SymbolAccessibility.Accessible) {
                    const name = symbolToTypeReferenceName(type.aliasSymbol);
-                    const typeArgumentNodes = toTypeArgumentNodes(type.aliasTypeArguments, context);
+                    const typeArgumentNodes = type.aliasTypeArguments && mapToTypeNodeArray(type.aliasTypeArguments, context);


consider moving existential check into mapToTypeNodeArray

rbuckton · 2017-05-16T21:15:08Z

                    else if (type.target.objectFlags & ObjectFlags.Tuple) {
                        if (typeArguments.length > 0) {
-                            const tupleConstituentNodes = mapToTypeNodeArray(typeArguments.slice(0, getTypeReferenceArity(type)), context, /*addInElementTypeFlag*/ false, /*addInFirstTypeArgumentFlag*/ false);
+                            const slice = typeArguments.slice(0, getTypeReferenceArity(type));


If you add the check to mapToTypeNodeArray you don't need to break this up into two statements.

rbuckton · 2017-05-16T21:15:43Z

                                // the default outer type arguments), we don't show the group.
                                if (!rangeEquals(outerTypeParameters, typeArguments, start, i)) {
-                                    const typeArgumentNodes = createNodeArray(toTypeArgumentNodes(typeArguments.slice(start, i), context));
+                                    const slice = typeArguments.slice(start, i);


rbuckton · 2017-05-16T21:16:01Z

                            const typeParameterCount = (type.target.typeParameters || emptyArray).length;
                            const slice = typeArguments && typeArguments.slice(i, typeParameterCount);
-                            typeArgumentNodes = toTypeArgumentNodes(slice, context);
+                            typeArgumentNodes = slice && mapToTypeNodeArray(slice, context);


Arthur Ozga added 30 commits March 30, 2017 14:14

Basic functionality

7e25179

* pass context as argument in xToNode methods * make sourcefile optional in printer * start consolidating NodeBuilderFlags and TypeFormatFlags

Readonly type literal members

55388d1

Merge branch 'master' into typeToStringViaTypeNode

cddccf1

No space in empty type litereal

10afa2f

typo

d0f3716

Basic parens handling and stubbed TypeFormatFlags

384d546

Add aliasing test

c0adf99

parens and this param for function type

1774c67

fix inTypeAlias logic

d2bda47

Use symbolToName instead of copying

a5bef21

emit type parameter's default

b8bb0c2

convert type format flags

a1d2049

accessibility check

818bfdc

Better qualified name handling

e43377d

UseFullyQualifiedType

965b98f

Parameter improvements

b00aea5

Type Literal and Binding Name formatting

d9b68e9

fix malformed nodes

51c76ac

initializer issues

ac68e8d

Parenthesize function types

e0438db

mapped type indent

89f8cca

trailing comma

ad58572

add typeArguments to identifier

41452a5

type arguments in qualified names

ac6018a

Accept baselines and update test

d5f34da

remove souceFile checks

a5e93c4

cleanup

82109d3

move state to flags

c099938

use EmitFlags.SingleLine

0432257

handle todo

7d48dee

Remove unused flags

3640abd

msftclas added the cla-not-required label May 12, 2017

Arthur Ozga added 2 commits May 12, 2017 13:09

Merge branch 'master' into typeToStringViaTypeNode

62dbf12

cleanup

0588f8b

rbuckton suggested changes May 12, 2017

View reviewed changes

rbuckton reviewed May 12, 2017

View reviewed changes

Arthur Ozga added 8 commits May 12, 2017 16:27

respond to comments

a32bc98

handle todo

9e9054b

temp

6fd86b4

parenthesization in factory

20a4e32

update tests

d8f29a5

avoid ascii escaping literal text

bba41a8

revert baselines

a9ab30c

no ascii escaping in param names

b2176c1

rbuckton approved these changes May 16, 2017

View reviewed changes

Arthur Ozga added 2 commits May 16, 2017 15:28

respond to comments

0820f69

Merge branch 'master' into typeToStringViaTypeNode

11019e4

aozgaa merged commit ebcbd8f into microsoft:master May 17, 2017

aozgaa mentioned this pull request May 22, 2017

Object literal formatting #16007

Merged

aozgaa mentioned this pull request Aug 11, 2017

Deduplicate Type Emit #17746

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

aozgaa commented May 12, 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!

rbuckton left a comment

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!

3 participants

aozgaa commented May 12, 2017 •

edited

Loading