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

Backtick Javascript Escape Bug: Adds Semicolon #3265

Closed
bdkjones opened this issue Nov 28, 2013 · 18 comments
Closed

Backtick Javascript Escape Bug: Adds Semicolon #3265

bdkjones opened this issue Nov 28, 2013 · 18 comments

Comments

@bdkjones
Copy link

If you use the backtick to escape a piece of Javascript that ends with a semicolon like this:

`alert("hello");`

The compiler duplicates the semicolon, turning the line into:

alert("hello");;

It looks like the compiler is just blindly throwing a semicolon on the end of any backtick-escaped Javascript snippets. This is definitely bad behavior. It's present in 1.6.3.

Can we get this one fixed before 2016? (Given that the other bugs related to backticks haven't been fixed in a year and the amount of whining that took place in those threads about just how dumb it was to ever write anything other than CoffeeScript, I understand that this feature isn't the most popular one with the team. Still, it's badly broken and needs some attention, please.)

@bdkjones
Copy link
Author

The other backtick issue is here: #1504

It shows that the compiler is not smart enough to deal with escaped backticks. That is, if we have this in a Coffee file:

`alert("Here is a backtick: \` that is escaped");`

The compiler forgets to remove the escape character before the middle backtick, producing this JS:

alert("Here is a backtick: \` that is escaped");

The correct result would be:

alert("Here is a backtick: ` that is escaped");

@vendethiel
Copy link
Collaborator

Should have been fixed when #1473 was closed. I recall it correctly compiling at that time ...

@bdkjones
Copy link
Author

After reading #1473 and related issues, I can't tell if you guys changed the behavior or not.

I definitely don't agree that CoffeeScript should be adding semicolons after backtick-escaped Javascript. That's a very dangerous and very large assumption that a compiler should not make. The user should be responsible for deciding if a semicolon is required in his Javascript snippet and, if not, it should not be added by the CoffeeScript compiler.

@xixixao
Copy link
Contributor

xixixao commented Nov 29, 2013

@bdkjones Care to give an instructive example where the current behavior is bad? (I assume alert("hello");; is not it)

@vendethiel
Copy link
Collaborator

I guess for comments/shebangs (use-cases have been posted in previous threads)

@bdkjones
Copy link
Author

I understand that the double semicolon does not produce an error in Javascript, but it's still sloppy compiling and should be fixed.

It's not necessary for CoffeeScript to parse the backtick-escaped Javascript in order to determine whether or not a semicolon is necessary. It should simply be the user's responsibility to include a terminating semicolon in their Javascript if it's necessary.

In short, the backtick syntax should just be a way to drop a snippet of Javascript directly into the compiled output file, character for character. CoffeeScript should make no assumptions about the content of that Javascript snippet.

@jashkenas
Copy link
Owner

Interpolated JavaScript is an expression, not a statement. That's the only assumption that CoffeeScript makes.

@carlsmith
Copy link
Contributor

This bug breaks automatic source code mapping in Chrome and FireFox, and probably user scripts too. In order to use source maps, you have to provide a directive in the format of a single line comment. This CoffeeScript...

`//# sourceURL=/foo.js`

...compiles to this JavaScript [note the last char]...

//# sourceURL=/foo.js;

CoffeeScript is changing the filename in a directive.

This breaks CoffeeShop too, and has to be manually worked around. Chrome identifies eval'ed code in tracebacks by giving them the filename in the source URL. If it's not provided, then Chrome calls every input eval, so you can't do source mapping on tracebacks. You don't have any way of knowing which input contains the item. I currently have to add and remove semicolons manually.

This is definitely a bug.

@bdkjones
Copy link
Author

Good luck. I tried to get this fixed a long time ago and was told that the behavior was correct.

I also just manually work around it.

On Sep 20, 2014, at 05:46, Carl Smith notifications@github.com wrote:

This bug breaks automatic source code mapping in Chrome and FireFox, and probably user scripts too. In order to use source maps, you have to provide a directive in the format of a single line comment. This CoffeeScript...

//# sourceURL=/foo.js
...compiles to this JavaScript [note the last char]...

//# sourceURL=/foo.js;
CoffeeScript is changing the filename in a directive.

This breaks CoffeeShop too, and has to be manually worked around. Chrome identifies eval'ed code in tracebacks by giving them the filename in the source URL. If it's not provided, then Chrome calls every input eval, so you can't do source mapping on tracebacks. You don't have any way of knowing which input contains the item. I currently have to add and remove semicolons manually.

This is definitely a bug.


Reply to this email directly or view it on GitHub.

@jashkenas
Copy link
Owner

@carlsmith

What exact change would you propose we make? Removing the semicolon would be a seriously backwards-incompatible change.

@akre54
Copy link

akre54 commented Sep 22, 2014

Maybe test for the sourcemap comment pragma?

@jashkenas
Copy link
Owner

That'd be a pretty disgusting special case to throw in. Let's try not to go in that direction.

@bdkjones
Copy link
Author

I think the correct solution is to remove the semicolon and require that any backtick-escaped JavaScript be closed correctly (e.g. require the user to put a semicolon at the end if one is needed). That's the best, permanent solution.

Getting there is pretty straightforward. The next release should warn the user anytime a backtick-escaped piece of JavaScript is not terminated with a semicolon and advise them that the next release of the CoffeeScript compiler will no longer add semicolons automatically.

Then, the proximate release drops the semicolon behavior.

I don't think the change will negatively affect many people for two reasons:

  1. Backtick-escaped JavaScript is probably pretty rare. Most folks aren't using it.
  2. Those that are using it likely already have their JavaScript snippets closed properly; they aren't relying on the CoffeeScript compiler to add the last semicolon.

@goto-bus-stop
Copy link

Can work around it pretty easily for now(?):

`//# sourceURL=/foo.js
`
… code …

//# sourceURL=/foo.js
;
… code …

@jashkenas
Copy link
Owner

Since there's a workaround now listed above — I'll repeat my opinion that we shouldn't be changing this behavior. Embedded JavaScript in CoffeeScript is intended for JavaScript expressions. Not for comments. CoffeeScript has comments already — we don't want to be facilitating a special syntax for "embedded JavaScript comments"...

@carlsmith
Copy link
Contributor

@goto-bus-stop - Nice one. That works for me.

@jashkenas - No issue with using the work around, but if browsers are now parsing single-line comments at the ends of minified scripts, eval'ed code and the top of user scripts, then they're not really comments. Being's it hasn't come up a lot yet, and there's a work around, it's probably fine anyway.

@bdkjones
Copy link
Author

@goto-bus-stop clever!

@jashkenas That's one way of looking at the issue. But this is a real-world example of a case in which the CoffeeScript compiler fails. It should be fixed, even though there's a hacky workaround. Any good compiler handles edge cases correctly. Something like this would be fixed by the LLVM team or the GCC team. CoffeeScript should hold itself to the same standard: when there's a demonstrated failure case, the compiler should be fixed.

@jashkenas
Copy link
Owner

they're not really comments

Bingo. Using comment syntax to specify them was a horrible abuse ;)

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

No branches or pull requests

7 participants