Check that inputs and outputs do not share memory in sample! (#793)

Otherwise the result is incorrect. Unfortunately, there is no completely
reliable way to check whether two arrays alias, but this is the best effort
we can make.

For custom array types like `AbstractWeights`, `Base.mightalias` falls back to
using `Base.dataids`, which is less smart than the `SubArray` method,
generating false positives when passing two views with the same parent
but disjoint indices. In practice this is unlikely to matter, as
using the same argument for values and weights is weird.

Author: nalimilan

src/sampling.jl

@@ -445,9 +445,14 @@ items appear in the same order as in `a`) should be taken.
 
 Optionally specify a random number generator `rng` as the first argument
 (defaults to `Random.GLOBAL_RNG`).
+
+Output array `a` must not be the same object as `x` or `wv`
+nor share memory with them, or the result may be incorrect.
 """
 function sample!(rng::AbstractRNG, a::AbstractArray, x::AbstractArray;
                  replace::Bool=true, ordered::Bool=false)
+    Base.mightalias(a, x) &&
+        throw(ArgumentError("output array a must not share memory with input array x"))
     1 == firstindex(a) == firstindex(x) ||
         throw(ArgumentError("non 1-based arrays are not supported"))
     n = length(a)
@@ -893,6 +898,10 @@ efraimidis_aexpj_wsample_norep!(a::AbstractArray, wv::AbstractWeights, x::Abstra
 
 function sample!(rng::AbstractRNG, a::AbstractArray, wv::AbstractWeights, x::AbstractArray;
                  replace::Bool=true, ordered::Bool=false)
+    Base.mightalias(a, x) &&
+        throw(ArgumentError("output array a must not share memory with input array x"))
+    Base.mightalias(a, wv) &&
+        throw(ArgumentError("output array a must not share memory with weights array wv"))
     1 == firstindex(a) == firstindex(wv) == firstindex(x) ||
         throw(ArgumentError("non 1-based arrays are not supported"))
     n = length(a)

src/weights.jl

@@ -22,6 +22,8 @@ sum(wv::AbstractWeights) = wv.sum
 isempty(wv::AbstractWeights) = isempty(wv.values)
 size(wv::AbstractWeights) = size(wv.values)
 
+Base.dataids(wv::AbstractWeights) = Base.dataids(wv.values)
+
 Base.convert(::Type{Vector}, wv::AbstractWeights) = convert(Vector, wv.values)
 
 @propagate_inbounds function Base.getindex(wv::AbstractWeights, i::Integer)
@@ -347,7 +349,7 @@ uweights(::Type{T}, s::Int) where {T<:Real} = UnitWeights{T}(s)
 """
     varcorrection(w::UnitWeights, corrected=false)
 
-* `corrected=true`: ``\\frac{1}{n - 1}``, where ``n`` is the length of the weight vector
+* `corrected=true`: ``\\frac{n}{n - 1}``, where ``n`` is the length of the weight vector
 * `corrected=false`: ``\\frac{1}{n}``, where ``n`` is the length of the weight vector
 
 This definition is equivalent to the correction applied to unweighted data.

test/sampling.jl

@@ -245,3 +245,18 @@ test_same(replace=true, ordered=true)
 test_same(replace=false, ordered=true)
 test_same(replace=true, ordered=false)
 test_same(replace=false, ordered=false)
+
+# Test that sample! throws when output shares memory with inputs
+x = rand(10)
+y = rand(10)
+@test_throws ArgumentError sample!(x, x)
+@test_throws ArgumentError sample!(x, weights(y), x)
+@test_throws ArgumentError sample!(x, weights(x), y)
+@test_throws ArgumentError sample!(x, weights(x), x)
+@test_throws ArgumentError sample!(view(x, 2:4), view(x, 3:5))
+@test_throws ArgumentError sample!(view(x, 2:4), weights(view(x, 3:5)), y)
+@test_throws ArgumentError sample!(view(x, 2:4), weights(view(x, 3:5)), view(x, 1:2))
+# These corner cases should theoretically succeed
+# but the second currently fails as Base.mightalias is not smart enough
+sample!(view(x, 2:4), view(x, 5:6))
+@test_broken sample!(view(x, 2:4), weights(view(x, 5:6)), y)

test/weights.jl

@@ -21,6 +21,8 @@ weight_funcs = (weights, aweights, fweights, pweights)
     @test wv ==  w
     @test sum(wv) === 6.0
     @test !isempty(wv)
+    @test Base.mightalias(w, wv)
+    @test !Base.mightalias([1], wv)
 
     b  = trues(3)
     bv = f(b)