Merge #785

785: Add and fix Aqua tests r=charleskawczynski a=charleskawczynski This PR adds [Aqua](https://github.com/JuliaTesting/Aqua.jl) tests, specifically, `Aqua.test_unbound_args(ClimaCore)`, which tests that no functions defined in ClimaCore run into [this julia issue](JuliaLang/julia#29393). Adding this test revealed 9 instances of this issue, and the PR also includes fixes so that we pass all of the tests. Co-authored-by: Charles Kawczynski <[email protected]>
CliMA · Jul 1, 2022 · 25a4c04 · 25a4c04
2 parents c139012 + f23fb10
commit 25a4c04
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 26 deletions.
diff --git a/src/DataLayouts/broadcast.jl b/src/DataLayouts/broadcast.jl
@@ -261,7 +261,7 @@ end
 function Base.similar(
     ::Union{IF{<:Any, Ni, A}, Broadcast.Broadcasted{IFStyle{Ni, A}}},
     ::Type{Eltype},
-) where {S, Ni, A, Eltype}
+) where {Ni, A, Eltype}
     Nf = typesize(eltype(A), Eltype)
     array = MArray{Tuple{Ni, Nf}, eltype(A), 2, Ni * Nf}(undef)
     return IF{Eltype, Ni}(array)
@@ -432,8 +432,8 @@ end
 
 function Base.copyto!(
     dest::IJFH{S, Nij},
-    bc::Union{IJFH{S, Nij}, Base.Broadcast.Broadcasted{IJFHStyle{Nij, A}}},
-) where {S, Nij, A}
+    bc::Union{IJFH{S, Nij}, Base.Broadcast.Broadcasted{<:IJFHStyle{Nij}}},
+) where {S, Nij}
     _, _, _, _, Nh = size(bc)
     @inbounds for h in 1:Nh
         slab_dest = slab(dest, h)
@@ -445,8 +445,8 @@ end
 
 function Base.copyto!(
     dest::IFH{S, Ni},
-    bc::Union{IFH{S, Ni}, Base.Broadcast.Broadcasted{IFHStyle{Ni, A}}},
-) where {S, Ni, A}
+    bc::Union{IFH{S, Ni}, Base.Broadcast.Broadcasted{<:IFHStyle{Ni}}},
+) where {S, Ni}
     _, _, _, _, Nh = size(bc)
     @inbounds for h in 1:Nh
         slab_dest = slab(dest, h)

diff --git a/src/Fields/field_iterator.jl b/src/Fields/field_iterator.jl
@@ -47,7 +47,6 @@ function isa_12_covariant_field(
     f::Type{CF},
 ) where {
     FT,
-    T,
     CF <: Geometry.AxisTensor{
         FT,
         1,
@@ -62,7 +61,6 @@ function isa_3_covariant_field(
     f::Type{CF},
 ) where {
     FT,
-    T,
     CF <: Geometry.AxisTensor{
         FT,
         1,

diff --git a/src/Geometry/axistensors.jl b/src/Geometry/axistensors.jl
@@ -288,8 +288,7 @@ end
 # LinearAlgebra
 
 check_axes(::A, ::A) where {A} = nothing
-check_axes(ax1, ax2) where {A} =
-    throw(DimensionMismatch("$ax1 and $ax2 do not match"))
+check_axes(ax1, ax2) = throw(DimensionMismatch("$ax1 and $ax2 do not match"))
 
 check_dual(ax1, ax2) = _check_dual(ax1, ax2, dual(ax2))
 _check_dual(::A, _, ::A) where {A} = nothing

diff --git a/src/Operators/stencilcoefs.jl b/src/Operators/stencilcoefs.jl
@@ -9,7 +9,7 @@ struct StencilCoefs{lbw, ubw, C <: NTuple}
     coefs::C
 end
 
-function StencilCoefs{lbw, ubw}(coefs::NTuple{N, T}) where {lbw, ubw, N, T}
+function StencilCoefs{lbw, ubw}(coefs::NTuple{N}) where {lbw, ubw, N}
     if !(
         (lbw isa Integer && ubw isa Integer) ||
         (lbw isa PlusHalf && ubw isa PlusHalf)
@@ -23,6 +23,7 @@ function StencilCoefs{lbw, ubw}(coefs::NTuple{N, T}) where {lbw, ubw, N, T}
     if N == 0 # no reason to support edge case of an empty stencil
         error("Stencil cannot be empty")
     end
+    T = eltype(coefs)
     if !isconcretetype(T) # must be compatible with DataLayouts
         error("Stencil coefficients must all have the same concrete type")
     end

diff --git a/src/Spaces/Spaces.jl b/src/Spaces/Spaces.jl
@@ -38,4 +38,7 @@ include("hybrid.jl")
 include("triangulation.jl")
 include("dss.jl")
 
+horizontal_space(space::ExtrudedFiniteDifferenceSpace) = space.horizontal_space
+horizontal_space(space::AbstractSpace) = space
+
 end # module
diff --git a/src/Spaces/dss.jl b/src/Spaces/dss.jl
@@ -691,14 +691,31 @@ Create the ghost buffer, if necessary, load the send buffer and start communicat
 
 Part of [`Spaces.weighted_dss!`](@ref).
 """
-function weighted_dss_start!(
+weighted_dss_start!(data, space, ghost_buffer) =
+    weighted_dss_start!(data, space, horizontal_space(space), ghost_buffer)
+
+weighted_dss_start!(
+    data,
+    space::ExtrudedFiniteDifferenceSpace{S, H},
+    hspace,
+    ghost_buffer,
+) where {S, H <: SpectralElementSpace2D{<:DistributedTopology2D}} =
+    _weighted_dss_start!(data, space, hspace, ghost_buffer)
+
+weighted_dss_start!(
     data,
-    space::Union{ExtrudedFiniteDifferenceSpace{S, H}, H},
+    space::H,
+    hspace,
     ghost_buffer,
-) where {S, T <: DistributedTopology2D, H <: SpectralElementSpace2D{T}}
-    topology =
-        space isa ExtrudedFiniteDifferenceSpace ?
-        space.horizontal_space.topology : space.topology
+) where {H <: SpectralElementSpace2D{<:DistributedTopology2D}} =
+    _weighted_dss_start!(data, space, hspace, ghost_buffer)
+
+function weighted_dss_start!(data, space, hspace, ghost_buffer)
+    return nothing
+end
+
+function _weighted_dss_start!(data, space, hspace, ghost_buffer)
+    topology = hspace.topology
     if ghost_buffer isa GhostBuffer
         # 1) copy send data to buffer
         fill_send_buffer!(topology, data, ghost_buffer)
@@ -708,10 +725,6 @@ function weighted_dss_start!(
     return nothing
 end
 
-function weighted_dss_start!(data, space, ghost_buffer)
-    return nothing
-end
-
 """
     weighted_dss_internal!(data, space, ghost_buffer)
 
@@ -747,13 +760,28 @@ Complete communication. Perform weighted dss for ghost faces and vertices.
 
 Part of [`Spaces.weighted_dss!`](@ref).
 """
+weighted_dss_ghost!(data, space, ghost_buffer) =
+    weighted_dss_ghost!(data, space, horizontal_space(space), ghost_buffer)
+
 function weighted_dss_ghost!(
     data,
-    space::Union{ExtrudedFiniteDifferenceSpace{S, H}, H},
+    space::ExtrudedFiniteDifferenceSpace{S, H},
+    hspace,
     ghost_buffer,
-) where {S, T <: DistributedTopology2D, H <: SpectralElementSpace2D{T}}
-    hspace =
-        space isa ExtrudedFiniteDifferenceSpace ? space.horizontal_space : space
+) where {S, H <: SpectralElementSpace2D{<:DistributedTopology2D}}
+    _weighted_dss_ghost!(data, space, hspace, ghost_buffer)
+end
+
+function weighted_dss_ghost!(
+    data,
+    space::H,
+    hspace,
+    ghost_buffer,
+) where {H <: SpectralElementSpace2D{<:DistributedTopology2D}}
+    _weighted_dss_ghost!(data, space, hspace, ghost_buffer)
+end
+
+function _weighted_dss_ghost!(data, space, hspace, ghost_buffer)
     topology = hspace.topology
     local_geometry = local_geometry_data(space)
     local_weights = hspace.local_dss_weights
@@ -793,6 +821,6 @@ function weighted_dss_ghost!(
     return data
 end
 
-function weighted_dss_ghost!(data, space, ghost_buffer)
+function weighted_dss_ghost!(data, space, hspace, ghost_buffer)
     return data
 end
diff --git a/test/Project.toml b/test/Project.toml
@@ -1,4 +1,5 @@
 [deps]
+Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
 AssociatedLegendrePolynomials = "2119f1ac-fb78-50f5-8cc0-dda848ebdb19"
 BlockArrays = "8e7c35d0-a365-5155-bbbb-fb81a777f24e"
 CUDA = "052768ef-5323-5732-b1bb-66c8b64840ba"
@@ -27,8 +28,9 @@ Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 UnPack = "3a884ed6-31ef-47d7-9d2a-63182c4928ed"
 
 [compat]
-OrdinaryDiffEq = "5.64.0"
+Aqua = "0.5"
 DiffEqBase = "~6.84.0"
+OrdinaryDiffEq = "5.64.0"
 
 [extras]
 CPUSummary = "2a0fbf3d-bb9c-48f3-b0a9-814d99fd7ab9"
diff --git a/test/aqua.jl b/test/aqua.jl
@@ -0,0 +1,12 @@
+using Test
+import ClimaCore
+using Aqua
+
+@testset "Aqua tests - unbound args" begin
+    # This tests that we don't accidentally run into
+    # https://github.com/JuliaLang/julia/issues/29393
+    Aqua.test_unbound_args(ClimaCore)
+
+    # returns a vector of all unbound args
+    # ua = Aqua.detect_unbound_args_recursively(ClimaCore)
+end
diff --git a/test/runtests.jl b/test/runtests.jl
@@ -60,6 +60,7 @@ using Base: operator_associativity
 
 @time include("Limiters/limiter.jl")
 @time include("Limiters/distributed.jl")
+@time include("aqua.jl") # Code quality checks
 
 if "CUDA" in ARGS
     @time include("gpu/cuda.jl")