odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.17k stars 550 forks source link

Fix partial parsing of `infinity` #3675

Closed Feoramund closed 1 month ago

Feoramund commented 1 month ago

I found that infinity was being parsed incorrectly. Even though it checks for "infinity" it only seems to care about "inf" and will incorrectly report only 3 (or 4 with a sign +/-) characters as having been parsed. It also erroneously reported 1 and 2 characters parsed for "i" and "in" respectively. I used the following program to get my results:

package main

import "core:fmt"
import "core:strconv"

main :: proc () {
    n: int
    s := "infinity"

    // unsigned
    for i in 1 ..< len(s) + 1 {
        ss := s[:i]
        f, ok := strconv.parse_f64(ss, &n)
        fmt.printfln("%s = %f %i %t", ss, f, n, ok)
    }

    // positive
    s = "+infinity"
    for i in 1 ..< len(s) + 1 {
        ss := s[:i]
        f, ok := strconv.parse_f64(ss, &n)
        fmt.printfln("%s = %f %i %t", ss, f, n, ok)
    }

    // negative
    s = "-infinity"
    for i in 1 ..< len(s) + 1 {
        ss := s[:i]
        f, ok := strconv.parse_f64(ss, &n)
        fmt.printfln("%s = %f %i %t", ss, f, n, ok)
    }
}
i = 0.000 1 false
in = 0.000 2 false
inf = +Inf 3 true
infi = +Inf 3 false
infin = +Inf 3 false
infini = +Inf 3 false
infinit = +Inf 3 false
infinity = +Inf 3 false
+ = 0.000 0 false
+i = 0.000 1 false
+in = 0.000 2 false
+inf = +Inf 4 true
+infi = +Inf 4 false
+infin = +Inf 4 false
+infini = +Inf 4 false
+infinit = +Inf 4 false
+infinity = +Inf 4 false
- = 0.000 0 false
-i = 0.000 1 false
-in = 0.000 2 false
-inf = -Inf 4 true
-infi = -Inf 4 false
-infin = -Inf 4 false
-infini = -Inf 4 false
-infinit = -Inf 4 false
-infinity = -Inf 4 false

To be clear, this change makes "inf" and "infinity" the only proper infinity strings that can be parsed now, as opposed to any substring of "infi" to "infinit". I figured this was in line with the spirit of the code before, but I can change that if need be.

Added the beginnings of a strconv test suite, too.

I did a check for issues around this and noticed #2670 a moment before I was going to send this PR, so I took care of that and merged in test_issue_2087's tests into the new strconv test suite.

Fixes #2670

gingerBill commented 1 month ago

Shouldn't infinity = +Inf 3 false be true?

Feoramund commented 1 month ago

It should have been. Note that those results were from before this patch set. Here is what I have currently with the same program:

i = 0.000 0 false
in = 0.000 0 false
inf = +Inf 3 true
infi = 0.000 0 false
infin = 0.000 0 false
infini = 0.000 0 false
infinit = 0.000 0 false
infinity = +Inf 8 true
+ = 0.000 0 false
+i = 0.000 0 false
+in = 0.000 0 false
+inf = +Inf 4 true
+infi = 0.000 0 false
+infin = 0.000 0 false
+infini = 0.000 0 false
+infinit = 0.000 0 false
+infinity = +Inf 9 true
- = 0.000 0 false
-i = 0.000 0 false
-in = 0.000 0 false
-inf = -Inf 4 true
-infi = 0.000 0 false
-infin = 0.000 0 false
-infini = 0.000 0 false
-infinit = 0.000 0 false
-infinity = -Inf 9 true
Feoramund commented 1 month ago

Do you think that infi to infinit should be treated as an infinite value but return false? I had another look at how the float parsing handles trailing characters and figured that might be more sensible than what I have now.

EDIT: For example, an output like this:

i          = 0.000 0 false
in         = 0.000 0 false
inf        = +Inf 3 true
infi       = +Inf 3 false
infin      = +Inf 3 false
infini     = +Inf 3 false
infinit    = +Inf 3 false
infinity   = +Inf 8 true
infinitye  = +Inf 8 false
infinityee = +Inf 8 false
i          = 0.000 0 false
in         = 0.000 0 false
inf        = +Inf 3 true
infi       = +Inf 3 false
infin      = +Inf 3 false
infine     = +Inf 3 false
infinee    = +Inf 3 false

It's also possible that intermediate infinity substrings could be true, but that would be a more lenient approach. I'm in favor of the strict parsing requirement above.

i          = 0.000 0 false
in         = 0.000 0 false
inf        = +Inf 3 true
infi       = +Inf 4 true
infin      = +Inf 5 true
infini     = +Inf 6 true
infinit    = +Inf 7 true
infinity   = +Inf 8 true
infinitye  = +Inf 8 false
infinityee = +Inf 8 false
i          = 0.000 0 false
in         = 0.000 0 false
inf        = +Inf 3 true
infi       = +Inf 4 true
infin      = +Inf 5 true
infine     = +Inf 5 false
infinee    = +Inf 5 false
gingerBill commented 1 month ago

I think infi to inifinit should return false, since they are technically incomplete.

Feoramund commented 1 month ago

Here's my current output with the latest changes.

parse_f64:
i          = 0.000 0 false
in         = 0.000 0 false
inf        = +Inf 3 true
infi       = +Inf 3 false
infin      = +Inf 3 false
infini     = +Inf 3 false
infinit    = +Inf 3 false
infinity   = +Inf 8 true
infinityy  = +Inf 8 false
infinityyy = +Inf 8 false

parse_f64_prefix:
i          = 0.000 0 false
in         = 0.000 0 false
inf        = +Inf 3 true
infi       = +Inf 3 true
infin      = +Inf 3 true
infini     = +Inf 3 true
infinit    = +Inf 3 true
infinity   = +Inf 8 true
infinityy  = +Inf 8 true
infinityyy = +Inf 8 true