Repository: amzn/ion-go (go)

Issue:
Fix Unicode and Hex Escape Parsing in String/Symbol/CLOB
As per #17, there appears to be an issue around escape processing for Unicode escapes (i.e. `\uHHHH`, `\UHHHHHHHH`, and `\xHH`), we should make sure this is working correctly in parser, because for at least CLOBs, `\xHH` does not appear to process correctly when parsing `clobNewlines.ion` in the test vectors.

Pull request description:
Fixes String/Symbol hex escape processing.
* Removes `unquoted` field and use of `strconv.Unquote` from `String`.
* Unifies `\x`, `\u`, and `\U` processing in `parse_text_simple.go`
  for CLOB and String/Symbol.

Incremental improvement towards #17.  There is a bit more DRY than
I'd prefer in the String/Symbol switch statements, but I think it
is important to get these fixes out and address that more as a
follow-up for the lexer/parser refactor.

Depends on #19

Fixes #18

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.


Patch:
diff --git a/ion/parse_text.go b/ion/parse_text.go
index 695872a..889a7c3 100644
--- a/ion/parse_text.go
+++ b/ion/parse_text.go
@@ -22,8 +22,8 @@ import (
 	"io/ioutil"
 	"runtime"
 
-	"github.com/pkg/errors"
 	"github.com/amzn/ion-go/internal/lex"
+	"github.com/pkg/errors"
 )
 
 // ParseText parses all of the bytes from the given Reader into an
@@ -173,7 +173,7 @@ func (p *parser) parseValue(allowOperator bool) Value {
 		case lex.IonSExpStart:
 			return p.parseSExpression(annotations)
 		case lex.IonString:
-			return String{annotations: annotations, text: doStringReplacements(p.next().Val)}
+			return String{annotations: annotations, text: p.doStringReplacements(p.next().Val)}
 		case lex.IonStringLong:
 			return p.parseLongString(annotations)
 		case lex.IonStructStart:
diff --git a/ion/parse_text_simple.go b/ion/parse_text_simple.go
index 34c990e..2fd73a7 100644
--- a/ion/parse_text_simple.go
+++ b/ion/parse_text_simple.go
@@ -20,6 +20,7 @@ import (
 	"github.com/amzn/ion-go/internal/lex"
 	"math"
 	"strconv"
+	"unicode/utf8"
 )
 
 // This file contains text parsers for Null, Padding, Bool, Symbol, String, Blob, and Clob.
@@ -35,13 +36,13 @@ func (p *parser) parseBinary(annotations []Symbol) Value {
 	case lex.IonBlob:
 		value = Blob{annotations: annotations, text: removeAny(item.Val, []byte(" \t\n\r\f\v"))}
 	case lex.IonClobShort:
-		value = Clob{annotations: annotations, text: doClobReplacements(item.Val)}
+		value = Clob{annotations: annotations, text: p.doClobReplacements(item.Val)}
 	case lex.IonClobLong:
 		text := item.Val
 		for peek := p.peekNonComment(); peek.Type == lex.IonClobLong; peek = p.peekNonComment() {
 			text = append(text, p.next().Val...)
 		}
-		value = Clob{annotations: annotations, text: doClobReplacements(text)}
+		value = Clob{annotations: annotations, text: p.doClobReplacements(text)}
 	default:
 		p.panicf("expected a blob or clob but found %q", item)
 	}
@@ -56,15 +57,53 @@ func (p *parser) parseBinary(annotations []Symbol) Value {
 func (p *parser) parseLongString(annotations []Symbol) String {
 	text := []byte{}
 	for item := p.peekNonComment(); item.Type == lex.IonStringLong; item = p.peekNonComment() {
-		text = append(text, doStringReplacements(p.next().Val)...)
+		text = append(text, p.doStringReplacements(p.next().Val)...)
 	}
 
 	return String{annotations: annotations, text: text}
 }
 
+// decodeHex takes a slice that is the input buffer and decodes it into a slice containing UTF-8 code units.
+// The start parameter is the index in the slice start of a hex-encoded rune to decode.
+// The escapeLen parameter is the length of the escape to decode and must be a length
+// that is a power of two, no longer than size of uint32, and within the length of the input slice.
+func (p *parser) decodeHex(input []byte, start int, hexLen int) []byte {
+	// Length must be a power of two and no larger that size of uint32.
+	if hexLen <= 0 || hexLen > 8 || hexLen%2 != 0 {
+		// calling code must give us a proper slice...
+		p.panicf("hex escape is invalid length (%d)", hexLen)
+	}
+	// Construct a working slice to decode with.
+	inLen := len(input)
+	if start < 0 || start >= inLen {
+		p.panicf("start of hex escape (%d) is negative or greater than or equal to input length (%d)", start, inLen)
+	}
+	end := start + hexLen
+	if end > inLen {
+		p.panicf("end of hex escape (%d) is greater than input length (%d)", end, inLen)
+	}
+	hex := input[start:end]
+
+	// Decode the hex string into a UTF-32 scalar.
+	buf := make([]byte, utf8.UTFMax)
+	var cp uint64
+	for i := 0; i < hexLen; i += 2 {
+		octet, errParse := strconv.ParseUint(string(hex[i:i+2]), 16, 8)
+		if errParse != nil {
+			p.panicf("invalid hex escape (%q) was not caught by lexer: %v", hex, errParse)
+		}
+		cp = (cp << 8) | octet
+	}
+
+	// Now serialize back as UTF-8 code units.
+	encodeLen := utf8.EncodeRune(buf, rune(cp))
+
+	return buf[0:encodeLen]
+}
+
 // doStringReplacements converts escaped characters into their equivalent
 // character while handling cases involving \r.
-func doStringReplacements(str []byte) []byte {
+func (p *parser) doStringReplacements(str []byte) []byte {
 	strLen := len(str)
 	ret := make([]byte, 0, strLen)
 	for index := 0; index < strLen; index++ {
@@ -101,6 +140,21 @@ func doStringReplacements(str []byte) []byte {
 			case '\'', '"', '\\':
 				ret = append(ret, next)
 				index++
+			case 'x':
+				index += 2
+				data := p.decodeHex(str, index, 2)
+				index += 1
+				ret = append(ret, data...)
+			case 'u':
+				index += 2
+				data := p.decodeHex(str, index, 4)
+				index += 3
+				ret = append(ret, data...)
+			case 'U':
+				index += 2
+				data := p.decodeHex(str, index, 8)
+				index += 7
+				ret = append(ret, data...)
 			default:
 				// Don't have anything special to do with the next character, so
 				// just add the current character and let the next one get added
@@ -116,7 +170,7 @@ func doStringReplacements(str []byte) []byte {
 }
 
 // doClobReplacements is like doStringReplacements but is restricted to escapes that CLOBs have.
-func doClobReplacements(str []byte) []byte {
+func (p *parser) doClobReplacements(str []byte) []byte {
 	strLen := len(str)
 	ret := make([]byte, 0, strLen)
 	for index := 0; index < strLen; index++ {
@@ -154,11 +208,10 @@ func doClobReplacements(str []byte) []byte {
 				ret = append(ret, next)
 				index++
 			case 'x':
-				// Decode the hex sequence
-				index += 2
-				octet, _ := strconv.ParseUint(string(str[index:index+2]), 16, 8)
 				index += 2
-				ret = append(ret, byte(octet))
+				data := p.decodeHex(str, index, 2)
+				index += 1
+				ret = append(ret, data...)
 			default:
 				// Don't have anything special to do with the next character, so
 				// just add the current character and let the next one get added
@@ -270,5 +323,5 @@ func (p *parser) parseSymbol(annotations []Symbol, allowOperator bool) Value {
 	// TODO: Figure out why the bytes in item.Val get overwritten when we don't
 	//       make an explicit copy of the data.
 	//return Symbol{annotations: annotations, quoted: quoted, text: doStringReplacements(item.Val)}
-	return Symbol{annotations: annotations, quoted: quoted, text: append([]byte{}, doStringReplacements(item.Val)...)}
+	return Symbol{annotations: annotations, quoted: quoted, text: append([]byte{}, p.doStringReplacements(item.Val)...)}
 }
diff --git a/ion/types_character.go b/ion/types_character.go
index 73f2623..25cf3de 100644
--- a/ion/types_character.go
+++ b/ion/types_character.go
@@ -15,34 +15,16 @@
 
 package ion
 
-import (
-	"strconv"
-)
-
 // This file contains the string-like types: String and Symbol.
 
 // String is a unicode text literal of arbitrary length.
 type String struct {
 	annotations []Symbol
 	text        []byte
-	unquoted    string
 }
 
 func (s String) Value() string {
-	if len(s.text) == 0 || len(s.unquoted) != 0 {
-		return s.unquoted
-	}
-
-	// Unquote translates escape sequences, e.g. `\u00ff`, into their utf8
-	// normalized value, e.g. "ÿ".  We need to surround the text with double
-	// quotes so that Unquote knows whether to treat it as literal text or not.
-	var err error
-	s.unquoted, err = strconv.Unquote(`"` + string(s.text) + `"`)
-	if err != nil {
-		s.unquoted = string(s.text)
-	}
-
-	return s.unquoted
+	return string(s.text)
 }
 
 // Annotations satisfies Value.


Test patch:
diff --git a/ion/parse_text_test.go b/ion/parse_text_test.go
index 91b3869..1af599d 100644
--- a/ion/parse_text_test.go
+++ b/ion/parse_text_test.go
@@ -45,6 +45,31 @@ func TestParseText(t *testing.T) {
 				String{text: []byte("longstring")},
 			}},
 		},
+		{
+			name: "escaped strings",
+			text: `"H\x48\u0048\U00000048" '''h\x68\u0068\U00000068'''`,
+			expected: &Digest{values: []Value{
+				String{text: []byte("HHHH")},
+				String{text: []byte("hhhh")},
+			}},
+		},
+
+		// Symbols
+
+		{
+			name: "symbol",
+			text: "'short symbol'",
+			expected: &Digest{values: []Value{
+				Symbol{text: []byte("short symbol")},
+			}},
+		},
+		{
+			name: "escaped symbols",
+			text: `'H\x48\u0048\U00000048'`,
+			expected: &Digest{values: []Value{
+				Symbol{text: []byte("HHHH")},
+			}},
+		},
 
 		// Numeric
 
@@ -142,6 +167,14 @@ func TestParseText(t *testing.T) {
 			text:     "{{ '''A\\nB'''\n'''foo''' }}",
 			expected: &Digest{values: []Value{Clob{text: []byte("A\nBfoo")}}},
 		},
+		{
+			name: "escaped clobs",
+			text: `{{"H\x48\x48H"}} {{'''h\x68\x68h'''}}`,
+			expected: &Digest{values: []Value{
+				Clob{text: []byte("HHHH")},
+				Clob{text: []byte("hhhh")},
+			}},
+		},
 
 		// Containers