Fix octal escapes starting with 0 #1

addaleax · 2016-12-14T23:54:40Z

Octal escape sequences starting with 0 should be expanded before the usual escape sequences to avoid ambiguity with the \0 escape.

This patch aligns the behaviour with how JS parses the sequences.

coveralls · 2016-12-14T23:58:10Z

Coverage remained the same at 100.0% when pulling ad7eba1 on addaleax:fix-octals into 81a30b5 on iamakulov:master.

addaleax · 2016-12-17T02:59:06Z

Actually, I’m just noticing that things are a little more tricky; Consider strings like \\x41. Right now (with or without this PR), unescape would first convert \\ to \ and then \x41 to A, which is obviously a little undesired.

addaleax · 2016-12-17T03:35:43Z

I’ve updated with a more comprehensive solution that is, unfortunately, also kind of a full rewrite of the src/ part here.

coveralls · 2016-12-17T03:36:57Z

Coverage decreased (-17.4%) to 82.609% when pulling 8b43d6f on addaleax:fix-octals into 81a30b5 on iamakulov:master.

coveralls · 2016-12-17T03:39:26Z

Coverage remained the same at 100.0% when pulling e7195ef on addaleax:fix-octals into 81a30b5 on iamakulov:master.

coveralls · 2016-12-17T03:39:26Z

Coverage remained the same at 100.0% when pulling e7195ef on addaleax:fix-octals into 81a30b5 on iamakulov:master.

coveralls · 2016-12-17T03:39:26Z

Coverage remained the same at 100.0% when pulling e7195ef on addaleax:fix-octals into 81a30b5 on iamakulov:master.

coveralls · 2016-12-17T03:39:27Z

Coverage remained the same at 100.0% when pulling e7195ef on addaleax:fix-octals into 81a30b5 on iamakulov:master.

iamakulov · 2016-12-23T21:11:16Z

Wow, thanks for the PR!

Also, I guess I should enable email notifications :D

iamakulov

Reviewed the solution. Thanks a lot, seriously, this fixes the problems I didn't even have a clue about!

Left a couple of notes regarding the code. Once we fix them, I think it's ready to merge and be published.

iamakulov · 2016-12-23T21:22:36Z

.nyc_output/28944.json

@@ -0,0 +1 @@
+{"/home/sqrt/src/unescape-js/dist/index.js":{"path":"/home/sqrt/src/unescape-js/dist/index.js","s":{"1":1,"2":1,"3":1,"4":1,"5":1,"6":5,"7":1,"8":2,"9":1,"10":18,"11":18,"12":2,"13":16,"14":3,"15":2,"16":1,"17":13,"18":2,"19":11,"20":1},"b":{"1":[2,16],"2":[3,13],"3":[2,1],"4":[2,11]},"f":{"1":5,"2":2,"3":18,"4":18},"fnMap":{"1":{"name":"fromHex","line":24,"loc":{"start":{"line":24,"column":14},"end":{"line":24,"column":36}}},"2":{"name":"fromOct","line":27,"loc":{"start":{"line":27,"column":14},"end":{"line":27,"column":36}}},"3":{"name":"(anonymous_3)","line":31,"loc":{"start":{"line":31,"column":18},"end":{"line":31,"column":36}}},"4":{"name":"(anonymous_4)","line":32,"loc":{"start":{"line":32,"column":41},"end":{"line":32,"column":95}}}},"statementMap":{"1":{"start":{"line":3,"column":0},"end":{"line":5,"column":3}},"2":{"start":{"line":7,"column":0},"end":{"line":7,"column":32}},"3":{"start":{"line":9,"column":0},"end":{"line":9,"column":105}},"4":{"start":{"line":11,"column":0},"end":{"line":22,"column":2}},"5":{"start":{"line":24,"column":0},"end":{"line":26,"column":2}},"6":{"start":{"line":25,"column":4},"end":{"line":25,"column":51}},"7":{"start":{"line":27,"column":0},"end":{"line":29,"column":2}},"8":{"start":{"line":28,"column":4},"end":{"line":28,"column":50}},"9":{"start":{"line":31,"column":0},"end":{"line":43,"column":2}},"10":{"start":{"line":32,"column":4},"end":{"line":42,"column":7}},"11":{"start":{"line":33,"column":8},"end":{"line":41,"column":9}},"12":{"start":{"line":34,"column":12},"end":{"line":34,"column":37}},"13":{"start":{"line":35,"column":15},"end":{"line":41,"column":9}},"14":{"start":{"line":36,"column":12},"end":{"line":36,"column":86}},"15":{"start":{"line":36,"column":34},"end":{"line":36,"column":57}},"16":{"start":{"line":36,"column":62},"end":{"line":36,"column":86}},"17":{"start":{"line":37,"column":15},"end":{"line":41,"column":9}},"18":{"start":{"line":38,"column":12},"end":{"line":38,"column":34}},"19":{"start":{"line":40,"column":12},"end":{"line":40,"column":47}},"20":{"start":{"line":45,"column":0},"end":{"line":45,"column":36}}},"branchMap":{"1":{"line":33,"type":"if","locations":[{"start":{"line":33,"column":8},"end":{"line":33,"column":8}},{"start":{"line":33,"column":8},"end":{"line":33,"column":8}}]},"2":{"line":35,"type":"if","locations":[{"start":{"line":35,"column":15},"end":{"line":35,"column":15}},{"start":{"line":35,"column":15},"end":{"line":35,"column":15}}]},"3":{"line":36,"type":"if","locations":[{"start":{"line":36,"column":12},"end":{"line":36,"column":12}},{"start":{"line":36,"column":12},"end":{"line":36,"column":12}}]},"4":{"line":37,"type":"if","locations":[{"start":{"line":37,"column":15},"end":{"line":37,"column":15}},{"start":{"line":37,"column":15},"end":{"line":37,"column":15}}]}}}}


I added .nyc_output to .gitignore. Could you please merge or rebase upon the master branch so that this file gets removed?

Sorry, done!

iamakulov · 2016-12-23T21:40:52Z

src/index.js

-    string = unescapeVariableHexSequence(string);
-    return string;
+    return string.replace(JSEscapeRegex, (_, match, longHex, varHex, shortHex, octal) => {
+        if (match[0] === 'x') {


As far as I get from short debugging, every time this function is invoked, among longHex, varHex, shortHex and octal, only a single parameter will have a value will be defined. Is this true? Can we write just something like

if (longHex !== undefined) { // ... } else if (varHex !== undefined) { // ... }

then? This will make the code easier to understand.

That seems to work, yeah. :)

iamakulov · 2016-12-23T21:43:46Z

src/index.js

-    [/\\"/g, '\"'],
-    [/\\\\/g, '\\']
-];
+const JSEscapeRegex = /\\(u(\{([0-9A-Fa-f]+)\}|[0-9A-Fa-f]{4})|x([0-9A-Fa-f]{2})|(\d{3})|['"tbrnfv0/\\])/g;


Nit: let's rename this to jsEscapeRegex? Only classes start from the upper-case letter usually.

Sure, whatever you prefer. :)

Octal escape sequences starting with 0 should be expanded before the usual escape sequences to avoid ambiguity with the `\0` escape. Also, accidental double unescaping, e.g. `\\x41 → \x41 → A` needs to be avoided. Fixing this requires using a single `.replace()` call per input string.

coveralls · 2016-12-25T18:21:46Z

Coverage remained the same at 100.0% when pulling 5ed0571 on addaleax:fix-octals into 56dc643 on iamakulov:master.

coveralls · 2016-12-25T18:24:10Z

Coverage remained the same at 100.0% when pulling 8862230 on addaleax:fix-octals into 56dc643 on iamakulov:master.

Simplify: duplicate the `u` part between the first and the second alternative. This reduces the regex depth.

`\/` is not a valid JS escape sequence.

coveralls · 2016-12-25T18:43:02Z

Coverage remained the same at 100.0% when pulling 1e7a605 on addaleax:fix-octals into 56dc643 on iamakulov:master.

coveralls · 2016-12-25T18:43:02Z

Coverage remained the same at 100.0% when pulling 1e7a605 on addaleax:fix-octals into 56dc643 on iamakulov:master.

coveralls · 2016-12-25T18:45:43Z

Coverage remained the same at 100.0% when pulling 5fbb4d0 on addaleax:fix-octals into 56dc643 on iamakulov:master.

coveralls · 2016-12-25T18:45:44Z

Coverage remained the same at 100.0% when pulling 5fbb4d0 on addaleax:fix-octals into 56dc643 on iamakulov:master.

* Swap `varHex` and `longHex` because it’s how they actually correspond to the regex * Reorder `if`s to have the same order as the parameters have * For uniformity, add the `specialCharacter` param as there’s now a corresponding alternative in the regex, and alias `match` with `__` as it’s not necessary anymore

iamakulov · 2016-12-25T18:48:12Z

Cool, thanks for the updates! I also altered the code slightly (added docs and refactored a bit). Let’s wait for the tests to finish, and I’m merging it.

coveralls · 2016-12-25T18:50:13Z

Coverage remained the same at 100.0% when pulling ef9b5b3 on addaleax:fix-octals into 56dc643 on iamakulov:master.

addaleax · 2016-12-25T18:55:32Z

Cool, thanks!

(Btw, the reason I included / there is that I was looking at regexes themselves, where \/ is valid – but of course it’s your call to decide what this module is for ;))

iamakulov · 2016-12-25T19:06:46Z

Btw, the reason I included / there is that I was looking at regexes themselves, where \/ is valid

Okay, I should’ve asked you, yeah :D Thanks for the explanation.

At the moment, I dedicate this module to parsing only string escape sequences. There’re two points that keep me from adding / back:

There’re much more regex escape sequences that just \/: it’s \., \d, etc. If we add regex support, we must support all of them.
I can’t quite imagine a real task that involves unescaping Regex escape sequences. If you have a real need to parse them, please share your task! It’ll really help me consider this feature.

Anyway, unescape-js@1.0.7 is here. Thank you for your changes!

addaleax · 2016-12-25T19:12:57Z

There’re much more regex escape sequences that just \/: it’s \., \d, etc. If we add regex support, we must support all of them.

Well, \/ is kind of special because it’s not special – it’s parsed the same way it is parsed when it’s inside a string, e.g. '\/' === '/'. ;)

I can’t quite imagine a real task that involves unescaping Regex escape sequences. If you have a real need to parse them, please share your task! It’ll really help me consider this feature.

Yeah no, just playing around with stuff. This isn’t a problem for me or anything. :)

iamakulov · 2016-12-25T19:19:55Z

Well, / is kind of special because it’s not special – it’s parsed the same way it is parsed when it’s inside a string, e.g. '/' === '/'. ;)

Good point! I think we can handle this as a generic case: i.e. when an escape sequence is invalid, just return the character it escapes (i.e. \/ → /, \- → -, etc.) Want to submit a PR? :–)

addaleax · 2016-12-25T19:22:19Z

Maybe, but not today… everything’s still a bit christmas-y right now ;)

iamakulov · 2016-12-25T19:22:53Z

Sure. Happy holidays then :–)

addaleax · 2016-12-25T19:26:57Z

Thanks – happy holidays to you too!

addaleax force-pushed the fix-octals branch 2 times, most recently from 8b43d6f to e7195ef Compare December 17, 2016 03:34

iamakulov requested changes Dec 23, 2016

View reviewed changes

addaleax force-pushed the fix-octals branch from e7195ef to 5ed0571 Compare December 25, 2016 18:18

addaleax force-pushed the fix-octals branch from 5ed0571 to 8862230 Compare December 25, 2016 18:19

iamakulov added 2 commits December 25, 2016 21:39

Simplify the regex + document it

1e7a605

Simplify: duplicate the `u` part between the first and the second alternative. This reduces the regex depth.

Remove / from special characters

5fbb4d0

`\/` is not a valid JS escape sequence.

iamakulov approved these changes Dec 25, 2016

View reviewed changes

iamakulov merged commit a881604 into iamakulov:master Dec 25, 2016

addaleax deleted the fix-octals branch December 25, 2016 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix octal escapes starting with 0 #1

Fix octal escapes starting with 0 #1

addaleax commented Dec 14, 2016

coveralls commented Dec 14, 2016 •

edited

Loading

addaleax commented Dec 17, 2016

addaleax commented Dec 17, 2016

coveralls commented Dec 17, 2016 •

edited

Loading

coveralls commented Dec 17, 2016

coveralls commented Dec 17, 2016

coveralls commented Dec 17, 2016

coveralls commented Dec 17, 2016

iamakulov commented Dec 23, 2016

iamakulov left a comment

iamakulov Dec 23, 2016

addaleax Dec 25, 2016

iamakulov Dec 23, 2016

addaleax Dec 25, 2016

iamakulov Dec 23, 2016

addaleax Dec 25, 2016

coveralls commented Dec 25, 2016 •

edited

Loading

coveralls commented Dec 25, 2016 •

edited

Loading

coveralls commented Dec 25, 2016

coveralls commented Dec 25, 2016 •

edited

Loading

coveralls commented Dec 25, 2016

coveralls commented Dec 25, 2016 •

edited

Loading

iamakulov commented Dec 25, 2016

coveralls commented Dec 25, 2016 •

edited

Loading

addaleax commented Dec 25, 2016

iamakulov commented Dec 25, 2016

addaleax commented Dec 25, 2016

iamakulov commented Dec 25, 2016

addaleax commented Dec 25, 2016

iamakulov commented Dec 25, 2016

addaleax commented Dec 25, 2016

		@@ -0,0 +1 @@
		{"/home/sqrt/src/unescape-js/dist/index.js":{"path":"/home/sqrt/src/unescape-js/dist/index.js","s":{"1":1,"2":1,"3":1,"4":1,"5":1,"6":5,"7":1,"8":2,"9":1,"10":18,"11":18,"12":2,"13":16,"14":3,"15":2,"16":1,"17":13,"18":2,"19":11,"20":1},"b":{"1":[2,16],"2":[3,13],"3":[2,1],"4":[2,11]},"f":{"1":5,"2":2,"3":18,"4":18},"fnMap":{"1":{"name":"fromHex","line":24,"loc":{"start":{"line":24,"column":14},"end":{"line":24,"column":36}}},"2":{"name":"fromOct","line":27,"loc":{"start":{"line":27,"column":14},"end":{"line":27,"column":36}}},"3":{"name":"(anonymous_3)","line":31,"loc":{"start":{"line":31,"column":18},"end":{"line":31,"column":36}}},"4":{"name":"(anonymous_4)","line":32,"loc":{"start":{"line":32,"column":41},"end":{"line":32,"column":95}}}},"statementMap":{"1":{"start":{"line":3,"column":0},"end":{"line":5,"column":3}},"2":{"start":{"line":7,"column":0},"end":{"line":7,"column":32}},"3":{"start":{"line":9,"column":0},"end":{"line":9,"column":105}},"4":{"start":{"line":11,"column":0},"end":{"line":22,"column":2}},"5":{"start":{"line":24,"column":0},"end":{"line":26,"column":2}},"6":{"start":{"line":25,"column":4},"end":{"line":25,"column":51}},"7":{"start":{"line":27,"column":0},"end":{"line":29,"column":2}},"8":{"start":{"line":28,"column":4},"end":{"line":28,"column":50}},"9":{"start":{"line":31,"column":0},"end":{"line":43,"column":2}},"10":{"start":{"line":32,"column":4},"end":{"line":42,"column":7}},"11":{"start":{"line":33,"column":8},"end":{"line":41,"column":9}},"12":{"start":{"line":34,"column":12},"end":{"line":34,"column":37}},"13":{"start":{"line":35,"column":15},"end":{"line":41,"column":9}},"14":{"start":{"line":36,"column":12},"end":{"line":36,"column":86}},"15":{"start":{"line":36,"column":34},"end":{"line":36,"column":57}},"16":{"start":{"line":36,"column":62},"end":{"line":36,"column":86}},"17":{"start":{"line":37,"column":15},"end":{"line":41,"column":9}},"18":{"start":{"line":38,"column":12},"end":{"line":38,"column":34}},"19":{"start":{"line":40,"column":12},"end":{"line":40,"column":47}},"20":{"start":{"line":45,"column":0},"end":{"line":45,"column":36}}},"branchMap":{"1":{"line":33,"type":"if","locations":[{"start":{"line":33,"column":8},"end":{"line":33,"column":8}},{"start":{"line":33,"column":8},"end":{"line":33,"column":8}}]},"2":{"line":35,"type":"if","locations":[{"start":{"line":35,"column":15},"end":{"line":35,"column":15}},{"start":{"line":35,"column":15},"end":{"line":35,"column":15}}]},"3":{"line":36,"type":"if","locations":[{"start":{"line":36,"column":12},"end":{"line":36,"column":12}},{"start":{"line":36,"column":12},"end":{"line":36,"column":12}}]},"4":{"line":37,"type":"if","locations":[{"start":{"line":37,"column":15},"end":{"line":37,"column":15}},{"start":{"line":37,"column":15},"end":{"line":37,"column":15}}]}}}}

Fix octal escapes starting with 0 #1

Fix octal escapes starting with 0 #1

Conversation

addaleax commented Dec 14, 2016

coveralls commented Dec 14, 2016 • edited Loading

addaleax commented Dec 17, 2016

addaleax commented Dec 17, 2016

coveralls commented Dec 17, 2016 • edited Loading

coveralls commented Dec 17, 2016

coveralls commented Dec 17, 2016

coveralls commented Dec 17, 2016

coveralls commented Dec 17, 2016

iamakulov commented Dec 23, 2016

iamakulov left a comment

Choose a reason for hiding this comment

iamakulov Dec 23, 2016

Choose a reason for hiding this comment

addaleax Dec 25, 2016

Choose a reason for hiding this comment

iamakulov Dec 23, 2016

Choose a reason for hiding this comment

addaleax Dec 25, 2016

Choose a reason for hiding this comment

iamakulov Dec 23, 2016

Choose a reason for hiding this comment

addaleax Dec 25, 2016

Choose a reason for hiding this comment

coveralls commented Dec 25, 2016 • edited Loading

coveralls commented Dec 25, 2016 • edited Loading

coveralls commented Dec 25, 2016

coveralls commented Dec 25, 2016 • edited Loading

coveralls commented Dec 25, 2016

coveralls commented Dec 25, 2016 • edited Loading

iamakulov commented Dec 25, 2016

coveralls commented Dec 25, 2016 • edited Loading

addaleax commented Dec 25, 2016

iamakulov commented Dec 25, 2016

addaleax commented Dec 25, 2016

iamakulov commented Dec 25, 2016

addaleax commented Dec 25, 2016

iamakulov commented Dec 25, 2016

addaleax commented Dec 25, 2016

coveralls commented Dec 14, 2016 •

edited

Loading

coveralls commented Dec 17, 2016 •

edited

Loading

coveralls commented Dec 25, 2016 •

edited

Loading

coveralls commented Dec 25, 2016 •

edited

Loading

coveralls commented Dec 25, 2016 •

edited

Loading

coveralls commented Dec 25, 2016 •

edited

Loading

coveralls commented Dec 25, 2016 •

edited

Loading