Fully deprecate the symbol display builder, reimplement in terms of node builder #18860

weswigham · 2017-09-29T22:30:15Z

While working on the new declaration emitter, I recognized that there were certain parts of the current declaration emitter which relied on the symbol display builder. For the most part, we had heavily duplicated logic between the symbol display builder and the node builder. The PR fully removes the symbol display builder; replacing it with the node builder and a printer, after upgrading our emitter to do some of its writes in the more granular fashion that the symbol display writer used to expect, and altering the node builder to be capable of the little bit of extra reporting the declaration emitter expects.

This enables me to use the node builder (as one would expect) in the new declaration emitter in a similar fashion to how it is now used in the current declaration emitter.

While I've tried to keep the baseline changes to the absolute minimum, there are a few correctness changes, and a few whitespace changes. The correctness changes are for mapped types - previously when the constraint was written, it would write the declared constraint, rather than the instantiated constraint (which was noticeable in declaration emit baselines, so this has now been fixed). The whitespace changes have to do with the emitter - parameter, type parameter, and type argument lists were marked as both SingleLine and Indented - causing objects written across multiple lines to have way more indents than they should - these are no longer marked as Indented, and so no longer randomly add indentation (even though they should never add newlines on their own). This improved some of our js baselines to actually match the input, which is a good thing (in addition to making the declaration output more closely match old output). Other minor changes are that mapped types are written on a single line when synthesized, rather than on multiple lines, and that the declaration emitter more aggressively uses the typeof operator where possible (as before a small bug in the symbol display builder was preventing it from finding some local aliases).

As the SymbolDisplayBuilder (and SymbolWriter, which has been subsumed into EmitTextWriter) was part of our public API, this is a breaking change - however new XToString APIs exist which take a writer and can be used to polyfill the functionality.

…ngle-line

rbuckton · 2017-11-11T00:49:22Z

src/compiler/checker.ts

+            if (!flags) {
                return result;
            }
+            if (flags & TypeFormatFlags.NoTruncation) {


Could we align the values between these two enums and just use a bitmask?

rbuckton · 2017-11-11T01:35:27Z

src/compiler/emitter.ts

        let ownWriter: EmitTextWriter;
+        let write = writeBase;
+        let pendingSemicolon = false;
+        const syntheticParent: TextRange = { pos: -1, end: -1 };


Possibly adds polymorphism because Node sets id and kind before pos and end, resulting in an incompatible hidden class. Consider moving pos and end up in Node, or give syntheticParent an id and a kind.

I've moved pos and end before the other fields - they were already first in the services constructor implementation for Node, so I think this only changes tsc.

rbuckton · 2017-11-11T01:38:52Z

src/compiler/emitter.ts

        function emitIdentifier(node: Identifier) {
-            write(getTextOfNode(node, /*includeTrivia*/ false));
-            emitTypeArguments(node, node.typeArguments);
+            (node.symbol ? writeSymbol : write)(getTextOfNode(node, /*includeTrivia*/ false), node.symbol);


Ternary makes this harder to read.

rbuckton · 2017-11-11T01:43:07Z

src/compiler/emitter.ts

            emitModifiers(node, node.modifiers);
            emitIfPresent(node.dotDotDotToken);
            if (node.name) {
+                const savedWrite = write;


You use this pattern in enough places to consider a helper to do this: emitWithWriter(node.name, writeParameter)

function emitNodeWithWriter(node: Node, writer: typeof write) { const savedWrite = write; write = writer; emit(node); write = savedWrite; }

rbuckton · 2017-11-11T01:46:01Z

src/compiler/emitter.ts

            emit(node.name);
            emitTypeParameters(node, node.typeParameters);
-            write(" = ");
+            writeSpace(); writePunctuation("="); writeSpace();


Separate lines

rbuckton · 2017-11-11T02:13:06Z

src/services/signatureHelp.ts

-                    typeChecker.getSymbolDisplayBuilder().buildDisplayForTypeParametersAndDelimiters(candidateSignature.typeParameters, writer, invocation));
+                const typeParameterParts = mapToDisplayParts(writer => {
+                    if (candidateSignature.typeParameters && candidateSignature.typeParameters.length) {
+                        const printer = createPrinter({ removeComments: true });


As above, you only need one printer.

rbuckton · 2017-11-11T02:14:07Z

src/services/signatureHelp.ts

-            const displayParts = mapToDisplayParts(writer =>
-                typeChecker.getSymbolDisplayBuilder().buildTypeParameterDisplay(typeParameter, writer, invocation));
+            const displayParts = mapToDisplayParts(writer => {
+                const printer = createPrinter({ removeComments: true });


Again, Printer is designed to be reused when passed the same set of initial arguments. No sense in recreating it all the time.

rbuckton · 2017-11-11T02:15:30Z

src/services/utilities.ts

+            getColumn: () => 0,
+            getLine: () => 0,
+            isAtStartOfLine: () => false,
+            rawWrite: noop,


Should this be noop or should it raise an error when called?

I've changed it to raise an error, since rawWrite is only used to print comments, and the display part writer should only be used when comment printing is disabled.

rbuckton · 2017-11-11T02:18:33Z

src/compiler/types.ts

        getTypeAtLocation(node: Node): Type;
        getTypeFromTypeNode(node: TypeNode): Type;
        signatureToString(signature: Signature, enclosingDeclaration?: Node, flags?: TypeFormatFlags, kind?: SignatureKind): string;
+        /* @internal */ signatureToString(signature: Signature, enclosingDeclaration?: Node, flags?: TypeFormatFlags, kind?: SignatureKind, writer?: EmitTextWriter): string;


Rather than overloading these, why not have:

/* @internal */ writeSignature(writer: EmitTextWriter, signature: Signature, ...): void; /* @internal */ writeType(writer: EmitTextWriter, type: Type, ...): void; /* @internal */ writeSymbol(writer: EmitTextWriter, symbol: Symbol, ...): void; /* @internal */ writeTypePredicate(writer: EmitTextWriter, predicate: TypePredicate, ...): void;

That is similar to how Printer splits up internal and public members.

rbuckton · 2017-11-11T02:19:55Z

src/compiler/types.ts

        UseOnlyExternalAliasing = 0x00000002,
+
+        // Build symbol name using property accesses and element accesses, instead of an entity name
+        AllowAnyNodeKind = 0x00000004,


The name AllowAnyNodeKind doesn't seem to align with the intended use described in the comment. Perhaps a better description or a more descriptive name would be better here.

Updated the description

…hidden class is made first to reduce future polymorphism

… rename function in emitter

weswigham · 2018-01-03T22:39:25Z

@rbuckton I've finally gotten around to syncing this and implementing your comments. Is there anything else you'd like to see?

mhegazy · 2018-01-08T20:41:14Z

@weswigham can you please refresh, sync with @rbuckton and merge

weswigham added 30 commits September 27, 2017 11:54

Remove SymbolWriter, give methods to EmitTextWriter

bc8a27c

Unification of writers is done-ish

1dff649

Make node builder support more flags

7fa4ec9

Write out mixins like we used to

fc0ae76

Accept prototype-free baselines

72d413a

Use instantiated constraint when building mapped type nodes

e68c111

Accept better mapped type baselines

5645096

Report inaccessible this in node builder

9501e62

Turns out there was a bug in our codefix, too

1f5972a

Symbol display builder usage falling

266f553

Replace signatureToString with a nodeBuilder solution

b143516

Replace the last internal usages of the symbol writer

6c227b4

Accept semicolon additions

0e58719

Accept updated symbol baseline output

a926577

Start using node builder for some LS operations

d8dfe84

Remove those pesky trailing semicolons on signatures

d76ddef

Get signature printing much closer to old output

50cf103

Parameter lists should not be indented by default, especially when si…

a4725a8

…ngle-line

Signatures up to snuff

ab99ee2

Type quickinfo emit is up to snuff

e28202a

Start of symbol writer replacement, needs a bit more for full compat

358c5c2

Slightly mor accurate to old behavior

4963b0f

Replicate qualified name type argument output correctly

581cf56

Bring back the old symbol baselines

5b90321

Mostly identical to old symbol emit now

83bea81

Perfectly matches old behavior thus far

8bb7230

Replace another usage of the symbol builder

cf6458f

Another usage removed

8a773ab

Another usage removed

1f5aec2

Remove final uses of symbol display builder

65a7a4b

weswigham added 2 commits November 10, 2017 16:52

Mimic default name declaration emit fix

adc43f0

Merge branch 'master' into couple-node-builder-with-symbol-tracker

f5c79d5

weswigham force-pushed the couple-node-builder-with-symbol-tracker branch from ec3f036 to f5c79d5 Compare November 11, 2017 01:25

rbuckton suggested changes Nov 11, 2017

View reviewed changes

Merge branch 'master' into couple-node-builder-with-symbol-tracker

03ddb06

aozgaa mentioned this pull request Dec 7, 2017

Omnibus fixes for telemetry-sourced crashes #20545

Merged

weswigham added 11 commits January 3, 2018 12:19

Merge branch 'master' into couple-node-builder-with-symbol-tracker

fb9fd83

Fix tests broken by merge

7a8db71

use isFunctionLike

450100a

Cleanup, sync TypeFormat and NodeBuilder flags

7fd3655

Reorder Node initialization so pos and end are first, so a TextRange …

3943e4c

…hidden class is made first to reduce future polymorphism

Use variable instead of ternary

bbffdd7

Write helper for emitNodeWithWriter

ad102a6

Emitter cleanup

5ac170b

Cleanup whitespace, comment

3e88144

Reuse printer

0abba6d

Raise error if display parts writer uses rawWrite

3e18178

weswigham force-pushed the couple-node-builder-with-symbol-tracker branch from 6862e50 to 3e18178 Compare January 3, 2018 22:07

weswigham added 2 commits January 3, 2018 14:18

Hide writer parameter through different function instead of overload,…

4cec0ca

… rename function in emitter

Make less printer

232d459

weswigham added 2 commits January 9, 2018 14:19

Merge branch 'master' into couple-node-builder-with-symbol-tracker

aa27a6a

fix lint

a481f7f

rbuckton approved these changes Jan 16, 2018

View reviewed changes

weswigham merged commit 76d9524 into microsoft:master Jan 16, 2018

This was referenced Jan 16, 2018

Accept corrected baseline #21201

Merged

Allow functions to be printed structurally in declaration emit even when they have symbols #21203

Merged

weswigham deleted the couple-node-builder-with-symbol-tracker branch February 26, 2018 23:55

microsoft locked and limited conversation to collaborators Jul 3, 2018

Fully deprecate the symbol display builder, reimplement in terms of node builder #18860

Fully deprecate the symbol display builder, reimplement in terms of node builder #18860

Uh oh!

Conversation

weswigham commented Sep 29, 2017

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!

weswigham commented Jan 3, 2018

Uh oh!

mhegazy commented Jan 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants