[[FIX]] Parse nested templates #2152

leebyron · 2015-02-04T21:43:16Z

ES6 template literals are nestable, jshint's parser is capable, but it's lexer isn't. This patch adds the concept of a stack of contexts to the lexer, replacing the previous boolean flag. This context stack pattern is borrowed from the awesome Acorn parser project, where it's used to enable more than just template literals.

Fixes: #2151
Fixes: #2082
Fixes: #1814

ES6 template literals are nestable, jshint's parser is capable, but it's lexer isn't. This patch adds the concept of a stack of contexts to the lexer, replacing the previous boolean flag. This context stack pattern is borrowed from the awesome Acorn parser project, where it's used to enable more than just template literals. Fixes: jshint#2151

coveralls · 2015-02-04T21:46:40Z

Coverage increased (+0.0%) to 94.77% when pulling c874029 on leebyron:templatenest into 0ebe613 on jshint:master.

leebyron · 2015-02-04T21:47:08Z

BTW, expanding the use of context can help solve #2082. Acorn uses the same approach to solve this problem.

Uses context introduced in previous diff to let lexer be contextually aware when parsing a "}" character. Fixes: jshint#2082

leebyron · 2015-02-04T22:02:45Z

Just also fixed #2082 in a 2nd commit in this patch, editing the description of the pull req now.

coveralls · 2015-02-04T22:05:25Z

Coverage decreased (-0.02%) to 94.75% when pulling f323327 on leebyron:templatenest into 0ebe613 on jshint:master.

caitp · 2015-02-04T22:42:03Z

src/lex.js

-    tokenType = this.inTemplate ? Token.TemplateTail : Token.StringLiteral;
-    this.inTemplate = false;
+    // Final value is either StringLiteral or TemplateTail
+    tokenType = tokenType === Token.TemplateHead ? Token.StringLiteral : Token.TemplateTail;


since you're fixing all this, you should also fix this: don't use Token.StringLiteral for no-substitution templates, otherwise they could be treated as valid directives, which is not true.

test case:

function fn() { `use strict`; return "\077"; } // should not yield any warnings

Great point. I'll add that to this PR

caitp · 2015-02-04T22:45:03Z

rest of it lgtm, the other thing could be a followup bug

leebyron · 2015-02-04T22:46:09Z

If you're comfortable merging before I get to fixing the directive issue, go nuts. I'll still follow up with that.

caitp · 2015-02-04T22:47:23Z

I think I will wait, I need to make sure the startLine thing can still work correctly with the stack structure, @jugglinmike or @rwaldron might also want to have a look

leebyron · 2015-02-04T23:27:43Z

Just added your suggestion, @caitp. Let me know if that's in line with what you were looking for.

coveralls · 2015-02-04T23:29:31Z

Coverage decreased (-0.04%) to 94.73% when pulling 61be438 on leebyron:templatenest into 0ebe613 on jshint:master.

caitp · 2015-02-05T00:08:35Z

src/lex.js

@@ -31,7 +31,13 @@ var Token = {
  RegExp: 9,
  TemplateHead: 10,
  TemplateMiddle: 11,
-  TemplateTail: 12
+  TemplateTail: 12,
+  TemplateString: 13


nit: NoSubstTemplate maybe?

Return a different kind of lexer token for full template strings such that they are not consumed by string directives.

leebyron · 2015-02-05T00:51:55Z

Changed to NoSubstTemplate and added that second test.

I think the error is expected, you get the same error for this example:

function fn() {
  42;
}

So it's not specific to templates.

coveralls · 2015-02-05T00:53:23Z

Coverage decreased (-0.04%) to 94.69% when pulling c402e25 on leebyron:templatenest into 13ae519 on jshint:master.

This is a slight alteration of jshint#2142 to cleanly rebase atop the previous diffs adding context, allowing it to work with nested templates. Fixes: jshint#1814

leebyron · 2015-02-05T19:37:11Z

@caitp - I just added another commit to this patch that covers what you were doing in #2142 but maintaining awareness of the context, so you won't need to rebase over this PR.

I took your test cases from that PR verbatim, but added cases for yield expressions as well.

coveralls · 2015-02-05T19:37:52Z

Coverage decreased (-0.03%) to 94.7% when pulling f76130c on leebyron:templatenest into 13ae519 on jshint:master.

leebyron · 2015-02-06T16:00:13Z

Anything else you guys would like me to cover here @caitp @rwaldron?

caitp · 2015-02-06T16:05:58Z

good enough, can you try to figure out why coverage decreased? might need some extra test cases

caitp · 2015-02-06T16:06:29Z

I'm guessing it's probably for the yield multi-line expressions

caitp · 2015-02-06T16:07:05Z

hmm nope, I'm wrong about that :<

caitp · 2015-02-06T16:09:57Z

oh, you can remove the quotmark handling for backticks, since templates aren't string tokens anymore

caitp · 2015-02-06T16:10:36Z

missing a case for this as well

leebyron · 2015-02-06T16:12:15Z

Good catch, I'll do that.

From looking at coverall's report, it's hard to tell what didn't get tested. It thinks coverage of style.js went down, but I didn't touch that file. Lex.js also went down but I can dig into that.

"}" should always yield a punctuator token, regardless of context.

coveralls · 2015-02-06T16:21:33Z

Coverage decreased (-0.0%) to 94.72% when pulling b3356dd on leebyron:templatenest into 13ae519 on jshint:master.

Remove quote key from template literal token.

caitp · 2015-02-06T16:24:29Z

Stuff in the last commit is okay, but I was referring to this << block is never hit because templates are not strings

leebyron · 2015-02-06T16:25:01Z

I just saw that too, and updated that commit :)

coveralls · 2015-02-06T16:26:04Z

Coverage increased (+0.02%) to 94.75% when pulling cf7bc41 on leebyron:templatenest into 13ae519 on jshint:master.

caitp · 2015-02-06T16:29:04Z

coverage still seems to have gone down a tiny bit but I think everything relevant to this PR is covered... will merge

leebyron · 2015-02-06T16:36:08Z

Thanks @caitp!

ES6 template literals are nestable, jshint's parser is capable, but it's lexer isn't. This patch adds the concept of a stack of contexts to the lexer, replacing the previous boolean flag. This context stack pattern is borrowed from the awesome Acorn parser project, where it's used to enable more than just template literals. Closes jshint#2151 Closes jshint#2152

leebyron force-pushed the templatenest branch from ec9ea59 to c874029 Compare February 4, 2015 21:44

[[FIX]] Allow object-literals within template strings

f323327

Uses context introduced in previous diff to let lexer be contextually aware when parsing a "}" character. Fixes: jshint#2082

caitp reviewed Feb 4, 2015
View reviewed changes

caitp reviewed Feb 5, 2015
View reviewed changes

[[FIX]] Templates are not String Directives

c402e25

Return a different kind of lexer token for full template strings such that they are not consumed by string directives.

leebyron force-pushed the templatenest branch from 61be438 to c402e25 Compare February 5, 2015 00:50

[[FIX]] Handle multi-line tokens after return or yield

f76130c

This is a slight alteration of jshint#2142 to cleanly rebase atop the previous diffs adding context, allowing it to work with nested templates. Fixes: jshint#1814

[[FIX]] Unfollowable path in lexer.

82ff928

"}" should always yield a punctuator token, regardless of context.

[[FIX]] Template literals are not strings

cf7bc41

Remove quote key from template literal token.

leebyron force-pushed the templatenest branch from b3356dd to cf7bc41 Compare February 6, 2015 16:24

caitp closed this in 3da1eaf Feb 6, 2015

leebyron deleted the templatenest branch February 6, 2015 18:48

nicolo-ribaudo mentioned this pull request Jun 22, 2015

[bug] Handle nested template strings. #2480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[[FIX]] Parse nested templates #2152

[[FIX]] Parse nested templates #2152

leebyron commented Feb 4, 2015

coveralls commented Feb 4, 2015

leebyron commented Feb 4, 2015

leebyron commented Feb 4, 2015

coveralls commented Feb 4, 2015

caitp Feb 4, 2015

caitp Feb 4, 2015

leebyron Feb 4, 2015

caitp commented Feb 4, 2015

leebyron commented Feb 4, 2015

caitp commented Feb 4, 2015

leebyron commented Feb 4, 2015

coveralls commented Feb 4, 2015

caitp Feb 5, 2015

leebyron commented Feb 5, 2015

coveralls commented Feb 5, 2015

leebyron commented Feb 5, 2015

coveralls commented Feb 5, 2015

leebyron commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

leebyron commented Feb 6, 2015

coveralls commented Feb 6, 2015

caitp commented Feb 6, 2015

leebyron commented Feb 6, 2015

coveralls commented Feb 6, 2015

caitp commented Feb 6, 2015

leebyron commented Feb 6, 2015

[[FIX]] Parse nested templates #2152

[[FIX]] Parse nested templates #2152

Conversation

leebyron commented Feb 4, 2015

coveralls commented Feb 4, 2015

leebyron commented Feb 4, 2015

leebyron commented Feb 4, 2015

coveralls commented Feb 4, 2015

caitp Feb 4, 2015

Choose a reason for hiding this comment

caitp Feb 4, 2015

Choose a reason for hiding this comment

leebyron Feb 4, 2015

Choose a reason for hiding this comment

caitp commented Feb 4, 2015

leebyron commented Feb 4, 2015

caitp commented Feb 4, 2015

leebyron commented Feb 4, 2015

coveralls commented Feb 4, 2015

caitp Feb 5, 2015

Choose a reason for hiding this comment

leebyron commented Feb 5, 2015

coveralls commented Feb 5, 2015

leebyron commented Feb 5, 2015

coveralls commented Feb 5, 2015

leebyron commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

caitp commented Feb 6, 2015

leebyron commented Feb 6, 2015

coveralls commented Feb 6, 2015

caitp commented Feb 6, 2015

leebyron commented Feb 6, 2015

coveralls commented Feb 6, 2015

caitp commented Feb 6, 2015

leebyron commented Feb 6, 2015