Handle non-bijective data-type conversions more robustly (#240)

mathieu17g · web-flow · commit 8d6e1be2790a · 2021-09-02T20:41:35.000-07:00
* Enrich docstring of `@convert` macro

* Add some control on `@convert` macro arguments

* Add CEnum.jl dependency for `@convert` macro arguments checks

* Move import CEnum in utils.jl to using CEnum in ArchGDAL.jl

* Removed in `@convert`macro a duplicate `@assert` on subtypes 2 when type 2 is an `UnionAll`

* Added tests on default types for non bijective conversion mapsets

* Formatted with JuliaFormatter

* Moved from Dict to ImmutableDict in `@convert` macro and modified relevant test cases accordingly

* Formatted with JuliaFormatter

* Suppressed ImmutableDict helper function in utils.jl. It works without

* For Julia version &lt; 1.5, add an `ImmutableDict `constructor based on a list of `Pair` arguments (available in Base since Julia 1.5)

* ... add Base prefix for ImmutableDict constructor based on a list of Pair arguments

* ... one more Base prefix added

* ... fallback to an `ImmutableDict` constructor based on a list of `Pair` arguments, based on a loop
diff --git a/Project.toml b/Project.toml
@@ -6,6 +6,7 @@ desc = "A high level API for GDAL - Geospatial Data Abstraction Library"
 version = "0.7.4"
 
 [deps]
+CEnum = "fa961155-64e5-5f13-b03f-caf6b980ea82"
 ColorTypes = "3da002f7-5984-5a60-b8a6-cbb66c0b333f"
 Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
 DiskArrays = "3c3547ce-8d99-4f5e-a174-61eb10b00ae3"
@@ -16,6 +17,7 @@ ImageCore = "a09fc81d-aa75-5fe9-8630-4744c3626534"
 Tables = "bd369af6-aec1-5ad0-b16a-f7cc5008161c"
 
 [compat]
+CEnum = "0.4"
 ColorTypes = "0.10, 0.11"
 DiskArrays = "0.2.4"
 GDAL = "1.1.3"
diff --git a/src/ArchGDAL.jl b/src/ArchGDAL.jl
@@ -7,6 +7,7 @@ using GeoInterface: GeoInterface
 using Tables: Tables
 using ImageCore: Normed, N0f8, N0f16, N0f32, ImageCore
 using ColorTypes: ColorTypes
+using CEnum
 
 const GFT = GeoFormatTypes
 
diff --git a/src/utils.jl b/src/utils.jl
@@ -1,39 +1,87 @@
+# An ImmutableDict constructor based on a list of Pair arguments has been introduced in Julia 1.6.0 and backported to Julia 1.5
+if VERSION < v"1.5"
+    function Base.ImmutableDict(KV::Pair{K,V}, KVs::Pair{K,V}...) where {K,V}
+        d = Base.ImmutableDict(KV)
+        for p in KVs
+            d = Base.ImmutableDict(d, p)
+        end
+        return d
+    end
+end
+
 """
-    @convert(args...)
+    @convert(<T1>::<T2>, 
+        <conversions>
+    )
+
+Generate `convert` functions both ways between ArchGDAL Enum of typeids (e.g. `ArchGDAL.OGRFieldType`) 
+and other types or typeids.
+
+ArchGDAL uses Enum types, listing typeids of various data container used in GDAL/OGR object model. 
+Some of these types are used to implement concrete types in julia through parametric composite types 
+based on those Enum of typeids (e.g. `Geometry` and `IGeometry` types with `OGRwkbGeometryType`)
+
+Other types or typeids can be:
+- GDAL CEnum.Cenum typeids (e.g. `GDAL.OGRFieldType`), 
+- Base primitive DataType types (e.g. `Bool`), 
+- other parametric composite types (e.g. `ImageCore.Normed`)
+
+# Arguments
+- `(<T1>::<T2>)::Expr`: source and target supertypes, where `T1<:Enum`  and `T2<:CEnum.Cenum || T2::Type{DataType} || T2::UnionAll}``
+- `(<stype1>::<stype2>)::Expr`: source and target subtypes or type ids with `stype1::T1` and 
+    - `stype2::T2 where T2<:CEnum.Cenum` or 
+    - `stype2::T2 where T2::Type{DataType}` or 
+    - `stype2<:T2`where T2<:UnionAll
+- ...
+
+**Note:** In the case where the mapping is not bijective, the last declared typeid of subtype is used. 
+Example: 
+```
+@convert(
+    OGRFieldType::DataType,
+    OFTInteger::Bool,
+    OFTInteger::Int16,
+    OFTInteger::Int32,
+)
+```
+will generate a `convert` functions giving:
+- `Int32` type for `OFTInteger` and not `Ìnt16`
+- `OFTInteger` OGRFieldType typeid for both `Int16` and `Int32`
 
-# General case:
+# Usage
+### General case:
 ```
-eval(@convert(GDALRWFlag::GDAL.GDALRWFlag,
+@convert(GDALRWFlag::GDAL.GDALRWFlag,
     GF_Read::GDAL.GF_Read,
     GF_Write::GDAL.GF_Write,
-))
+)
 ```
 does the equivalent of 
 ```
-const GDALRWFlag_to_GDALRWFlag_map = Dict(
+const GDALRWFlag_to_GDALRWFlag_map = ImmutableDict(
     GF_Read => GDAL.GF_Read,
     GF_Write => GDAL.GF_Write
 )
 Base.convert(::Type{GDAL.GDALRWFlag}, ft::GDALRWFlag) =
     GDALRWFlag_to_GDALRWFlag_map[ft]
 
-const GDALRWFlag_to_GDALRWFlag_map = Dict(
+const GDALRWFlag_to_GDALRWFlag_map = ImmutableDict(
     GDAL.GF_Read => GF_Read, 
     GDAL.GF_Write => GF_Write
 )
 Base.convert(::Type{GDALRWFlag}, ft::GDAL.GDALRWFlag) =
     GDALRWFlag_to_GDALRWFlag_map[ft]
 ```
-# Case where 1st type `<: Enum` and 2nd type `== DataType` or ìsa UnionAll`:
+### Case where 1st type `<: Enum` and 2nd type `== DataType` or `ìsa UnionAll`:
 ```
-eval(@convert(OGRFieldType::DataType,
+@convert(OGRFieldType::DataType,
     OFTInteger::Bool,
     OFTInteger::Int16,
-))
+)
 ```
 does the equivalent of
 ```
-const OGRFieldType_to_DataType_map = Dict(
+const OGRFieldType_to_DataType_map = ImmutableDict(
     OFTInteger => Bool, 
     OFTInteger => Int16,
 )
@@ -46,46 +94,48 @@ Base.convert(::Type{OGRFieldType}, ft::Type{Int16}) = OFTInteger
 
 """
 macro convert(args...)
-    @assert length(args) > 0
+    @assert length(args) > 1
     @assert args[1].head == :(::)
-    type1_symbol = args[1].args[1]
-    type2_symbol = args[1].args[2]
-    type1_string = replace(string(type1_symbol), "." => "_")
-    type2_string = replace(string(type2_symbol), "." => "_")
-    type1_isenum_and_type2_equaldatatype_or_isaunionall =
-        (eval(type1_symbol) <: Enum) &&
-        ((eval(type2_symbol) == DataType) || (eval(type2_symbol) isa UnionAll))
-    type1 = esc(type1_symbol)
-    type2 = esc(type2_symbol)
-    fwd_map = Expr[Expr(:tuple, esc.(a.args)...) for a in args[2:end]]
-    if type1_isenum_and_type2_equaldatatype_or_isaunionall
-        rev_to_enum_map = [Tuple(esc.(reverse(a.args))) for a in args[2:end]]
-    else
-        rev_map =
-            Expr[Expr(:tuple, esc.(reverse(a.args))...) for a in args[2:end]]
-    end
+    (T1, T2) = args[1].args
+
+    # Types and type ids / subtypes checks
+    stypes1, stypes2 = zip((eval.(a.args) for a in args[2:end])...)
+    @assert(eval(T1) <: Enum && all(isa.(stypes1, eval(T1))))
+    @assert(
+        ((eval(T2) <: CEnum.Cenum) && all(isa.(stypes2, eval(T2)))) ||
+        ((eval(T2) isa Type{DataType}) && all(isa.(stypes2, eval(T2)))) ||
+        ((eval(T2) isa UnionAll) && all((<:).(stypes2, eval(T2))))
+    )
+
+    # Types other representations
+    (T1_string, T2_string) = replace.(string.((T1, T2)), "." => "_")
+    (type1, type2) = esc.((T1, T2))
+
+    # Subtypes forward and backward mapping
+    fwd_map = [Expr(:call, esc(:Pair), esc.(a.args)...) for a in args[2:end]]
+    rev_map =
+        [Expr(:call, esc(:Pair), esc.(reverse(a.args))...) for a in args[2:end]]
 
+    #! Convert functions generation
     result_expr = Expr(:block)
-    # Forward conversion (no case of 1st type == DataType and 2nd type <: Enum handled)
-    type1_to_type2_map_name =
-        esc(Symbol(type1_string * "_to_" * type2_string * "_map"))
+
+    #* Forward conversions from ArchGDAL typeids
+    T1_to_T2_map_name = esc(Symbol(T1_string * "_to_" * T2_string * "_map"))
     push!(
         result_expr.args,
-        :(const $type1_to_type2_map_name = Dict([$(fwd_map...)])),
+        :(const $T1_to_T2_map_name = Base.ImmutableDict([$(fwd_map...)]...)),
     )
     push!(
         result_expr.args,
         :(function Base.convert(::Type{$type2}, ft::$type1)
-            return get($type1_to_type2_map_name, ft) do
-                return error("Unknown type: $ft")
-            end
+            return $T1_to_T2_map_name[ft]
         end),
     )
-    # Reverse conversion
-    if type1_isenum_and_type2_equaldatatype_or_isaunionall
-        for stypes in rev_to_enum_map
-            eval(type2_symbol) isa UnionAll &&
-                @assert eval(stypes[1].args[1]) <: eval(type2_symbol)
+
+    #* Reverse conversions to ArchGDAL typeids
+    # Optimization for conversion from types
+    if !(eval(T2) <: CEnum.Cenum)
+        for stypes in [Tuple(esc.(reverse(a.args))) for a in args[2:end]]
             push!(
                 result_expr.args,
                 :(
@@ -98,22 +148,24 @@ macro convert(args...)
                 ),
             )
         end
+        # Conversion from typeids
     else
-        type2_to_type1_map_name =
-            esc(Symbol(type2_string * "_to_" * type1_string * "_map"))
+        T2_to_T1_map_name = esc(Symbol(T2_string * "_to_" * T1_string * "_map"))
         push!(
             result_expr.args,
-            :(const $type2_to_type1_map_name = Dict([$(rev_map...)])),
+            :(
+                const $T2_to_T1_map_name =
+                    Base.ImmutableDict([$(rev_map...)]...)
+            ),
         )
         push!(
             result_expr.args,
             :(function Base.convert(::Type{$type1}, ft::$type2)
-                return get($type2_to_type1_map_name, ft) do
-                    return error("Unknown type: $ft")
-                end
+                return $T2_to_T1_map_name[ft]
             end),
         )
     end
+
     return result_expr
 end
 
diff --git a/test/test_types.jl b/test/test_types.jl
@@ -4,6 +4,32 @@ const AG = ArchGDAL;
 import ImageCore
 
 @testset "test_types.jl" begin
+    @testset "Testing convert macro" begin
+        @testset "Basic conversions" begin
+            @test Base.convert(AG.GDALDataType, UInt8) == AG.GDT_Byte
+        end
+
+        @testset "Error on conversion non implemented" begin
+            @test_throws MethodError Base.convert(AG.GDALDataType, Int64)
+            @test_throws KeyError Base.convert(DataType, AG.GDT_TypeCount)
+            @test_throws KeyError Base.convert(ImageCore.Normed, AG.GDT_Float32)
+            @test_throws KeyError Base.convert(DataType, AG.OFSTMaxSubType)
+        end
+
+        @testset "Default types for non bijective conversion mapset" begin
+            # Default type for AG.OFTInteger is Int32
+            @test Base.convert(AG.OGRFieldType, Bool) == AG.OFTInteger
+            @test Base.convert(AG.OGRFieldType, Int16) == AG.OFTInteger
+            @test Base.convert(AG.OGRFieldType, Int32) == AG.OFTInteger
+            @test Base.convert(DataType, AG.OFTInteger) == Int32
+
+            # Default type for AG.OFTReal is Float64 
+            @test Base.convert(AG.OGRFieldType, Float32) == AG.OFTReal
+            @test Base.convert(AG.OGRFieldType, Float64) == AG.OFTReal
+            @test Base.convert(DataType, AG.OFTReal) == Float64
+        end
+    end
+
     @testset "Testing GDAL Type Methods" begin
         @testset "GDAL Open Flags" begin
             @test AG.OF_READONLY | 0x04 == 0x04
@@ -21,18 +47,6 @@ import ImageCore
 
             @test AG.iscomplex(AG.GDT_CFloat32) == true
             @test AG.iscomplex(AG.GDT_CFloat64) == true
-
-            @test Base.convert(AG.GDALDataType, UInt8) == AG.GDT_Byte
-            @test_throws MethodError Base.convert(AG.GDALDataType, Int64)
-            @test_throws ErrorException Base.convert(DataType, AG.GDT_TypeCount)
-            @test_throws ErrorException Base.convert(
-                ImageCore.Normed,
-                AG.GDT_Float32,
-            )
-            @test_throws ErrorException Base.convert(
-                DataType,
-                AG.OFSTMaxSubType,
-            )
         end
 
         @testset "GDAL Colors and Palettes" begin
