x/text/internal/colltab: Avoid using Elem.Primary() == 0 in numeric.go

Elem.Primary() == 0 has odd ordering properties, starting at 1 allows 0
  to consistently be ordered before other numbers when non-numeric text
  follows a 0
  Also fixes an issue comparing numbers > 269 characters with
  numbers < 270 characters

Fixes golang/go#25554

Author: lordwelch

collate/collate_test.go

@@ -6,6 +6,7 @@ package collate
 
 import (
 	"bytes"
+	"strings"
 	"testing"
 
 	"golang.org/x/text/internal/colltab"
@@ -474,6 +475,19 @@ func TestNumeric(t *testing.T) {
 		{"A-2", "A-12", -1},
 		{"A-12", "A-2", 1},
 		{"A-0001", "A-1", 0},
+		{"0000-", "1-", -1},
+		{"00001", "1", 0},
+		{"00", "00", 0},
+		{"0", "00", 0},
+		{"00", "0", 0},
+		{"01", "001", 0},
+		{"01", "1", 0},
+		{"1", "01", 0},
+		{"9-A", "0-A", 1},
+		{"99-A", "0-A", 1},
+		{"9-A", "1-A", 1},
+		{"99-A", "1-A", 1},
+		{strings.Repeat("9", 270)+"-A", "1-A", 1},
 	} {
 		if got := c.CompareString(tt.a, tt.b); got != tt.want {
 			t.Errorf("%d: CompareString(%s, %s) = %d; want %d", i, tt.a, tt.b, got, tt.want)

internal/colltab/numeric.go

@@ -224,13 +224,13 @@ func (nc *numberConverter) update(elems []Elem) bool {
 		return false
 	}
 	nc.nDigits++
-	return nc.nDigits < maxDigits
+	return nc.nDigits+1 < maxDigits
 }
 
 // result fills in the length element for the digit sequence and returns the
 // completed collation elements.
 func (nc *numberConverter) result() []Elem {
-	e, _ := MakeElem(nc.nDigits, defaultSecondary, defaultTertiary, 0)
+	e, _ := MakeElem(nc.nDigits+1, defaultSecondary, defaultTertiary, 0)
 	nc.elems[nc.lenIndex] = e
 	return nc.elems
 }

internal/colltab/numeric_test.go

@@ -78,34 +78,36 @@ func TestNumericAppendNext(t *testing.T) {
 		{"a", p(5)},
 		{"klm", p(99)},
 		{"aa", p(5, 5)},
-		{"1", p(120, 1, 101)},
-		{"0", p(120, 0)},
-		{"01", p(120, 1, 101)},
-		{"0001", p(120, 1, 101)},
-		{"10", p(120, 2, 101, 100)},
-		{"99", p(120, 2, 119, 119)},
-		{"9999", p(120, 4, 119, 119, 119, 119)},
-		{"1a", p(120, 1, 101, 5)},
-		{"0b", p(120, 0, 6)},
-		{"01c", p(120, 1, 101, 8, 2)},
-		{"10x", p(120, 2, 101, 100, 200)},
-		{"99y", p(120, 2, 119, 119, 201)},
-		{"9999nop", p(120, 4, 119, 119, 119, 119, 121)},
+		{"1", p(120, 2, 101)},
+		{"0", p(120, 1)},
+		{"00", p(120, 1)},
+		{"01", p(120, 2, 101)},
+		{"0001", p(120, 2, 101)},
+		{"02", p(120, 2, 102)},
+		{"10", p(120, 3, 101, 100)},
+		{"99", p(120, 3, 119, 119)},
+		{"9999", p(120, 5, 119, 119, 119, 119)},
+		{"1a", p(120, 2, 101, 5)},
+		{"0b", p(120, 1, 6)},
+		{"01c", p(120, 2, 101, 8, 2)},
+		{"10x", p(120, 3, 101, 100, 200)},
+		{"99y", p(120, 3, 119, 119, 201)},
+		{"9999nop", p(120, 5, 119, 119, 119, 119, 121)},
 
 		// Allow follow-up collation elements if they have a zero non-primary.
-		{"١٢٩", []Elem{e(120), e(3), e(101), tPlus3, e(102), tPlus3, e(119), tPlus3}},
+		{"١٢٩", []Elem{e(120), e(4), e(101), tPlus3, e(102), tPlus3, e(119), tPlus3}},
 		{
 			"１２９",
 			[]Elem{
-				e(120), e(3),
+				e(120), e(4),
 				e(101, digSec, digTert+1),
 				e(102, digSec, digTert+3),
 				e(119, digSec, digTert+1),
 			},
 		},
 
 		// Ensure AppendNext* adds to the given buffer.
-		{"a10", p(5, 120, 2, 101, 100)},
+		{"a10", p(5, 120, 3, 101, 100)},
 	} {
 		nw := NewNumericWeighter(numWeighter)
 
@@ -137,12 +139,12 @@ func TestNumericOverflow(t *testing.T) {
 
 	got, n := nw.AppendNextString(nil, manyDigits)
 
-	if n != maxDigits {
-		t.Errorf("n: got %d; want %d", n, maxDigits)
+	if n != maxDigits-1 { // A digit is lost because elem.Primary() == 0 has odd ordering properties and is skipped
+		t.Errorf("n: got %d; want %d", n, maxDigits-1)
 	}
 
 	if got[1].Primary() != maxDigits {
-		t.Errorf("primary(e[1]): got %d; want %d", n, maxDigits)
+		t.Errorf("primary(e[1]): got %d; want %d", got[1].Primary(), maxDigits)
 	}
 }