You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "ASF GitHub Bot (JIRA)" <ji...@apache.org> on 2018/09/12 16:58:00 UTC
[jira] [Commented] (THRIFT-4631) Codegen Creates Invalid Ruby for
Recursive Structs
[ https://issues.apache.org/jira/browse/THRIFT-4631?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16612460#comment-16612460 ]
ASF GitHub Bot commented on THRIFT-4631:
----------------------------------------
cgardens opened a new pull request #1592: THRIFT-4631 Fix Ruby codegen to gen valid ruby for recursive structs
URL: https://github.com/apache/thrift/pull/1592
Client: Ruby
re: https://issues.apache.org/jira/browse/THRIFT-4631
The goal of this PR is to alter the thrift ruby code generator so that in cases where we define structs that rely on recursion, it generates valid ruby.
The codegen include a `generate_forward_declaration` method which gets called before `generate_struct`. It allows us to eagerly declare ruby classes so that all classes are declared before any of the recursion within the classes executes. Without implementing something like this you get an `uninitialized constant` exception.
I've added tests to catch the original issue as well.
The delta caused by the this change is adding class declarations at the top of the generated files. Here's the delta alone. Full copies of the before / after below.
```
class RecTree; end
class RecList; end
class CoRec; end
class CoRec2; end
class VectorTest; end
```
What gets generated now for Recursive.thrift
```
#
# Autogenerated by Thrift Compiler (1.0.0-dev)
#
# DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
#
require 'thrift'
class RecTree
include ::Thrift::Struct, ::Thrift::Struct_Union
CHILDREN = 1
ITEM = 2
FIELDS = {
CHILDREN => {:type => ::Thrift::Types::LIST, :name => 'children', :element => {:type => ::Thrift::Types::STRUCT, :class => ::RecTree}},
ITEM => {:type => ::Thrift::Types::I16, :name => 'item'}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class RecList
include ::Thrift::Struct, ::Thrift::Struct_Union
NEXTITEM = 1
ITEM = 3
FIELDS = {
NEXTITEM => {:type => ::Thrift::Types::STRUCT, :name => 'nextitem', :class => ::RecList},
ITEM => {:type => ::Thrift::Types::I16, :name => 'item'}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class CoRec
include ::Thrift::Struct, ::Thrift::Struct_Union
OTHER = 1
FIELDS = {
OTHER => {:type => ::Thrift::Types::STRUCT, :name => 'other', :class => ::CoRec2}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class CoRec2
include ::Thrift::Struct, ::Thrift::Struct_Union
OTHER = 1
FIELDS = {
OTHER => {:type => ::Thrift::Types::STRUCT, :name => 'other', :class => ::CoRec}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class VectorTest
include ::Thrift::Struct, ::Thrift::Struct_Union
LISTER = 1
FIELDS = {
LISTER => {:type => ::Thrift::Types::LIST, :name => 'lister', :element => {:type => ::Thrift::Types::STRUCT, :class => ::RecList}}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
```
What is generated using this PR:
```
#
# Autogenerated by Thrift Compiler (1.0.0-dev)
#
# DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
#
require 'thrift'
class RecTree; end
class RecList; end
class CoRec; end
class CoRec2; end
class VectorTest; end
class RecTree
include ::Thrift::Struct, ::Thrift::Struct_Union
CHILDREN = 1
ITEM = 2
FIELDS = {
CHILDREN => {:type => ::Thrift::Types::LIST, :name => 'children', :element => {:type => ::Thrift::Types::STRUCT, :class => ::RecTree}},
ITEM => {:type => ::Thrift::Types::I16, :name => 'item'}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class RecList
include ::Thrift::Struct, ::Thrift::Struct_Union
NEXTITEM = 1
ITEM = 3
FIELDS = {
NEXTITEM => {:type => ::Thrift::Types::STRUCT, :name => 'nextitem', :class => ::RecList},
ITEM => {:type => ::Thrift::Types::I16, :name => 'item'}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class CoRec
include ::Thrift::Struct, ::Thrift::Struct_Union
OTHER = 1
FIELDS = {
OTHER => {:type => ::Thrift::Types::STRUCT, :name => 'other', :class => ::CoRec2}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class CoRec2
include ::Thrift::Struct, ::Thrift::Struct_Union
OTHER = 1
FIELDS = {
OTHER => {:type => ::Thrift::Types::STRUCT, :name => 'other', :class => ::CoRec}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
class VectorTest
include ::Thrift::Struct, ::Thrift::Struct_Union
LISTER = 1
FIELDS = {
LISTER => {:type => ::Thrift::Types::LIST, :name => 'lister', :element => {:type => ::Thrift::Types::STRUCT, :class => ::RecList}}
}
def struct_fields; FIELDS; end
def validate
end
::Thrift::Struct.generate_accessors self
end
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
> Codegen Creates Invalid Ruby for Recursive Structs
> --------------------------------------------------
>
> Key: THRIFT-4631
> URL: https://issues.apache.org/jira/browse/THRIFT-4631
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Compiler, Ruby - Library
> Affects Versions: 0.11.0
> Reporter: charles giardina
> Priority: Minor
>
> Though thrift supports defining recursive structs, it appears that the Ruby codegen does not generate valid Ruby for for recursive structs.
> e.g. Here's an example of what I'm seeing and how to recreate it using Recursive.thrift in the Thrift test suite.
> * add {{$(THRIFT) --gen rb ../Recursive.thrift}} to {{test/rb/Makefile.am}}
> * and add {{require "recursive_types"}} to {{test/rb/generation/test_struct.rb}}
> * run {{make -k check}} from {{test/rb}}
> * this ends up raising the following exception:
> {code:java}
> thrift/test/rb/gen-rb/recursive_types.rb:50:in `<class:CoRec>': uninitialized constant CoRec2 (NameError)
> Did you mean? CoRec
> from thrift/test/rb/gen-rb/recursive_types.rb:45:in `<top (required)>'
> from thrift/test/rb/generation/test_struct.rb:22:in `require'
> from thrift/test/rb/generation/test_struct.rb:22:in `<top (required)>'
> from test_suite.rb:20:in `require'
> from test_suite.rb:20:in `block in <main>'
> from test_suite.rb:20:in `each'
> from test_suite.rb:20:in `<main>'
> make: *** [check] Error 1
> {code}
>
> I've copied the generated code below. But essentially the issue appears to be that recursive structs are referencing constants or classes that have not yet been declared. I have some ideas on how to address this and can post a proposed solution.
>
> generated code:
> {code:java}
> #
> # Autogenerated by Thrift Compiler (0.11.0)
> #
> # DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
> #
> require 'thrift'
> module RecursiveExample
> class Node < ::Thrift::Union
> include ::Thrift::Struct_Union
> class << self
> def inner_node(val)
> Node.new(:inner_node, val)
> end
> def leaf_node(val)
> Node.new(:leaf_node, val)
> end
> end
> INNER_NODE = 1
> LEAF_NODE = 2
> FIELDS = {
> INNER_NODE => {:type => ::Thrift::Types::STRUCT, :name => 'inner_node', :class => ::RecursiveExample::InnerNode, :optional => true},
> LEAF_NODE => {:type => ::Thrift::Types::STRUCT, :name => 'leaf_node', :class => ::RecursiveExample::LeafNode, :optional => true}
> }
> def struct_fields; FIELDS; end
> def validate
> raise(StandardError, 'Union fields are not set.') if get_set_field.nil? || get_value.nil?
> end
> ::Thrift::Union.generate_accessors self
> end
> class InnerNode
> include ::Thrift::Struct, ::Thrift::Struct_Union
> NODE = 1
> FIELDS = {
> NODE => {:type => ::Thrift::Types::STRUCT, :name => 'node', :class => ::RecursiveExample::Node}
> }
> def struct_fields; FIELDS; end
> def validate
> raise ::Thrift::ProtocolException.new(::Thrift::ProtocolException::UNKNOWN, 'Required field node is unset!') unless @node end
> ::Thrift::Struct.generate_accessors self
> end
> class LeafNode
> include ::Thrift::Struct, ::Thrift::Struct_Union
> FIELDS = {
> }
> def struct_fields; FIELDS; end
> def validate
> end
> ::Thrift::Struct.generate_accessors self
> end
> end
> {code}
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)