tryparse: parse string to Nullable

[tanmaykm]

Introduces methods that parse a string as the indicated type and return a Nullable with the result instead of throwing exception:

• maybeint{T<:Integer}(::Type{T<:Integer},s::AbstractString)
• maybefloat32(s::AbstractString) and maybefloat64(s::AbstractString)

Ref: discussions at #9316, #3631, #5704

[dhoegh]

Just a thought why not maybe{T}(::Type{T},s::AbstractString) for all types, hence you collect the functions similar to zero.

[tanmaykm]

Yes, maybe will be nicer. Thanks for the suggestion.

[nalimilan]

Introducing the maybe terminology while the type is called Nullable is a bit weird. Also, for consistency with the current parseint, parsefloat, you could merge all of these functions into something like parse(::Type{Int}, ...), parse(::Type{Float}, ...), parse(::Type{Nullable{Int}}, ...), parse(::Type{Nullable{Float}}, ...), etc.

EDIT: fix types by adding ::Type{...}

[ivarne]

@nalimilan That seems like a massive improvement in API. Seems like Nullable will soon need some shorter name.

[dhoegh]

+1 for the API @nalimilan.

[tanmaykm]

@nalimilan parse is even better. Shall update the PR and manual, unless there are any other views.

Not sure why the AppVeyor build is failing though. Seems spurious, but it failed even after re-running the build.

[nalimilan]

Or to avoid the conflict in meanings with the current parse function, the new one could be named something like parseval. (It would also be a funny pun with a French mathematician's name...)

[nalimilan]

@tanmaykm The AppVeyor failure doesn't sound so spurious, as the failure happens in maybefloat64. But why would it happen only on Windows, I don't know.

[tanmaykm]

The AppVeyor logs report:

exception on 3: ERROR: test failed: (NaN == 64.0)
614 in expression: get(maybefloat64("\x034")) == 64.0

whereas strings.jl line 1315 is:

@test get(maybefloat64("64")) == 64.0

Seems like some sort of corruption.

[vtjnash]

sadly, that probably is due to #7906

[vtjnash]

bump. maybe we can merge this now?

[toivoh]

About api:
I'm not so sure about using eg

parse(Int, ...)

and

parse(Nullable{Int}, ...)

to make the distinction of whether a non-Int input should be allowed and returned add a null result. The latter call looks to like that it would look for a text representation of a (nullable) Int or null, and throw an error if neither was found.

[tanmaykm]

Will rebase and test again.

[JeffBezanson]

@toivoh good point. We could have parse and tryparse. Together they could replace parseint, parsefloat, float64_isvalid, and float32_isvalid, so still a net reduction of 2 functions. This PR should definitely remove the _isvalid functions.

[JeffBezanson]

We should avoid duplicating all of this code.

[tanmaykm]

Yes, will be removing the duplication. Will pass the error string back through an optional ArgumentError parameter so that only methods that throw would incur the overhead of creating it.

[tanmaykm]

Help! Any clues as to why this build is failing? https://travis-ci.org/JuliaLang/julia/builds/54050279

[nalimilan]

Usually this kind of error means that a compile-time function call relies on a type that's defined in a file that's included later in the bootstrap process. Here, it looks like parsing the literal array at

julia/base/combinatorics.jl

Line 6 in 5b6468a

const _fact_table128 =

implies a call to parseint which involves BigInt in one way or another. But I have a hard time understanding why this would only happen on 32-bit, and how your changes may have triggered this.

[tanmaykm]

Thanks @nalimilan, I was able to reorder a portion of combinatorics.jl to fix the build issue. Not sure if the Appveyor issue is related.

[tanmaykm]

This now exports the tryparse method:

• tryparse{T<:Integer}(::Type{T<:Integer},s::AbstractString)
• tryparse(::Type{Float..},s::AbstractString)
• a few variants of the above

And:

• tryparse(Float.., ...) call the corresponding C functions jl_try_strtof, jl_try_substrtof, jl_try_strtod and jl_try_substrtod.
• The parseint, parsefloat, float64_isvalid and float32_isvalid methods wrap the corresponding tryparse methods.
• The jl_strtod, jl_strtof, ... functions are wrappers over the jl_try_str... functions.

This should fix #10498 as well.

I shall rebase and add documentation if this looks fine. Should the ..._isvalid methods be deprecated?

[ViralBShah]

Unless the ..._isvalid methods were exported, I don't think deprecation is necessary.

[tkelman]

float64_isvalid is used by JSON.jl, and presumably other packages as well. A deprecation would help lessen some breakage.

[nalimilan]

I don't think you should put these lines here. This file contains almost exclusively includes. Have you tried simply moving include("combinatorics.jl") below the BigInt section?

[tanmaykm]

There are cross dependencies that break if the entire file is shifted. These were the only UInt128 dependent code that was there. I thought of putting these in an int128funcs.jl file, but this seemed too small to go into a separate file.

[JeffBezanson]

We have to come up with something else. Having a factorial table in sysimg.jl is bananas. What depends on combinatorics.jl? I thought almost nothing would depend on this; it's all extra library functions that we've discussed removing from Base.

[tanmaykm]

parsing UInt128 literals seems to require BigInt in 32 bit environment

[JeffBezanson]

Looks like gmp.jl depends on factorial and binomial. The simple generic definitions of those can be moved to intfuncs.jl. That should solve it.

[tanmaykm]

I think it will be fine if I move the binomial and factorial from gmp.jl and mpfr.jl into combinatorics.jl.

[tanmaykm]

Ok. So should they be moved to intfuncs.jl or combinatorics.jl since it already has the other variants?

[nalimilan]

I think I liked better parse(Nullable{Int}, ...) than tryparse(Int, ...). Especially since as @ivarne said above, with a new syntax for Nullable, we could imagine writing it as parse(Int?, ...). But well...

Are there any other cases where a function should come in two versions, one returning a Nullable? Just to check that the try* pattern generalizes well.

[JeffBezanson]

Could you move this branch to JuliaLang so I can work on it too?
I just moved a copy there, but I'll delete it and we can use yours instead.

[JeffBezanson]

@nalimilan Parsing numbers seems to be unusual in that (1) it is very performance-critical, (2) it has a failure mode that is exactly halfway to needing an exception. Half the time, you want an exception if the string is not a number, and half the time it just means you will handle the string differently. That's why I think parse/tryparse is justified.

[tanmaykm]

Ok. Moved to the tryparse branch on JuliaLang

[JeffBezanson]

replaced by #10543