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

ASI hazard in tagged templates (need [NoLT here]?) #943

Closed
bswierczynski opened this Issue Jun 26, 2017 · 15 comments

Comments

Projects
None yet
7 participants
@bswierczynski

bswierczynski commented Jun 26, 2017

A New Line before a TemplateLiteral does not trigger ASI if the template literal is preceded by a MemberExpression.

This allows for constructs that caused some confusion on the web: semicolons were not inserted where some people expected them to be.

Example 1 (gotcha)

let some_ident = 42
let foo = 43 + some_ident // expecting ASI to add a semicolon here

`id
className
someOtherAttrs`.split('\n').forEach(attr => { /*do something */ })

// Uncaught TypeError: some_ident is not a function

some_ident from the previous line is treated as the template handler function for the template on the next line.

Example 2 (gotcha)
Or even when concatenating files, if the preceding one is wrapped in an IIFE:

// Previous file:
(function() {
  // ...
})() // expecting ASI to add a semicolon here

// Next file:
`id
className
someOtherAttrs`.split('\n').forEach(attr => { /* ... */ })

// Uncaught TypeError: (intermediate value)(...) is not a function

The IIFE's result is treated as the template handler for the template literal on the next line.

A rare hazard
This makes only the third such ASI-hazard in the language. The other two can be avoided by never starting a line with a ( or [. With tagged templates, ` has been added to that list.

This hazard was recognized by standard JS:
https://www.npmjs.com/package/standard

[edit: as @bakkot noticed, there are more ASI hazards, not recognized by Standard]

Potential fix
Make MemberExpression: MemberExpression TemplateLiteral a restricted rule:

MemberExpression: MemberExpression [no LineTerminator here] TemplateLiteral

This would make sure that ASI inserts semicolons before lines that start with a template literal.

Example 3 (Fix breaks compatibility)
The problem is, adding [NoLT here] could break existing code that might be intentionally trying to use the following format:

let h = SafeHTML
`
<h1>${foo}</h1>
<p><${bar}</p>
`

Is it to late for a change? Maybe. I don't have a strong personal opinion on this (I tend to use semicolons anyways), but since Brendan and AWB said this hasn't been discussed and asked me to file a bug, here it is. Any insights?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jun 26, 2017

Member

The latter example seems somewhat useful for DSLs, in that it mirrors docblocks a bit.

Member

ljharb commented Jun 26, 2017

The latter example seems somewhat useful for DSLs, in that it mirrors docblocks a bit.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Jun 26, 2017

Contributor

This makes only the third such ASI-hazard in the language

Not so; -, +, and / are also hazards. (Aside: the standard project has done a major disservice to the community by implying - or rather asserting outright - that [ and ( were the only hazards. This is not true; this was never true.) And there's almost certainly going to be more if we get class fields.

In any case, tagged templates have been shipping for a while; I personally don't think we should be making such major changes to the semantics of existing code.

Contributor

bakkot commented Jun 26, 2017

This makes only the third such ASI-hazard in the language

Not so; -, +, and / are also hazards. (Aside: the standard project has done a major disservice to the community by implying - or rather asserting outright - that [ and ( were the only hazards. This is not true; this was never true.) And there's almost certainly going to be more if we get class fields.

In any case, tagged templates have been shipping for a while; I personally don't think we should be making such major changes to the semantics of existing code.

@bswierczynski

This comment has been minimized.

Show comment
Hide comment
@bswierczynski

bswierczynski Jun 26, 2017

@bakkot you are correct. Perhaps lines starting with - and + were deemed too rare in practice to pose a signifficant threat. Although some folks used them (or !) to force a function expression in IIFE modules.

And "/"? You mean a regex literal being parsed as division?

IMHO it's probably too late for restricting the production. It'll just remain an ASI gotcha, like return <NL>{ foo: 42 }.

bswierczynski commented Jun 26, 2017

@bakkot you are correct. Perhaps lines starting with - and + were deemed too rare in practice to pose a signifficant threat. Although some folks used them (or !) to force a function expression in IIFE modules.

And "/"? You mean a regex literal being parsed as division?

IMHO it's probably too late for restricting the production. It'll just remain an ASI gotcha, like return <NL>{ foo: 42 }.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Jun 26, 2017

Contributor

And "/"? You mean a regex literal being parsed as division?

Yes. It usually ends up as a syntax error, but it's still a hazard:

x = 0
/b/.test(y) && console.log('hit') // SyntaxError: unexpected token '.'
Contributor

bakkot commented Jun 26, 2017

And "/"? You mean a regex literal being parsed as division?

Yes. It usually ends up as a syntax error, but it's still a hazard:

x = 0
/b/.test(y) && console.log('hit') // SyntaxError: unexpected token '.'
@bswierczynski

This comment has been minimized.

Show comment
Hide comment
@bswierczynski

bswierczynski Jun 26, 2017

Yeah, but I figured that if you add a flag, it won't even be a syntax error:

x = 0
/b/i.test(y) && console.log('hit') 

Nasty! But should be pretty rare in practice. I know that [] and () did bite a few folks. ` might do that too.

bswierczynski commented Jun 26, 2017

Yeah, but I figured that if you add a flag, it won't even be a syntax error:

x = 0
/b/i.test(y) && console.log('hit') 

Nasty! But should be pretty rare in practice. I know that [] and () did bite a few folks. ` might do that too.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Jun 26, 2017

Contributor

I expect the '`' hazard is likely to be rare in practice too, for what it's worth.

Contributor

bakkot commented Jun 26, 2017

I expect the '`' hazard is likely to be rare in practice too, for what it's worth.

@bswierczynski

This comment has been minimized.

Show comment
Hide comment
@bswierczynski

bswierczynski Jun 26, 2017

Hopefully, but I did come up with relatively reasonable (IMHO) examples.

I forgot to put a .split() into those in the original post. Just fixed them.

bswierczynski commented Jun 26, 2017

Hopefully, but I did come up with relatively reasonable (IMHO) examples.

I forgot to put a .split() into those in the original post. Just fixed them.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jun 27, 2017

Member

Previous discussion on this: https://mail.mozilla.org/pipermail/es-discuss/2015-February/041530.html (/cc @ikarienator)

I believe this was brought up at a TC39 meeting and @allenwb was against the [no Lineterminator here] addition.

Member

michaelficarra commented Jun 27, 2017

Previous discussion on this: https://mail.mozilla.org/pipermail/es-discuss/2015-February/041530.html (/cc @ikarienator)

I believe this was brought up at a TC39 meeting and @allenwb was against the [no Lineterminator here] addition.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jun 27, 2017

Member

Also, I've written code like this, and this change would almost surely be web incompatible.

Member

michaelficarra commented Jun 27, 2017

Also, I've written code like this, and this change would almost surely be web incompatible.

@gsklee

This comment has been minimized.

Show comment
Hide comment
@gsklee

gsklee Jun 27, 2017

I don't know what's with this sudden panic, but the behavior described by @rauschma aligns perfectly well with the rest of the language:

const f = Number.isInteger
(10)

console.log(f); // true
const f = String.raw
`abc`.split('')

console.log(f); // ["a", "b", "c"]

Tagged template literals are simply function calls in disguise, the so-called "tags" are just functions. If we'd like to "fix" this behavior then we'll also have to face the syntax inconsistency it'll bring.

gsklee commented Jun 27, 2017

I don't know what's with this sudden panic, but the behavior described by @rauschma aligns perfectly well with the rest of the language:

const f = Number.isInteger
(10)

console.log(f); // true
const f = String.raw
`abc`.split('')

console.log(f); // ["a", "b", "c"]

Tagged template literals are simply function calls in disguise, the so-called "tags" are just functions. If we'd like to "fix" this behavior then we'll also have to face the syntax inconsistency it'll bring.

@bswierczynski

This comment has been minimized.

Show comment
Hide comment
@bswierczynski

bswierczynski Jun 27, 2017

@gsklee
You are right that this is all perfectly logical if you know how ASI works. I think the fear is that many (if not most) folks don't even realize the basic rule of "ASI won't touch anything unless there is a parse problem in the first place". They assume that it more-or-less just adds a semicolon to the end of a line. This tagged template syntax causes a practical gotcha.

I've fixed bugs caused by lines starting with (. IIRC, most of them were file concatenation bugs. So you didn't even have the context of the previous line: just a file starting with (. And an error magically surfacing in the concatenated file. Perhaps nowadays, this will be less of an issue thanks to modern modules and tools that automatically wrap files and add semicolons to prevent this.

Whether the syntax rule had a [NoLT] clause in it or not, the result could bite someone. Like the infamous gotcha with the return statement, where [NoLT] actually causes confusion (depending on the brace style you use):

return // semicolon is inserted here
{
  foo: 42
}

@michaelficarra
Thanks for the link! The topic didn't get much attention back then...

I don't see how this ASI behaviour can be changed now.

bswierczynski commented Jun 27, 2017

@gsklee
You are right that this is all perfectly logical if you know how ASI works. I think the fear is that many (if not most) folks don't even realize the basic rule of "ASI won't touch anything unless there is a parse problem in the first place". They assume that it more-or-less just adds a semicolon to the end of a line. This tagged template syntax causes a practical gotcha.

I've fixed bugs caused by lines starting with (. IIRC, most of them were file concatenation bugs. So you didn't even have the context of the previous line: just a file starting with (. And an error magically surfacing in the concatenated file. Perhaps nowadays, this will be less of an issue thanks to modern modules and tools that automatically wrap files and add semicolons to prevent this.

Whether the syntax rule had a [NoLT] clause in it or not, the result could bite someone. Like the infamous gotcha with the return statement, where [NoLT] actually causes confusion (depending on the brace style you use):

return // semicolon is inserted here
{
  foo: 42
}

@michaelficarra
Thanks for the link! The topic didn't get much attention back then...

I don't see how this ASI behaviour can be changed now.

@gsklee

This comment has been minimized.

Show comment
Hide comment
@gsklee

gsklee Jun 27, 2017

@bswierczynski Well, maybe next time, you could let these folks know that ASI is not some kind of random magic, but part of the language spec with very rigid and formal rules. If they would like to omit semicolons and use ASI, then it's their responsibilities to make sure they understand how ASI works, and make it work. As far as I can tell, the oft-misunderstood ASI only has three very straight-forward rules. I do not believe that the language is to be blamed for any ASI mishaps out there at all.

gsklee commented Jun 27, 2017

@bswierczynski Well, maybe next time, you could let these folks know that ASI is not some kind of random magic, but part of the language spec with very rigid and formal rules. If they would like to omit semicolons and use ASI, then it's their responsibilities to make sure they understand how ASI works, and make it work. As far as I can tell, the oft-misunderstood ASI only has three very straight-forward rules. I do not believe that the language is to be blamed for any ASI mishaps out there at all.

@bswierczynski

This comment has been minimized.

Show comment
Hide comment
@bswierczynski

bswierczynski Jun 27, 2017

@gsklee
That's exactly what I've done and what I'll continue to do. I literally gave a seminar titled "Automatic Semicolon Insertion and Other Parsing Oddities" where I presented those rules. That experience is part of the reason I know people -- good devs -- are often not fully aware of how this really works.

What folks usually do at best is just stick to the simplified rules listed by Standard.

The ASI errors that I had to trace and fix a couple of years ago (before Standard) were a bit more complicated. Because actually, if you looked at just this one file that had a wrapping IIFE in it, a semicolon WOULD be added to the end of it as per Rule 2. However, since the file was concatenated with another file (also with an IIFE), Rule 2 no longer applied as it was no longer the "end of input stream".

I'm super-into the formal language rules myself, but I think -- and I pressume you'd probably agree -- that "everyone should just get to precisely know all the tiny formal rules!" should not be an excuse for putting confusing foot guns into the language. So it's all about balance: expressive power vs consistency vs potential confusion etc.

bswierczynski commented Jun 27, 2017

@gsklee
That's exactly what I've done and what I'll continue to do. I literally gave a seminar titled "Automatic Semicolon Insertion and Other Parsing Oddities" where I presented those rules. That experience is part of the reason I know people -- good devs -- are often not fully aware of how this really works.

What folks usually do at best is just stick to the simplified rules listed by Standard.

The ASI errors that I had to trace and fix a couple of years ago (before Standard) were a bit more complicated. Because actually, if you looked at just this one file that had a wrapping IIFE in it, a semicolon WOULD be added to the end of it as per Rule 2. However, since the file was concatenated with another file (also with an IIFE), Rule 2 no longer applied as it was no longer the "end of input stream".

I'm super-into the formal language rules myself, but I think -- and I pressume you'd probably agree -- that "everyone should just get to precisely know all the tiny formal rules!" should not be an excuse for putting confusing foot guns into the language. So it's all about balance: expressive power vs consistency vs potential confusion etc.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 27, 2017

Member

I think the point was that devs who are willing to follow something as... unusual... as "standard" need to be willing to actually read the language standard and get to precisely know all its tiny formal rules.

Member

domenic commented Jun 27, 2017

I think the point was that devs who are willing to follow something as... unusual... as "standard" need to be willing to actually read the language standard and get to precisely know all its tiny formal rules.

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Aug 25, 2017

Member

We don't have consensus to at the NLTH restriction, so I'm going to close this.

Member

bterlson commented Aug 25, 2017

We don't have consensus to at the NLTH restriction, so I'm going to close this.

@bterlson bterlson closed this Aug 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment