-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
Replace Nullable{T} with Union{Some{T}, Void} #23642
Conversation
base/null.jl
Outdated
|
||
convert(::Type{Option{T}}, ::Null) where {T} = null | ||
convert(::Type{Option }, ::Null) = null | ||
# FIXME: find out why removing these two methods makes addprocs() fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has turned out to be very hard to debug. Without these two methods, addprocs(2)
hangs and finally times out, leaving two julia processes using 100% CPU in the background. Replacing all occurrences of nothing
with null
in base/distributed/* didn't fix it. The output isn't very helpful. Any help on how to debug this is welcome!
julia> addprocs(2)
ERROR: Timed out waiting to read host:port string from worker.
read_worker_host_port(::Pipe) at ./distributed/cluster.jl:302
connect(::Base.Distributed.LocalManager, ::Int64, ::WorkerConfig) at ./distributed/managers.jl:395
create_worker(::Base.Distributed.LocalManager, ::WorkerConfig) at ./distributed/cluster.jl:507
setup_launched_worker(::Base.Distributed.LocalManager, ::WorkerConfig, ::Array{Int64,1}) at ./distributed/cluster.jl:453
(::getfield(Base.Distributed, Symbol("##41#44")){Base.Distributed.LocalManager,WorkerConfig})() at ./task.jl:335
...and 1 more exception(s).
Stacktrace:
[1] sync_end() at ./task.jl:287
[2] macro expansion at ./task.jl:303 [inlined]
[3] #addprocs_locked#38(::Array{Any,1}, ::Function, ::Base.Distributed.LocalManager) at ./distributed/cluster.jl:407
[4] #addprocs#37(::Array{Any,1}, ::Function, ::Base.Distributed.LocalManager) at ./distributed/cluster.jl:371
[5] #addprocs#257(::Bool, ::Array{Any,1}, ::Function, ::Int64) at ./distributed/managers.jl:314
[6] addprocs(::Int64) at ./distributed/managers.jl:313
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps related to automatic conversion thru fallback constructor? #23273
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally this rather happens when assigning to an Option
field. It's quite rare to call Option(x)
.
I've finally found a way to see the backtrace from the workers, by adding print statements to read_worker_host_port
. Turns out the TCPSocket
inner constructor used nothing
to set a Nullable
field to null. Now it works!
test/choosetests.jl
Outdated
"boundscheck", "error", "cartesian", "asmvariant", "osutils", | ||
"channels", "iostream", "specificity", "codegen", "codevalidation", | ||
# FIXME: fix ambiguities | ||
"ambiguous" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails due to ambiguities between the following methods:
convert(::Type{Union{Null, Some{T}}}, x::Some) where T
convert(::Type{Union{Null, Some{T}}}, ::Void) where T
convert(::Type{Union{Null, Some{T}}}, ::Null) where T
How can I fix this? My attempts have not been successful so far.
Is |
Thanks so much for working on this! It's exciting to see this finally coming to fruition. My take on the issues you've enumerated: Naming:
Supporting conversion to
Broadcasting:
Yet another type:
|
As one data point it's what Rust uses. Not sure what else uses it. |
Yes, according to Wikipedia, EDIT: |
base/null.jl
Outdated
an `Option` object is null, and [`unwrap`](@ref) to access the value inside the `Some` | ||
object if not. | ||
""" | ||
Option{T} = Union{Null, Some{T}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need a const
, right?
Ah, good point about #23637. Though the issue with
Why not, but there are probably very few cases in which one would like to convert from
Yet in most languages supporting Anyway, we can always start without this and add it later if needed. That feature doesn't seem to be used in Base, which might indicate that it's not essential (but be sure people will ask for it). But I'll have to actively remove code to prevent it from working at all, since the basics already exist due to the |
Great! I didn't follow closely this switch-away-from-nullable effort, so I don't see why we wouldn't have |
The problem with using |
@rfourquet We discussed that a bit in #22682. There's no intrinsic reason to use That said, one small drawback of reusing |
I like all the general ideas here. My somewhat non-bikeshedding comment: we should definitely have different values for "or maybe not" (that is, My purely bikeshedding comments: For For |
My two cents:
|
Another option is |
@jamesonquinn The ship has sailed on using
What do you mean? What exact behavior would you expect these operators would do in each case? |
@TotalVerb That wouldn't help with more complex cases like |
Thanks for your answers. So I read a bit more the threads, in which some of my impressions were already expressed:
struct Maybe{T}
value::Union{Some{T}, None}
Maybe{T}() where T = new(None())
Maybe{T}(x) where T = new(Some(x))
end I can't come up with solid arguments for now, but I feel that it's my prefered solution:
PS: note that I saw e.g here the similar proposition |
@rfourquet I don't understand the point of defining Regarding the fact that people could define their custom |
I interpreted the last paragraph of this comment as meaning that the I was not aware of the "counterfactual return type problem" (presented there), and I agree that it's quite convincing. |
@nalimilan Using This is merely one of many examples where I think the semantics of these values would be different. |
OK, I see. I wanted this behavior (three-valued logic) too for a long time, see JuliaStats/NullableArrays.jl#85 for a long discussion about that. Unfortunately, it turned out to be quite inconvenient, since Julia expects |
BTW, I've just created the Nullables.jl package by taking all code and tests from Base. It works on Julia 0.6. and 0.7. Comments welcome (please file issues there), at some point it should probably be moved to JuliaArchive. |
No, that's incorrect for things like |
I think this Anyway, the discussion about |
I'd prefer we not support |
Use it everywhere get() was called and nothing would not trigger an error immediately.
MaybeValue is just a simplified version of Nullable reserved for internal use which wraps a value or no value in a type-stable way. Information about whether a value is present or not is carried separately.
Alléluia 🎉 |
Fantastique! |
Excellent work here, Milan, and thanks for sticking through so many rebases. You're a hero! |
There are two contradictory entries in |
Good catch @mlhetland. See #25573. |
Also add coalesce() function to return first non-nothing value and unwrap Some objects. Use the notnothing() function internally where it makes sense to assert that the result is different from nothing. Use custom MaybeValue wrapper for ProductIterator to work around a performance regression due to type instability (information about whether a value is present or not is carried separately).
This is a first proposal to fix #22682. Tests pass but I haven't updated docs and the
broadcast
code needs checking. What this PR does currently is:Nullable
, moved to a new Nullables.jl packageNull
/null
from Nulls.jl,Some{T}
andOptional{T} = Union{Null, Some{T}}
as a replacement forNullable
in cases where either 1) one wants to force explicit unwrapping and/or 2) one needs to allow wrappingnull
inside anOption
and be able to distinguish it from anull
(typically, fortryget
on dictionaries ortryparse
on a nullable value).Union{T, Null}
is still available to represent missing values, and will allow silent propagation of nulls once we implement operators onNull
.get(::Nullable[, y])
tounwrap(::Some[, y])
: this is not strictly needed, but allows seeing to extent to which it's used in Base, and evaluating how simpler the code would be if we usedUnion{Null, T}
rather thanUnion{Null, Some{T}}
(and therefore didn't require unwrapping)Issues to discuss include:
Option
(Rust, Scala, ML, F#) could beOptional
(Swift, C++17, Java 8) orMaybe
(Haskell). It seems that the two former are the most common.Some
(used by Rust, Scala, ML, F#, Swift) could also beValue
orJust
(as in Haskell).convert(Some{T}, ::T)
. At first I thought it was convenient (we allowed it forNullable
), but I realized it wasn't a great idea since it implies thatconvert(Option{T}, x)
givesSome(x)
in general butnull
whenx = null
, which is a trap transforming a wrappednull
(which should beSome(null)
) into a nullOption
, defeating the purpose of using a wrapper in the first place.broadcast
: things likebroadcast(exp, Some(1))
work, but an error is thrown forbroadcast(exp, null)
, which makes this operation mostly useless. We can fix this by defining operators onNull
(just like in Nulls.jl), but that won't work for all functions. We should be able to add a special rule forNull
to thebroadcast
machinery, that's probably worth it since that's one of the main features ofOption
in many languages.Option
in addition toVoid
/nothing
andNull
/null
, which could beNone
/none
for consistency with other languages. The main argument in favor of this would be to distinguish clearlyOption
from?T
/Union{Null, T}
, since the latter is intended for missing values in data with automatic propagation of nulls. But I think most people were opposed to this in the discussion.