Skip to content

Ensure that emitter calls callbacks #18284

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.

Sign up for GitHub

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
9 commits merged into from
Sep 7, 2017
Merged

Ensure that emitter calls callbacks #18284

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9815359
Ensure that emitter calls calbacks
Sep 5, 2017
7d931bb
Move new parameter to end of parameters
Sep 7, 2017
7d56deb
Merge branch 'master' into extract-method-format
Sep 7, 2017
5b44de1
Fix for ConditionalExpression
Sep 7, 2017
aae877d
Make suggested changes to emitter
Sep 7, 2017
8f775dc
Fix parameter ordering
Sep 7, 2017
bca3b09
Respond to minor comments
Sep 7, 2017
15e2663
Remove potentially expensive assertion
Sep 7, 2017
b33dbec
More emitter cleanup
Sep 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 64 additions & 43 deletions src/compiler/emitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1035,10 +1035,8 @@ namespace ts {

function emitTypeLiteral(node: TypeLiteralNode) {
write("{");
// If the literal is empty, do not add spaces between braces.
if (node.members.length > 0) {
emitList(node, node.members, getEmitFlags(node) & EmitFlags.SingleLine ? ListFormat.SingleLineTypeLiteralMembers : ListFormat.MultiLineTypeLiteralMembers);
}
const flags = getEmitFlags(node) & EmitFlags.SingleLine ? ListFormat.SingleLineTypeLiteralMembers : ListFormat.MultiLineTypeLiteralMembers;
emitList(node, node.members, flags | ListFormat.NoSpaceIfEmpty);
write("}");
}

Expand Down Expand Up @@ -1095,11 +1093,14 @@ namespace ts {
increaseIndent();
}
writeIfPresent(node.readonlyToken, "readonly ");
write("[");
emit(node.typeParameter.name);
write(" in ");
emit(node.typeParameter.constraint);
write("]");

if (onEmitNode) {
Copy link
Contributor

@rbuckton rbuckton Sep 6, 2017

Choose a reason for hiding this comment

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

We really should avoid calling onEmitNode directly, and should be calling emit instead, except in this case we need to emit a type parameter in a different fashion. For anyone writing a custom transform, they won't know whether their onEmitNode is being called against a normal TypeParameter or the TypeParameter of a MappedType. That is the reason we have EmitHint, as it allows us to provide a hint as to the context in which we are emitting a node.

I would recommend we add MappedTypeParameter to the EmitHint enum and add a branch to pipelineEmitWithHint for that branch that calls emitMappedTypeParameter. Then we can replace this code with a call to pipelineEmitWithNotification(EmitHint.MappedTypeParameter, node.typeParameter).

onEmitNode(EmitHint.Unspecified, node.typeParameter, emitMappedTypeParameter);
}
else {
emitMappedTypeParameter(EmitHint.Unspecified, node.typeParameter);
}

writeIfPresent(node.questionToken, "?");
write(": ");
emit(node.type);
Expand All @@ -1114,6 +1115,14 @@ namespace ts {
write("}");
}

function emitMappedTypeParameter(_hint: EmitHint, node: TypeParameterDeclaration) {
write("[");
Copy link
Contributor

@rbuckton rbuckton Sep 6, 2017

Choose a reason for hiding this comment

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

We parse the opening and closing brackets in parseMappedType, so their tokens do not belong to the type parameter. Writing the [ and ] tokens should happen in emitMappedType.

emit(node.name);
write(" in ");
emit(node.constraint);
write("]");
}

function emitLiteralType(node: LiteralTypeNode) {
emitExpression(node.literal);
}
Expand Down Expand Up @@ -1159,33 +1168,22 @@ namespace ts {

function emitArrayLiteralExpression(node: ArrayLiteralExpression) {
const elements = node.elements;
if (elements.length === 0) {
write("[]");
}
else {
const preferNewLine = node.multiLine ? ListFormat.PreferNewLine : ListFormat.None;
emitExpressionList(node, elements, ListFormat.ArrayLiteralExpressionElements | preferNewLine);
}
const preferNewLine = node.multiLine ? ListFormat.PreferNewLine : ListFormat.None;
emitExpressionList(node, elements, ListFormat.ArrayLiteralExpressionElements | preferNewLine);
}

function emitObjectLiteralExpression(node: ObjectLiteralExpression) {
const properties = node.properties;
if (properties.length === 0) {
write("{}");
const indentedFlag = getEmitFlags(node) & EmitFlags.Indented;
if (indentedFlag) {
increaseIndent();
}
else {
const indentedFlag = getEmitFlags(node) & EmitFlags.Indented;
if (indentedFlag) {
increaseIndent();
}

const preferNewLine = node.multiLine ? ListFormat.PreferNewLine : ListFormat.None;
const allowTrailingComma = currentSourceFile.languageVersion >= ScriptTarget.ES5 ? ListFormat.AllowTrailingComma : ListFormat.None;
emitList(node, properties, ListFormat.ObjectLiteralExpressionProperties | allowTrailingComma | preferNewLine);
const preferNewLine = node.multiLine ? ListFormat.PreferNewLine : ListFormat.None;
const allowTrailingComma = currentSourceFile.languageVersion >= ScriptTarget.ES5 ? ListFormat.AllowTrailingComma : ListFormat.None;
emitList(node, node.properties, ListFormat.ObjectLiteralExpressionProperties | allowTrailingComma | preferNewLine);

if (indentedFlag) {
decreaseIndent();
}
if (indentedFlag) {
decreaseIndent();
}
}

Expand Down Expand Up @@ -1286,7 +1284,8 @@ namespace ts {
emitTypeParameters(node, node.typeParameters);
emitParametersForArrow(node, node.parameters);
emitWithPrefix(": ", node.type);
write(" =>");
write(" ");
emit(node.equalsGreaterThanToken);
}

function emitDeleteExpression(node: DeleteExpression) {
Expand Down Expand Up @@ -1382,7 +1381,10 @@ namespace ts {
}

function emitYieldExpression(node: YieldExpression) {
write(node.asteriskToken ? "yield*" : "yield");
write("yield");
if (node.asteriskToken) {
Copy link
Contributor

@rbuckton rbuckton Sep 6, 2017

Choose a reason for hiding this comment

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

No need to guard against undefined, as that is already handled in emit

emit(node.asteriskToken);
}
emitExpressionWithPrefix(" ", node.expression);
}

Expand Down Expand Up @@ -1662,7 +1664,11 @@ namespace ts {
function emitFunctionDeclarationOrExpression(node: FunctionDeclaration | FunctionExpression) {
emitDecorators(node, node.decorators);
emitModifiers(node, node.modifiers);
write(node.asteriskToken ? "function* " : "function ");
write("function");
Copy link
Contributor

@rbuckton rbuckton Sep 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No need to guard against undefined as that is already handled in emit

Edit: I was incorrect. Apparently emit does not have this guard.

Copy link
Author

@ghost ghost Sep 7, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

I get a test failure in the test PrinterAPI printFile removeComments if I try to emit the asterisk unconditionally. emit doesn't seem to check for an undefined node.

if (node.asteriskToken) {
emit(node.asteriskToken);
}
write(" ");
emitIdentifierName(node.name);
emitSignatureAndBody(node, emitSignatureHead);
}
Expand Down Expand Up @@ -2068,9 +2074,7 @@ namespace ts {
function emitJsxExpression(node: JsxExpression) {
if (node.expression) {
write("{");
if (node.dotDotDotToken) {
write("...");
}
writeIfPresent(node.dotDotDotToken, "...");
emitExpression(node.expression);
write("}");
}
Expand Down Expand Up @@ -2130,7 +2134,7 @@ namespace ts {

if (emitAsSingleStatement) {
write(" ");
emit(statements[0]);
emitSingleElementList(statements);
}
else {
emitList(parentNode, statements, ListFormat.CaseOrDefaultClauseStatements);
Expand Down Expand Up @@ -2384,7 +2388,7 @@ namespace ts {

function emitParametersForArrow(parentNode: FunctionTypeNode | ArrowFunction, parameters: NodeArray<ParameterDeclaration>) {
if (canEmitSimpleArrowHead(parentNode, parameters)) {
emit(parameters[0]);
emitSingleElementList(parameters);
}
else {
emitParameters(parentNode, parameters);
Expand All @@ -2395,6 +2399,17 @@ namespace ts {
emitList(parentNode, parameters, ListFormat.IndexSignatureParameters);
}

function emitSingleElementList(list: NodeArray<Node>) {
Copy link
Contributor

@rbuckton rbuckton Sep 6, 2017

Choose a reason for hiding this comment

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

Can you craft suitable ListFormat bitmasks to use instead and just call emitList where applicable instead of creating a special cased list emit function?

Debug.assert(list.length === 1);
if (onBeforeEmitNodeArray) {
onBeforeEmitNodeArray(list);
}
emit(list[0]);
if (onAfterEmitNodeArray) {
onAfterEmitNodeArray(list);
}
}

function emitList(parentNode: Node, children: NodeArray<Node>, format: ListFormat, start?: number, count?: number) {
emitNodeList(emit, parentNode, children, format, start, count);
}
Expand Down Expand Up @@ -2427,7 +2442,7 @@ namespace ts {
if (format & ListFormat.MultiLine) {
writeLine();
}
else if (format & ListFormat.SpaceBetweenBraces) {
else if (format & ListFormat.SpaceBetweenBraces && !(format & ListFormat.NoSpaceIfEmpty)) {
write(" ");
}
}
Expand Down Expand Up @@ -2570,7 +2585,7 @@ namespace ts {

function writeIfPresent(node: Node, text: string) {
if (node) {
write(text);
writeTokenAndCallCallbacks(node, text);
}
}

Expand All @@ -2580,16 +2595,20 @@ namespace ts {
: writeTokenText(token, pos);
}

function writeTokenNode(node: Node) {
function writeTokenAndCallCallbacks(node: Node, text: string) {
Copy link
Contributor

@rbuckton rbuckton Sep 6, 2017

Choose a reason for hiding this comment

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

Maybe we can simplify this by replacing all calls to writeIfPresent with emit, as emit already guards against undefined, will in turn eventually call writeTokenNode, and may give us better comment preservation and source-maps.

if (onBeforeEmitToken) {
onBeforeEmitToken(node);
}
writeTokenText(node.kind);
write(text);
if (onAfterEmitToken) {
onAfterEmitToken(node);
}
}

function writeTokenNode(node: Node) {
writeTokenAndCallCallbacks(node, tokenToString(node.kind));
}

function writeTokenText(token: SyntaxKind, pos?: number) {
const tokenString = tokenToString(token);
write(tokenString);
Expand Down Expand Up @@ -3107,6 +3126,8 @@ namespace ts {
NoTrailingNewLine = 1 << 16, // Do not emit a trailing NewLine for a MultiLine list.
NoInterveningComments = 1 << 17, // Do not emit comments between each node

NoSpaceIfEmpty = 1 << 18, // If the literal is empty, do not add spaces between braces.

// Precomputed Formats
Modifiers = SingleLine | SpaceBetweenSiblings | NoInterveningComments,
HeritageClauses = SingleLine | SpaceBetweenSiblings,
Expand All @@ -3118,7 +3139,7 @@ namespace ts {
IntersectionTypeConstituents = AmpersandDelimited | SpaceBetweenSiblings | SingleLine,
ObjectBindingPatternElements = SingleLine | AllowTrailingComma | SpaceBetweenBraces | CommaDelimited | SpaceBetweenSiblings,
ArrayBindingPatternElements = SingleLine | AllowTrailingComma | CommaDelimited | SpaceBetweenSiblings,
ObjectLiteralExpressionProperties = PreserveLines | CommaDelimited | SpaceBetweenSiblings | SpaceBetweenBraces | Indented | Braces,
ObjectLiteralExpressionProperties = PreserveLines | CommaDelimited | SpaceBetweenSiblings | SpaceBetweenBraces | Indented | Braces | NoSpaceIfEmpty,
ArrayLiteralExpressionElements = PreserveLines | CommaDelimited | SpaceBetweenSiblings | AllowTrailingComma | Indented | SquareBrackets,
CommaListElements = CommaDelimited | SpaceBetweenSiblings | SingleLine,
CallExpressionArguments = CommaDelimited | SpaceBetweenSiblings | SingleLine | Parenthesis,
Expand Down
35 changes: 32 additions & 3 deletions src/compiler/factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ namespace ts {
|| node.questionToken !== questionToken
|| node.type !== type
|| node.initializer !== initializer
? updateNode(createParameter(decorators, modifiers, dotDotDotToken, name, node.questionToken, type, initializer), node)
? updateNode(createParameter(decorators, modifiers, dotDotDotToken, name, questionToken, type, initializer), node)
: node;
}

Expand Down Expand Up @@ -1016,19 +1016,48 @@ namespace ts {
return node;
}

/* @deprecated */ export function updateArrowFunction(
node: ArrowFunction,
modifiers: ReadonlyArray<Modifier> | undefined,
typeParameters: ReadonlyArray<TypeParameterDeclaration> | undefined,
parameters: ReadonlyArray<ParameterDeclaration>,
type: TypeNode | undefined,
body: ConciseBody): ArrowFunction;
export function updateArrowFunction(
node: ArrowFunction,
modifiers: ReadonlyArray<Modifier> | undefined,
typeParameters: ReadonlyArray<TypeParameterDeclaration> | undefined,
parameters: ReadonlyArray<ParameterDeclaration>,
type: TypeNode | undefined,
body: ConciseBody) {
equalsGreaterThanToken: Token<SyntaxKind.EqualsGreaterThanToken>,
body: ConciseBody): ArrowFunction;
export function updateArrowFunction(
node: ArrowFunction,
modifiers: ReadonlyArray<Modifier> | undefined,
typeParameters: ReadonlyArray<TypeParameterDeclaration> | undefined,
parameters: ReadonlyArray<ParameterDeclaration>,
type: TypeNode | undefined,
equalsGreaterThanTokenOrBody: Token<SyntaxKind.EqualsGreaterThanToken> | ConciseBody,
Copy link
Author

@ghost ghost Sep 6, 2017

Choose a reason for hiding this comment

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

Need to ensure that we use the => from the new tree, which has a correct position. The => from the old tree has a different position that causes assertion errors in the formatter.

Copy link
Contributor

@mhegazy mhegazy Sep 6, 2017

Choose a reason for hiding this comment

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

Can we add it at the end instead?

Copy link
Contributor

@rbuckton rbuckton Sep 7, 2017

Choose a reason for hiding this comment

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

We should always keep elements in the create/update methods in the order they are parsed.

bodyOrUndefined?: ConciseBody): ArrowFunction {
let equalsGreaterThanToken: Token<SyntaxKind.EqualsGreaterThanToken>;
let body: ConciseBody;
if (bodyOrUndefined === undefined) {
equalsGreaterThanToken = node.equalsGreaterThanToken;
body = cast(equalsGreaterThanTokenOrBody, isConciseBody);
}
else {
equalsGreaterThanToken = cast(equalsGreaterThanTokenOrBody, (n): n is Token<SyntaxKind.EqualsGreaterThanToken> =>
n.kind === SyntaxKind.EqualsGreaterThanToken);
body = bodyOrUndefined;
}

return node.modifiers !== modifiers
|| node.typeParameters !== typeParameters
|| node.parameters !== parameters
|| node.type !== type
|| node.equalsGreaterThanToken !== equalsGreaterThanToken
|| node.body !== body
? updateNode(createArrowFunction(modifiers, typeParameters, parameters, type, node.equalsGreaterThanToken, body), node)
? updateNode(createArrowFunction(modifiers, typeParameters, parameters, type, equalsGreaterThanToken, body), node)
: node;
}

Expand Down
1 change: 1 addition & 0 deletions src/compiler/transformers/es2017.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ namespace ts {
/*typeParameters*/ undefined,
visitParameterList(node.parameters, visitor, context),
/*type*/ undefined,
node.equalsGreaterThanToken,
getFunctionFlags(node) & FunctionFlags.Async
? transformAsyncFunctionBody(node)
: visitFunctionBody(node.body, visitor, context)
Expand Down
1 change: 1 addition & 0 deletions src/compiler/transformers/esnext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,7 @@ namespace ts {
/*typeParameters*/ undefined,
visitParameterList(node.parameters, visitor, context),
/*type*/ undefined,
node.equalsGreaterThanToken,
transformFunctionBody(node)
);
enclosingFunctionFlags = savedEnclosingFunctionFlags;
Expand Down
1 change: 1 addition & 0 deletions src/compiler/transformers/ts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2291,6 +2291,7 @@ namespace ts {
/*typeParameters*/ undefined,
visitParameterList(node.parameters, visitor, context),
/*type*/ undefined,
node.equalsGreaterThanToken,
visitFunctionBody(node.body, visitor, context)
);
return updated;
Expand Down
8 changes: 6 additions & 2 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4666,8 +4666,12 @@ namespace ts {

/* @internal */
export function isNodeArray<T extends Node>(array: ReadonlyArray<T>): array is NodeArray<T> {
return array.hasOwnProperty("pos")
&& array.hasOwnProperty("end");
const res = array.hasOwnProperty("pos") && array.hasOwnProperty("end");
if (res) {
Copy link
Contributor

@rbuckton rbuckton Sep 6, 2017

Choose a reason for hiding this comment

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

We possibly call this a lot between visitor and factory. Do the added checks have any perf impact?

const { pos, end } = array as NodeArray<T>;
Debug.assert(typeof pos === "number" && typeof end === "number");
}
return res;
}

// Literals
Expand Down
1 change: 1 addition & 0 deletions src/compiler/visitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ namespace ts {
nodesVisitor((<ArrowFunction>node).typeParameters, visitor, isTypeParameterDeclaration),
visitParameterList((<ArrowFunction>node).parameters, visitor, context, nodesVisitor),
visitNode((<ArrowFunction>node).type, visitor, isTypeNode),
visitNode((<ArrowFunction>node).equalsGreaterThanToken, visitor, n => n.kind === SyntaxKind.EqualsGreaterThanToken),
visitFunctionBody((<ArrowFunction>node).body, visitor, context));

case SyntaxKind.DeleteExpression:
Expand Down
1 change: 1 addition & 0 deletions src/services/formatting/formatting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,7 @@ namespace ts.formatting {
parent: Node,
parentStartLine: number,
parentDynamicIndentation: DynamicIndentation): void {
Debug.assert(isNodeArray(nodes));

const listStartToken = getOpenTokenForList(parent, nodes);
const listEndToken = getCloseTokenForOpenToken(listStartToken);
Expand Down
6 changes: 4 additions & 2 deletions src/services/refactors/extractMethod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,11 +656,13 @@ namespace ts.refactor.extractMethod {
const typeParametersAndDeclarations = arrayFrom(typeParameterUsages.values()).map(type => ({ type, declaration: getFirstDeclaration(type) }));
const sortedTypeParametersAndDeclarations = typeParametersAndDeclarations.sort(compareTypesByDeclarationOrder);

const typeParameters: ReadonlyArray<TypeParameterDeclaration> = sortedTypeParametersAndDeclarations.map(t => t.declaration as TypeParameterDeclaration);
const typeParameters: ReadonlyArray<TypeParameterDeclaration> | undefined = sortedTypeParametersAndDeclarations.length === 0
? undefined
: sortedTypeParametersAndDeclarations.map(t => t.declaration as TypeParameterDeclaration);

// Strictly speaking, we should check whether each name actually binds to the appropriate type
// parameter. In cases of shadowing, they may not.
const callTypeArguments: ReadonlyArray<TypeNode> | undefined = typeParameters.length > 0
const callTypeArguments: ReadonlyArray<TypeNode> | undefined = typeParameters !== undefined
? typeParameters.map(decl => createTypeReferenceNode(decl.name, /*typeArguments*/ undefined))
: undefined;

Expand Down
Loading