Merge #1276

1276: Fix and test boundscheck in DataLayouts for empty fields r=charleskawczynski a=charleskawczynski

Closes #1272.

This PR at least fixes the bug (imprecise bounds checks). I plan to open an issue about whether we want empty fields to traverse the space or not-- I see no application where traversing the space would provide useable functionality. However, I could see it incurring unnecessary cost.

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>

Author: bors[bot]

src/DataLayouts/DataLayouts.jl

@@ -356,21 +356,23 @@ end
 end
 
 @inline function slab(data::IJFH{S, Nij}, h::Integer) where {S, Nij}
-    @boundscheck (1 <= h <= length(data)) || throw(BoundsError(data, (h,)))
+    @boundscheck (1 <= h <= size(parent(data), 4)) ||
+                 throw(BoundsError(data, (h,)))
     dataview = @inbounds view(parent(data), :, :, :, h)
     IJF{S, Nij}(dataview)
 end
 
 @inline function slab(data::IJFH{S, Nij}, v::Integer, h::Integer) where {S, Nij}
-    @boundscheck (v >= 1 && 1 <= h <= length(data)) ||
+    @boundscheck (v >= 1 && 1 <= h <= size(parent(data), 4)) ||
                  throw(BoundsError(data, (v, h)))
     dataview = @inbounds view(parent(data), :, :, :, h)
     IJF{S, Nij}(dataview)
 end
 
 @inline function column(data::IJFH{S, Nij}, i, j, h) where {S, Nij}
-    @boundscheck (1 <= j <= Nij && 1 <= i <= Nij && 1 <= h <= length(data)) ||
-                 throw(BoundsError(data, (i, j, h)))
+    @boundscheck (
+        1 <= j <= Nij && 1 <= i <= Nij && 1 <= h <= size(parent(data), 4)
+    ) || throw(BoundsError(data, (i, j, h)))
     dataview = @inbounds view(parent(data), i, j, :, h)
     DataF{S}(dataview)
 end
@@ -449,14 +451,15 @@ function Base.fill!(data::IFH, val)
 end
 
 @inline function slab(data::IFH{S, Ni}, h::Integer) where {S, Ni}
-    @boundscheck (1 <= h <= length(data)) || throw(BoundsError(data, (h,)))
+    @boundscheck (1 <= h <= size(parent(data), 3)) ||
+                 throw(BoundsError(data, (h,)))
     dataview = @inbounds view(parent(data), :, :, h)
     IF{S, Ni}(dataview)
 end
 Base.@propagate_inbounds slab(data::IFH, v::Integer, h::Integer) = slab(data, h)
 
 @inline function column(data::IFH{S, Ni}, i, h) where {S, Ni}
-    @boundscheck (1 <= h <= length(data) && 1 <= i <= Ni) ||
+    @boundscheck (1 <= h <= size(parent(data), 3) && 1 <= i <= Ni) ||
                  throw(BoundsError(data, (i, h)))
     dataview = @inbounds view(parent(data), i, :, h)
     DataF{S}(dataview)
@@ -969,7 +972,7 @@ end
 end
 
 @inline function Base.setindex!(data::VF{S}, val, v::Integer) where {S}
-    @boundscheck (1 <= v <= length(parent(data))) ||
+    @boundscheck (1 <= v <= size(parent(data), 1)) ||
                  throw(BoundsError(data, (v,)))
     @inbounds set_struct!(
         parent(data),

test/Fields/field.jl

@@ -94,6 +94,46 @@ end
     @test axes(point_field) isa Spaces.PointSpace
 end
 
+# Requires `--check-bounds=yes`
+@testset "Constructing & broadcasting over empty fields" begin
+    FT = Float32
+    for space in TU.all_spaces(FT)
+        f = TU.FieldFromNamedTuple(space, (;))
+        @. f += f
+    end
+
+    function test_broken_throws(f)
+        try
+            @. f += 1
+            # we want to throw exception, test is broken
+            @test_broken false
+        catch
+            # we want to throw exception, unexpected pass
+            @test_broken true
+        end
+    end
+    empty_field(space) = TU.FieldFromNamedTuple(space, (;))
+
+    # Broadcasting over the wrong size should error
+    test_broken_throws(empty_field(TU.PointSpace(FT)))
+    test_broken_throws(empty_field(TU.SpectralElementSpace1D(FT)))
+    test_broken_throws(empty_field(TU.SpectralElementSpace2D(FT)))
+    test_broken_throws(empty_field(TU.ColumnCenterFiniteDifferenceSpace(FT)))
+    test_broken_throws(empty_field(TU.ColumnFaceFiniteDifferenceSpace(FT)))
+    test_broken_throws(empty_field(TU.SphereSpectralElementSpace(FT)))
+    test_broken_throws(empty_field(TU.CenterExtrudedFiniteDifferenceSpace(FT)))
+    test_broken_throws(empty_field(TU.FaceExtrudedFiniteDifferenceSpace(FT)))
+
+    # TODO: performance optimization: shouldn't we do
+    #       nothing when broadcasting over empty fields?
+    #       This is otherwise a performance penalty if
+    #       users regularly rely on empty fields. In particular:
+    #        - does iterating over empty fields load data?
+    #        - what is the overhead in iterating over empty fields?
+    #        - what is the use case of anything useful that can be
+    #          done by iterating over empty fields?
+end
+
 @testset "Broadcasting interception for tuple-valued fields" begin
     n1 = n2 = 1
     Nij = 4