Limit broadcast mechanism over Nullables (#19787)

* Limit broadcast mechanism over Nullables

* Test that broadcast doesn't propagate too much

* Update manual section on Nullables

* Address TotalVerb comments

* Address Sacha0 comments

Author: p-zubieta

NEWS.md

@@ -55,13 +55,10 @@ This section lists changes that do not have deprecation warnings.
 
   * `broadcast` now treats `Ref` (except for `Ptr`) arguments as 0-dimensional
     arrays ([#18965]).
+
   * `broadcast` now handles missing data (`Nullable`s) allowing operations to
-    be lifted over `Nullable`s, as if the `Nullable` were like an array with
-    zero or one element. ([#16961]). Note that many situations where `Nullable`
-    types had been treated like scalars before will no longer work. For
-    example, `get.(xs)` on `xs::Array{T <: Nullable}` will now treat the
-    nullables as a container, and attempt to operate on the data contained.
-    This use case will need to be migrated to `map(get, xs)`.
+    be lifted over mixtures of `Nullable`s and scalars, as if the `Nullable`
+    were like an array with zero or one element. ([#16961], [#19787]).
 
   * The runtime now enforces when new method definitions can take effect ([#17057]).
     The flip-side of this is that new method definitions should now reliably actually
@@ -804,3 +801,4 @@ Language tooling improvements
 [#19543]: https://github.com/JuliaLang/julia/issues/19543
 [#19598]: https://github.com/JuliaLang/julia/issues/19598
 [#19635]: https://github.com/JuliaLang/julia/issues/19635
+[#19787]: https://github.com/JuliaLang/julia/issues/19787

base/broadcast.jl

@@ -5,10 +5,12 @@ module Broadcast
 using Base.Cartesian
 using Base: promote_eltype_op, linearindices, tail, OneTo, to_shape,
             _msk_end, unsafe_bitgetindex, bitcache_chunks, bitcache_size, dumpbitcache,
-            nullable_returntype, null_safe_eltype_op, hasvalue, is_nullable_array
+            nullable_returntype, null_safe_eltype_op, hasvalue
 import Base: broadcast, broadcast!
 export broadcast_getindex, broadcast_setindex!, dotview
 
+typealias ScalarType Union{Type{Any}, Type{Nullable}}
+
 ## Broadcasting utilities ##
 # fallbacks for some special cases
 @inline broadcast(f, x::Number...) = f(x...)
@@ -28,37 +30,28 @@ containertype(::Type) = Any
 containertype{T<:Ptr}(::Type{T}) = Any
 containertype{T<:Tuple}(::Type{T}) = Tuple
 containertype{T<:Ref}(::Type{T}) = Array
-containertype{T<:AbstractArray}(::Type{T}) =
-    is_nullable_array(T) ? Array{Nullable} : Array
+containertype{T<:AbstractArray}(::Type{T}) = Array
 containertype{T<:Nullable}(::Type{T}) = Nullable
 containertype(ct1, ct2) = promote_containertype(containertype(ct1), containertype(ct2))
 @inline containertype(ct1, ct2, cts...) = promote_containertype(containertype(ct1), containertype(ct2, cts...))
 
 promote_containertype(::Type{Array}, ::Type{Array}) = Array
 promote_containertype(::Type{Array}, ct) = Array
 promote_containertype(ct, ::Type{Array}) = Array
-promote_containertype(::Type{Tuple}, ::Type{Any}) = Tuple
-promote_containertype(::Type{Any}, ::Type{Tuple}) = Tuple
+promote_containertype(::Type{Tuple}, ::ScalarType) = Tuple
+promote_containertype(::ScalarType, ::Type{Tuple}) = Tuple
 promote_containertype(::Type{Any}, ::Type{Nullable}) = Nullable
 promote_containertype(::Type{Nullable}, ::Type{Any}) = Nullable
-promote_containertype(::Type{Nullable}, ::Type{Array}) = Array{Nullable}
-promote_containertype(::Type{Array}, ::Type{Nullable}) = Array{Nullable}
-promote_containertype(::Type{Array{Nullable}}, ::Type{Array{Nullable}}) =
-    Array{Nullable}
-promote_containertype(::Type{Array{Nullable}}, ::Type{Array}) = Array{Nullable}
-promote_containertype(::Type{Array}, ::Type{Array{Nullable}}) = Array{Nullable}
-promote_containertype(::Type{Array{Nullable}}, ct) = Array{Nullable}
-promote_containertype(ct, ::Type{Array{Nullable}}) = Array{Nullable}
 promote_containertype{T}(::Type{T}, ::Type{T}) = T
 
 ## Calculate the broadcast indices of the arguments, or error if incompatible
 # array inputs
 broadcast_indices() = ()
 broadcast_indices(A) = broadcast_indices(containertype(A), A)
-broadcast_indices(::Union{Type{Any}, Type{Nullable}}, A) = ()
+broadcast_indices(::ScalarType, A) = ()
 broadcast_indices(::Type{Tuple}, A) = (OneTo(length(A)),)
 broadcast_indices(::Type{Array}, A::Ref) = ()
-broadcast_indices{T<:Array}(::Type{T}, A) = indices(A)
+broadcast_indices(::Type{Array}, A) = indices(A)
 @inline broadcast_indices(A, B...) = broadcast_shape((), broadcast_indices(A), map(broadcast_indices, B)...)
 # shape (i.e., tuple-of-indices) inputs
 broadcast_shape(shape::Tuple) = shape
@@ -132,9 +125,7 @@ end
 
 Base.@propagate_inbounds _broadcast_getindex(A, I) = _broadcast_getindex(containertype(A), A, I)
 Base.@propagate_inbounds _broadcast_getindex(::Type{Array}, A::Ref, I) = A[]
-Base.@propagate_inbounds _broadcast_getindex(::Type{Any}, A, I) = A
-Base.@propagate_inbounds _broadcast_getindex(::Union{Type{Any},
-                                              Type{Nullable}}, A, I) = A
+Base.@propagate_inbounds _broadcast_getindex(::ScalarType, A, I) = A
 Base.@propagate_inbounds _broadcast_getindex(::Any, A, I) = A[I]
 
 ## Broadcasting core
@@ -285,28 +276,20 @@ ftype(f, A) = typeof(f)
 ftype(f, A...) = typeof(a -> f(a...))
 ftype(T::Type, A...) = Type{T}
 
-# nullables need to be treated like scalars sometimes and like containers
-# other times, so there are two variants of typestuple.
-
-# if the first argument is Any, then Nullable should be treated like a
-# scalar; if the first argument is Array, then Nullable should be treated
-# like a container.
-typestuple(::Type, a) = (Base.@_pure_meta; Tuple{eltype(a)})
-typestuple(::Type{Any}, a::Nullable) = (Base.@_pure_meta; Tuple{typeof(a)})
-typestuple(::Type, T::Type) = (Base.@_pure_meta; Tuple{Type{T}})
-typestuple{T}(::Type{T}, a, b...) = (Base.@_pure_meta; Tuple{typestuple(T, a).types..., typestuple(T, b...).types...})
+typestuple(a) = (Base.@_pure_meta; Tuple{eltype(a)})
+typestuple(T::Type) = (Base.@_pure_meta; Tuple{Type{T}})
+typestuple(a, b...) = (Base.@_pure_meta; Tuple{typestuple(a).types..., typestuple(b...).types...})
 
-# these functions take the variant of typestuple to be used as first argument
-ziptype{T}(::Type{T}, A) = typestuple(T, A)
-ziptype{T}(::Type{T}, A, B) = (Base.@_pure_meta; Iterators.Zip2{typestuple(T, A), typestuple(T, B)})
-@inline ziptype{T}(::Type{T}, A, B, C, D...) = Iterators.Zip{typestuple(T, A), ziptype(T, B, C, D...)}
+ziptype(A) = typestuple(A)
+ziptype(A, B) = (Base.@_pure_meta; Iterators.Zip2{typestuple(A), typestuple(B)})
+@inline ziptype(A, B, C, D...) = Iterators.Zip{typestuple(A), ziptype(B, C, D...)}
 
-_broadcast_type{S}(::Type{S}, f, T::Type, As...) = Base._return_type(f, typestuple(S, T, As...))
-_broadcast_type{T}(::Type{T}, f, A, Bs...) = Base._default_eltype(Base.Generator{ziptype(T, A, Bs...), ftype(f, A, Bs...)})
+_broadcast_type(f, T::Type, As...) = Base._return_type(f, typestuple(T, As...))
+_broadcast_type(f, A, Bs...) = Base._default_eltype(Base.Generator{ziptype(A, Bs...), ftype(f, A, Bs...)})
 
 # broadcast methods that dispatch on the type of the final container
 @inline function broadcast_c(f, ::Type{Array}, A, Bs...)
-    T = _broadcast_type(Any, f, A, Bs...)
+    T = _broadcast_type(f, A, Bs...)
     shape = broadcast_indices(A, Bs...)
     iter = CartesianRange(shape)
     if isleaftype(T)
@@ -317,21 +300,14 @@ _broadcast_type{T}(::Type{T}, f, A, Bs...) = Base._default_eltype(Base.Generator
     end
     return broadcast_t(f, Any, shape, iter, A, Bs...)
 end
-@inline function broadcast_c(f, ::Type{Array{Nullable}}, A, Bs...)
-    @inline rec(x) = broadcast(f, x)
-    @inline rec(x, y) = broadcast(f, x, y)
-    @inline rec(x, y, z) = broadcast(f, x, y, z)
-    @inline rec(xs...) = broadcast(f, xs...)
-    broadcast_c(rec, Array, A, Bs...)
-end
 function broadcast_c(f, ::Type{Tuple}, As...)
     shape = broadcast_indices(As...)
     n = length(shape[1])
     return ntuple(k->f((_broadcast_getindex(A, k) for A in As)...), n)
 end
 @inline function broadcast_c(f, ::Type{Nullable}, a...)
     nonnull = all(hasvalue, a)
-    S = _broadcast_type(Array, f, a...)
+    S = _broadcast_type(f, a...)
     if isleaftype(S) && null_safe_eltype_op(f, a...)
         Nullable{S}(f(map(unsafe_get, a)...), nonnull)
     else
@@ -347,28 +323,21 @@ end
 """
     broadcast(f, As...)
 
-Broadcasts the arrays, tuples, `Ref`, nullables, and/or scalars `As` to a
+Broadcasts the arrays, tuples, `Ref`s, nullables, and/or scalars `As` to a
 container of the appropriate type and dimensions. In this context, anything
-that is not a subtype of `AbstractArray`, `Ref` (except for `Ptr`s) or `Tuple`,
+that is not a subtype of `AbstractArray`, `Ref` (except for `Ptr`s), `Tuple`,
 or `Nullable` is considered a scalar. The resulting container is established by
 the following rules:
 
  - If all the arguments are scalars, it returns a scalar.
  - If the arguments are tuples and zero or more scalars, it returns a tuple.
- - If there is at least an array or a `Ref` in the arguments, it returns an array
-   (and treats any `Ref` as a 0-dimensional array of its contents and any tuple
-   as a 1-dimensional array) expanding singleton dimensions.
+ - If the arguments contain at least one array or `Ref`, it returns an array
+   (expanding singleton dimensions), and treats `Ref`s as 0-dimensional arrays,
+   and tuples as a 1-dimensional arrays.
 
-The following additional rules apply to `Nullable` arguments:
-
- - If there is at least a `Nullable`, and all the arguments are scalars or
-   `Nullable`, it returns a `Nullable`.
- - If there is at least an array or a `Ref` with `Nullable` entries, or there
-   is at least an array or a `Ref` (perhaps with scalar entries instead of
-   `Nullable` entries) and a nullable, then the result is an array of
-   `Nullable` entries.
- - If there is a tuple and a nullable, the result is an error, as this case is
-   not currently supported.
+The following additional rule applies to `Nullable` arguments: If there is at
+least one `Nullable`, and all the arguments are scalars or `Nullable`, it
+returns a `Nullable` treating `Nullable`s as "containers".
 
 A special syntax exists for broadcasting: `f.(args...)` is equivalent to
 `broadcast(f, args...)`, and nested `f.(g.(args...))` calls are fused into a
@@ -433,21 +402,8 @@ Nullable{String}("XY")
 julia> broadcast(/, 1.0, Nullable(2.0))
 Nullable{Float64}(0.5)
 
-julia> [Nullable(1), Nullable(2), Nullable()] .* 3
-3-element Array{Nullable{Int64},1}:
- 3
- 6
- #NULL
-
-julia> [1+im, 2+2im, 3+3im] ./ Nullable{Int}()
-3-element Array{Nullable{Complex{Float64}},1}:
- #NULL
- #NULL
- #NULL
-
-julia> Ref(7) .+ Nullable(3)
-0-dimensional Array{Nullable{Int64},0}:
-10
+julia> (1 + im) ./ Nullable{Int}()
+Nullable{Complex{Float64}}()
 ```
 """
 @inline broadcast(f, A, Bs...) = broadcast_c(f, containertype(A, Bs...), A, Bs...)

base/nullable.jl

@@ -302,10 +302,6 @@ hasvalue(x) = true
 all(f::typeof(hasvalue), t::Tuple) = f(t[1]) & all(f, tail(t))
 all(f::typeof(hasvalue), t::Tuple{}) = true
 
-is_nullable_array(::Any) = false
-is_nullable_array{T}(::Type{T}) = eltype(T) <: Nullable
-is_nullable_array(A::AbstractArray) = eltype(A) <: Nullable
-
 # Overloads of null_safe_op
 # Unary operators
 

base/sparse/higherorderfns.jl

@@ -73,7 +73,7 @@ function _noshapecheck_map{Tf,N}(f::Tf, A::SparseVecOrMat, Bs::Vararg{SparseVecO
     fofzeros = f(_zeros_eltypes(A, Bs...)...)
     fpreszeros = _iszero(fofzeros)
     maxnnzC = fpreszeros ? min(length(A), _sumnnzs(A, Bs...)) : length(A)
-    entrytypeC = Base.Broadcast._broadcast_type(Any, f, A, Bs...)
+    entrytypeC = Base.Broadcast._broadcast_type(f, A, Bs...)
     indextypeC = _promote_indtype(A, Bs...)
     C = _allocres(size(A), indextypeC, entrytypeC, maxnnzC)
     return fpreszeros ? _map_zeropres!(f, C, A, Bs...) :
@@ -101,7 +101,7 @@ function _diffshape_broadcast{Tf,N}(f::Tf, A::SparseVecOrMat, Bs::Vararg{SparseV
     fofzeros = f(_zeros_eltypes(A, Bs...)...)
     fpreszeros = _iszero(fofzeros)
     indextypeC = _promote_indtype(A, Bs...)
-    entrytypeC = Base.Broadcast._broadcast_type(Any, f, A, Bs...)
+    entrytypeC = Base.Broadcast._broadcast_type(f, A, Bs...)
     shapeC = to_shape(Base.Broadcast.broadcast_indices(A, Bs...))
     maxnnzC = fpreszeros ? _checked_maxnnzbcres(shapeC, A, Bs...) : _densennz(shapeC)
     C = _allocres(shapeC, indextypeC, entrytypeC, maxnnzC)

doc/src/manual/types.md

@@ -1426,15 +1426,3 @@ conveniently using `.`-prefixed operators:
 julia> Nullable(2) ./ Nullable(3) .+ Nullable(1.0)
 Nullable{Float64}(1.66667)
 ```
-
-[`broadcast()`](@ref) also allows one to work with multiple data at the same
-time, without manually writing for loops. This enables performing the same
-operation to arrays where the data is possibly missing; for example
-
-```julia
-julia> [Nullable(2), Nullable(), Nullable(3)] .+ 3
-3-element Array{Nullable{Int64},1}:
- 5
- #NULL
- 6
-```

test/broadcast.jl

@@ -363,7 +363,7 @@ StrangeType18623(x,y) = (x,y)
 let
     f(A, n) = broadcast(x -> +(x, n), A)
     @test @inferred(f([1.0], 1)) == [2.0]
-    g() = (a = 1; Base.Broadcast._broadcast_type(Any, x -> x + a, 1.0))
+    g() = (a = 1; Base.Broadcast._broadcast_type(x -> x + a, 1.0))
     @test @inferred(g()) === Float64
 end
 
@@ -411,3 +411,10 @@ Base.Broadcast.broadcast_c(f, ::Type{Array19745}, A, Bs...) =
     @test isa(aa .+ 1, Array19745)
     @test isa(aa .* aa', Array19745)
 end
+
+# broadcast should only "peel off" one container layer
+@test get.([Nullable(1), Nullable(2)]) == [1, 2]
+let io = IOBuffer()
+    broadcast(x -> print(io, x), [Nullable(1.0)])
+    @test String(take!(io)) == "Nullable{Float64}(1.0)"
+end

test/nullable.jl

@@ -507,48 +507,6 @@ end
 @test Nullable(10.5) ===
     @inferred(broadcast(+, 1, 2, Nullable(3), Nullable(4.0), Nullable(1//2)))
 
-# broadcasting for arrays
-@test istypeequal(@inferred(broadcast(+, [1, 2, 3], Nullable{Int}(1))),
-                  Nullable{Int}[2, 3, 4])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, 2, 3], 1)),
-                  Nullable{Int}[2, 3, 4])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, 2, 3], Nullable(1))),
-                  Nullable{Int}[2, 3, 4])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, Nullable()], Nullable(1))),
-                  Nullable{Int}[2, Nullable()])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
-                                         Nullable{Int}())),
-                  Nullable{Int}[Nullable(), Nullable()])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
-                                         Nullable{Int}[1, Nullable()])),
-                  Nullable{Int}[Nullable(), Nullable()])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
-                                         Nullable{Int}[Nullable(), 1])),
-                  Nullable{Int}[Nullable(), 2])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), Nullable()],
-                                         Nullable{Int}[1, 2])),
-                  Nullable{Int}[Nullable(), Nullable()])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[Nullable(), 1],
-                                         Nullable{Int}[1])),
-                  Nullable{Int}[Nullable(), 2])
-@test istypeequal(@inferred(broadcast(+, Nullable{Float64}[1.0, 2.0],
-                                         Nullable{Float64}[1.0 2.0; 3.0 4.0])),
-                  Nullable{Float64}[2.0 3.0; 5.0 6.0])
-@test istypeequal(@inferred(broadcast(+, Nullable{Int}[1, 2], [1, 2], 1)),
-                  Nullable{Int}[3, 5])
-
-@test istypeequal(@inferred(broadcast(/, 1, Nullable{Int}[1, 2, 4])),
-                  Nullable{Float64}[1.0, 0.5, 0.25])
-@test istypeequal(@inferred(broadcast(muladd, Nullable(2), 42,
-                                      [Nullable(1337), Nullable{Int}()])),
-                  Nullable{Int}[1421, Nullable()])
-
-# heterogenous types (not inferrable)
-@test istypeequal(broadcast(+, Any[1, 1.0], Nullable(1//2)),
-                  Any[Nullable(3//2), Nullable(1.5)])
-@test istypeequal(broadcast(+, Any[Nullable(1) Nullable(1.0)], Nullable(big"1")),
-                  Any[Nullable(big"2") Nullable(big"2.0")])
-
 # test fast path taken
 for op in (+, *, -)
     for b1 in (false, true)
@@ -557,11 +515,6 @@ for op in (+, *, -)
                 @inferred(broadcast(op, Nullable{Int}(1, b1),
                                         Nullable{Int}(2, b2)))
         end
-        A = [1, 2, 3]
-        res = @inferred(broadcast(op, A, Nullable{Int}(1, b1)))
-        @test res[1] === Nullable{Int}(op(1, 1), b1)
-        @test res[2] === Nullable{Int}(op(2, 1), b1)
-        @test res[3] === Nullable{Int}(op(3, 1), b1)
     end
 end