-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Tagged templates ES3 & 5 #1589
Tagged templates ES3 & 5 #1589
Conversation
Example: foo `bar` should compile to foo([“bar”])
function foo(a: string[], b: number, c: number) { }
foo`A${ 1 }B${ 2, 3 }C`; Now, with your change, this emits foo(["A", "B", "C"], 1, 2, 3); whereas it should emit Regular untagged template strings have a function that checks whether each expression inside needs to be parenthesized or not - see how needsParens is computed inside emitTemplateExpression. You need to do something similar. |
Example: foo`A${ 1 }B${ 2, 3 }C`;
Thanks! Fixed |
You should have a function like So I'd rather have the raw property on - if we're doing it, we should do it more correctly, and t's only a matter of time until someone actually wants that. I think that if we're supporting downlevel, let's just do it here. However, let's wait for input from @mhegazy and @jonathandturner. |
And while it's true that the comma operator has the lowest precedence so it's enough to directly check |
@@ -2048,7 +2048,12 @@ module ts { | |||
} | |||
|
|||
function getTemplateLiteralAsStringLiteral(node: LiteralExpression): string { | |||
return '"' + escapeString(node.text) + '"'; | |||
if (node.parent.kind === SyntaxKind.TaggedTemplateExpression) { | |||
// Emit tagged template as foo(["string"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 spaces instead of tabs.
@DanielRosenwasser How would the raw property be emitted? Something like this: var __template; // Create variable at top of the file
foo((__template = ['A', 'B'], __template.raw = ['A', 'B'], __template), 1); |
Using a single var x = foo`12\n3${ bar`45\n6${ 789 }` }`; var x = foo((__template = ["12\n3", ""], __template.raw = ["12\\n3", ""], __template), bar((__template = ["45\n6", ""], __template.raw = ["45\\n6", ""], __template), 789)); It does seem to be well-defined per the ES spec. __template is assigned 123... first and then overwritten with 456... before calling bar(), but foo is still called with 123... because that was what __template evaluated to at the time. Maybe create a new temporary for each tagged template string, just like destructuring creates a new temporary for each destructuring assignment. var a = ["45\n6", ""]; a.raw = ["45\\n6", ""];
var b = ["12\n3", ""]; b.raw = ["12\\n3", ""];
var x = foo(b, bar(a, 789)); Edit: Fixed emit example. |
For comparison, 6to5 uses a helper function: var _taggedTemplateLiteral = function (strings, raw) {
return Object.freeze(Object.defineProperties(strings, {
raw: {
value: Object.freeze(raw)
}
}));
};
var x = foo(_taggedTemplateLiteral(["12\n3", ""], ["12\\n3", ""]), bar(_taggedTemplateLiteral(["45\n6", ""], ["45\\n6", ""]), 789)); (We could simplify it to |
I wouldn't prefer a function for this, it doesn't really reduce the complexity very much since the code with the variable isn't very complicated. I don't see an advantage (yet) of using multiple vars. I could see the advantage of having one variable per function (instead of per file), since uglifyjs for example can mangle those names very well. |
The advantage of multiple vars or the utility function is that it avoids using the same variable for multiple invocations, like in the nested template string example I gave. Granted, I haven't been able to think of a way to break it, since ES guarantees that
Since the strings and strings.raw expressions can only contain strings, not code, they can't have any other side effects on the shared variable apart from the assignment. Any code which does have side effects (such as a template expression containing a tagged template string) will appear after them in the parameter list and so won't affect them. It just looks ugly and unreadable, and you have to resort to language-lawyering like above to demonstrate its correctness. It's also arguably not the kind of JS a human would write, which is one of TS's principles. So my personal order of preference is:
|
It would appear that you can get away with one variable if you concede some other functionality. Here are the concerns:
We could support interning, but because we don't stop you from modifying the array, it's actually at odds with immutability - unless we freeze the arrays, in which case we can't support ES3. My personal intuition is that if you want performance, there's an easy enough workaround - don't use a tagged template. Convert it to a function call. Again, I'd like to get feedback from @mhegazy but we can certainly continue discussing this. |
Destructuring assignment already emits a local variable per assignment. Looking at the code, it just tries increasing variable names until it finds one that isn't used in the current scope and all parent scopes. Temporaries are kept track of in a tempVariables array that is updated for each scope as emit traverses the AST.
You could use Object.freeze for ES5 and not for ES3, or you could just not use it altogether as a compromise. If you go with the utility function route you can let the user override it with their own implementation of the function (ala __extends) that does use Object.freeze() if they so desire. That said...
This is interesting. I didn't know about this, but you're right. (Template strings are registered in a registry, and if the strings and raw strings of a particular template are identical to one already in the registry, the registered string's template object is used for the first argument to the tag function. See 12.3.7 and 12.2.8.2.2) FWIW it does not seem to be followed by FF at the moment. function foo(arr) { return arr; }
foo`123` === foo`123`; // false (And also not by any other ES6 shims.) Perhaps it would be fine for TS's emit to also not bother. Or, as you said, you could use Object.freeze() to atleast maintain immutability, and not support this downlevel emit for ES3. |
Spoke with @mhegazy about this. We'd like to get this in for 1.5 if possible. The approach we're favoring is actually one similar to destructuring - one variable for every invocation. This is to favor a reader's perspective when trying to reason about the code, so it may be more ideal. If you need any pointers in pursuing this direction, just let us know. |
Ok, will try to do that. Just to be sure, where will the variable statement be emitted? I think we want before the statement which contains a tagged template. Thus if the tagged template is a part of another tagged template and another statement, it will be written above that statement. The only problem or inconsistency I see with this approach is the case when a tagged template is used within the condition of a for (or while) loop. The idea is that a template array won't be reused but will be reassigned every time it's needed. But in a for loop, how will that work? Since if you'd emit it like I described it above, it will emit:
Is this an edge-case that doesn't matter? |
Just do the same as destructuring: var x, y;
for (; { x, y } = { x: 5, y: 6 };) {
} var x, y;
for (; (_a = { x: 5, y: 6 }, x = _a.x, y = _a.y, _a);) {
}
var _a; |
I think that would actually be ideal and appropriate.
Exactly |
Ok, I've implemented the variable approach, except for the loops. Current codegen: // Typescript
foo `${foo `A${1}B`}`;
// Javascript
var _a = ["", ""];
_a.raw = ["", ""];
var _b = ["A", "B"];
_b.raw = ["A", "B"];
foo(_a, foo(_b, 1)); Have I implemented the functionality in the right functions and files? Also when you have a comment before a statement with a tagged template, the temporary variable will be emitted before the comment: // Typescript
// Comment
foo `${foo `A${1}B`}`;
// Javascript
var _a = ["", ""];
_a.raw = ["", ""];
var _b = ["A", "B"];
_b.raw = ["A", "B"];
// Comment
foo(_a, foo(_b, 1)); Can this be solved easily? |
@@ -371,9 +372,25 @@ module ts { | |||
function getDestructuringParameterName(node: Declaration) { | |||
return "__" + indexOf((<SignatureDeclaration>node.parent).parameters, node); | |||
} | |||
|
|||
function bindTaggedTemplateExpression(node: TaggedTemplateExpression) { | |||
if (file.languageVersion >= ScriptTarget.ES6) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline/curlies around the if
body.
For lambda expressions returning the tagged template string, the temporaries are generated outside the lambda instead of inside. Regular functions are fine: var bar = () => foo`A${ 1 }B`;
(() => foo`C${ 2 }D`)();
var baz = function () { return foo`E${ 3 }F`; };
(function () { return foo`G${ 4 }H`; })(); var _a = ["A", "B"];
_a.raw = ["A", "B"];
var bar = function () { return foo(_a, 1); };
var _b = ["C", "D"];
_b.raw = ["C", "D"];
(function () { return foo(_b, 2); })();
var baz = function () {
var _a = ["E", "F"];
_a.raw = ["E", "F"];
return foo(_a, 3);
};
(function () {
var _a = ["G", "H"];
_a.raw = ["G", "H"];
return foo(_a, 4);
})(); _a and _b for the first two could be emitted inside the anonymous function instead, similar to the last two. |
This crashes the compiler: function foo(...args) { }
class A {
x = foo`A${ 1 }B`;
}
(You can use these as tests even if you change the code per DanielRosenwasser's comments.) Edit: Same with function bar(x = foo`A${ 1 }B`) { } and class C {
bar(x = foo`A${ 1 }B`) { }
} but not with var y = {
bar: (x = foo`A${ 1 }B`) => undefined
};
var z = {
bar(x = foo`A${ 1 }B`) { return undefined; }
}; |
@DanielRosenwasser Thanks for the feedback. I think the difficulty is that for destructuring, the variable is emitted in the current node (if it is a variable declaration) or before it (for example in a for loop), but for the tagged templates we need it before the node. For destructuring the variable is added to an array and when it's possible to emit these variable declarations it will emit them. The variable declarations are thus emitted after the destructuring node, but that doesn't matter since the variable will be hoisted. For these templates we want the variables before the tagged template, since only the declaration, not the initialization will be hoisted. An alternative would be to emit for (var i = 0; foo((_a = ["Lorem", "Ipsum"], _a.raw = ["Lorem", "Ipsum"], _a), i); i++) {}
var _a; But we decided we didn't want that. So I think it's necessary to determine where those temporary variables should be emitted before the emit actually starts. Or am I missing something? @Arnavion It searches for a statement that is the parent of the tagged template, but as you pointed out that isn't working always, since not all tagged templates have to have a statement as parent, or it might point to the wrong parent. |
@ivogabe assignment expressions permit binding patterns on the left-hand side as well, so destructurings are actually also an expression-level construct, and suffer from the same issue. Currently the emit for those temps occurs at the end of the enclosing scope - this is permissible given JavaScript's hoisting semantics. See |
Looks great! I just had a few nits to take care of (mostly about commenting functions to make their purpose and behavior clear). |
|
||
// Newline normalization | ||
text = text.replace(/\r\n?/g, "\n"); | ||
text = escapeString(text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these steps for the cooked strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh maybe these are the steps that uncook the string
👍 |
Conflicts: tests/baselines/reference/taggedTemplateStringsTypeArgumentInference.js tests/baselines/reference/taggedTemplateStringsWithOverloadResolution3.j s tests/baselines/reference/taggedTemplateStringsWithTypeErrorInFunctionEx pressionsInSubstitutionExpression.js tests/baselines/reference/templateStringInObjectLiteral.js
Thanks for the feedback everyone! I've added some new commits, can you re-review it? @CyrusNajmabadi That was indeed for the raw strings. I've added a comment to explain it. Is that clear enough? |
Odd, the baselines seem to have changed for a few files. I swear, we're almost there! |
That's strange, on my machine they pass. I've run the tests again and they don't error. Any idea how that's happening? |
The tests pass on my system too, so try pulling from master and pushing again. Travis might just need to be triggered. |
There were some baselines that changed after merging the master, should be fixed now. Does Travis pull commits from both the master as the branch that is tested? |
Yeah, I believe what happens is that Travis will run tests on a given branch and then on a hypothetically merged branch. Just waiting on @mhegazy to take a look at this change. |
Due to tests introduced by #2143, can you pull in from master and update the baselines? |
👍 thanks @ivogabe |
I've merged the master again. It looks like taggedTemplateStringsWithUnicodeEscapes is wrong, the |
Tagged templates ES3 & 5
No that's fine, it's meant to be broken until #2047 is fixed. Actually, I'm more concerned that we don't preserve string escapes in the output, but I'll take care of it there where we'll have easier means. Well I think that's it - really big thanks for this feature! Our team, and I think the community, will definitely be happy to see this, and you've done a great job! |
I've implemented basic tagged templates emit when targeting ES3 or ES5.
is compiled to
See also #1590. I haven't updated the tests yet.