Skip to content
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.

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

Correctly cache tagged template objects in modules #18300

Merged
merged 29 commits into from
Oct 3, 2017
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c4b1a9d
Update emit for tagged templates to use a per-site cached template ob…
DanielRosenwasser Sep 7, 2017
166af8c
Accepted baselines.
DanielRosenwasser Sep 7, 2017
aa634ba
Added printer test for 'new (f().x)'.
DanielRosenwasser Sep 14, 2017
4beb9b0
Accepted (incorrect) baselines.
DanielRosenwasser Sep 14, 2017
6a9fa83
Parenthesize new'd expressions based on the leftmost node (or the fir…
DanielRosenwasser Sep 14, 2017
c966059
Accepted baselines.
DanielRosenwasser Sep 14, 2017
8fbb304
Add a test case for conditional expressions just in case.
DanielRosenwasser Sep 14, 2017
7871e08
Accepted baselines.
DanielRosenwasser Sep 14, 2017
9f669d0
Explicit fall-through.
DanielRosenwasser Sep 14, 2017
e9c6dfe
Remove freezing behavior from tagged template helper.
DanielRosenwasser Sep 15, 2017
1656790
Accepted baselines.
DanielRosenwasser Sep 15, 2017
b137f24
%s/getTemplateObject/makeTemplateObject
DanielRosenwasser Sep 18, 2017
5565709
Accepted baselines.
DanielRosenwasser Sep 18, 2017
9907453
Merge branch 'master' into correctlyCacheTaggedTemplates
DanielRosenwasser Sep 21, 2017
1cb5eb9
Merge branch 'master' into correctlyCacheTaggedTemplates
DanielRosenwasser Sep 25, 2017
886a29b
Added tests for import helpers with & without a declared template obj…
DanielRosenwasser Sep 26, 2017
1841afe
Ensure that the import helper is checked for tagged templates, and up…
DanielRosenwasser Sep 28, 2017
0b7538d
Accepted baselines.
DanielRosenwasser Sep 28, 2017
b406d54
git Merge branch 'master' into correctlyCacheTaggedTemplates
DanielRosenwasser Sep 28, 2017
4ec1643
Fall back to old behavior for tagged template emit in global files.
DanielRosenwasser Sep 28, 2017
d039942
Accepted baselines.
DanielRosenwasser Sep 28, 2017
5da45fb
Addressed code review feedback.
DanielRosenwasser Sep 30, 2017
f94bded
Added test for module & global examples.
DanielRosenwasser Sep 30, 2017
81b3e85
Accepted baselines.
DanielRosenwasser Sep 30, 2017
a23d1bf
Updated helper for marginally better minification.
DanielRosenwasser Sep 30, 2017
e2c6aac
Accepted baselines.
DanielRosenwasser Sep 30, 2017
b80b2ee
Move first/last shortcuts closer to the end of the helper flags.
DanielRosenwasser Oct 2, 2017
babe3cb
Flatten the '__makeTemplateObject' helper to use less vertical screen…
DanielRosenwasser Oct 3, 2017
8fd638c
Accepted baselines.
DanielRosenwasser Oct 3, 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
4 changes: 4 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16664,6 +16664,9 @@ namespace ts {
}

function checkTaggedTemplateExpression(node: TaggedTemplateExpression): Type {
if (languageVersion < ScriptTarget.ES2015) {
Copy link

Choose a reason for hiding this comment

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

Apparent change in file permissions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed.

checkExternalEmitHelpers(node, ExternalEmitHelpers.MakeTemplateObject);
}
return getReturnTypeOfSignature(getResolvedSignature(node));
}

Expand Down Expand Up @@ -24019,6 +24022,7 @@ namespace ts {
case ExternalEmitHelpers.AsyncDelegator: return "__asyncDelegator";
case ExternalEmitHelpers.AsyncValues: return "__asyncValues";
case ExternalEmitHelpers.ExportStar: return "__exportStar";
case ExternalEmitHelpers.MakeTemplateObject: return "__makeTemplateObject";
Copy link

Choose a reason for hiding this comment

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

Not added in this PR, but all of the strings here are duplicated elsewhere in the emitter -- could we move this function to there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth considering a shorter name than __makeTemplateObject?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rbuckton Babel uses _taggedTemplateLiteral which is two characters longer. If you can think of something by Monday afternoon before I merge this in, I will consider it 😄

Copy link
Member

Choose a reason for hiding this comment

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

__template? (I like this since it doesn't have any camel-casing)
__templatize?
__intoTemplate?
__addRaw?
__createTemplateArg? (not really shorter, I know)

default: Debug.fail("Unrecognized helper");
}
}
Expand Down
21 changes: 13 additions & 8 deletions src/compiler/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3877,15 +3877,15 @@ namespace ts {
* @param expression The Expression node.
*/
export function parenthesizeForNew(expression: Expression): LeftHandSideExpression {
const emittedExpression = skipPartiallyEmittedExpressions(expression);
switch (emittedExpression.kind) {
const leftmostExpr = getLeftmostExpression(expression, /*stopAtCallExpressions*/ true);
switch (leftmostExpr.kind) {
case SyntaxKind.CallExpression:
return createParen(expression);

case SyntaxKind.NewExpression:
return (<NewExpression>emittedExpression).arguments
Copy link
Member

Choose a reason for hiding this comment

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

I would swap this back to the original order.

? <LeftHandSideExpression>expression
: createParen(expression);
return !(leftmostExpr as NewExpression).arguments
? createParen(expression)
: <LeftHandSideExpression>expression;
}

return parenthesizeForAccess(expression);
Expand Down Expand Up @@ -3966,7 +3966,7 @@ namespace ts {
}
}

const leftmostExpressionKind = getLeftmostExpression(emittedExpression).kind;
const leftmostExpressionKind = getLeftmostExpression(emittedExpression, /*stopAtCallExpressions*/ false).kind;
if (leftmostExpressionKind === SyntaxKind.ObjectLiteralExpression || leftmostExpressionKind === SyntaxKind.FunctionExpression) {
return setTextRange(createParen(expression), expression);
}
Expand Down Expand Up @@ -4012,7 +4012,7 @@ namespace ts {
}
}

function getLeftmostExpression(node: Expression): Expression {
function getLeftmostExpression(node: Expression, stopAtCallExpressions: boolean) {
while (true) {
switch (node.kind) {
case SyntaxKind.PostfixUnaryExpression:
Expand All @@ -4028,6 +4028,10 @@ namespace ts {
continue;

case SyntaxKind.CallExpression:
if (stopAtCallExpressions) {
return node;
}
// falls through
case SyntaxKind.ElementAccessExpression:
case SyntaxKind.PropertyAccessExpression:
node = (<CallExpression | PropertyAccessExpression | ElementAccessExpression>node).expression;
Expand All @@ -4040,10 +4044,11 @@ namespace ts {

return node;
}

}

export function parenthesizeConciseBody(body: ConciseBody): ConciseBody {
if (!isBlock(body) && getLeftmostExpression(body).kind === SyntaxKind.ObjectLiteralExpression) {
if (!isBlock(body) && getLeftmostExpression(body, /*stopAtCallExpressions*/ false).kind === SyntaxKind.ObjectLiteralExpression) {
return setTextRange(createParen(<Expression>body), body);
}

Expand Down
78 changes: 64 additions & 14 deletions src/compiler/transformers/es2015.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,13 @@ namespace ts {
let currentSourceFile: SourceFile;
let currentText: string;
let hierarchyFacts: HierarchyFacts;
let taggedTemplateStringDeclarations: VariableDeclaration[];

function recordTaggedTemplateString(temp: Identifier) {
taggedTemplateStringDeclarations = append(
taggedTemplateStringDeclarations,
createVariableDeclaration(temp));
}

/**
* Used to track if we are emitting body of the converted loop
Expand Down Expand Up @@ -307,6 +314,7 @@ namespace ts {

currentSourceFile = undefined;
currentText = undefined;
taggedTemplateStringDeclarations = undefined;
hierarchyFacts = HierarchyFacts.None;
return visited;
}
Expand Down Expand Up @@ -520,6 +528,11 @@ namespace ts {
addCaptureThisForNodeIfNeeded(statements, node);
statementOffset = addCustomPrologue(statements, node.statements, statementOffset, visitor);
addRange(statements, visitNodes(node.statements, visitor, isStatement, statementOffset));
if (taggedTemplateStringDeclarations) {
statements.push(
createVariableStatement(/*modifiers*/ undefined,
createVariableDeclarationList(taggedTemplateStringDeclarations)));
}
addRange(statements, endLexicalEnvironment());
exitSubtree(ancestorFacts, HierarchyFacts.None, HierarchyFacts.None);
return updateSourceFileNode(
Expand Down Expand Up @@ -3636,11 +3649,10 @@ namespace ts {
// Visit the tag expression
const tag = visitNode(node.tag, visitor, isExpression);

// Allocate storage for the template site object
const temp = createTempVariable(hoistVariableDeclaration);

// Build up the template arguments and the raw and cooked strings for the template.
const templateArguments: Expression[] = [temp];
// We start out with 'undefined' for the first argument and revisit later
// to avoid walking over the template string twice and shifting all our arguments over after the fact.
const templateArguments: Expression[] = [undefined];
const cookedStrings: Expression[] = [];
const rawStrings: Expression[] = [];
const template = node.template;
Expand All @@ -3658,16 +3670,25 @@ namespace ts {
}
}

// NOTE: The parentheses here is entirely optional as we are now able to auto-
// parenthesize when rebuilding the tree. This should be removed in a
// future version. It is here for now to match our existing emit.
return createParen(
inlineExpressions([
createAssignment(temp, createArrayLiteral(cookedStrings)),
createAssignment(createPropertyAccess(temp, "raw"), createArrayLiteral(rawStrings)),
createCall(tag, /*typeArguments*/ undefined, templateArguments)
])
);
const helperCall = createTemplateObjectHelper(context, createArrayLiteral(cookedStrings), createArrayLiteral(rawStrings));

// Create a variable to cache the template object if we're in a module.
// Do not do this in the global scope, as any variable we currently generate could conflict with
// variables from outside of the current compilation. In the future, we can revisit this behavior.
if (isExternalModule(currentSourceFile)) {
const tempVar = createTempVariable(recordTaggedTemplateString);
templateArguments[0] = createLogicalOr(
tempVar,
createAssignment(
tempVar,
helperCall)
);
}
else {
templateArguments[0] = helperCall;
}

return createCall(tag, /*typeArguments*/ undefined, templateArguments);
}

/**
Expand Down Expand Up @@ -4036,6 +4057,18 @@ namespace ts {
);
}

function createTemplateObjectHelper(context: TransformationContext, cooked: ArrayLiteralExpression, raw: ArrayLiteralExpression) {
context.requestEmitHelper(templateObjectHelper);
return createCall(
getHelperName("__makeTemplateObject"),
/*typeArguments*/ undefined,
[
cooked,
raw
]
);
}

const extendsHelper: EmitHelper = {
name: "typescript:extends",
scoped: false,
Expand All @@ -4052,4 +4085,21 @@ namespace ts {
};
})();`
};

const templateObjectHelper: EmitHelper = {
name: "typescript:makeTemplateObject",
scoped: false,
priority: 0,
text: `
var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {
Copy link
Member

Choose a reason for hiding this comment

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

What about:

cooked.raw = raw;
if (Object.freeze) Object.freeze(cooked);
return cooked;

Or, the one-line alternative:

return Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : (cooked.raw = raw, cooked);

Copy link
Member Author

Choose a reason for hiding this comment

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

Uglify already produces a shorter output given this current implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Slightly better one-liner (two less characters, just as readable and correct):

return Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : cooked.raw = raw, cooked;

Which is, coincidentally, how uglify restructures what daniel has right now when it minifies it.

if (Object.defineProperty) {
Object.defineProperty(cooked, "raw", { value: raw });
}
else {
cooked.raw = raw;
}
return cooked;
};`
};

}
36 changes: 19 additions & 17 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4271,22 +4271,24 @@ namespace ts {
*/
/* @internal */
export const enum ExternalEmitHelpers {
Extends = 1 << 0, // __extends (used by the ES2015 class transformation)
Assign = 1 << 1, // __assign (used by Jsx and ESNext object spread transformations)
Rest = 1 << 2, // __rest (used by ESNext object rest transformation)
Decorate = 1 << 3, // __decorate (used by TypeScript decorators transformation)
Metadata = 1 << 4, // __metadata (used by TypeScript decorators transformation)
Param = 1 << 5, // __param (used by TypeScript decorators transformation)
Awaiter = 1 << 6, // __awaiter (used by ES2017 async functions transformation)
Generator = 1 << 7, // __generator (used by ES2015 generator transformation)
Values = 1 << 8, // __values (used by ES2015 for..of and yield* transformations)
Read = 1 << 9, // __read (used by ES2015 iterator destructuring transformation)
Spread = 1 << 10, // __spread (used by ES2015 array spread and argument list spread transformations)
Await = 1 << 11, // __await (used by ES2017 async generator transformation)
AsyncGenerator = 1 << 12, // __asyncGenerator (used by ES2017 async generator transformation)
AsyncDelegator = 1 << 13, // __asyncDelegator (used by ES2017 async generator yield* transformation)
AsyncValues = 1 << 14, // __asyncValues (used by ES2017 for..await..of transformation)
ExportStar = 1 << 15, // __exportStar (used by CommonJS/AMD/UMD module transformation)
Extends = 1 << 0, // __extends (used by the ES2015 class transformation)
Copy link
Member

Choose a reason for hiding this comment

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

this change is an example of why aligned comments make me sad :(

Copy link
Member Author

Choose a reason for hiding this comment

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

:(

Assign = 1 << 1, // __assign (used by Jsx and ESNext object spread transformations)
Rest = 1 << 2, // __rest (used by ESNext object rest transformation)
Decorate = 1 << 3, // __decorate (used by TypeScript decorators transformation)
Metadata = 1 << 4, // __metadata (used by TypeScript decorators transformation)
Param = 1 << 5, // __param (used by TypeScript decorators transformation)
Awaiter = 1 << 6, // __awaiter (used by ES2017 async functions transformation)
Generator = 1 << 7, // __generator (used by ES2015 generator transformation)
Values = 1 << 8, // __values (used by ES2015 for..of and yield* transformations)
Read = 1 << 9, // __read (used by ES2015 iterator destructuring transformation)
Spread = 1 << 10, // __spread (used by ES2015 array spread and argument list spread transformations)
Await = 1 << 11, // __await (used by ES2017 async generator transformation)
AsyncGenerator = 1 << 12, // __asyncGenerator (used by ES2017 async generator transformation)
AsyncDelegator = 1 << 13, // __asyncDelegator (used by ES2017 async generator yield* transformation)
AsyncValues = 1 << 14, // __asyncValues (used by ES2017 for..await..of transformation)
ExportStar = 1 << 15, // __exportStar (used by CommonJS/AMD/UMD module transformation)
MakeTemplateObject = 1 << 16, // __makeTemplateObject (used for constructing template string array objects)
" LastPlusOne",

// Helpers included by ES2015 for..of
ForOfIncludes = Values,
Expand All @@ -4304,7 +4306,7 @@ namespace ts {
SpreadIncludes = Read | Spread,

FirstEmitHelper = Extends,
LastEmitHelper = ExportStar
LastEmitHelper = ExternalEmitHelpers[" LastPlusOne"] - 1
Copy link

@ghost ghost Sep 29, 2017

Choose a reason for hiding this comment

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

You could also just move this next to the last one to make it obvious that these should be updated together. Same for FirstEmitHelper.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. 👎 for the magic string in the enum.

}

export const enum EmitHint {
Expand Down
25 changes: 25 additions & 0 deletions src/harness/unittests/printer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,31 @@ namespace ts {
createSourceFile("source.ts", "", ScriptTarget.ES2015)
));

printsCorrectly("newExpressionWithPropertyAccessOnCallExpression", {}, printer => printer.printNode(
EmitHint.Unspecified,
createNew(
createPropertyAccess(
createCall(
createIdentifier("f"), /*typeArguments*/ undefined, /*argumentsArray*/ undefined),
"x"),
/*typeArguments*/ undefined,
/*argumentsArray*/ undefined
),
createSourceFile("source.ts", "", ScriptTarget.ESNext))
);

printsCorrectly("newExpressionOnConditionalExpression", {}, printer => printer.printNode(
EmitHint.Unspecified,
createNew(
createConditional(
createIdentifier("x"), createToken(SyntaxKind.QuestionToken),
createIdentifier("y"), createToken(SyntaxKind.ColonToken),
createIdentifier("z")),
/*typeArguments*/ undefined,
/*argumentsArray*/ undefined
),
createSourceFile("source.ts", "", ScriptTarget.ESNext))
);

printsCorrectly("emptyGlobalAugmentation", {}, printer => printer.printNode(
EmitHint.Unspecified,
Expand Down
14 changes: 11 additions & 3 deletions tests/baselines/reference/asOperator3.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,20 @@ var g = tag `Hello ${123} World` as string;
var h = tag `Hello` as string;

//// [asOperator3.js]
var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {
if (Object.defineProperty) {
Object.defineProperty(cooked, "raw", { value: raw });
}
else {
cooked.raw = raw;
}
return cooked;
};
var a = "" + (123 + 456);
var b = "leading " + (123 + 456);
var c = 123 + 456 + " trailing";
var d = "Hello " + 123 + " World";
var e = "Hello";
var f = 1 + (1 + " end of string");
var g = (_a = ["Hello ", " World"], _a.raw = ["Hello ", " World"], tag(_a, 123));
var h = (_b = ["Hello"], _b.raw = ["Hello"], tag(_b));
var _a, _b;
var g = tag(__makeTemplateObject(["Hello ", " World"], ["Hello ", " World"]), 123);
var h = tag(__makeTemplateObject(["Hello"], ["Hello"]));
12 changes: 10 additions & 2 deletions tests/baselines/reference/asOperatorASI.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,23 @@ as(Foo); // should emit


//// [asOperatorASI.js]
var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {
if (Object.defineProperty) {
Object.defineProperty(cooked, "raw", { value: raw });
}
else {
cooked.raw = raw;
}
return cooked;
};
var Foo = /** @class */ (function () {
function Foo() {
}
return Foo;
}());
// Example 1
var x = 10;
(_a = ["Hello world"], _a.raw = ["Hello world"], as(_a)); // should not error
as(__makeTemplateObject(["Hello world"], ["Hello world"])); // should not error
// Example 2
var y = 20;
as(Foo); // should emit
var _a;
Loading