Parser accepts garbage values for `Int` et al. #171

Closed
lfairy opened this Issue Dec 14, 2013 · 4 comments

Projects

None yet

3 participants

@lfairy
lfairy commented Dec 14, 2013

I've noticed two issues with the current conversion code:

  1. Floating point values are silently coerced using floor:

    > decode "[1.1]" :: Maybe Int
    Just [1]
    > decode "[-1.1]" :: Maybe Int
    Just [-2]
    
  2. The result isn't checked for overflow:

    > decode "[10000000000000000000]" :: Maybe Int
    Just [-8446744073709551616]
    

Is there a good reason for this behavior (performance, compatibility)?

@bos
Owner
bos commented Dec 18, 2013

The rounding to lowest behaviour of floor is a bit surprising here; that looks like it was unintentional. @basvandijk, got an opinion there?

The rounding and overflow behaviours are intentional, though: rounding is basically "doing the right thing", and normal Haskell programs do not perform overflow checks:

> 10000000000000000000 :: Int
-8446744073709551616

You can always override these behaviours by writing newtype wrappers that perform overflow checks or refuse to round.

@lfairy
lfairy commented Dec 18, 2013

Ah, that makes sense.

As for the floor issue, Jackson (a Java library I found just now) truncates. So truncate seems to have the most precedent here.

@basvandijk
Collaborator

The rounding to lowest behaviour of floor is a bit surprising here; that looks like it was unintentional. @basvandijk, got an opinion there?

Floor should "round to the lowest" because floor x must return the greatest integer not greater than x:

> floor (-1.1::Double)::Int
-2

... rounding is basically "doing the right thing" ...

Why is rounding the right thing? Coercion like that is uncommon in Haskell. It's better to use explicit conversions since it prevents bugs due to silent coercion. Although I don't care to much about it, I do think rejecting fractional values when expecting an integral is the right thing to do. I wonder if and how much existing code will break if we change this...

@lfairy
lfairy commented Dec 18, 2013

Although I don't care to much about it, I do think rejecting fractional values when expecting an integral is the right thing to do.

I mulled over it a bit, and I think I'm starting to agree with you here. As far as I know, integers are used for two things:

  • Indexing (e.g. position in a list or a database)
  • Counting (e.g. number of views for a website)

In neither of these cases do fractional values make sense.

As for performance, I did experiment with that suggestion a few days ago. On the test input (12345), it's about four times faster than floor and truncate. (big and funny are 1e10 and 10000000000e-10 respectively. They aren't terribly useful here.)

To summarize, we should reject fractional values because:

  • They don't make sense in context
  • Accepting them could hide bugs
  • It's faster
@bos bos closed this Jul 10, 2015
@lfairy lfairy added a commit to lfairy/aeson that referenced this issue Jul 11, 2015
@lfairy lfairy Use `truncate` instead of `floor` in Integral instances
Whether we should accept fractional parts at all is up to debate (#171).
But rounding toward negative infinity is wrong either way.
a18a615
@lfairy lfairy added a commit to lfairy/aeson that referenced this issue Jul 11, 2015
@lfairy lfairy Use `truncate` instead of `floor` in Integral instances
Whether we should accept fractional parts at all is up to debate (#171).
But rounding toward negative infinity is wrong either way.

Truncation is the default in most languages, so it will cause the least
surprise to users of the library.
014ab56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment