- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 5.7k
 
Replace Nullable{T} with Union{Some{T}, Void} #23642
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
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:313There 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 THow 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))
endI 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
broadcastcode needs checking. What this PR does currently is:Nullable, moved to a new Nullables.jl packageNull/nullfrom Nulls.jl,Some{T}andOptional{T} = Union{Null, Some{T}}as a replacement forNullablein cases where either 1) one wants to force explicit unwrapping and/or 2) one needs to allow wrappingnullinside anOptionand be able to distinguish it from anull(typically, fortrygeton dictionaries ortryparseon 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 beValueorJust(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 butnullwhenx = 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 forNullto thebroadcastmachinery, that's probably worth it since that's one of the main features ofOptionin many languages.Optionin addition toVoid/nothingandNull/null, which could beNone/nonefor consistency with other languages. The main argument in favor of this would be to distinguish clearlyOptionfrom?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.