Repository: amzn/ion-go
Pull request #66 (2020-06-12 18:16:41)

Fix underscore in binary, Int and hex numbers
This is to fix invalid placement of underscore in numbers:

- Multiple underscores: ```12___3```
- AfterRadixPrefix: ```0x_76```
- BeforeDecimalPoint: ```12_.44```
- Trailing underscores: ```0b1011_```



codecov-commenter: # [Codecov](https://codecov.io/gh/amzn/ion-go/pull/66?src=pr&el=h1) Report
> Merging [#66](https://codecov.io/gh/amzn/ion-go/pull/66?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-go/commit/82b4807fba6db435d6ffca0b304c72ad015080ef&el=desc) will **increase** coverage by `0.10%`.
> The diff coverage is `53.84%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-go/pull/66/graphs/tree.svg?width=650&height=150&src=pr&token=OBDT1TNoNE)](https://codecov.io/gh/amzn/ion-go/pull/66?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   76.17%   76.27%   +0.10%     
==========================================
  Files          22       22              
  Lines        4625     4637      +12     
==========================================
+ Hits         3523     3537      +14     
+ Misses        679      678       -1     
+ Partials      423      422       -1     
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-go/pull/66?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [ion/tokenizer.go](https://codecov.io/gh/amzn/ion-go/pull/66/diff?src=pr&el=tree#diff-aW9uL3Rva2VuaXplci5nbw==) | `80.00% <53.84%> (+0.31%)` | :arrow_up: |
| [ion/textreader.go](https://codecov.io/gh/amzn/ion-go/pull/66/diff?src=pr&el=tree#diff-aW9uL3RleHRyZWFkZXIuZ28=) | `86.14% <0.00%> (+0.50%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-go/pull/66?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-go/pull/66?src=pr&el=footer). Last update [82b4807...74c4cf0](https://codecov.io/gh/amzn/ion-go/pull/66?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

zslayton: Looks good. 👍 Could you merge from `master` and resolve the conflicts in a new commit? Then I can squash+merge.

Patch:
diff --git a/ion/skipper.go b/ion/skipper.go
index cf74ebb..ac58d4c 100644
--- a/ion/skipper.go
+++ b/ion/skipper.go
@@ -185,7 +185,7 @@ func (t *tokenizer) skipHex() (int, error) {
 	return t.skipRadix(isX, isHexDigit)
 }
 
-func (t *tokenizer) skipRadix(pok, dok matcher) (int, error) {
+func (t *tokenizer) skipRadix(isRadixMarker, isValidForRadix matcher) (int, error) {
 	c, err := t.read()
 	if err != nil {
 		return 0, err
@@ -201,7 +201,7 @@ func (t *tokenizer) skipRadix(pok, dok matcher) (int, error) {
 	if c != '0' {
 		return 0, t.invalidChar(c)
 	}
-	if err = t.expect(pok); err != nil {
+	if err = t.expect(isRadixMarker); err != nil {
 		return 0, err
 	}
 
@@ -210,7 +210,7 @@ func (t *tokenizer) skipRadix(pok, dok matcher) (int, error) {
 		if err != nil {
 			return 0, err
 		}
-		if !dok(c) {
+		if !isValidForRadix(c) {
 			break
 		}
 	}
diff --git a/ion/tokenizer.go b/ion/tokenizer.go
index 6b5888c..d8d4b08 100644
--- a/ion/tokenizer.go
+++ b/ion/tokenizer.go
@@ -725,7 +725,7 @@ func (t *tokenizer) readHex() (string, error) {
 	return t.readRadix(isX, isHexDigit)
 }
 
-func (t *tokenizer) readRadix(pok, dok matcher) (string, error) {
+func (t *tokenizer) readRadix(isRadixMarker, isValidForRadix matcher) (string, error) {
 	w := strings.Builder{}
 
 	c, err := t.read()
@@ -750,12 +750,20 @@ func (t *tokenizer) readRadix(pok, dok matcher) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	if !pok(c) {
+	if !isRadixMarker(c) {
 		return "", t.invalidChar(c)
 	}
 	w.WriteByte(byte(c))
 
-	c, err = t.readRadixDigits(dok, &w)
+	// At this point we have either 0x or 0b, and it cannot be followed by _
+	nextChar, err2 := t.peek()
+	if err2 != nil {
+		return "", err
+	}
+	if nextChar == '_' {
+		return "", t.invalidChar(c)
+	}
+	c, err = t.readRadixDigits(isValidForRadix, &w)
 	if err != nil {
 		return "", err
 	}
@@ -772,7 +780,7 @@ func (t *tokenizer) readRadix(pok, dok matcher) (string, error) {
 	return w.String(), nil
 }
 
-func (t *tokenizer) readRadixDigits(dok matcher, w io.ByteWriter) (int, error) {
+func (t *tokenizer) readRadixDigits(isValidForRadix matcher, w io.ByteWriter) (int, error) {
 	var c int
 	var err error
 
@@ -782,12 +790,22 @@ func (t *tokenizer) readRadixDigits(dok matcher, w io.ByteWriter) (int, error) {
 			return 0, err
 		}
 		if c == '_' {
+			nextChar, err := t.peek()
+			if err != nil {
+				return 0, err
+			}
+			if !isValidForRadix(nextChar) {
+				return 0, t.invalidChar(c)
+			}
 			continue
 		}
-		if !dok(c) {
+		if !isValidForRadix(c) {
 			return c, nil
 		}
-		w.WriteByte(byte(c))
+		err := w.WriteByte(byte(c))
+		if err != nil {
+			return 0, err
+		}
 	}
 }
 


Test patch:
diff --git a/ion/integration_test.go b/ion/integration_test.go
index 4dc549f..daf975a 100644
--- a/ion/integration_test.go
+++ b/ion/integration_test.go
@@ -144,9 +144,6 @@ var malformedIonsSkipList = []string{
 	"annotationNested.10n",
 	"annotationSymbolIDUnmapped.10n",
 	"annotationSymbolIDUnmapped.ion",
-	"binaryIntWithMultipleUnderscores.ion",
-	"binaryIntWithTrailingUnderscore.ion",
-	"binaryIntWithUnderscoreAfterRadixPrefix.ion",
 	"blobLenTooLarge.10n",
 	"clobLenTooLarge.10n",
 	"clobWithLongLiteralBlockCommentAtEnd.ion",
@@ -163,17 +160,10 @@ var malformedIonsSkipList = []string{
 	"clob_u00FF.ion",
 	"clob_u01FF.ion",
 	"decimalLenTooLarge.10n",
-	"decimalWithMultipleUnderscores.ion",
-	"decimalWithTrailingUnderscore.ion",
-	"decimalWithUnderscoreBeforeDecimalPoint.ion",
 	"emptyAnnotatedInt.10n",
 	"fieldNameSymbolIDUnmapped.10n",
 	"fieldNameSymbolIDUnmapped.ion",
-	"hexIntWithMultipleUnderscores.ion",
-	"hexIntWithTrailingUnderscore.ion",
-	"hexIntWithUnderscoreAfterRadixPrefix.ion",
-	"intWithMultipleUnderscores.ion",
-	"intWithTrailingUnderscore.ion",
+	"floatLenTooLarge.10n",
 	"invalidVersionMarker_ion_0_0.ion",
 	"invalidVersionMarker_ion_1234_0.ion",
 	"invalidVersionMarker_ion_1_1.ion",