Allow lossless integer to float conversions

[saracen]

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 #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.

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 #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/adIBcMkCTlU

That second value is inaccurate, but that's just how floats work. It's also inconsistent because n = 17777215 and n = 17777215.0 will behave different. I think that check should probably be removed. What do you think?

[saracen]

@arp242

I think the float32 boundary check could be removed. I'd expect loss here, and unlike integers, the TOML spec makes no mention about them being represented losslessly.

The error check for int -> float I'd keep, but only because the spec says:

If an integer cannot be represented losslessly, an error must be thrown.

In practice, you could probably remove both checks and just cast to float with loss in both cases and nobody would complain 👍 I don't mind which route we take.

[arp242]

Oh, right. Well, let's keep it then. If it really proves to be a problem for some people later on we can always relax things (possibly via an option), which will be a compatible change. The reverse of "don't add check and add one later" won't be compatible as it has a chance of breaking people's existing TOML files.