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

Regular expression literal can have its whitespace removed #19

Closed
li-a opened this issue Oct 30, 2019 · 5 comments
Closed

Regular expression literal can have its whitespace removed #19

li-a opened this issue Oct 30, 2019 · 5 comments

Comments

@li-a
Copy link

li-a commented Oct 30, 2019

Consider this legal JS (found in highcharts 4.1.4, already apparently minified by Closure Compiler):

{dSetter:function(a,b,c){a&&a.join&&(a=a.join(" "));/(NaN| {2}|^$)/.test(a)&&(a="M 0 0");c.setAttribute(b,a);this[b]=a}}

JSMin outputs:

{dSetter:function(a,b,c){a&&a.join&&(a=a.join(" "));/(NaN|{2}|^$)/.test(a)&&(a="M 0 0");c.setAttribute(b,a);this[b]=a}}

The RegExp literal has been modified (| { --> |{, which also happens to be invalid syntax).

Seems to be distinct from #11 because testing across history indicates the problem was only introduced by cec896f (which is newer than #11).

@douglascrockford
Copy link
Owner

A problem I have had historically is that if one of my tools accepts code that is badly written, then it is assumed that 'I' am approving, which I am not. That is why made that change in 2012. Up until now, no one has complained.

There is a lot of stupid stuff happening in that highcharts code, but I am past caring about it. JSMin will now accept it.

@li-a
Copy link
Author

li-a commented Oct 31, 2019

Thanks for looking. The "badly written" code here is output from minification, not meant for human consumption. Highcharts source happens to look friendlier:

if (/(NaN| {2}|^$)/.test(value)) {
	value = 'M 0 0';
}

(and minifies fine w/ JsMin).

Ugliness or other badness aside, as far as I can tell, the JsMin input here is valid ES 3 and the output is not. We have an easy workaround for this one, but we believe this hits many codebase files that are not highcharts.

I didn't realize JsMin was being maintained, and we have seen the usage warning/caveats, but I filed this in case someone else is searching around. Will post a fix if we make one.

(Of course we shouldn't be passing pre-minified code through JsMin anyway; that's a future fix.)

@li-a
Copy link
Author

li-a commented Oct 31, 2019

Some simplified failure cases:

  • ;/a# b/; --> ;/a#b/;
  • ;/a| b/; --> ;/a|b/;
  • ;/# /; --> ;/#/;
  • ;/| /; --> Error: Unterminated regular expression at line 0 and column 7

The leading semicolon seems to be significant.

@douglascrockford
Copy link
Owner

I think this is fixed in the latest.

Why are you minifying minified code?

@li-a
Copy link
Author

li-a commented Nov 1, 2019

Confirmed all our cases are resolved. I didn't notice changes went in, thanks for looking.

The double minification fix is planned.

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

No branches or pull requests

2 participants