Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Handle quoting U+0000 properly #64

Merged
merged 20 commits into from
Mar 13, 2019
Merged

Handle quoting U+0000 properly #64

merged 20 commits into from
Mar 13, 2019

Conversation

bzz
Copy link
Contributor

@bzz bzz commented Mar 7, 2019 •
edited
Loading

Fixes #62

TODOs

  • add high-level fixtures to reproduce Parsing fails on valid JS: string literal with \0 #62
  • add low-level unit test to reproduce the cause - strconv_test.go
  • make TestUnquoteSingle pass:
    modify unquoteSingle, so \0 is handled like escape sequence, resulting in U+0000
  • make TestUnquoteSingleAndQuoteBack pass:
    modify quoteSingle so U+0000 is handled like in JS and results in \0 (instead of Go \x00)
    That is not possible as right now we are loosing information of the type of escape sequence being used in JS (\000, \x00, and \0 - all end up quoted back in Hex format)
    Instead, test fixtures are update to use this canonical form.

bzz added 2 commits March 7, 2019 07:03
@bzz
add tests to highligh null quoting issue
535f594 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: add fixtures to reproduce the failure
399f6ed 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz bzz self-assigned this Mar 7, 2019
@bzz bzz requested a review from dennwc March 7, 2019 06:10
dennwc
dennwc reviewed Mar 7, 2019
View reviewed changes
driver/normalizer/strconv.go Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
bzz added 3 commits March 11, 2019 17:07
@bzz
strconv: porting more details from stdlib
7d784b9 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: refactor to table
9799217 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
sem: fix processing \0 escaped sequence in string literal
a769379 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
Copy link
Contributor Author

bzz commented Mar 11, 2019

Perf test for comparison of 2 impl of unquoting \0 by replacing it with \x00

go test -bench=. ./driver/normalizer
goos: darwin
goarch: amd64
pkg: github.com/bblfsh/javascript-driver/driver/normalizer
BenchmarkReplacingNullEscape_Iterative-4   	 5000000	       303 ns/op	      80 B/op	       2 allocs/op
BenchmarkReplacingNullEscape_Regexp-4      	 1000000	      1089 ns/op	     168 B/op	       7 allocs/op
PASS
ok  	github.com/bblfsh/javascript-driver/driver/normalizer	2.977s

@bzz bzz requested review from dennwc and creachadair March 11, 2019 19:27
@bzz
Copy link
Contributor Author

bzz commented Mar 11, 2019

WIP, but is already open for a feedback

creachadair
creachadair reviewed Mar 11, 2019
View reviewed changes
driver/normalizer/strconv.go Show resolved Hide resolved
driver/normalizer/strconv.go Show resolved Hide resolved
driver/normalizer/strconv.go Outdated Show resolved Hide resolved
bzz added 3 commits March 11, 2019 20:46
@bzz
sem: update semantic annotations after the fix
bda63b1 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
review: replace copy-paste \w stdlib call
631f385 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: refactoring to simplify reverse case
2527d5a 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz bzz requested a review from creachadair March 11, 2019 19:53
@bzz
review: replace strconv.ErrSyntax
ea68ea3 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
creachadair
creachadair reviewed Mar 11, 2019
View reviewed changes
driver/normalizer/strconv.go Show resolved Hide resolved
driver/normalizer/strconv.go Outdated Show resolved Hide resolved
bzz added 2 commits March 12, 2019 13:49
@bzz
review: more tests for corner cases
fc24125 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: adjust for quote/unquote asymmetry
ec16186 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz bzz marked this pull request as ready for review March 12, 2019 16:56
@bzz
Copy link
Contributor Author

bzz commented Mar 12, 2019

Here are the updated benchmark, including simpler impl suggested in #64 (comment)

goos: darwin
goarch: amd64
pkg: github.com/bblfsh/javascript-driver/driver/normalizer
BenchmarkReplacingNullEscape_Iterative-4   	 2000000	       745 ns/op	     144 B/op	       8 allocs/op
BenchmarkReplacingNullEscape_Regexp-4      	  500000	      3334 ns/op	     480 B/op	      27 allocs/op
BenchmarkReplacingNullEscape_Simple-4      	 2000000	       633 ns/op	     112 B/op	       9 allocs/op
PASS
ok  	github.com/bblfsh/javascript-driver/driver/normalizer	5.897s

It is not just simpler, but is even a bit faster! 👏

@bzz
unqoteSingle: switch to simple strings-based impl
8c08ccf 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz bzz requested a review from creachadair March 12, 2019 17:20
@bzz
Copy link
Contributor Author

bzz commented Mar 12, 2019

All feedback addressed, ready for another round.

creachadair
creachadair reviewed Mar 12, 2019
View reviewed changes
driver/normalizer/strconv.go Show resolved Hide resolved
driver/normalizer/strconv.go Show resolved Hide resolved
driver/normalizer/strconv.go Outdated Show resolved Hide resolved
@creachadair @bzz
review: update godoc
c521aea 
Co-Authored-By: bzz <bzz@users.noreply.github.com>
@bzz bzz requested a review from creachadair March 13, 2019 14:19
creachadair
creachadair approved these changes Mar 13, 2019
View reviewed changes
Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor cosmetic comments, otherwise looks good to me!

Thanks!

driver/normalizer/strconv.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
dennwc
dennwc suggested changes Mar 13, 2019
View reviewed changes
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests need some changes :)

driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
bzz added 5 commits March 13, 2019 17:23
@bzz
doc: better wording though review
8659709 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: refactoring to use subtests
5c5255c 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: refactoring benchmark
3f88050 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: drop experimental benchmark
9085dab 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
test: cosmetic - consistent string literal syntax
bfd8c9b 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
Copy link
Contributor Author

bzz commented Mar 13, 2019

@creachadair @dennwc thank you very much for thorough and prompt reviews! All feedback addressed, ready for another round.

@bzz bzz requested a review from dennwc March 13, 2019 17:12
creachadair
creachadair approved these changes Mar 13, 2019
View reviewed changes
Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more suggestion on the comment, but I think this is in good shape.
Thanks for your patience!

driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
@bzz
test: better doc for the canonicalQuoted cases
7f92d74 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
dennwc
dennwc approved these changes Mar 13, 2019
View reviewed changes
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only one small change remains.

driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved
@bzz
test: refactor benchmark, breaking down by testcase
e7a074f 
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
Copy link
Contributor Author

bzz commented Mar 13, 2019 •
edited
Loading

🎉 merging as soon as CI is green.

Thank you everyone for your kind reviews!

@bzz bzz merged commit 4eb5b4c into bblfsh:master Mar 13, 2019
@bzz bzz deleted the fix-62 branch March 13, 2019 18:21
@bzz bzz mentioned this pull request Mar 22, 2019
Closed
3 tasks
@bzz bzz mentioned this pull request Apr 9, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dennwc dennwc dennwc approved these changes

@creachadair creachadair creachadair approved these changes

Assignees

@bzz bzz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parsing fails on valid JS: string literal with \0
3 participants
@bzz @creachadair @dennwc