Fix other escape strings from JS #81

bzz · 2019-03-29T19:04:28Z

Fixes the rest of the #75 - case of check: key "value": invalid syntax ("\ ")"

Details of the applied solution are described in #81 (comment)

creachadair

Could you please expand the description before merging to say what cases this PR is fixing?

This looks fine so far, but since it's marked WIP I'll hold off on approving till you're ready.

driver/normalizer/strconv_test.go

fixtures/string-literal.js

bzz · 2019-04-02T13:46:41Z

Rebased on laster master and early feedback amended.

Right now it's still WIP as it only contains the reproducible example + tests but no a fix yet.

Signed-off-by: Alexander Bezzubov <[email protected]>

bzz · 2019-04-09T13:30:24Z

rebased on latest master

bzz · 2019-04-09T15:02:54Z

As we now have discovered many edge cases where escape sequence handing in the native driver is very different from Go - current approach from #64 does not seem to scale well, so a proposed solution that we discussed with @dennwc yesterday is:

in native/annotated mode: always take un-normalized (or quoted) value as token (includes starting \", \' and \n, etc in content)
in semantic mode, always take normalized (or unquoted) value as token (does not include those characters)

That would allow us to avoid re-implementing complex JS escape sequence handling on the Go side.

Although this seems to be consistent with initial intent #32 (comment) and seems to be the case for other drivers, I'm not sure how would that affect client's expectations (e.g see #32 and esp bblfsh/sdk#291) and want all of us to be on board, so \cc @creachadair and @dennwc

creachadair · 2019-04-09T15:40:35Z

in native/annotated mode: always take un-normalized value as token (includes \", \', etc)

in semantic mode, always take normalized value as toke (does not include such characters)

That would allow us to avoid re-implementing complex JS escape sequence handling on the Go side.

That seems reasonable to me. I also like about it that its easy to understand which to expect: If you're upstream of canonicalization, you get the ambient encoding, downstream you get the normalized form.

Does this mean, though, that we will have to explicitly define the string encoding rules we use inside the canonical AST? (I didn't find it anywhere, and it looks like right now we implicitly assume Go's rules).

bzz · 2019-04-10T19:23:25Z

I guess that is something I'm trying to understand here as well.

After a series of experiments, I learned that go, java, bash, cpp, csharp are the only drivers where the original quotas used for literals are preserved in annotated mode.

php and python do not preserve that information and thus 'b\ncq' shows up as "@token":"b\ncq","@type":"Str"

bzz · 2019-04-10T20:15:11Z

The format below is:

string literal
and it's represenation in

annotated
semantc

modes.

go

"b\nc"

"@token":"\"b\\nc\""
"@type":"uast:String","Format":"","Value":"b\nc"

java

"b\nc"

"@token":"\"b\\nc\""
"@type":"uast:String","Format":"","Value":"b\nc"

javascript

"b\nc"

"@token":"\"b\\nc\""
"@type":"uast:String","Format":"","Value":"b\nc"

'b\ncq'

"@token":"'b\\ncq'"
"@type":"uast:String","Format":"single","Value":"b\ncq"

bash

"b\nc"

"@token":"\"b\\nc\"","@type":"string","children":["@role":["Expression","String","Literal"],"@token":"b\\nc","@type":"string_content"]
"@type":"uast:String","Format":"","Value":"b\\nc"

'b\ncq'

"@token":"'b\\ncq'","@type":"unevaluated_string2","children":[]
"@type":"uast:String","Format":"","Value":"'b\\ncq'"

cpp

"b\nc" (not sure yet, why is it twice :/ steps to reproduce in details)

"@token":"\"b\\nc\"","@type":"CPPASTLiteralExpression","ExpressionType":"const char [4]","ExpressionValueCategory":"LVALUE","IsLValue":true,"kind":"string_literal"
"@type":"uast:String","Format":"","Value":"b\nc"

"@token":"\"b\\nc\"","@type":"CPPASTLiteralExpression","ExpressionType":"const char [4]","ExpressionValueCategory":"LVALUE","IsLValue":true,"kind":"string_literal"
"@type":"uast:String","Format":"","Value":"b\nc"

const char* b = 'b';
const char* bc = "b\nc";

 bblfsh-cli -o json -l cpp -m annotated \
    -q "//*[@role='String' and @role='Literal']" cpp.cpp \
    | jq -c '.[] | del(."@pos") | del(.Token."@pos") | del(."@role")'

 bblfsh-cli -o json -l cpp -m semantic \
    -q '//uast:String' cpp.cpp \
   | jq -c '.[] | del(."@pos")'

csharp

"b\nc"

"@type":"StringLiteralExpression","IsMissing":false,"IsStructuredTrivia":false,"Token":{"@role":["Literal","String"],"@type":"StringLiteralToken","IsMissing":false,"Text":"\"b\\nc\"","Value":"b\nc","ValueText":"b\nc"}

"@type":"uast:String","Format":"","Value":"b\nc"

php

"b\nc"

"@token":"b\nc","@type":"Scalar_String","attributes":{"kind":2}
"@type":"uast:String","Format":"","Value":"b\nc"

'b\ncq'

"@token":"b\\ncq","@type":"Scalar_String","attributes":{"kind":1}
"@type":"uast:String","Format":"raw","Value":"b\\ncq"

python

"b\nc"

"@token":"b\nc","@type":"Str"
"@type":"uast:String","Format":"","Value":"b\nc"

'b\ncq'

"@token":"b\ncq","@type":"Str"
"@type":"uast:String","Format":"","Value":"b\ncq"

dennwc · 2019-04-10T20:38:12Z

Semantic in Bash is incorrect (should be b\ncq)
Annotated in PHP and Python is incorrect (should be \"b\\ncq\" and 'b\\ncq')

Everything else looks good.

In Annotated the string should match the source file (including quotes). In Semantic there should be no quotes and the string should be unescaped according to the language rules (same as if you print it from that language).

bzz · 2019-04-11T13:40:06Z

I'm using this as kind-of umbrella issue for these type of problems, so ~~will create~~ new appropriate issues for Bash, PHP and Python were created.

So, the same approach will be used as solution for:

semantic: string literal escape sequence handing java-driver#111
semantic: string literal escape sequence handing cpp-driver#35 check: key "LiteralValue": invalid syntax ("\0")"
javascript-driver (this one)

The proposed solution:

keep annotated mode as it is now
in semantic mode, instead of unquoting on Go side the value from annotated mode (leads to these bugs), we'll just use a different AST node that already provides a normalized by a native driver string, according the language specific rules.

Fixes bblfsh#75 Annotated: - New field added to the string literals `normValue` It is used as value only in Semnatic mode. Explicit Go-side strconv between annotated and semantic is not neeed this way. Signed-off-by: Alexander Bezzubov <[email protected]>

Signed-off-by: Alexander Bezzubov <[email protected]>

bzz · 2019-04-15T13:58:25Z

Fix applied in 683324b :

in Annotated, rawValue is now preserved as normValue (contains normalised string, previously discarded)
in Semantic, format: single is kept the same way but normValue is used instead of manual unquoting

All fixtures updated in fbed224.
@dennwc @creachadair ci is great and it's ready for review now.

dennwc

One small change required but looks good otherwise.

driver/normalizer/normalizer.go

bzz · 2019-04-17T11:33:21Z

All feedback addressed in fc57c53 - could you please give it a final glance, @dennwc ?

dennwc

Yes, exactly what I had in mind, thanks!

driver/normalizer/normalizer.go

Signed-off-by: Alexander Bezzubov <[email protected]>

bzz · 2019-04-18T11:45:41Z

Thank you every one for reviews! Merging after CI is green

bzz self-assigned this Mar 29, 2019

creachadair reviewed Mar 29, 2019

View reviewed changes

driver/normalizer/strconv_test.go Outdated Show resolved Hide resolved

fixtures/string-literal.js Outdated Show resolved Hide resolved

bzz force-pushed the fix/other-escapes-75 branch from 758380a to e5d8d05 Compare April 2, 2019 13:45

bzz added 2 commits April 9, 2019 15:29

sem: tests + fixtures for other escapes

db1786f

Signed-off-by: Alexander Bezzubov <[email protected]>

sem: tests+fixtures for more failing escape seq

13d607f

Signed-off-by: Alexander Bezzubov <[email protected]>

bzz force-pushed the fix/other-escapes-75 branch from 5c17858 to 13d607f Compare April 9, 2019 13:29

bzz added 2 commits April 15, 2019 15:50

test: update all fixtures \w extra node in string literal

fbed224

Signed-off-by: Alexander Bezzubov <[email protected]>

bzz changed the title ~~[WIP] Fix other escape strings from JS~~ Fix other escape strings from JS Apr 15, 2019

bzz requested review from dennwc and creachadair April 15, 2019 13:54

creachadair approved these changes Apr 15, 2019

View reviewed changes

bzz mentioned this pull request Apr 16, 2019

Fix string literal escapes bblfsh/java-driver#115

Merged

dennwc approved these changes Apr 16, 2019

View reviewed changes

driver/normalizer/normalizer.go Outdated Show resolved Hide resolved

dennwc approved these changes Apr 17, 2019

View reviewed changes

driver/normalizer/normalizer.go Outdated Show resolved Hide resolved

sem: simplify custom Op following code review

f20fe64

Signed-off-by: Alexander Bezzubov <[email protected]>

bzz force-pushed the fix/other-escapes-75 branch from fc57c53 to f20fe64 Compare April 18, 2019 11:44

bzz merged commit 9470950 into bblfsh:master Apr 18, 2019

bzz deleted the fix/other-escapes-75 branch April 18, 2019 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix other escape strings from JS #81

Fix other escape strings from JS #81

bzz commented Mar 29, 2019 •

edited

Loading

creachadair left a comment

bzz commented Apr 2, 2019

bzz commented Apr 9, 2019

bzz commented Apr 9, 2019 •

edited

Loading

creachadair commented Apr 9, 2019

bzz commented Apr 10, 2019 •

edited

Loading

bzz commented Apr 10, 2019

dennwc commented Apr 10, 2019 •

edited

Loading

bzz commented Apr 11, 2019 •

edited

Loading

bzz commented Apr 15, 2019

dennwc left a comment

bzz commented Apr 17, 2019

dennwc left a comment

bzz commented Apr 18, 2019

Fix other escape strings from JS #81

Fix other escape strings from JS #81

Conversation

bzz commented Mar 29, 2019 • edited Loading

creachadair left a comment

Choose a reason for hiding this comment

bzz commented Apr 2, 2019

bzz commented Apr 9, 2019

bzz commented Apr 9, 2019 • edited Loading

creachadair commented Apr 9, 2019

bzz commented Apr 10, 2019 • edited Loading

bzz commented Apr 10, 2019

go

java

javascript

bash

cpp

csharp

php

python

dennwc commented Apr 10, 2019 • edited Loading

bzz commented Apr 11, 2019 • edited Loading

bzz commented Apr 15, 2019

dennwc left a comment

Choose a reason for hiding this comment

bzz commented Apr 17, 2019

dennwc left a comment

Choose a reason for hiding this comment

bzz commented Apr 18, 2019

bzz commented Mar 29, 2019 •

edited

Loading

bzz commented Apr 9, 2019 •

edited

Loading

bzz commented Apr 10, 2019 •

edited

Loading

dennwc commented Apr 10, 2019 •

edited

Loading

bzz commented Apr 11, 2019 •

edited

Loading