Enhance decoding to include offset_to properties

.credo.exs

@@ -124,7 +124,7 @@
         {Credo.Check.Refactor.MatchInCondition, []},
         {Credo.Check.Refactor.NegatedConditionsInUnless, []},
         {Credo.Check.Refactor.NegatedConditionsWithElse, []},
-        {Credo.Check.Refactor.Nesting, []},
+        {Credo.Check.Refactor.Nesting, [max_nesting: 3]},
         {Credo.Check.Refactor.UnlessWithElse, []},
         {Credo.Check.Refactor.WithClauses, []},
 

.github/workflows/ci.yml

@@ -8,15 +8,15 @@ on:
 
 env:
   MIX_ENV: test
-  
+
 jobs:
   build:
     runs-on: ubuntu-latest
     name: OTP ${{matrix.otp}} / Elixir ${{matrix.elixir}}
     strategy:
       matrix:
-        otp: ['23.3', '24.0']
-        elixir: ['1.12.1', '1.11.4']
+        otp: ['28.5', '29.0']
+        elixir: ['1.18.4', '1.19.0']
     steps:
     - uses: actions/checkout@v2
     - uses: erlef/setup-beam@v1
@@ -30,4 +30,4 @@ jobs:
     - name: Run tests
       run: mix test
     - name: Run dialyzer
-      run: mix dialyzer
+      run: mix dialyzer

lib/decode_error.ex

@@ -0,0 +1,4 @@
+defmodule Exgencode.DecodeError do
+  @moduledoc false
+  defexception [:message]
+end

lib/exgencode.ex

@@ -346,6 +346,12 @@ defmodule Exgencode do
         {field_name, props[:decode]}
       end)
 
+    offset_targets =
+      for {field_name, props} <- field_list, props[:offset_to] != nil, into: %{} do
+        {props[:offset_to], field_name}
+      end
+      |> Macro.escape()
+
     struct_fields =
       for {field_name, props} <- field_list, props[:type] not in [:constant, :skip] do
         {field_name, props[:default]}
@@ -376,15 +382,24 @@ defmodule Exgencode do
         end
 
         def decode(pdu, binary, version) do
-          do_decode(pdu, binary, unquote(fields_for_decodes), version)
+          do_decode(pdu, binary, unquote(fields_for_decodes), version, bit_size(binary))
         end
 
-        defp do_decode(pdu, binary, [{field, decode_fun} | rest], version) do
+        defp do_decode(pdu, binary, [{field, decode_fun} | rest], version, pdu_bit_size) do
+          binary =
+            Exgencode.EncodeDecode.skip_to_offset(
+              pdu,
+              field,
+              binary,
+              pdu_bit_size,
+              unquote(offset_targets)
+            )
+
           {new_pdu, rest_binary} = decode_fun.(version).(pdu, binary)
-          do_decode(new_pdu, rest_binary, rest, version)
+          do_decode(new_pdu, rest_binary, rest, version, pdu_bit_size)
         end
 
-        defp do_decode(pdu, rest_bin, [], _) do
+        defp do_decode(pdu, rest_bin, [], _, _) do
           {pdu, rest_bin}
         end
       end

lib/exgencode/encode_decode.ex

@@ -303,6 +303,45 @@ defmodule Exgencode.EncodeDecode do
     wrap_conditional_decode(props, basic_fun)
   end
 
+  def skip_to_offset(pdu, field, rest_binary, pdu_bit_size, offset_targets) do
+    case offset_targets do
+      %{^field => offset_field} ->
+        do_skip_to_offset(pdu, field, offset_field, rest_binary, pdu_bit_size)
+
+      _ ->
+        rest_binary
+    end
+  end
+
+  defp do_skip_to_offset(pdu, field, offset_field, rest_binary, pdu_bit_size) do
+    case Map.get(pdu, offset_field) do
+      offset when offset in [0, nil] ->
+        rest_binary
+
+      offset ->
+        cursor = pdu_bit_size - bit_size(rest_binary)
+        target = offset * 8
+
+        cond do
+          target == cursor ->
+            rest_binary
+
+          target < cursor ->
+            raise Exgencode.DecodeError,
+                  "offset field #{inspect(offset_field)} of #{inspect(field)} cannot point backwards!"
+
+          target > pdu_bit_size ->
+            raise Exgencode.DecodeError,
+                  "offset_field #{inspect(offset_field)} cannot point outside binary!"
+
+          true ->
+            gap_bits = target - cursor
+            <<_padding::size(gap_bits), rest::bitstring>> = rest_binary
+            rest
+        end
+    end
+  end
+
   def wrap_custom_encode(field_name, encode_fun) do
     quote do
       fn pdu ->

lib/exgencode/validator.ex

@@ -126,6 +126,12 @@ defmodule Exgencode.Validator do
   end
 
   def validate_pdu(pdu_name, fields) do
+    validate_pdu_size(pdu_name, fields)
+    validate_offset_ordering(pdu_name, fields)
+    validate_no_duplicate_offsets(pdu_name, fields)
+  end
+
+  defp validate_pdu_size(pdu_name, fields) do
     total_size =
       fields
       |> Enum.reject(fn {_field_name, props} -> props[:type] == :variable end)
@@ -143,6 +149,40 @@ defmodule Exgencode.Validator do
         )
   end
 
+  defp validate_offset_ordering(pdu_name, fields) do
+    Enum.reduce(fields, [], fn {field_name, props}, seen_names ->
+      target = props[:offset_to]
+
+      if not is_nil(target) and target in seen_names do
+        raise ArgumentError,
+              "#{inspect(Macro.to_string(pdu_name))} #{inspect(target)}: backward offsets are unsupported!"
+      end
+
+      [field_name | seen_names]
+    end)
+  end
+
+  defp validate_no_duplicate_offsets(pdu_name, fields) do
+    Enum.reduce(fields, [], fn {field_name, props}, seen_targets ->
+      target = props[:offset_to]
+
+      cond do
+        is_nil(target) ->
+          seen_targets
+
+        target in seen_targets ->
+          raise_argument_error(
+            pdu_name,
+            field_name,
+            "multiple offset fields pointing to target #{inspect(target)} is unsupported!"
+          )
+
+        true ->
+          [target | seen_targets]
+      end
+    end)
+  end
+
   defp raise_argument_error(pdu_name, field_name, msg) do
     raise ArgumentError,
           "Badly defined field #{inspect(field_name)} in #{inspect(pdu_name |> Macro.to_string())} - " <>

mix.exs

@@ -4,8 +4,8 @@ defmodule Exgencode.Mixfile do
   def project do
     [
       app: :exgencode,
-      version: "2.5.2",
-      elixir: "~> 1.7",
+      version: "2.5.3",
+      elixir: "~> 1.18",
       start_permanent: Mix.env() == :prod,
       deps: deps(),
       package: package(),
@@ -26,8 +26,8 @@ defmodule Exgencode.Mixfile do
   defp deps do
     [
       {:ex_doc, "~> 0.20", only: :dev, runtime: false},
-      {:credo, "~> 1.0", only: [:dev, :test]},
-      {:dialyxir, "~> 1.1.0", only: [:dev, :test], runtime: false}
+      {:credo, "~> 1.7.13", only: [:dev, :test]},
+      {:dialyxir, "~> 1.4.0", only: [:dev, :test], runtime: false}
     ]
   end
 

mix.lock

@@ -1,12 +1,12 @@
 %{
-  "bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"},
-  "credo": {:hex, :credo, "1.5.6", "e04cc0fdc236fefbb578e0c04bd01a471081616e741d386909e527ac146016c6", [:mix], [{:bunt, "~> 0.2.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2.8", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "4b52a3e558bd64e30de62a648518a5ea2b6e3e5d2b164ef5296244753fc7eb17"},
-  "dialyxir": {:hex, :dialyxir, "1.1.0", "c5aab0d6e71e5522e77beff7ba9e08f8e02bad90dfbeffae60eaf0cb47e29488", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "07ea8e49c45f15264ebe6d5b93799d4dd56a44036cf42d0ad9c960bc266c0b9a"},
+  "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"},
+  "credo": {:hex, :credo, "1.7.19", "cc52129665fc7c15143d47838fda0f9cd6dac9ceced7bf4da6f85fcbfe64b12a", [:mix], [{:bunt, "~> 0.2.1 or ~> 1.0", [hex: :bunt, repo: "hexpm", optional: false]}, {:file_system, "~> 0.2 or ~> 1.0", [hex: :file_system, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "2d8bc95d5a7bb99dd2613621d4f08c6a3575c3fd4b62e6a2b48a100352a557b8"},
+  "dialyxir": {:hex, :dialyxir, "1.4.7", "dda948fcee52962e4b6c5b4b16b2d8fa7d50d8645bbae8b8685c3f9ecb7f5f4d", [:mix], [{:erlex, ">= 0.2.8", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "b34527202e6eb8cee198efec110996c25c5898f43a4094df157f8d28f27d9efe"},
   "earmark_parser": {:hex, :earmark_parser, "1.4.13", "0c98163e7d04a15feb62000e1a891489feb29f3d10cb57d4f845c405852bbef8", [:mix], [], "hexpm", "d602c26af3a0af43d2f2645613f65841657ad6efc9f0e361c3b6c06b578214ba"},
-  "erlex": {:hex, :erlex, "0.2.6", "c7987d15e899c7a2f34f5420d2a2ea0d659682c06ac607572df55a43753aa12e", [:mix], [], "hexpm", "2ed2e25711feb44d52b17d2780eabf998452f6efda104877a3881c2f8c0c0c75"},
+  "erlex": {:hex, :erlex, "0.2.9", "7debbbaa9f4f368b8cd648983e0f1d7963028508e9c59e9d4ed504e94ef52a55", [:mix], [], "hexpm", "8cfffc0ec7159e6d73de2ab28a588064de80f88b2798d5cbe4482cbbc200178b"},
   "ex_doc": {:hex, :ex_doc, "0.24.2", "e4c26603830c1a2286dae45f4412a4d1980e1e89dc779fcd0181ed1d5a05c8d9", [:mix], [{:earmark_parser, "~> 1.4.0", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1", [hex: :makeup_erlang, repo: "hexpm", optional: false]}], "hexpm", "e134e1d9e821b8d9e4244687fb2ace58d479b67b282de5158333b0d57c6fb7da"},
-  "file_system": {:hex, :file_system, "0.2.10", "fb082005a9cd1711c05b5248710f8826b02d7d1784e7c3451f9c1231d4fc162d", [:mix], [], "hexpm", "41195edbfb562a593726eda3b3e8b103a309b733ad25f3d642ba49696bf715dc"},
-  "jason": {:hex, :jason, "1.2.2", "ba43e3f2709fd1aa1dce90aaabfd039d000469c05c56f0b8e31978e03fa39052", [:mix], [{:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "18a228f5f0058ee183f29f9eae0805c6e59d61c3b006760668d8d18ff0d12179"},
+  "file_system": {:hex, :file_system, "1.1.1", "31864f4685b0148f25bd3fbef2b1228457c0c89024ad67f7a81a3ffbc0bbad3a", [:mix], [], "hexpm", "7a15ff97dfe526aeefb090a7a9d3d03aa907e100e262a0f8f7746b78f8f87a5d"},
+  "jason": {:hex, :jason, "1.4.5", "2e3a008590b0b8d7388c20293e9dcc9cf3e5d642fd2a114e4cbbb52e595d940a", [:mix], [{:decimal, "~> 1.0 or ~> 2.0 or ~> 3.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "b0c823996102bcd0239b3c2444eb00409b72f6a140c1950bc8b457d836b30684"},
   "makeup": {:hex, :makeup, "1.0.5", "d5a830bc42c9800ce07dd97fa94669dfb93d3bf5fcf6ea7a0c67b2e0e4a7f26c", [:mix], [{:nimble_parsec, "~> 0.5 or ~> 1.0", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "cfa158c02d3f5c0c665d0af11512fed3fba0144cf1aadee0f2ce17747fba2ca9"},
   "makeup_elixir": {:hex, :makeup_elixir, "0.15.1", "b5888c880d17d1cc3e598f05cdb5b5a91b7b17ac4eaf5f297cb697663a1094dd", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.1", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "db68c173234b07ab2a07f645a5acdc117b9f99d69ebf521821d89690ae6c6ec8"},
   "makeup_erlang": {:hex, :makeup_erlang, "0.1.1", "3fcb7f09eb9d98dc4d208f49cc955a34218fc41ff6b84df7c75b3e6e533cc65f", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "174d0809e98a4ef0b3309256cbf97101c6ec01c4ab0b23e926a9e17df2077cbb"},

test/exgencode_test.exs

@@ -87,11 +87,15 @@ defmodule ExgencodeTest do
 
     nested_pdu = %TestPdu.NestedVersionedMsg{nested: %TestPdu.VersionedMsg{}}
     binary = <<2::size(16), 10::size(16)>>
-    assert {^nested_pdu, <<>>} = Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "1.0.0")
+
+    assert {^nested_pdu, <<>>} =
+             Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "1.0.0")
 
     nested_pdu = %TestPdu.NestedVersionedMsg{nested: %TestPdu.VersionedMsg{newerField: 111}}
     binary = <<2::size(16), 10::size(16), 111::size(8)>>
-    assert {^nested_pdu, <<>>} = Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "2.0.0")
+
+    assert {^nested_pdu, <<>>} =
+             Exgencode.Pdu.decode(%TestPdu.NestedVersionedMsg{}, binary, "2.0.0")
   end
 
   test "versioned encode/decode symmetry" do
@@ -569,14 +573,14 @@ defmodule ExgencodeTest do
   test "custom size function" do
     pdu = %TestPdu.CustomSizeFunPdu{custom: {5, 1024}}
 
-    assert <<2, 8, 5, 0, 0, 0, 4, 0, 8, 4>> = Exgencode.Pdu.encode(pdu)
+    assert <<2, 9, 5, 0, 0, 0, 4, 0, 8, 4>> = Exgencode.Pdu.encode(pdu)
 
     offsets = Exgencode.Pdu.set_offsets(pdu)
 
     assert {offsets, <<>>} ==
              Exgencode.Pdu.decode(
                %TestPdu.CustomSizeFunPdu{},
-               <<2, 8, 5, 0, 0, 0, 4, 0, 8, 4>>,
+               <<2, 9, 5, 0, 0, 0, 4, 0, 8, 4>>,
                nil
              )
   end
@@ -585,4 +589,104 @@ defmodule ExgencodeTest do
     pdu = %TestPdu.OffsetMadnessPdu{}
     assert <<3, 4, 0, 11, 5, 0>> = Exgencode.Pdu.encode(pdu)
   end
+
+  test "decode reaches offset_to : skips gap" do
+    binary = <<7, 0, 3, "abc", 0, 0xDE, 0xAD, 0xBE, 0xEF>>
+
+    assert {%TestPdu.OffsetMsg{
+              offset_to_footer: 7,
+              name_length: 3,
+              name: "abc",
+              footer: 0xDEADBEEF
+            }, <<>>} == Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary)
+  end
+
+  test "decode with zero-length gap (offset == cursor) is unchanged" do
+    binary = <<6, 0, 3, "abc", 0xDE, 0xAD, 0xBE, 0xEF>>
+
+    assert {%TestPdu.OffsetMsg{
+              offset_to_footer: 6,
+              name_length: 3,
+              name: "abc",
+              footer: 0xDEADBEEF
+            }, <<>>} == Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary)
+  end
+
+  test "decode with offset 0 leaves the target field absent" do
+    binary = <<0, 0, 3, "abc">>
+
+    assert {%TestPdu.OffsetMsg{
+              offset_to_footer: 0,
+              name_length: 3,
+              name: "abc",
+              footer: nil
+            }, <<>>} == Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary)
+  end
+
+  test "multiple offsets with offset_to gap" do
+    binary = <<4, 8, 0, 2, "XY", 0, 0, 0xBE, 0xEF>>
+
+    assert {%TestPdu.MultiOffsetMsg{
+              offset_to_a: 4,
+              offset_to_b: 8,
+              len_a: 2,
+              field_a: "XY",
+              field_b: 0xBEEF
+            }, <<>>} == Exgencode.Pdu.decode(%TestPdu.MultiOffsetMsg{}, binary)
+  end
+
+  test "decode raises on a backwards offset" do
+    binary = <<4, 0, 3, "abc", 0xDE, 0xAD, 0xBE, 0xEF>>
+
+    assert_raise Exgencode.DecodeError, ~r/point backwards/, fn ->
+      Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary)
+    end
+  end
+
+  test "decode raises on an offset past the end of the binary" do
+    binary = <<20, 0, 3, "abc", 0, 0xDE, 0xAD, 0xBE, 0xEF>>
+
+    assert_raise Exgencode.DecodeError, ~r/cannot point outside binary/, fn ->
+      Exgencode.Pdu.decode(%TestPdu.OffsetMsg{}, binary)
+    end
+  end
+
+  test "offsets in a nested subrecord stay relative to that subrecord" do
+    binary = <<9, 7, 0, 3, "abc", 0, 0xDE, 0xAD, 0xBE, 0xEF>>
+
+    assert {%TestPdu.NestedOffsetMsg{
+              header: 9,
+              sub: %TestPdu.OffsetMsg{
+                offset_to_footer: 7,
+                name_length: 3,
+                name: "abc",
+                footer: 0xDEADBEEF
+              }
+            }, <<>>} == Exgencode.Pdu.decode(%TestPdu.NestedOffsetMsg{}, binary)
+  end
+
+  test "defining two offset fields to the same target is rejected at compile time" do
+    assert_raise ArgumentError, ~r/multiple offset fields pointing/, fn ->
+      defmodule BadDup do
+        import Exgencode
+
+        defpdu DuplicateOffsetPdu,
+          off_a: [size: 8, offset_to: :target],
+          off_b: [size: 8, offset_to: :target],
+          target: [size: 8, conditional: :off_a]
+      end
+    end
+  end
+
+  test "defining an offset field after its target is rejected at compile time" do
+    assert_raise ArgumentError, ~r/backward offsets are unsupported/, fn ->
+      defmodule BadOrder do
+        import Exgencode
+
+        defpdu BackwardsOffsetPdu,
+          target: [size: 8],
+          off: [size: 8, offset_to: :target]
+      end
+    end
+  end
 end

test/helpers/test_pdu.ex

@@ -157,9 +157,9 @@ defmodule Exgencode.TestPdu do
         <<vals::size*8, rest::binary>> = val
         {struct(pdu, %{custom: {size, vals}}), rest}
       end,
-      size: fn %CustomSizeFunPdu{custom: {size, _vals}} -> size * 8 end
+      size: fn %CustomSizeFunPdu{custom: {size, _vals}} -> (size + 1) * 8 end
     ],
-    anotherWhyNot: [offset_to: :oneMore, size: 8],
+    anotherWhyNot: [size: 8, default: 8],
     oneMore: [default: 4, size: 8]
 
   defpdu OffsetMadnessPdu,
@@ -168,4 +168,21 @@ defmodule Exgencode.TestPdu do
     somethingIrrelevant: [default: 11, size: 16],
     anotherOffset: [offset_to: :somethingElse, size: 8],
     somethingElse: [default: 0, size: 8]
+
+  defpdu OffsetMsg,
+    offset_to_footer: [size: 8, offset_to: :footer, default: 0x00],
+    name_length: [size: 16, default: 0],
+    name: [type: :variable, size: :name_length, conditional: :name_length],
+    footer: [size: 32, conditional: :offset_to_footer]
+
+  defpdu MultiOffsetMsg,
+    offset_to_a: [size: 8, offset_to: :field_a, default: 0x00],
+    offset_to_b: [size: 8, offset_to: :field_b, default: 0x00],
+    len_a: [size: 16, default: 0],
+    field_a: [type: :variable, size: :len_a, conditional: :len_a],
+    field_b: [size: 16, conditional: :offset_to_b]
+
+  defpdu NestedOffsetMsg,
+    header: [size: 8, default: 0],
+    sub: [type: :subrecord, default: %OffsetMsg{}]
 end