encoding/asn1: only omit optional elements matching default value.

ASN.1 elements can be optional, and can have a default value.
Traditionally, Go has omitted elements that are optional and that have
the zero value. I believe that's a bug (see [1]).

This change causes an optional element with a default value to only be
omitted when it has that default value. The previous behaviour of
omitting optional, zero elements with no default is retained because
it's used quite a lot and will break things if changed.

[1] https://groups.google.com/d/msg/Golang-nuts/9Ss6o9CW-Yo/KL_V7hFlyOAJ

Fixes #7780.

R=bradfitz

LGTM=bradfitz
R=golang-codereviews, bradfitz, rsc
CC=golang-codereviews, r
https://golang.org/cl/86960045

3 files changed, 37 insertions, 6 deletions

diff --git a/src/pkg/encoding/asn1/asn1.go b/src/pkg/encoding/asn1/asn1.go
index ec7f91c1bb..b06aec3e40 100644
--- a/src/pkg/encoding/asn1/asn1.go
+++ b/src/pkg/encoding/asn1/asn1.go
@@ -822,8 +822,19 @@ func parseField(v reflect.Value, bytes []byte, initOffset int, params fieldParam
 	return
 }
 
+// canHaveDefaultValue reports whether k is a Kind that we will set a default
+// value for. (A signed integer, essentially.)
+func canHaveDefaultValue(k reflect.Kind) bool {
+	switch k {
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		return true
+	}
+
+	return false
+}
+
 // setDefaultValue is used to install a default value, from a tag string, into
-// a Value. It is successful is the field was optional, even if a default value
+// a Value. It is successful if the field was optional, even if a default value
 // wasn't provided or it failed to install it into the Value.
 func setDefaultValue(v reflect.Value, params fieldParameters) (ok bool) {
 	if !params.optional {
@@ -833,9 +844,8 @@ func setDefaultValue(v reflect.Value, params fieldParameters) (ok bool) {
 	if params.defaultValue == nil {
 		return
 	}
-	switch val := v; val.Kind() {
-	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
-		val.SetInt(*params.defaultValue)
+	if canHaveDefaultValue(v.Kind()) {
+		v.SetInt(*params.defaultValue)
 	}
 	return
 }
diff --git a/src/pkg/encoding/asn1/marshal.go b/src/pkg/encoding/asn1/marshal.go
index e26fe59b30..b2f104b4cb 100644
--- a/src/pkg/encoding/asn1/marshal.go
+++ b/src/pkg/encoding/asn1/marshal.go
@@ -513,8 +513,22 @@ func marshalField(out *forkableWriter, v reflect.Value, params fieldParameters)
 		return
 	}
 
-	if params.optional && reflect.DeepEqual(v.Interface(), reflect.Zero(v.Type()).Interface()) {
-		return
+	if params.optional && params.defaultValue != nil && canHaveDefaultValue(v.Kind()) {
+		defaultValue := reflect.New(v.Type()).Elem()
+		defaultValue.SetInt(*params.defaultValue)
+
+		if reflect.DeepEqual(v.Interface(), defaultValue.Interface()) {
+			return
+		}
+	}
+
+	// If no default value is given then the zero value for the type is
+	// assumed to be the default value. This isn't obviously the correct
+	// behaviour, but it's what Go has traditionally done.
+	if params.optional && params.defaultValue == nil {
+		if reflect.DeepEqual(v.Interface(), reflect.Zero(v.Type()).Interface()) {
+			return
+		}
 	}
 
 	if v.Type() == rawValueType {
diff --git a/src/pkg/encoding/asn1/marshal_test.go b/src/pkg/encoding/asn1/marshal_test.go
index a15acbed01..5b0115f28c 100644
--- a/src/pkg/encoding/asn1/marshal_test.go
+++ b/src/pkg/encoding/asn1/marshal_test.go
@@ -58,6 +58,10 @@ type omitEmptyTest struct {
 	A []string `asn1:"omitempty"`
 }
 
+type defaultTest struct {
+	A int `asn1:"optional,default:1"`
+}
+
 type testSET []int
 
 var PST = time.FixedZone("PST", -8*60*60)
@@ -133,6 +137,9 @@ var marshalTests = []marshalTest{
 	{omitEmptyTest{[]string{}}, "3000"},
 	{omitEmptyTest{[]string{"1"}}, "30053003130131"},
 	{"Σ", "0c02cea3"},
+	{defaultTest{0}, "3003020100"},
+	{defaultTest{1}, "3000"},
+	{defaultTest{2}, "3003020102"},
 }
 
 func TestMarshal(t *testing.T) {