Skip to content

Commit

Permalink
refactor: reify OneOf
Browse files Browse the repository at this point in the history
  • Loading branch information
ahamez committed Jan 24, 2025
1 parent 89866ab commit 16824e7
Show file tree
Hide file tree
Showing 11 changed files with 71 additions and 54 deletions.
12 changes: 6 additions & 6 deletions lib/google/protobuf/value.ex
Original file line number Diff line number Diff line change
Expand Up @@ -13,47 +13,47 @@ defmodule Protox.Google.Protobuf.Value do
fields: %{
null_value:
Protox.Field.new!(
kind: {:oneof, :kind},
kind: %Protox.OneOf{parent: :kind},
label: :optional,
name: :null_value,
tag: 1,
type: {:enum, Google.Protobuf.NullValue}
),
number_value:
Protox.Field.new!(
kind: {:oneof, :kind},
kind: %Protox.OneOf{parent: :kind},
label: :optional,
name: :number_value,
tag: 2,
type: :double
),
string_value:
Protox.Field.new!(
kind: {:oneof, :kind},
kind: %Protox.OneOf{parent: :kind},
label: :optional,
name: :string_value,
tag: 3,
type: :string
),
bool_value:
Protox.Field.new!(
kind: {:oneof, :kind},
kind: %Protox.OneOf{parent: :kind},
label: :optional,
name: :bool_value,
tag: 4,
type: :bool
),
struct_value:
Protox.Field.new!(
kind: {:oneof, :kind},
kind: %Protox.OneOf{parent: :kind},
label: :optional,
name: :struct_value,
tag: 5,
type: {:message, Google.Protobuf.Struct}
),
list_value:
Protox.Field.new!(
kind: {:oneof, :kind},
kind: %Protox.OneOf{parent: :kind},
label: :optional,
name: :list_value,
tag: 6,
Expand Down
18 changes: 9 additions & 9 deletions lib/protox/define_decoder.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ defmodule Protox.DefineDecoder do
@moduledoc false
# Internal. Generates the decoder of a message.

alias Protox.{Field, Scalar}
alias Protox.{Field, OneOf, Scalar}
use Protox.{Float, WireTypes}

def define(msg_name, fields, required_fields, opts \\ []) do
Expand Down Expand Up @@ -238,7 +238,7 @@ defmodule Protox.DefineDecoder do
[]
end

defp make_delimited_case(_vars, _keep_set_fields, _single_generated, %Field{kind: {:oneof, _}}) do
defp make_delimited_case(_vars, _keep_set_fields, _single_generated, %Field{kind: %OneOf{}}) do
[]
end

Expand Down Expand Up @@ -293,7 +293,7 @@ defmodule Protox.DefineDecoder do

defp make_update_field(
value,
%Field{label: :proto3_optional, kind: {:oneof, _}, type: {:message, _}} = field,
%Field{label: :proto3_optional, kind: %OneOf{}, type: {:message, _}} = field,
vars,
_wrap_value
) do
Expand All @@ -310,29 +310,29 @@ defmodule Protox.DefineDecoder do

defp make_update_field(
value,
%Field{kind: {:oneof, parent_field}, type: {:message, _}} = field,
%Field{kind: %OneOf{}, type: {:message, _}} = field,
vars,
_wrap_value
) do
quote do
case unquote(vars.msg).unquote(parent_field) do
case unquote(vars.msg).unquote(field.kind.parent) do
{unquote(field.name), previous_value} ->
{unquote(parent_field),
{unquote(field.kind.parent),
{unquote(field.name), Protox.MergeMessage.merge(previous_value, unquote(value))}}

_ ->
{unquote(parent_field), {unquote(field.name), unquote(value)}}
{unquote(field.kind.parent), {unquote(field.name), unquote(value)}}
end
end
end

defp make_update_field(value, %Field{kind: {:oneof, parent_field}} = field, _vars, _wrap_value) do
defp make_update_field(value, %Field{kind: %OneOf{}} = field, _vars, _wrap_value) do
case field.label do
:proto3_optional ->
quote(do: {unquote(field.name), unquote(value)})

_ ->
quote(do: {unquote(parent_field), {unquote(field.name), unquote(value)}})
quote(do: {unquote(field.kind.parent), {unquote(field.name), unquote(value)}})
end
end

Expand Down
16 changes: 8 additions & 8 deletions lib/protox/define_encoder.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ defmodule Protox.DefineEncoder do
@moduledoc false
# Internal. Generates the encoder of a message.

alias Protox.{Field, Scalar}
alias Protox.{Field, OneOf, Scalar}

def define(fields, required_fields, syntax, opts \\ []) do
{unknown_fields_name, _opts} = Keyword.pop!(opts, :unknown_fields_name)
Expand Down Expand Up @@ -178,21 +178,21 @@ defmodule Protox.DefineEncoder do
end
end

# Generate the AST to encode child `field.name` of oneof `parent_field`
# Generate the AST to encode child `field.name` of a oneof
defp make_encode_field_body(
%Field{kind: {:oneof, parent_field}} = child_field,
%Field{kind: %OneOf{}} = field,
_required,
_syntax,
vars
) do
key = make_key_bytes(child_field.tag, child_field.type)
key = make_key_bytes(field.tag, field.type)
var = Macro.var(:child_field_value, __MODULE__)
encode_value_ast = get_encode_value_body(child_field.type, var)
encode_value_ast = get_encode_value_body(field.type, var)

case child_field.label do
case field.label do
:proto3_optional ->
quote do
case unquote(vars.msg).unquote(child_field.name) do
case unquote(vars.msg).unquote(field.name) do
nil -> []
unquote(var) -> [unquote(key), unquote(encode_value_ast)]
end
Expand All @@ -202,7 +202,7 @@ defmodule Protox.DefineEncoder do
# The dispatch on the correct child is performed by the parent encoding function,
# this is why we don't check if the child is set.
quote do
{_, unquote(var)} = unquote(vars.msg).unquote(parent_field)
{_, unquote(var)} = unquote(vars.msg).unquote(field.kind.parent)
[unquote(key), unquote(encode_value_ast)]
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/protox/define_message.ex
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
defmodule Protox.DefineMessage do
@moduledoc false

alias Protox.{Field, Scalar}
alias Protox.{Field, OneOf, Scalar}

def define(messages, opts \\ []) do
for {_msg_name, msg = %Protox.Message{}} <- messages do
Expand Down Expand Up @@ -103,7 +103,7 @@ defmodule Protox.DefineMessage do
for %Field{label: label, name: name, kind: kind} <- fields do
case kind do
:map -> {name, Macro.escape(%{})}
{:oneof, parent} -> make_oneof_field(label, name, parent)
%OneOf{parent: parent} -> make_oneof_field(label, name, parent)
:packed -> {name, []}
:unpacked -> {name, []}
%Scalar{} when syntax == :proto2 -> {name, nil}
Expand Down
6 changes: 3 additions & 3 deletions lib/protox/defs.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ defmodule Protox.Defs do
@moduledoc false
# Internal. Helpers to work with list of fields.

alias Protox.Field
alias Protox.{Field, OneOf}

# Extract oneofs and regroup them by parent field.
def split_oneofs(fields) do
{all_oneofs, others} =
Enum.split_with(fields, fn
%Field{kind: {:oneof, _}} -> true
%Field{kind: %OneOf{}} -> true
%Field{} -> false
end)

Expand All @@ -31,5 +31,5 @@ defmodule Protox.Defs do
)
end

defp oneof_group_by(%Field{kind: {:oneof, parent}}), do: parent
defp oneof_group_by(field), do: field.kind.parent
end
13 changes: 12 additions & 1 deletion lib/protox/kind.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,22 @@ defmodule Protox.Scalar do
defstruct [:default_value]
end

defmodule Protox.OneOf do
@moduledoc false

@type t() :: %__MODULE__{
parent: atom()
}

@enforce_keys [:parent]
defstruct [:parent]
end

defmodule Protox.Kind do
@moduledoc false

@typedoc """
This type indicates how a field is encoded.
"""
@type t() :: Protox.Scalar.t() | :packed | :unpacked | :map | {:oneof, atom()}
@type t() :: Protox.Scalar.t() | :packed | :unpacked | :map | Protox.OneOf.t()
end
4 changes: 2 additions & 2 deletions lib/protox/merge_message.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule Protox.MergeMessage do
This module provides a helper function to merge messages.
"""

alias Protox.{Field, Scalar}
alias Protox.{Field, OneOf, Scalar}

@doc """
Singular fields of `msg` will be overwritten, if specified in `from`, except for
Expand Down Expand Up @@ -96,6 +96,6 @@ defmodule Protox.MergeMessage do
defp merge_oneof(_msg, v1, nil), do: v1
defp merge_oneof(_msg, _v1, v2), do: v2

defp oneof_message?(%Field{kind: {:oneof, _}, type: {:message, _}}), do: true
defp oneof_message?(%Field{kind: %OneOf{}, type: {:message, _}}), do: true
defp oneof_message?(_), do: false
end
6 changes: 2 additions & 4 deletions lib/protox/parse.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ defmodule Protox.Parse do

import Protox.Guards

alias Protox.{Definition, Field, Message, Scalar}
alias Protox.{Definition, Field, Message, OneOf, Scalar}

alias Protox.Google.Protobuf.{
DescriptorProto,
Expand Down Expand Up @@ -387,9 +387,7 @@ defmodule Protox.Parse do
end

defp get_kind(_syntax, upper, %FieldDescriptorProto{oneof_index: index}) when index != nil do
parent = String.to_atom(Enum.at(upper.oneof_decl, index).name)

{:oneof, parent}
%OneOf{parent: String.to_atom(Enum.at(upper.oneof_decl, index).name)}
end

defp get_kind(_syntax, _upper, %FieldDescriptorProto{
Expand Down
22 changes: 14 additions & 8 deletions test/protox/defs_test.exs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
defmodule Protox.DefsTest do
use ExUnit.Case

alias Protox.{Field, Scalar}
alias Protox.{Field, OneOf, Scalar}

@defs [
Field.new!(tag: 8, label: nil, name: :msg_k, kind: :map, type: {:int32, :string}),
Expand Down Expand Up @@ -40,12 +40,18 @@ defmodule Protox.DefsTest do
),
Field.new!(tag: 6, label: :repeated, name: :msg_i, kind: :packed, type: :float),
Field.new!(tag: 7, label: :repeated, name: :msg_j, kind: :unpacked, type: {:message, Sub}),
Field.new!(tag: 10, label: :optional, name: :msg_n, kind: {:oneof, :msg_m}, type: :string),
Field.new!(
tag: 10,
label: :optional,
name: :msg_n,
kind: %OneOf{parent: :msg_m},
type: :string
),
Field.new!(
tag: 11,
label: :optional,
name: :msg_o,
kind: {:oneof, :msg_m},
kind: %OneOf{parent: :msg_m},
type: {:message, Sub}
),
Field.new!(tag: 12, label: nil, name: :msg_p, kind: :map, type: {:int32, {:enum, E}}),
Expand All @@ -60,11 +66,11 @@ defmodule Protox.DefsTest do
tag: 118,
label: :optional,
name: :msg_oneof_double,
kind: {:oneof, :msg_oneof_field},
kind: %OneOf{parent: :msg_oneof_field},
type: :double
),
Field.new!(
kind: {:oneof, :_optional},
kind: %OneOf{parent: :_optional},
label: :proto3_optional,
name: :optional,
tag: 11,
Expand All @@ -82,14 +88,14 @@ defmodule Protox.DefsTest do
tag: 10,
label: :optional,
name: :msg_n,
kind: {:oneof, :msg_m},
kind: %OneOf{parent: :msg_m},
type: :string
),
Field.new!(
tag: 11,
label: :optional,
name: :msg_o,
kind: {:oneof, :msg_m},
kind: %OneOf{parent: :msg_m},
type: {:message, Sub}
)
],
Expand All @@ -98,7 +104,7 @@ defmodule Protox.DefsTest do
tag: 118,
label: :optional,
name: :msg_oneof_double,
kind: {:oneof, :msg_oneof_field},
kind: %OneOf{parent: :msg_oneof_field},
type: :double
)
]
Expand Down
6 changes: 3 additions & 3 deletions test/protox/parse_test.exs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
defmodule Protox.ParseTest do
use ExUnit.Case

alias Protox.Scalar
alias Protox.{OneOf, Scalar}

setup_all do
file_descriptor_set_bin_path = Protox.TmpFs.tmp_file_path!(".bin")
Expand Down Expand Up @@ -45,10 +45,10 @@ defmodule Protox.ParseTest do
assert field(fs, 64) == {nil, :map_sfixed32_sfixed32, :map, {:sfixed32, :sfixed32}}
assert field(fs, 75) == {:repeated, :packed_int32, :packed, :int32}
assert field(fs, 89) == {:repeated, :unpacked_int32, :unpacked, :int32}
assert field(fs, 111) == {:optional, :oneof_uint32, {:oneof, :oneof_field}, :uint32}
assert field(fs, 111) == {:optional, :oneof_uint32, %OneOf{parent: :oneof_field}, :uint32}

assert field(fs, 119) ==
{:optional, :oneof_enum, {:oneof, :oneof_field},
{:optional, :oneof_enum, %OneOf{parent: :oneof_field},
{:enum, ProtobufTestMessages.Proto3.TestAllTypesProto3.NestedEnum}}

assert %{} = messages[ProtobufTestMessages.Proto3.NullHypothesisProto3].fields
Expand Down
Loading

0 comments on commit 16824e7

Please sign in to comment.