Correctly cache tagged template objects in modules #18300

DanielRosenwasser · 2017-09-07T03:26:23Z

This PR is a work in progress at the moment

Remaining things to work on:

Bikeshed on helper name.
Bikeshed on helper semantics
Bikeshed on template object initialization location
Add helper for tslib
Add test for tslib
Investigate adding behavioral tests
Fix weird case with un-parenthesized new expressions

DanielRosenwasser · 2017-09-07T03:26:50Z

tests/baselines/reference/taggedTemplateStringsWithManyCallAndMemberExpressions.js

 var f;
-var x = new new new (_a = ["abc", "def"], _a.raw = ["abc", "def"], f(_a, 0)).member("hello")(42) === true;
+var x = new new new f(_a || (_a = __getTemplateObject(["abc", "def"], ["abc", "def"])), 0).member("hello")(42) === true;


This is a regression. @rbuckton, any idea why we don't auto-parenthesize here?

…jects.

…st node that is a call expression).

…ect creator.

…date its name.

ghost · 2017-09-29T20:17:24Z

src/compiler/checker.ts

@@ -16664,6 +16664,9 @@ namespace ts {
        }

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


Apparent change in file permissions?

Should be fixed.

ghost · 2017-09-29T20:19:51Z

src/compiler/checker.ts

@@ -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";


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

ghost · 2017-09-29T20:22:00Z

src/compiler/transformers/es2015.ts

@@ -279,6 +279,16 @@ namespace ts {
        let currentSourceFile: SourceFile;
        let currentText: string;
        let hierarchyFacts: HierarchyFacts;
+        let taggedTemplateStringDeclarations: VariableDeclaration[];
+        function recordTaggedTemplateString(temp: Identifier) {
+            const decl = createVariableDeclaration(temp);


taggedTemplateStringDeclarations = append(taggedTemplateStringDeclarations, createVariableDeclaration(temp));

ghost · 2017-09-29T20:24:50Z

src/compiler/transformers/es2015.ts

+            // Allocate storage for the template site object if we're in a module.
+            // In the global scope, any variable we currently generate could conflict with
+            // variables from outside of the current compilation.
+            const temp = isExternalModule(currentSourceFile) ? createTempVariable(recordTaggedTemplateString) : undefined;


Could this variable be created closer to its use, or is there some side effect I'm missing?
If you do move it next to its use, you could just replace !temp ? ... with !isExternalModule(currentSourceFile) ? ...

ghost · 2017-09-29T20:29:58Z

src/compiler/transformers/es2015.ts

+        text: `
+            var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {
+                if (Object.defineProperty) {
+                    return Object.defineProperty(cooked, "raw", { value: raw });


Would be more readable if both branches shared return cooked;:

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

Otherwise it looks like they return different things...

Alternatively, you can go for even less readable, and do return Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : (cooked.raw = raw, cooked);. Can cram the entire helper into one line and it's not bad, actually:

var __makeTemplateObject = (this && this.__makeTemplateObject) || (cooked, raw) => Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : (cooked.raw = raw, cooked);

Too bad we don't transpile helpers 👅

Spoke with Wesley; apparently Uglify can do a better job on the following thanks to ternary/comma precedence rules:

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

ghost · 2017-09-29T20:31:51Z

src/compiler/types.ts

@@ -4304,7 +4306,7 @@ namespace ts {
        SpreadIncludes = Read | Spread,

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


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

Agreed. 👎 for the magic string in the enum.

sandersn

Would be nice to add a test that shows off the caching in action.

sandersn · 2017-09-07T16:13:49Z

src/compiler/types.ts

-        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)


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

sandersn · 2017-09-28T21:17:43Z

src/compiler/factory.ts

            case SyntaxKind.CallExpression:
                return createParen(expression);

            case SyntaxKind.NewExpression:
-                return (<NewExpression>emittedExpression).arguments


I would swap this back to the original order.

sandersn · 2017-09-29T22:15:51Z

src/compiler/transformers/es2015.ts

+            // If we're in the global scope, we risk having conflicting variables.
+            // Since we currently lack the infrastructure to create sufficiently unique names,
+            // we'll fall back to creating the template object on every invocation.
+            templateArguments[0] = !temp ?


if isExternalModule is cheap to call, it would be nice to do so here; much easier to read.

Also consider flipping the conditional.

weswigham

I'd like to see a test that is downlevel, with modules, without importHelpers - just because I don't think I see any covering that configuration. Other than that, most everything else seems fine.

weswigham · 2017-09-29T23:02:58Z

...s/baselines/reference/printerApi/printsNodeCorrectly.newExpressionOnConditionalExpression.js

@@ -0,0 +1 @@
+new (x ? y : z)


Was this a drive-by new test and fix?

Ah, yes:

Fix weird case with un-parenthesized new expressions

rbuckton · 2017-10-01T21:13:57Z

I won't have time to look at this until Monday, but be aware that there was a Needs Consensus PR regarding template literal cache lifetime last week that reached consensus. I don't know yet whether this affects this change.

DanielRosenwasser · 2017-10-02T03:39:36Z

@rbuckton within a module, this PR reflects those semantics. At the global scope, we continue to do the same thing as we've always done until we have a better mechanism.

@sanders @weswigham should be there now in tests/cases/compiler/taggedTemplatesInModuleAndGlobal.ts.

rbuckton · 2017-09-07T03:50:45Z

src/compiler/transformers/es2015.ts

+        priority: 0,
+        text: `
+            var __getTemplateObject = (this && this.__getTemplateObject) || function (cooked, raw) {
+                if (Object.freeze && Object.defineProperty) {


Do you need the defineProperty call? I suppose it makes the property non-enumerable...

rbuckton · 2017-10-02T03:51:53Z

src/compiler/types.ts

@@ -4304,7 +4306,7 @@ namespace ts {
        SpreadIncludes = Read | Spread,

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


Agreed. 👎 for the magic string in the enum.

rbuckton · 2017-10-02T03:57:07Z

src/compiler/transformers/es2015.ts

+        scoped: false,
+        priority: 0,
+        text: `
+            var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {


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);

Uglify already produces a shorter output given this current implementation.

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.

rbuckton · 2017-10-02T04:00:34Z

src/compiler/checker.ts

@@ -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";


Would it be worth considering a shorter name than __makeTemplateObject?

@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 😄

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

… real estate.

msftclas added the cla-not-required label Sep 7, 2017

DanielRosenwasser commented Sep 7, 2017

View reviewed changes

DanielRosenwasser requested review from rbuckton, sandersn and weswigham September 7, 2017 03:28

DanielRosenwasser mentioned this pull request Sep 7, 2017

Tagged template literals compiled incorrectly, TemplateStringsArray not cached #17956

Closed

DanielRosenwasser added 2 commits September 6, 2017 20:39

Update emit for tagged templates to use a per-site cached template ob…

c4b1a9d

…jects.

Accepted baselines.

166af8c

DanielRosenwasser force-pushed the correctlyCacheTaggedTemplates branch from 587c8a8 to 166af8c Compare September 7, 2017 03:40

DanielRosenwasser mentioned this pull request Sep 8, 2017

Design Meeting Notes, 9/8/2017 #18339

Closed

DanielRosenwasser added 11 commits September 14, 2017 14:00

Added printer test for 'new (f().x)'.

aa634ba

Accepted (incorrect) baselines.

4beb9b0

Parenthesize new'd expressions based on the leftmost node (or the fir…

6a9fa83

…st node that is a call expression).

Accepted baselines.

c966059

Add a test case for conditional expressions just in case.

8fbb304

Accepted baselines.

7871e08

Explicit fall-through.

9f669d0

Remove freezing behavior from tagged template helper.

e9c6dfe

Accepted baselines.

1656790

%s/getTemplateObject/makeTemplateObject

b137f24

Accepted baselines.

5565709

DanielRosenwasser mentioned this pull request Sep 18, 2017

Add the '__makeTemplateObject' helper. microsoft/tslib#39

Merged

Merge branch 'master' into correctlyCacheTaggedTemplates

9907453

DanielRosenwasser mentioned this pull request Sep 21, 2017

Incorrect emit for IIFE with assertion in property access in call expression #18668

Closed

Merge branch 'master' into correctlyCacheTaggedTemplates

1cb5eb9

msftgits removed the cla-not-required label Sep 26, 2017

DanielRosenwasser added 4 commits September 27, 2017 17:07

Added tests for import helpers with & without a declared template obj…

886a29b

…ect creator.

Ensure that the import helper is checked for tagged templates, and up…

1841afe

…date its name.

Accepted baselines.

0b7538d

git Merge branch 'master' into correctlyCacheTaggedTemplates

b406d54

Fall back to old behavior for tagged template emit in global files.

4ec1643

ghost reviewed Sep 29, 2017

View reviewed changes

sandersn requested changes Sep 29, 2017

View reviewed changes

weswigham requested changes Sep 29, 2017

View reviewed changes

Accepted baselines.

d039942

DanielRosenwasser force-pushed the correctlyCacheTaggedTemplates branch from 9f3db7f to f22a1fe Compare September 30, 2017 00:14

Addressed code review feedback.

5da45fb

DanielRosenwasser force-pushed the correctlyCacheTaggedTemplates branch from f22a1fe to 5da45fb Compare September 30, 2017 00:17

DanielRosenwasser added 4 commits September 29, 2017 23:01

Added test for module & global examples.

f94bded

Accepted baselines.

81b3e85

Updated helper for marginally better minification.

a23d1bf

Accepted baselines.

e2c6aac

rbuckton requested changes Oct 2, 2017

View reviewed changes

DanielRosenwasser added 3 commits October 2, 2017 13:23

Move first/last shortcuts closer to the end of the helper flags.

b80b2ee

Flatten the '__makeTemplateObject' helper to use less vertical screen…

babe3cb

… real estate.

Accepted baselines.

8fd638c

DanielRosenwasser changed the title ~~Correctly cache tagged template objects~~ Oct 3, 2017

DanielRosenwasser merged commit 301c90c into master Oct 3, 2017

DanielRosenwasser deleted the correctlyCacheTaggedTemplates branch October 3, 2017 19:07

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly cache tagged template objects in modules #18300

Correctly cache tagged template objects in modules #18300

DanielRosenwasser commented Sep 7, 2017 •

edited

Loading

DanielRosenwasser Sep 7, 2017 •

edited

Loading

ghost Sep 29, 2017

DanielRosenwasser Sep 30, 2017

ghost Sep 29, 2017

DanielRosenwasser Sep 30, 2017

ghost Sep 29, 2017

ghost Sep 29, 2017

ghost Sep 29, 2017 •

edited by ghost

Loading

weswigham Sep 29, 2017

DanielRosenwasser Sep 30, 2017

ghost Sep 29, 2017 •

edited by ghost

Loading

rbuckton Oct 2, 2017

sandersn left a comment

sandersn Sep 7, 2017

DanielRosenwasser Sep 30, 2017

sandersn Sep 28, 2017

sandersn Sep 29, 2017

sandersn Sep 29, 2017

weswigham left a comment

weswigham Sep 29, 2017

weswigham Sep 29, 2017

rbuckton commented Oct 1, 2017

DanielRosenwasser commented Oct 2, 2017 •

edited

Loading

rbuckton Sep 7, 2017

rbuckton Oct 2, 2017

rbuckton Oct 2, 2017

DanielRosenwasser Oct 2, 2017

weswigham Oct 2, 2017

rbuckton Oct 2, 2017

DanielRosenwasser Oct 2, 2017

weswigham Oct 2, 2017

Correctly cache tagged template objects in modules #18300

Correctly cache tagged template objects in modules #18300

Conversation

DanielRosenwasser commented Sep 7, 2017 • edited Loading

DanielRosenwasser Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 29, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 29, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton commented Oct 1, 2017

DanielRosenwasser commented Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Sep 7, 2017 •

edited

Loading

DanielRosenwasser Sep 7, 2017 •

edited

Loading

ghost Sep 29, 2017 •

edited by ghost

Loading

ghost Sep 29, 2017 •

edited by ghost

Loading

DanielRosenwasser commented Oct 2, 2017 •

edited

Loading