Skip to content

Generate DecodeFrom() methods for Golang #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Nov 15, 2021
Merged

Generate DecodeFrom() methods for Golang #71

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c3262c3
WIP: generate DecodeFrom() Go methods
2opremio Nov 11, 2021
acc19fc
Progress
2opremio Nov 11, 2021
11075ae
Do not return an old error
2opremio Nov 11, 2021
127b211
Error on invalid input discriminants
2opremio Nov 11, 2021
70b912e
Do not initialize 0-length slices and remove redundant error checks
2opremio Nov 11, 2021
1968364
Resolve remaining TODOs
2opremio Nov 12, 2021
e72592d
Remove ReadAll and modify DecodeFrom() to returned consumed length
2opremio Nov 12, 2021
ccbd4d3
Increment read counter before checking for the error
2opremio Nov 12, 2021
101c6a6
Refine slice limit error
2opremio Nov 12, 2021
b6855aa
Remove redundant error returns for unions
2opremio Nov 12, 2021
548f250
Ensure decoded enum values are valid
2opremio Nov 12, 2021
c077a53
Address review feedback
2opremio Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
297 changes: 274 additions & 23 deletions lib/xdrgen/generators/go.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,16 +178,22 @@ def render_definition(out, defn)
when AST::Definitions::Struct ;
render_struct out, defn
render_struct_encode_to_interface out, defn
render_decoder_from_interface out, name(defn)
render_struct_decode_from_interface out, defn
render_binary_interface out, name(defn)
render_xdr_type_interface out, name(defn)
when AST::Definitions::Enum ;
render_enum out, defn
render_enum_encode_to_interface out, defn
render_decoder_from_interface out, name(defn)
render_enum_decode_from_interface out, defn
render_binary_interface out, name(defn)
render_xdr_type_interface out, name(defn)
when AST::Definitions::Union ;
render_union out, defn
render_union_encode_to_interface out, defn
render_decoder_from_interface out, name(defn)
render_union_decode_from_interface out, defn
render_binary_interface out, name(defn)
render_xdr_type_interface out, name(defn)
when AST::Definitions::Typedef ;
Expand All @@ -197,6 +203,8 @@ def render_definition(out, defn)
# for the type because that will be a Go compiler error.
if defn.sub_type != :optional
render_typedef_encode_to_interface out, defn
render_decoder_from_interface out, name(defn)
render_typedef_decode_from_interface out, defn
render_binary_interface out, name(defn)
render_xdr_type_interface out, name(defn)
end
Expand Down Expand Up @@ -417,30 +425,8 @@ def render_typedef_encode_to_interface(out, typedef)
out.break
end

def render_binary_interface(out, name)
out.puts "// MarshalBinary implements encoding.BinaryMarshaler."
out.puts "func (s #{name}) MarshalBinary() ([]byte, error) {"
out.puts " b := bytes.Buffer{}"
out.puts " e := xdr.NewEncoder(&b)"
out.puts " err := s.EncodeTo(e)"
out.puts " return b.Bytes(), err"
out.puts "}"
out.break
out.puts "// UnmarshalBinary implements encoding.BinaryUnmarshaler."
out.puts "func (s *#{name}) UnmarshalBinary(inp []byte) error {"
out.puts " _, err := Unmarshal(bytes.NewReader(inp), s)"
out.puts " return err"
out.puts "}"
out.break
out.puts "var ("
out.puts " _ encoding.BinaryMarshaler = (*#{name})(nil)"
out.puts " _ encoding.BinaryUnmarshaler = (*#{name})(nil)"
out.puts ")"
out.break
end

# render_encode_to_body assumes there is an `e` variable containing an
# xdr.Encoder, and a variable defined by `name` that is the value to
# xdr.Encoder, and a variable defined by `var` that is the value to
# encode.
def render_encode_to_body(out, var, type, self_encode:)
def check_error(str)
Expand Down Expand Up @@ -545,6 +531,259 @@ def check_error(str)
end
end

def render_struct_decode_from_interface(out, struct)
name = name(struct)
out.puts "// DecodeFrom decodes this value using the Decoder."
out.puts "func (s *#{name}) DecodeFrom(d *xdr.Decoder) (int, error) {"
out.puts " var err error"
out.puts " var n, nTmp int"
declared_variables = []
struct.members.each do |m|
mn = name(m)
render_decode_from_body(out, "s.#{mn}", m.type, declared_variables: declared_variables, self_encode: false)
end
out.puts " return n, nil"
out.puts "}"
out.break
end

def render_union_decode_from_interface(out, union)
name = name(union)
out.puts "// DecodeFrom decodes this value using the Decoder."
out.puts "func (u *#{name}) DecodeFrom(d *xdr.Decoder) (int, error) {"
out.puts " var err error"
out.puts " var n, nTmp int"
render_decode_from_body(out, "u.#{name(union.discriminant)}", union.discriminant.type, declared_variables: [], self_encode: false)
switch_for(out, union, "u.#{name(union.discriminant)}") do |arm, kase|
out2 = StringIO.new
if arm.void?
out2.puts "// Void"
else
mn = name(arm)
type = arm.type
out2.puts " u.#{mn} = new(#{reference arm.type})"
render_decode_from_body(out2, "(*u.#{mn})",type, declared_variables: [], self_encode: false)
end
out2.puts " return n, nil"
out2.string
end
unless union.default_arm.present?
out.puts " return n, fmt.Errorf(\"union #{name} has invalid #{name(union.discriminant)} (#{reference union.discriminant.type}) switch value '%d'\", u.#{name(union.discriminant)})"
end
out.puts "}"
out.break
end

def render_enum_decode_from_interface(out, typedef)
name = name(typedef)
type = typedef
out.puts <<-EOS.strip_heredoc
// DecodeFrom decodes this value using the Decoder.
func (e *#{name}) DecodeFrom(d *xdr.Decoder) (int, error) {
v, n, err := d.DecodeInt()
if err != nil {
return n, fmt.Errorf("decoding #{name}: %s", err)
}
if _, ok := #{private_name type}Map[v]; !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Given that most of our enums start at zero and are sequential, and given that use of the map will now be in the hot path, I wonder if for enums that we detect have that property, if we could simply replace this with a if v >= len(...Map) { // error }? We don't have to do this, and we definitely don't have to do this in this PR, but maybe something to evaluate if there is a saving here over hitting the map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, However:

There are many enums with negative values. Some examples are: BucketEntryType, CreateAccountResultCode, PaymentResultCode, PathPaymentStrictReceiveResultCode ....

Not even a range is generally a possibility since the XDR definition can contain scattered values.

We could infer a range where available and use that, but I think it is an overoptimization (without further proof of a bottleneck) and definitely out of the scope of this PR.

Let's later see if further profiling reveals map access as a problem.

return n, fmt.Errorf("'%d' is not a valid #{name} enum value", v)
}
*e = #{name}(v)
return n, nil
}
EOS
end

def render_typedef_decode_from_interface(out, typedef)
name = name(typedef)
type = typedef.declaration.type
out.puts "// DecodeFrom decodes this value using the Decoder."
out.puts "func (s *#{name}) DecodeFrom(d *xdr.Decoder) (int, error) {"
out.puts " var err error"
out.puts " var n, nTmp int"
var = "s"
sub_var_type = ""
case type
when AST::Typespecs::UnsignedHyper, AST::Typespecs::Hyper, AST::Typespecs::UnsignedInt, AST::Typespecs::Int, AST::Typespecs::String
sub_var_type = reference(type)
end
if (type.is_a?(AST::Typespecs::Opaque) && !type.fixed?) || (type.is_a?(AST::Typespecs::Simple) && type.sub_type == :var_array)
var = "(*s)"
end
unless sub_var_type.empty?
out.puts " var v #{sub_var_type}"
var = "v"
end
render_decode_from_body(out, var, type, declared_variables: [], self_encode: true)
out.puts " *s = #{name}(v)" unless sub_var_type.empty?
out.puts " return n, nil"
out.puts "}"
out.break
end

def render_variable_declaration(out, indent, var, type, declared_variables:)
unless declared_variables.include?var
out.puts "#{indent}var #{var} #{type}"
declared_variables.append(var)
end
end

# render_decode_from_body assumes there is an `d` variable containing an
# xdr.Decoder, and a variable defined by `var` that is the value to
# encode.
def render_decode_from_body(out, var, type, declared_variables:, self_encode:)
tail = <<-EOS.strip_heredoc
n += nTmp
if err != nil {
return n, fmt.Errorf("decoding #{name type}: %s", err)
}
EOS
optional = type.sub_type == :optional
if optional
render_variable_declaration(out, " ", 'b', "bool", declared_variables: declared_variables)
out.puts " b, nTmp, err = d.DecodeBool()"
out.puts tail
out.puts " #{var} = nil"
out.puts " if b {"
out.puts " #{var} = new(#{name type})"
end
case type
when AST::Typespecs::UnsignedHyper
out.puts " #{var}, nTmp, err = d.DecodeUhyper()"
out.puts tail
when AST::Typespecs::Hyper
out.puts " #{var}, nTmp, err = d.DecodeHyper()"
out.puts tail
when AST::Typespecs::UnsignedInt
out.puts " #{var}, nTmp, err = d.DecodeUint()"
out.puts tail
when AST::Typespecs::Int
out.puts " #{var}, nTmp, err = d.DecodeInt()"
out.puts tail
when AST::Typespecs::String
arg = "0"
arg = type.decl.resolved_size unless type.decl.resolved_size.nil?
out.puts " #{var}, nTmp, err = d.DecodeString(#{arg})"
out.puts tail
when AST::Typespecs::Opaque
if type.fixed?
out.puts " nTmp, err = d.DecodeFixedOpaqueInplace(#{var}[:])"
out.puts tail
else
arg = "0"
arg = type.decl.resolved_size unless type.decl.resolved_size.nil?
out.puts " #{var}, nTmp, err = d.DecodeOpaque(#{arg})"
out.puts tail
end
when AST::Typespecs::Simple
case type.sub_type
when :simple, :optional
optional_within = type.is_a?(AST::Identifier) && type.resolved_type.sub_type == :optional
if optional_within
render_variable_declaration(out, " ", 'b', "bool", declared_variables: declared_variables)
out.puts " b, nTmp, err = d.DecodeBool()"
out.puts tail
out.puts " #{var} = nil"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 I really like that this sets the value of every field.

out.puts " if b {"
out.puts " #{var} = new(#{name type.resolved_type.declaration.type})"
end
var = "(*#{name type})(#{var})" if self_encode
out.puts " nTmp, err = #{var}.DecodeFrom(d)"
out.puts tail
if optional_within
out.puts " }"
end
when :array
out.puts " for i := 0; i < len(#{var}); i++ {"
element_var = "#{var}[i]"
optional_within = type.is_a?(AST::Identifier) && type.resolved_type.sub_type == :optional
if optional_within
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively, it seems like this optional block (and above) should be able to be handled with a recursive data-type, like a Option<Foo> type-thing, and having a render_optional method... 🤔 but this is probably good enough for now.

Copy link
Contributor

@paulbellamy paulbellamy Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm thinking something like:

def render_decode_from_body(out, var, type, declared_variables:, self_encode:)
  # ...
  if type.sub_type == :optional
    return render_optional(out, var, type, type, declared_variables: declared_variables, self_encode: self_encode) do |inner_type|
      # Recurse into the unwrapped inner type
      render_decode_from_body(out, var, inner_type, declared_variables: declared_variables, self_encode: self_encode)
    end
  end
  # ...
end

def render_optional(out, var, type, declared_variables:, self_encode:)
  render_variable_declaration(out, "  ", 'b', "bool", declared_variables: declared_variables)
  out.puts "  b, nTmp, err = d.DecodeBool()"
  out.puts tail
  out.puts "  #{var} = nil"
  out.puts "  if b {"
  out.puts "     #{var} = new(#{name type.resolved_type.declaration.type})"
  # TODO: Unwrap the inner_type (without the optional wrapper) here
  yield inner_type
  out.puts "  }"
end

But, actually you could do a similar block thing for arrays as well, I think...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, probably outside the scope of this PR.

out.puts " var eb bool"
out.puts " eb, nTmp, err = d.DecodeBool()"
out.puts tail
out.puts " #{var} = nil"
out.puts " if eb {"
var = "(*#{element_var})"
end
out.puts " nTmp, err = #{element_var}.DecodeFrom(d)"
out.puts tail
if optional_within
out.puts " }"
end
out.puts " }"
when :var_array
render_variable_declaration(out, " ", 'l', "uint32", declared_variables: declared_variables)
out.puts " l, nTmp, err = d.DecodeUint()"
out.puts tail
unless type.decl.resolved_size.nil?
out.puts " if l > #{type.decl.resolved_size} {"
out.puts " return n, fmt.Errorf(\"decoding #{name type}: data size (%d) exceeds size limit (#{type.decl.resolved_size})\", l)"
out.puts " }"
end
out.puts " #{var} = nil"
out.puts " if l > 0 {"
out.puts " #{var} = make([]#{name type}, l)"
out.puts " for i := uint32(0); i < l; i++ {"
element_var = "#{var}[i]"
optional_within = type.is_a?(AST::Identifier) && type.resolved_type.sub_type == :optional
if optional_within
out.puts " var eb bool"
out.puts " eb, nTmp, err = d.DecodeBool()"
out.puts tail
out.puts " #{element_var} = nil"
out.puts " if eb {"
out.puts " #{element_var} = new(#{name type.resolved_type.declaration.type})"
var = "(*#{element_var})"
end
out.puts " nTmp, err = #{element_var}.DecodeFrom(d)"
out.puts tail
if optional_within
out.puts " }"
end
out.puts " }"
out.puts " }"
else
raise "Unknown sub_type: #{type.sub_type}"
end
when AST::Definitions::Base
if self_encode
out.puts " nTmp, err = #{name type}(#{var}).DecodeFrom(d)"
else
out.puts " nTmp, err = #{var}.DecodeFrom(d)"
end
out.puts tail
else
out.puts " nTmp, err = e.Decode(#{var})"
out.puts tail
end
if optional
out.puts " }"
end
end

def render_binary_interface(out, name)
out.puts "// MarshalBinary implements encoding.BinaryMarshaler."
out.puts "func (s #{name}) MarshalBinary() ([]byte, error) {"
out.puts " b := bytes.Buffer{}"
out.puts " e := xdr.NewEncoder(&b)"
out.puts " err := s.EncodeTo(e)"
out.puts " return b.Bytes(), err"
out.puts "}"
out.break
out.puts "// UnmarshalBinary implements encoding.BinaryUnmarshaler."
out.puts "func (s *#{name}) UnmarshalBinary(inp []byte) error {"
out.puts " r := bytes.NewReader(inp)"
out.puts " d := xdr.NewDecoder(r)"
out.puts " _, err := s.DecodeFrom(d)"
out.puts " return err"
out.puts "}"
out.break
out.puts "var ("
out.puts " _ encoding.BinaryMarshaler = (*#{name})(nil)"
out.puts " _ encoding.BinaryUnmarshaler = (*#{name})(nil)"
out.puts ")"
out.break
end

def render_xdr_type_interface(out, name)
out.puts "// xdrType signals that this type is an type representing"
out.puts "// representing XDR values defined by this package."
Expand All @@ -554,6 +793,10 @@ def render_xdr_type_interface(out, name)
out.break
end

def render_decoder_from_interface(out, name)
out.puts "var _ decoderFrom = (*#{name})(nil)"
end

def render_top_matter(out)
out.puts <<-EOS.strip_heredoc
// Package #{@namespace || "main"} is generated from:
Expand All @@ -576,8 +819,16 @@ def render_top_matter(out)
xdrType()
}

type decoderFrom interface {
DecodeFrom(d *xdr.Decoder) (int, error)
}

// Unmarshal reads an xdr element from `r` into `v`.
func Unmarshal(r io.Reader, v interface{}) (int, error) {
if decodable, ok := v.(decoderFrom); ok {
d := xdr.NewDecoder(r)
return decodable.DecodeFrom(d)
}
// delegate to xdr package's Unmarshal
return xdr.Unmarshal(r, v)
}
Expand Down