Repository: BurntSushi/toml
Pull request #325 (2021-11-10 21:01:50)

Allow lossless integer to float conversions
According to the specification, 64-bit signed integers should be accepted providing they can be represented losslessly.

At the moment, integers to be converted to a float immediately return an error. This change permits integers and only returns an error if they would overflow the float type they're being converted to as they have no problem being represented losslessly.

In addition, this change returns an error if a float is provided but cannot fit within the float type specified by the struct.

This relates to issue https://github.com/BurntSushi/toml/issues/60, which I believe was incorrectly closed:

> BurntSushi:
> This appears to be working as intended. In TOML, floats are indicated by the presence of a decimal point and at least one trailing 0.

Whilst this statement is true, that concerns the input of a float. I believe an argument can be made that the input here concerns an integer, that just so happens to be unmarsharled into a float. It *can* be represented losslessly. Only integers that cannot be should return an error.

arp242: Thanks for your patch! I need to think a little bit about this change, read through the other issue, and consider things. I'm feeling pretty sick at the moment so give me a week or so. Feel free to ping me if I forget after this.

It would be helpful to add a little bit of detail on what problem this solves. As in, what is currently hard or impossible to do and how this solves it. Unless I missed it, that's not really mentioned in the issue you linked.

I don't think this is really a "TOML specification issue" as such by the way; it's really up to the implementation to decide what can be accepted for which values depending on what makes sense for the specific language, environment, etc.
saracen: > It would be helpful to add a little bit of detail on what problem this solves. As in, what is currently hard or impossible to do and how this solves it. Unless I missed it, that's not really mentioned in the issue you linked.

```golang
type Config struct {
    IdleScaleFactor float64 
}
```

If a user inputs `IdleScaleFactor = 1` they receive the error: `toml: cannot load TOML value of type int64 into a Go float`.

`IdleScaleFactor = 1.0` will however work.

One possible solution to this would be to define a `IntOrFloat` struct with an implemented `UnmarshalTOML`. This approach does work, but for us it's slightly more involved to implement because we have other unmarshalers and would need to further implement the same solution for them too.

Our users are typically of a technical background, so it is likely they'll be able to figure out what the problem is from the error message, but it's a poor experience nevertheless.

The arguments in https://github.com/BurntSushi/toml/issues/60 are the same: For their users, it's unintuitive.

So the problem this solves is a user experience issue for anybody populating config and for developers it lifts the burden 
from them needing to implement a special type.

The argument against supporting this was that an `int` to `float` conversion can lose information and that it was a natural in Go for TOML integers to map to Go integer types and TOML floats to map to Go float types.

> I don't think this is really a "TOML specification issue" as such by the way; it's really up to the implementation to decide what can be accepted for which values depending on what makes sense for the specific language, environment, etc.

I think this slightly touches on both a specification issue and what makes sense for the language.

For Go and other libraries, it's not uncommon to expect an integer to be converted to a float upon being unmarshalled:
- The `encoding/json` package can do it: https://play.golang.org/p/_Ajc2z_Npyz
- The `encoding/xml` library can do it: https://play.golang.org/p/K2FmPiAicA9
- The `flag` package can do it: https://play.golang.org/p/P6rXPM_v2bW

Where this becomes a spec issue is that those other decoders convert the provided integer and can lose information, but the TOML specification makes it clear that an error should be thrown if an integer is provided and information would be lost.

For other conversions, this library already adheres to that rule. A really large number would not fit within a `uint8` and an error is thrown. This PR does just that with integer to float conversions, ensuring that if information would be lost, an error is thrown.

> I'm feeling pretty sick at the moment so give me a week or so. Feel free to ping me if I forget after this.

I'm sorry to hear that, I hope you feel better soon! There's no rush. I'll catch up with you once you're better. Thank you!

arp242: I rebased this branch on master and added a commit with some boring/pedantic stylistic changes I didn't want to bother you with.

I think this mostly looks okay; but I'm not sure we need to check for maximum safe natural numbers; if I specified a float in Go then I'm fine with some (possible) loss of accuracy, right? If you want/need fully accurate representation then you should use an int. It's useful to have this check in some use cases, and probably unwanted in other use cases; if you want such a check, then it's something people should implement themselves.
arp242: e.g. the encoding/json library also doesn't check for this: https://play.golang.org/p/faOdIf91u-J

That second value is inaccurate, but that's just how floats work. I think that check should probably be removed. What do you think?

Patch:
diff --git a/README.md b/README.md
index 64410cf..d472eb5 100644
--- a/README.md
+++ b/README.md
@@ -1,10 +1,6 @@
-## TOML parser and encoder for Go with reflection
-
 TOML stands for Tom's Obvious, Minimal Language. This Go package provides a
 reflection interface similar to Go's standard library `json` and `xml`
-packages. This package also supports the `encoding.TextUnmarshaler` and
-`encoding.TextMarshaler` interfaces so that you can define custom data
-representations. (There is an example of this below.)
+packages.
 
 Compatible with TOML version [v1.0.0](https://toml.io/en/v1.0.0).
 
@@ -16,26 +12,25 @@ v0.4.0`).
 
 This library requires Go 1.13 or newer; install it with:
 
-    $ go get github.com/BurntSushi/toml
+    % go get github.com/BurntSushi/toml@latest
 
 It also comes with a TOML validator CLI tool:
 
-    $ go get github.com/BurntSushi/toml/cmd/tomlv
-    $ tomlv some-toml-file.toml
+    % go install github.com/BurntSushi/toml/cmd/tomlv@latest
+    % tomlv some-toml-file.toml
 
 ### Testing
+This package passes all tests in [toml-test] for both the decoder and the
+encoder.
 
-This package passes all tests in
-[toml-test](https://github.com/BurntSushi/toml-test) for both the decoder
-and the encoder.
+[toml-test]: https://github.com/BurntSushi/toml-test
 
 ### Examples
+This package works similar to how the Go standard library handles XML and JSON.
+Namely, data is loaded into Go values via reflection.
 
-This package works similarly to how the Go standard library handles XML and
-JSON. Namely, data is loaded into Go values via reflection.
-
-For the simplest example, consider some TOML file as just a list of keys
-and values:
+For the simplest example, consider some TOML file as just a list of keys and
+values:
 
 ```toml
 Age = 25
@@ -61,9 +56,8 @@ And then decoded with:
 
 ```go
 var conf Config
-if _, err := toml.Decode(tomlData, &conf); err != nil {
-  // handle error
-}
+err := toml.Decode(tomlData, &conf)
+// handle error
 ```
 
 You can also use struct tags if your struct field name doesn't map to a TOML
@@ -75,15 +69,14 @@ some_key_NAME = "wat"
 
 ```go
 type TOML struct {
-  ObscureKey string `toml:"some_key_NAME"`
+    ObscureKey string `toml:"some_key_NAME"`
 }
 ```
 
 Beware that like other most other decoders **only exported fields** are
 considered when encoding and decoding; private fields are silently ignored.
 
-### Using the `encoding.TextUnmarshaler` interface
-
+### Using the `Marshaler` and `encoding.TextUnmarshaler` interfaces
 Here's an example that automatically parses duration strings into
 `time.Duration` values:
 
@@ -136,7 +129,6 @@ To target TOML specifically you can implement `UnmarshalTOML` TOML interface in
 a similar way.
 
 ### More complex usage
-
 Here's an example of how to load the example from the official spec page:
 
 ```toml
@@ -217,4 +209,3 @@ Note that a case insensitive match will be tried if an exact match can't be
 found.
 
 A working example of the above can be found in `_examples/example.{go,toml}`.
-
diff --git a/decode.go b/decode.go
index c4b6287..dd671af 100644
--- a/decode.go
+++ b/decode.go
@@ -39,6 +39,13 @@ type Primitive struct {
 	context   Key
 }
 
+// The significand precision for float32 and float64 is 24 and 53 bits; this is
+// the range a natural number can be stored in a float without loss of data.
+const (
+	maxSafeFloat32Int = 16777215         // 2^24-1
+	maxSafeFloat64Int = 9007199254740991 // 2^53-1
+)
+
 // PrimitiveDecode is just like the other `Decode*` functions, except it
 // decodes a TOML value that has already been parsed. Valid primitive values
 // can *only* be obtained from values filled by the decoder functions,
@@ -217,9 +224,7 @@ func (md *MetaData) unify(data interface{}, rv reflect.Value) error {
 			return e("unsupported type %s", rv.Type())
 		}
 		return md.unifyAnything(data, rv)
-	case reflect.Float32:
-		fallthrough
-	case reflect.Float64:
+	case reflect.Float32, reflect.Float64:
 		return md.unifyFloat64(data, rv)
 	}
 	return e("unsupported type %s", rv.Kind())
@@ -357,6 +362,9 @@ func (md *MetaData) unifyFloat64(data interface{}, rv reflect.Value) error {
 	if num, ok := data.(float64); ok {
 		switch rv.Kind() {
 		case reflect.Float32:
+			if num < -math.MaxFloat32 || num > math.MaxFloat32 {
+				return e("value %f is out of range for float32", num)
+			}
 			fallthrough
 		case reflect.Float64:
 			rv.SetFloat(num)
@@ -365,6 +373,25 @@ func (md *MetaData) unifyFloat64(data interface{}, rv reflect.Value) error {
 		}
 		return nil
 	}
+
+	if num, ok := data.(int64); ok {
+		switch rv.Kind() {
+		case reflect.Float32:
+			if num < -maxSafeFloat32Int || num > maxSafeFloat32Int {
+				return e("value %d is out of range for float32", num)
+			}
+			fallthrough
+		case reflect.Float64:
+			if num < -maxSafeFloat64Int || num > maxSafeFloat64Int {
+				return e("value %d is out of range for float64", num)
+			}
+			rv.SetFloat(float64(num))
+		default:
+			panic("bug")
+		}
+		return nil
+	}
+
 	return badtype("float", data)
 }
 
@@ -431,6 +458,12 @@ func (md *MetaData) unifyAnything(data interface{}, rv reflect.Value) error {
 func (md *MetaData) unifyText(data interface{}, v encoding.TextUnmarshaler) error {
 	var s string
 	switch sdata := data.(type) {
+	case Marshaler:
+		text, err := sdata.MarshalTOML()
+		if err != nil {
+			return err
+		}
+		s = string(text)
 	case TextMarshaler:
 		text, err := sdata.MarshalText()
 		if err != nil {
diff --git a/encode.go b/encode.go
index 0ea6e9a..da5b337 100644
--- a/encode.go
+++ b/encode.go
@@ -64,13 +64,22 @@ var quotedReplacer = strings.NewReplacer(
 	"\x7f", `\u007f`,
 )
 
+// Marshaler is the interface implemented by types that can marshal themselves
+// into valid TOML.
+type Marshaler interface {
+	MarshalTOML() ([]byte, error)
+}
+
 // Encoder encodes a Go to a TOML document.
 //
 // The mapping between Go values and TOML values should be precisely the same as
-// for the Decode* functions. Similarly, the TextMarshaler interface is
-// supported by encoding the resulting bytes as strings. If you want to write
-// arbitrary binary data then you will need to use something like base64 since
-// TOML does not have any binary types.
+// for the Decode* functions.
+//
+// The toml.Marshaler and encoder.TextMarshaler interfaces are supported to
+// encoding the value as custom TOML.
+//
+// If you want to write arbitrary binary data then you will need to use
+// something like base64 since TOML does not have any binary types.
 //
 // When encoding TOML hashes (Go maps or structs), keys without any sub-hashes
 // are encoded first.
@@ -83,7 +92,7 @@ var quotedReplacer = strings.NewReplacer(
 // structs. (e.g. [][]map[string]string is not allowed but []map[string]string
 // is okay, as is []map[string][]string).
 //
-// NOTE: Only exported keys are encoded due to the use of reflection. Unexported
+// NOTE: only exported keys are encoded due to the use of reflection. Unexported
 // keys are silently discarded.
 type Encoder struct {
 	// The string to use for a single indentation level. The default is two
@@ -130,12 +139,13 @@ func (enc *Encoder) safeEncode(key Key, rv reflect.Value) (err error) {
 }
 
 func (enc *Encoder) encode(key Key, rv reflect.Value) {
-	// Special case. Time needs to be in ISO8601 format.
-	// Special case. If we can marshal the type to text, then we used that.
-	// Basically, this prevents the encoder for handling these types as
-	// generic structs (or whatever the underlying type of a TextMarshaler is).
+	// Special case: time needs to be in ISO8601 format.
+	//
+	// Special case: if we can marshal the type to text, then we used that. This
+	// prevents the encoder for handling these types as generic structs (or
+	// whatever the underlying type of a TextMarshaler is).
 	switch t := rv.Interface().(type) {
-	case time.Time, encoding.TextMarshaler:
+	case time.Time, encoding.TextMarshaler, Marshaler:
 		enc.writeKeyValue(key, rv, false)
 		return
 	// TODO: #76 would make this superfluous after implemented.
@@ -200,13 +210,19 @@ func (enc *Encoder) eElement(rv reflect.Value) {
 			enc.wf(v.In(time.UTC).Format(format))
 		}
 		return
+	case Marshaler:
+		s, err := v.MarshalTOML()
+		if err != nil {
+			encPanic(err)
+		}
+		enc.writeQuoted(string(s))
+		return
 	case encoding.TextMarshaler:
-		// Use text marshaler if it's available for this value.
-		if s, err := v.MarshalText(); err != nil {
+		s, err := v.MarshalText()
+		if err != nil {
 			encPanic(err)
-		} else {
-			enc.writeQuoted(string(s))
 		}
+		enc.writeQuoted(string(s))
 		return
 	}
 


Test patch:
diff --git a/decode_test.go b/decode_test.go
index 8d1f624..9b003fd 100644
--- a/decode_test.go
+++ b/decode_test.go
@@ -3,6 +3,7 @@ package toml
 import (
 	"fmt"
 	"io/ioutil"
+	"math"
 	"os"
 	"reflect"
 	"strings"
@@ -266,6 +267,50 @@ func TestDecodeIntOverflow(t *testing.T) {
 	}
 }
 
+func TestDecodeFloatOverflow(t *testing.T) {
+	tests := []struct {
+		value    string
+		overflow bool
+	}{
+		{fmt.Sprintf(`F32 = %f`, math.MaxFloat64), true},
+		{fmt.Sprintf(`F32 = %f`, -math.MaxFloat64), true},
+		{fmt.Sprintf(`F32 = %f`, math.MaxFloat32*1.1), true},
+		{fmt.Sprintf(`F32 = %f`, -math.MaxFloat32*1.1), true},
+		{fmt.Sprintf(`F32 = %d`, maxSafeFloat32Int+1), true},
+		{fmt.Sprintf(`F32 = %d`, -maxSafeFloat32Int-1), true},
+		{fmt.Sprintf(`F64 = %d`, maxSafeFloat64Int+1), true},
+		{fmt.Sprintf(`F64 = %d`, -maxSafeFloat64Int-1), true},
+
+		{fmt.Sprintf(`F32 = %f`, math.MaxFloat32), false},
+		{fmt.Sprintf(`F32 = %f`, -math.MaxFloat32), false},
+		{fmt.Sprintf(`F32 = %d`, maxSafeFloat32Int), false},
+		{fmt.Sprintf(`F32 = %d`, -maxSafeFloat32Int), false},
+		{fmt.Sprintf(`F64 = %f`, math.MaxFloat64), false},
+		{fmt.Sprintf(`F64 = %f`, -math.MaxFloat64), false},
+		{fmt.Sprintf(`F64 = %f`, math.MaxFloat32), false},
+		{fmt.Sprintf(`F64 = %f`, -math.MaxFloat32), false},
+		{fmt.Sprintf(`F64 = %d`, maxSafeFloat64Int), false},
+		{fmt.Sprintf(`F64 = %d`, -maxSafeFloat64Int), false},
+	}
+
+	for _, tt := range tests {
+		t.Run("", func(t *testing.T) {
+			var tab struct {
+				F32 float32
+				F64 float64
+			}
+			_, err := Decode(tt.value, &tab)
+
+			if tt.overflow && err == nil {
+				t.Fatal("expected error, but err is nil")
+			}
+			if (tt.overflow && !errorContains(err, "out of range")) || (!tt.overflow && err != nil) {
+				t.Fatalf("unexpected error:\n%v", err)
+			}
+		})
+	}
+}
+
 func TestDecodeSizedInts(t *testing.T) {
 	type table struct {
 		U8  uint8
diff --git a/encode_test.go b/encode_test.go
index 02bcef2..1a2ac83 100644
--- a/encode_test.go
+++ b/encode_test.go
@@ -336,6 +336,11 @@ type (
 	food  struct{ F []string }
 	fun   func()
 	cplx  complex128
+
+	sound2 struct{ S string }
+	food2  struct{ F []string }
+	fun2   func()
+	cplx2  complex128
 )
 
 // This is intentionally wrong (pointer receiver)
@@ -347,6 +352,14 @@ func (c cplx) MarshalText() ([]byte, error) {
 	return []byte(fmt.Sprintf("(%f+%fi)", real(cplx), imag(cplx))), nil
 }
 
+func (s *sound2) MarshalTOML() ([]byte, error) { return []byte(s.S), nil }
+func (f food2) MarshalTOML() ([]byte, error)   { return []byte(strings.Join(f.F, ", ")), nil }
+func (f fun2) MarshalTOML() ([]byte, error)    { return []byte("why would you do this?"), nil }
+func (c cplx2) MarshalTOML() ([]byte, error) {
+	cplx := complex128(c)
+	return []byte(fmt.Sprintf("(%f+%fi)", real(cplx), imag(cplx))), nil
+}
+
 func TestEncodeTextMarshaler(t *testing.T) {
 	x := struct {
 		Name    string
@@ -396,6 +409,55 @@ Fun = "why would you do this?"
 	}
 }
 
+func TestEncodeTOMLMarshaler(t *testing.T) {
+	x := struct {
+		Name    string
+		Labels  map[string]string
+		Sound   sound
+		Sound2  *sound
+		Food    food
+		Food2   *food
+		Complex cplx
+		Fun     fun
+	}{
+		Name:   "Goblok",
+		Sound:  sound{"miauw"},
+		Sound2: &sound{"miauw"},
+		Labels: map[string]string{
+			"type":  "cat",
+			"color": "black",
+		},
+		Food:    food{[]string{"chicken", "fish"}},
+		Food2:   &food{[]string{"chicken", "fish"}},
+		Complex: complex(42, 666),
+		Fun:     func() { panic("x") },
+	}
+
+	var buf bytes.Buffer
+	if err := NewEncoder(&buf).Encode(x); err != nil {
+		t.Fatal(err)
+	}
+
+	want := `Name = "Goblok"
+Sound2 = "miauw"
+Food = "chicken, fish"
+Food2 = "chicken, fish"
+Complex = "(42.000000+666.000000i)"
+Fun = "why would you do this?"
+
+[Labels]
+  color = "black"
+  type = "cat"
+
+[Sound]
+  S = "miauw"
+`
+
+	if buf.String() != want {
+		t.Error("\n" + buf.String())
+	}
+}
+
 // Would previously fail on 32bit architectures; can test with:
 //   GOARCH=386         go test -c &&  ./toml.test
 //   GOARCH=arm GOARM=7 go test -c && qemu-arm ./toml.test