Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

parse(Bool, s::String3) giving InexactError #57

Closed
anandijain opened this issue Nov 21, 2022 · 3 comments · Fixed by JuliaLang/julia#47782
Closed

parse(Bool, s::String3) giving InexactError #57

anandijain opened this issue Nov 21, 2022 · 3 comments · Fixed by JuliaLang/julia#47782
Labels
bug Something isn't working

Comments

@anandijain
Copy link 

julia> using InlineStrings

julia> String3("0")
"0"

julia> parse(Bool, String3("0"))
ERROR: InexactError: Bool(10)

julia> parse(Bool, String(String3("0")))
false

is this expected to work? It was confusing hitting this when working with a DataFrame
@nickrobinson251 nickrobinson251 added the bug Something isn't working label Nov 21, 2022
@bkamins
Copy link

bkamins commented Nov 21, 2022

This is a bug in Julia Base in my opinion.

@bkamins
Copy link

bkamins commented Nov 21, 2022

Also to be covered:

julia> parse(Bool, "false")
false

julia> parse(Bool, String7("false"))
ERROR: InexactError: Bool(10)

@quinnj - how do you want to handle this. I think:

  1. A PR to Base Julia should be made copying the implementation of
tryparse_internal(::Type{Bool}, sbuff::Union{String,SubString{String}}, startpos::Int, endpos::Int, base::Integer, raise::Bool)

to

tryparse_internal(::Type{Bool}, sbuff::AbstractString, startpos::Int, endpos::Int, base::Integer, raise::Bool)
  1. adding a workaround in InlineStrings.jl to make sure parse works for older Julia versions.

quinnj added a commit to JuliaLang/julia that referenced this issue Dec 2, 2022
@quinnj
Generalize Bool parse method to AbstractString
413b6d4 
Fixes JuliaStrings/InlineStrings.jl#57.

We currently have a specialization for `parse(Bool, ::Union{String, SubString{String})`
where `true` and `false` are parsed appropriately. The restriction to
`Union{String, SubString{String}}`, however, means we don't get this behavior
for other `AbstractString`s. In the linked issue above, for InlineStrings, we end up
going through the generic integer parsing codepath which results in an `InexactError`
when we try to do `Bool(10)`.

The proposal in this PR takes advantage of the fact that there is only the 2 comparisons
where we do `_memcmp` that require the input string to be "dense" (in memory), and otherwise,
we just do a comparison against a `SubString` of the input string.

Relatedly, I've wanted to introduce the concept of an abstrac type like:

```julia
abstract type MemoryAddressableString <: AbstractString
```

where the additional required interface would be being able to call
`pointer(::MemoryAddressableString)`, since a lot of our string algorithms depend on doing
these kind of pointer operations and hence makes it quite a pain to implement your own
custom string type.
@quinnj quinnj mentioned this issue Dec 2, 2022
Merged
@quinnj
Copy link
Member

quinnj commented Dec 2, 2022

Ok, PR to fix Base is up: JuliaLang/julia#47782. Will look into what we should do in InlineStrings as well.

quinnj added a commit to JuliaLang/julia that referenced this issue Dec 6, 2022
@quinnj @StefanKarpinski @nickrobinson251
Generalize Bool parse method to AbstractString (#47782)
63830a6 
* Generalize Bool parse method to AbstractString

Fixes JuliaStrings/InlineStrings.jl#57.

We currently have a specialization for `parse(Bool, ::Union{String, SubString{String})`
where `true` and `false` are parsed appropriately. The restriction to
`Union{String, SubString{String}}`, however, means we don't get this behavior
for other `AbstractString`s. In the linked issue above, for InlineStrings, we end up
going through the generic integer parsing codepath which results in an `InexactError`
when we try to do `Bool(10)`.

The proposal in this PR takes advantage of the fact that there is only the 2 comparisons
where we do `_memcmp` that require the input string to be "dense" (in memory), and otherwise,
we just do a comparison against a `SubString` of the input string.

Relatedly, I've wanted to introduce the concept of an abstrac type like:

```julia
abstract type MemoryAddressableString <: AbstractString
```

where the additional required interface would be being able to call
`pointer(::MemoryAddressableString)`, since a lot of our string algorithms depend on doing
these kind of pointer operations and hence makes it quite a pain to implement your own
custom string type.

* Apply suggestions from code review

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>
KristofferC pushed a commit to JuliaLang/julia that referenced this issue Jul 11, 2023
@quinnj @KristofferC
Generalize Bool parse method to AbstractString (#47782)
c984dd8 
* Generalize Bool parse method to AbstractString

Fixes JuliaStrings/InlineStrings.jl#57.

We currently have a specialization for `parse(Bool, ::Union{String, SubString{String})`
where `true` and `false` are parsed appropriately. The restriction to
`Union{String, SubString{String}}`, however, means we don't get this behavior
for other `AbstractString`s. In the linked issue above, for InlineStrings, we end up
going through the generic integer parsing codepath which results in an `InexactError`
when we try to do `Bool(10)`.

The proposal in this PR takes advantage of the fact that there is only the 2 comparisons
where we do `_memcmp` that require the input string to be "dense" (in memory), and otherwise,
we just do a comparison against a `SubString` of the input string.

Relatedly, I've wanted to introduce the concept of an abstrac type like:

```julia
abstract type MemoryAddressableString <: AbstractString
```

where the additional required interface would be being able to call
`pointer(::MemoryAddressableString)`, since a lot of our string algorithms depend on doing
these kind of pointer operations and hence makes it quite a pain to implement your own
custom string type.

* Apply suggestions from code review

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>
(cherry picked from commit 63830a6)
KristofferC pushed a commit to JuliaLang/julia that referenced this issue Oct 11, 2023
@quinnj
Generalize Bool parse method to AbstractString (#47782)
53dfef5 
* Generalize Bool parse method to AbstractString

Fixes JuliaStrings/InlineStrings.jl#57.

We currently have a specialization for `parse(Bool, ::Union{String, SubString{String})`
where `true` and `false` are parsed appropriately. The restriction to
`Union{String, SubString{String}}`, however, means we don't get this behavior
for other `AbstractString`s. In the linked issue above, for InlineStrings, we end up
going through the generic integer parsing codepath which results in an `InexactError`
when we try to do `Bool(10)`.

The proposal in this PR takes advantage of the fact that there is only the 2 comparisons
where we do `_memcmp` that require the input string to be "dense" (in memory), and otherwise,
we just do a comparison against a `SubString` of the input string.

Relatedly, I've wanted to introduce the concept of an abstrac type like:

```julia
abstract type MemoryAddressableString <: AbstractString
```

where the additional required interface would be being able to call
`pointer(::MemoryAddressableString)`, since a lot of our string algorithms depend on doing
these kind of pointer operations and hence makes it quite a pain to implement your own
custom string type.

* Apply suggestions from code review

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>
Co-authored-by: Nick Robinson <npr251@gmail.com>
(cherry picked from commit 63830a6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Generalize Bool parse method to AbstractString JuliaLang/julia
4 participants
@quinnj @bkamins @nickrobinson251 @anandijain