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

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Sep 7, 2017

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

Fixes #17956

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;
Copy link
Member Author

@DanielRosenwasser DanielRosenwasser Sep 7, 2017

Choose a reason for hiding this comment

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

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

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

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

@@ -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);
Copy link

Choose a reason for hiding this comment

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

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

// 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;
Copy link

Choose a reason for hiding this comment

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

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

text: `
var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {
if (Object.defineProperty) {
return Object.defineProperty(cooked, "raw", { value: raw });
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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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 👅

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

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

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.

:(

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.

// 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 ?
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Also consider flipping the conditional.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

Was this a drive-by new test and fix?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes:

Fix weird case with un-parenthesized new expressions

@rbuckton
Copy link
Member

rbuckton commented Oct 1, 2017

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
Copy link
Member Author

DanielRosenwasser commented Oct 2, 2017

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

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

Choose a reason for hiding this comment

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

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

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

FirstEmitHelper = Extends,
LastEmitHelper = ExportStar
LastEmitHelper = ExternalEmitHelpers[" LastPlusOne"] - 1
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.

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.

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

@DanielRosenwasser DanielRosenwasser changed the title Correctly cache tagged template objects Correctly cache tagged template objects in modules Oct 3, 2017
@DanielRosenwasser DanielRosenwasser merged commit 301c90c into master Oct 3, 2017
@DanielRosenwasser DanielRosenwasser deleted the correctlyCacheTaggedTemplates branch October 3, 2017 19:07
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants