[jira] [Commented] (THRIFT-5283) Ability to listen over unix socket in thrift Rust crate
[
https://issues.apache.org/jira/browse/THRIFT-5283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17564482#comment-17564482
]
Allen George commented on THRIFT-5283:
--
[~tokcum]I'm so sorry, and I totally understand your disappointment. Would you
be open to using a git hash as a dependency in your crate?
>From [the
>book|https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html]
>we have an example like this:
{{regex = { git = "https://github.com/rust-lang/regex;, branch = "next" }}}
You know that the commit is in master, and we're not about to remove it, so it
should be safe for you to do this until the next release.
> Ability to listen over unix socket in thrift Rust crate
> ---
>
> Key: THRIFT-5283
> URL: https://issues.apache.org/jira/browse/THRIFT-5283
> Project: Thrift
> Issue Type: New Feature
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: Prateek Kumar Nischal
>Priority: Major
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> The rust crate for [thrift|https://crates.io/crates/thrift] right now has the
> ability to create a server [but only over a TCP
> socket|https://github.com/apache/thrift/blob/master/lib/rs/src/server/threaded.rs#L172].
> {code}
> pub fn listen( self, listen_address: A) ->
> thrift::Result<()>
> {code}
> The API requires the trait
> [ToSocketAddrs|https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html] to
> be implemented which is not possible for a Unix Domain Socket.
> Other libraries, for example, python has an option to serve over unix
> sockets. eg
> {code:python}
> TSocket.TServerSocket(unix_socket='/tmp/service.sock')
> {code}
> It would be really nice to be able to get a similar API in rust as well
> unless there is a way to do it already. Please let me know if it already
> exists.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (THRIFT-5594) Update crates.io with version 0.16.0
[ https://issues.apache.org/jira/browse/THRIFT-5594?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17561578#comment-17561578 ] Allen George commented on THRIFT-5594: -- [~pascal.bach] [~TennyZhuang] I'm incredibly sorry for the delay. I've uploaded 0.16.0 to crates.io. Thank you for your patience, and apologies again. > Update crates.io with version 0.16.0 > > > Key: THRIFT-5594 > URL: https://issues.apache.org/jira/browse/THRIFT-5594 > Project: Thrift > Issue Type: Wish > Components: Rust - Library >Affects Versions: 0.16.0 >Reporter: Zhuang Tianyi >Assignee: Allen George >Priority: Major > > It seems that the version of the rs lib is 0.17 [in the > source|https://github.com/apache/thrift/blob/master/lib/rs/Cargo.toml], but > only 0.15 on [https://crates.io/crates/thrift] . Should anyone release it? -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Resolved] (THRIFT-5594) Update crates.io with version 0.16.0
[ https://issues.apache.org/jira/browse/THRIFT-5594?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George resolved THRIFT-5594. -- Resolution: Fixed > Update crates.io with version 0.16.0 > > > Key: THRIFT-5594 > URL: https://issues.apache.org/jira/browse/THRIFT-5594 > Project: Thrift > Issue Type: Wish > Components: Rust - Library >Affects Versions: 0.16.0 >Reporter: Zhuang Tianyi >Assignee: Allen George >Priority: Major > > It seems that the version of the rs lib is 0.17 [in the > source|https://github.com/apache/thrift/blob/master/lib/rs/Cargo.toml], but > only 0.15 on [https://crates.io/crates/thrift] . Should anyone release it? -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (THRIFT-5559) Processor can be implemented on handler trait itself
[
https://issues.apache.org/jira/browse/THRIFT-5559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17523999#comment-17523999
]
Allen George commented on THRIFT-5559:
--
[~fayalalebrun] That change sounds reasonable. I don't think that allowing
access should be a safety problem, so a PR would be totally fine.
> Processor can be implemented on handler trait itself
>
>
> Key: THRIFT-5559
> URL: https://issues.apache.org/jira/browse/THRIFT-5559
> Project: Thrift
> Issue Type: Proposal
> Components: Rust - Compiler
>Reporter: Francisco Ayala
>Priority: Major
>
> Right now the compiler produces code of the following form:
>
> {code:java}
> pub trait ServiceSyncHandler {
> fn handle_test_episode(, arg: types::Type1) ->
> thrift::Result;
> }
> pub struct ServiceSyncProcessor {
> handler: H,
> }
> impl ServiceSyncProcessor {
> pub fn new(handler: H) -> ServiceSyncProcessor {
> ServiceSyncProcessor {
> handler,
> }
> }
> fn process_test_episode(, incoming_sequence_number: i32, i_prot:
> dyn TInputProtocol, o_prot: dyn TOutputProtocol) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(,
> incoming_sequence_number, i_prot, o_prot)
> }
> } {code}
> There is this object called "ServiceSyncProcessor" which wraps the actual
> handler. However it has no other fields, and also only ever utilizes a
> reference to the handler. Thus my question is why aren't the methods
> implemented on the type itself? Like this:
>
>
> {code:java}
> impl dyn ServiceSyncHandler {
> fn process_test_episode(
> ,
> incoming_sequence_number: i32,
> i_prot: dyn TInputProtocol,
> o_prot: dyn TOutputProtocol,
> ) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(
> self,
> incoming_sequence_number,
> i_prot,
> o_prot,
> )
> }
> }{code}
> In my case this is limiting since I don't want the server to take ownership
> of the processor object, since I'm doing everything in a single thread and
> need this object for other purposes. It was easy enough implementing my own
> server, but this is now blocking me.
>
> My questions are thus: Is there a reason for this design? Would implementing
> the alternative I proposed be a welcome addition?
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Comment Edited] (THRIFT-5559) Processor can be implemented on handler trait itself
[
https://issues.apache.org/jira/browse/THRIFT-5559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17523266#comment-17523266
]
Allen George edited comment on THRIFT-5559 at 4/17/22 2:25 AM:
---
For example, consider:
{noformat}
Trait_A (file A)
+ - Trait_B (file B)
+- Uses types from E (file E)
+ - Trait_C (file C)
+- Trait D (file D)
{noformat}
When the generator runs it can:
1. Generate a trait definition for {{Trait_D}}
2. Generate a trait definition for {{Trait_C}} and also simply say {{Trait_C :
Trait_D}} to enforce the inheritance, without caring what methods/types
{{Trait_D}} has
3. Generate a trait definition for {{Trait_B}} and during that, resolve all the
types from {{E}}
4. Generate a trait definition for {{Trait_A}} and also simply say {{Trait_A :
Trait B + Trait_C}} without caring about the underlying methods/types
{{Trait_B}}, {{Trait_C}} or {{E}} has
Also note that you have to generate a corresponding processor for {{A}}, {{B}},
{{C}} and {{D}} in each separate generated file. This is very straightforward
because all you're doing is defining it in terms of the trait name defined in
the local file, not caring about how complex its inheritance hierarchy is.
If you run the generator on
https://github.com/apache/thrift/blob/master/lib/rs/test/src/bin/kitchen_sink_server.rs
you can see this at work.
was (Author: allengeorge):
For example, consider:
{noformat}
Trait_A
+ - Trait_B
+- Uses types from E
+ - Trait_C
+- Trait D
{noformat}
When the generator runs it can:
1. Generate a trait definition for {{Trait_D}}
2. Generate a trait definition for {{Trait_C}} and also simply say {{Trait_C :
Trait_D}} to enforce the inheritance, without caring what methods/types
{{Trait_D}} has
3. Generate a trait definition for {{Trait_B}} and during that, resolve all the
types from {{E}}
4. Generate a trait definition for {{Trait_A}} and also simply say {{Trait_A :
Trait B + Trait_C}} without caring about the underlying methods/types
{{Trait_B}}, {{Trait_C}} or {{E}} has
Also note that you have to generate a corresponding processor for {{A}}, {{B}},
{{C}} and {{D}}. This is very straightforward because all you're doing is
defining it in terms of the trait name defined in the local file, not caring
about how complex its inheritance hierarchy is.
If you run the generator on
https://github.com/apache/thrift/blob/master/lib/rs/test/src/bin/kitchen_sink_server.rs
you can see this at work.
> Processor can be implemented on handler trait itself
>
>
> Key: THRIFT-5559
> URL: https://issues.apache.org/jira/browse/THRIFT-5559
> Project: Thrift
> Issue Type: Proposal
> Components: Rust - Compiler
>Reporter: Francisco Ayala
>Priority: Major
>
> Right now the compiler produces code of the following form:
>
> {code:java}
> pub trait ServiceSyncHandler {
> fn handle_test_episode(, arg: types::Type1) ->
> thrift::Result;
> }
> pub struct ServiceSyncProcessor {
> handler: H,
> }
> impl ServiceSyncProcessor {
> pub fn new(handler: H) -> ServiceSyncProcessor {
> ServiceSyncProcessor {
> handler,
> }
> }
> fn process_test_episode(, incoming_sequence_number: i32, i_prot:
> dyn TInputProtocol, o_prot: dyn TOutputProtocol) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(,
> incoming_sequence_number, i_prot, o_prot)
> }
> } {code}
> There is this object called "ServiceSyncProcessor" which wraps the actual
> handler. However it has no other fields, and also only ever utilizes a
> reference to the handler. Thus my question is why aren't the methods
> implemented on the type itself? Like this:
>
>
> {code:java}
> impl dyn ServiceSyncHandler {
> fn process_test_episode(
> ,
> incoming_sequence_number: i32,
> i_prot: dyn TInputProtocol,
> o_prot: dyn TOutputProtocol,
> ) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(
> self,
> incoming_sequence_number,
> i_prot,
> o_prot,
> )
> }
> }{code}
> In my case this is limiting since I don't want the server to take ownership
> of the processor object, since I'm doing everything in a single thread and
> need this object for other purposes. It was easy enough implementing my own
> server, but this is now blocking me.
>
> My questions are thus: Is there a reason for this design? Would implementing
> the alternative I proposed be a welcome addition?
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Comment Edited] (THRIFT-5559) Processor can be implemented on handler trait itself
[
https://issues.apache.org/jira/browse/THRIFT-5559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17523264#comment-17523264
]
Allen George edited comment on THRIFT-5559 at 4/17/22 2:22 AM:
---
It's been a very, very long time since I've looked at the code as a whole, and
I'm doing this off memory but this was probably a consequence of having to
support the {{extends}} behavior for Thrift services the the simplest, most
robust way for the generator.
A service can extend any number of services from any number of Thrift files.
Extend hierarchies can also be arbitrarily complex. Methods on those services
can have types that themselves have to be resolved in the generated code and
may themselves come from multiple files. By defining traits and generating code
for them on a per-service basis, the generator only ever has to consider the
current file for method declarations. The only place where we have to consider
more than a single file is when doing trait inheritance, and that's fairly
simple, as I only have to consider the names of the traits being extended, not
the methods/types they contain or reference. Finally, the processor itself can
just be defined in terms of the extended trait (and again, the generator does
not have to care about the methods/types in the arbitrarily complex service
extends hierarchy).
was (Author: allengeorge):
It's been a very, very long time since I've looked at the code as a while, and
I'm doing this off memory but this was probably a consequence of having to
support the {{extends}} behavior for Thrift services the the simplest, most
robust way for the generator.
A service can extend any number of services from any number of Thrift files.
Extend hierarchies can also be arbitrarily complex. Methods on those services
can have types that themselves have to be resolved in the generated code and
may themselves come from multiple files. By defining traits and generating code
for them on a per-service basis, the generator only ever has to consider the
current file for method declarations. The only place where we have to consider
more than a single file is when doing trait inheritance, and that's fairly
simple, as I only have to consider the names of the traits being extended, not
the methods/types they contain or reference. Finally, the processor itself can
just be defined in terms of the extended trait (and again, the generator does
not have to care about the methods/types in the arbitrarily complex service
extends hierarchy).
> Processor can be implemented on handler trait itself
>
>
> Key: THRIFT-5559
> URL: https://issues.apache.org/jira/browse/THRIFT-5559
> Project: Thrift
> Issue Type: Proposal
> Components: Rust - Compiler
>Reporter: Francisco Ayala
>Priority: Major
>
> Right now the compiler produces code of the following form:
>
> {code:java}
> pub trait ServiceSyncHandler {
> fn handle_test_episode(, arg: types::Type1) ->
> thrift::Result;
> }
> pub struct ServiceSyncProcessor {
> handler: H,
> }
> impl ServiceSyncProcessor {
> pub fn new(handler: H) -> ServiceSyncProcessor {
> ServiceSyncProcessor {
> handler,
> }
> }
> fn process_test_episode(, incoming_sequence_number: i32, i_prot:
> dyn TInputProtocol, o_prot: dyn TOutputProtocol) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(,
> incoming_sequence_number, i_prot, o_prot)
> }
> } {code}
> There is this object called "ServiceSyncProcessor" which wraps the actual
> handler. However it has no other fields, and also only ever utilizes a
> reference to the handler. Thus my question is why aren't the methods
> implemented on the type itself? Like this:
>
>
> {code:java}
> impl dyn ServiceSyncHandler {
> fn process_test_episode(
> ,
> incoming_sequence_number: i32,
> i_prot: dyn TInputProtocol,
> o_prot: dyn TOutputProtocol,
> ) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(
> self,
> incoming_sequence_number,
> i_prot,
> o_prot,
> )
> }
> }{code}
> In my case this is limiting since I don't want the server to take ownership
> of the processor object, since I'm doing everything in a single thread and
> need this object for other purposes. It was easy enough implementing my own
> server, but this is now blocking me.
>
> My questions are thus: Is there a reason for this design? Would implementing
> the alternative I proposed be a welcome addition?
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Comment Edited] (THRIFT-5559) Processor can be implemented on handler trait itself
[
https://issues.apache.org/jira/browse/THRIFT-5559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17523264#comment-17523264
]
Allen George edited comment on THRIFT-5559 at 4/17/22 2:22 AM:
---
It's been a very, very long time since I've looked at the code as a whole, and
I'm doing this off memory but this was probably a consequence of having to
support the {{extends}} behavior for Thrift services in the simplest, most
robust way for the generator.
A service can extend any number of services from any number of Thrift files.
Extend hierarchies can also be arbitrarily complex. Methods on those services
can have types that themselves have to be resolved in the generated code and
may themselves come from multiple files. By defining traits and generating code
for them on a per-service basis, the generator only ever has to consider the
current file for method declarations. The only place where we have to consider
more than a single file is when doing trait inheritance, and that's fairly
simple, as I only have to consider the names of the traits being extended, not
the methods/types they contain or reference. Finally, the processor itself can
just be defined in terms of the extended trait (and again, the generator does
not have to care about the methods/types in the arbitrarily complex service
extends hierarchy).
was (Author: allengeorge):
It's been a very, very long time since I've looked at the code as a whole, and
I'm doing this off memory but this was probably a consequence of having to
support the {{extends}} behavior for Thrift services the the simplest, most
robust way for the generator.
A service can extend any number of services from any number of Thrift files.
Extend hierarchies can also be arbitrarily complex. Methods on those services
can have types that themselves have to be resolved in the generated code and
may themselves come from multiple files. By defining traits and generating code
for them on a per-service basis, the generator only ever has to consider the
current file for method declarations. The only place where we have to consider
more than a single file is when doing trait inheritance, and that's fairly
simple, as I only have to consider the names of the traits being extended, not
the methods/types they contain or reference. Finally, the processor itself can
just be defined in terms of the extended trait (and again, the generator does
not have to care about the methods/types in the arbitrarily complex service
extends hierarchy).
> Processor can be implemented on handler trait itself
>
>
> Key: THRIFT-5559
> URL: https://issues.apache.org/jira/browse/THRIFT-5559
> Project: Thrift
> Issue Type: Proposal
> Components: Rust - Compiler
>Reporter: Francisco Ayala
>Priority: Major
>
> Right now the compiler produces code of the following form:
>
> {code:java}
> pub trait ServiceSyncHandler {
> fn handle_test_episode(, arg: types::Type1) ->
> thrift::Result;
> }
> pub struct ServiceSyncProcessor {
> handler: H,
> }
> impl ServiceSyncProcessor {
> pub fn new(handler: H) -> ServiceSyncProcessor {
> ServiceSyncProcessor {
> handler,
> }
> }
> fn process_test_episode(, incoming_sequence_number: i32, i_prot:
> dyn TInputProtocol, o_prot: dyn TOutputProtocol) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(,
> incoming_sequence_number, i_prot, o_prot)
> }
> } {code}
> There is this object called "ServiceSyncProcessor" which wraps the actual
> handler. However it has no other fields, and also only ever utilizes a
> reference to the handler. Thus my question is why aren't the methods
> implemented on the type itself? Like this:
>
>
> {code:java}
> impl dyn ServiceSyncHandler {
> fn process_test_episode(
> ,
> incoming_sequence_number: i32,
> i_prot: dyn TInputProtocol,
> o_prot: dyn TOutputProtocol,
> ) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(
> self,
> incoming_sequence_number,
> i_prot,
> o_prot,
> )
> }
> }{code}
> In my case this is limiting since I don't want the server to take ownership
> of the processor object, since I'm doing everything in a single thread and
> need this object for other purposes. It was easy enough implementing my own
> server, but this is now blocking me.
>
> My questions are thus: Is there a reason for this design? Would implementing
> the alternative I proposed be a welcome addition?
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Comment Edited] (THRIFT-5559) Processor can be implemented on handler trait itself
[
https://issues.apache.org/jira/browse/THRIFT-5559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17523266#comment-17523266
]
Allen George edited comment on THRIFT-5559 at 4/17/22 2:21 AM:
---
For example, consider:
{noformat}
Trait_A
+ - Trait_B
+- Uses types from E
+ - Trait_C
+- Trait D
{noformat}
When the generator runs it can:
1. Generate a trait definition for {{Trait_D}}
2. Generate a trait definition for {{Trait_C}} and also simply say {{Trait_C :
Trait_D}} to enforce the inheritance, without caring what methods/types
{{Trait_D}} has
3. Generate a trait definition for {{Trait_B}} and during that, resolve all the
types from {{E}}
4. Generate a trait definition for {{Trait_A}} and also simply say {{Trait_A :
Trait B + Trait_C}} without caring about the underlying methods/types
{{Trait_B}}, {{Trait_C}} or {{E}} has
Also note that you have to generate a corresponding processor for {{A}}, {{B}},
{{C}} and {{D}}. This is very straightforward because all you're doing is
defining it in terms of the trait name defined in the local file, not caring
about how complex its inheritance hierarchy is.
If you run the generator on
https://github.com/apache/thrift/blob/master/lib/rs/test/src/bin/kitchen_sink_server.rs
you can see this at work.
was (Author: allengeorge):
For example, consider:
{noformat}
Trait_A
+ - Trait_B
+- Uses types from E
+ - Trait_C
+- Trait D
{noformat}
When the generator runs it can:
1. Generate a trait definition for {{Trait_D}}
2. Generate a trait definition for {{Trait_C}} and also simply say {{Trait_C :
Trait_D}} to enforce the inheritance
3. Generate a trait definition for {{Trait_B}} and during that, resolve all the
types from {{E}}
4. Generate a trait definition for {{Trait_A}} and also simply say {{Trait_A :
Trait B + Trait_C}} without caring about the underlying methods/types to
enforce the inheritance.
Also note that you have to generate a corresponding processor for {{A}}, {{B}},
{{C}} and {{D}}. This is very straightforward because all you're doing is
defining it in terms of the trait name defined in the local file, not caring
about how complex its inheritance hierarchy is.
If you run the generator on
https://github.com/apache/thrift/blob/master/lib/rs/test/src/bin/kitchen_sink_server.rs
you can see this at work.
> Processor can be implemented on handler trait itself
>
>
> Key: THRIFT-5559
> URL: https://issues.apache.org/jira/browse/THRIFT-5559
> Project: Thrift
> Issue Type: Proposal
> Components: Rust - Compiler
>Reporter: Francisco Ayala
>Priority: Major
>
> Right now the compiler produces code of the following form:
>
> {code:java}
> pub trait ServiceSyncHandler {
> fn handle_test_episode(, arg: types::Type1) ->
> thrift::Result;
> }
> pub struct ServiceSyncProcessor {
> handler: H,
> }
> impl ServiceSyncProcessor {
> pub fn new(handler: H) -> ServiceSyncProcessor {
> ServiceSyncProcessor {
> handler,
> }
> }
> fn process_test_episode(, incoming_sequence_number: i32, i_prot:
> dyn TInputProtocol, o_prot: dyn TOutputProtocol) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(,
> incoming_sequence_number, i_prot, o_prot)
> }
> } {code}
> There is this object called "ServiceSyncProcessor" which wraps the actual
> handler. However it has no other fields, and also only ever utilizes a
> reference to the handler. Thus my question is why aren't the methods
> implemented on the type itself? Like this:
>
>
> {code:java}
> impl dyn ServiceSyncHandler {
> fn process_test_episode(
> ,
> incoming_sequence_number: i32,
> i_prot: dyn TInputProtocol,
> o_prot: dyn TOutputProtocol,
> ) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(
> self,
> incoming_sequence_number,
> i_prot,
> o_prot,
> )
> }
> }{code}
> In my case this is limiting since I don't want the server to take ownership
> of the processor object, since I'm doing everything in a single thread and
> need this object for other purposes. It was easy enough implementing my own
> server, but this is now blocking me.
>
> My questions are thus: Is there a reason for this design? Would implementing
> the alternative I proposed be a welcome addition?
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (THRIFT-5559) Processor can be implemented on handler trait itself
[
https://issues.apache.org/jira/browse/THRIFT-5559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17523266#comment-17523266
]
Allen George commented on THRIFT-5559:
--
For example, consider:
{noformat}
Trait_A
+ - Trait_B
+- Uses types from E
+ - Trait_C
+- Trait D
{noformat}
When the generator runs it can:
1. Generate a trait definition for {{Trait_D}}
2. Generate a trait definition for {{Trait_C}} and also simply say {{Trait_C :
Trait_D}} to enforce the inheritance
3. Generate a trait definition for {{Trait_B}} and during that, resolve all the
types from {{E}}
4. Generate a trait definition for {{Trait_A}} and also simply say {{Trait_A :
Trait B + Trait_C}} without caring about the underlying methods/types to
enforce the inheritance.
Also note that you have to generate a corresponding processor for {{A}}, {{B}},
{{C}} and {{D}}. This is very straightforward because all you're doing is
defining it in terms of the trait name defined in the local file, not caring
about how complex its inheritance hierarchy is.
If you run the generator on
https://github.com/apache/thrift/blob/master/lib/rs/test/src/bin/kitchen_sink_server.rs
you can see this at work.
> Processor can be implemented on handler trait itself
>
>
> Key: THRIFT-5559
> URL: https://issues.apache.org/jira/browse/THRIFT-5559
> Project: Thrift
> Issue Type: Proposal
> Components: Rust - Compiler
>Reporter: Francisco Ayala
>Priority: Major
>
> Right now the compiler produces code of the following form:
>
> {code:java}
> pub trait ServiceSyncHandler {
> fn handle_test_episode(, arg: types::Type1) ->
> thrift::Result;
> }
> pub struct ServiceSyncProcessor {
> handler: H,
> }
> impl ServiceSyncProcessor {
> pub fn new(handler: H) -> ServiceSyncProcessor {
> ServiceSyncProcessor {
> handler,
> }
> }
> fn process_test_episode(, incoming_sequence_number: i32, i_prot:
> dyn TInputProtocol, o_prot: dyn TOutputProtocol) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(,
> incoming_sequence_number, i_prot, o_prot)
> }
> } {code}
> There is this object called "ServiceSyncProcessor" which wraps the actual
> handler. However it has no other fields, and also only ever utilizes a
> reference to the handler. Thus my question is why aren't the methods
> implemented on the type itself? Like this:
>
>
> {code:java}
> impl dyn ServiceSyncHandler {
> fn process_test_episode(
> ,
> incoming_sequence_number: i32,
> i_prot: dyn TInputProtocol,
> o_prot: dyn TOutputProtocol,
> ) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(
> self,
> incoming_sequence_number,
> i_prot,
> o_prot,
> )
> }
> }{code}
> In my case this is limiting since I don't want the server to take ownership
> of the processor object, since I'm doing everything in a single thread and
> need this object for other purposes. It was easy enough implementing my own
> server, but this is now blocking me.
>
> My questions are thus: Is there a reason for this design? Would implementing
> the alternative I proposed be a welcome addition?
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (THRIFT-5559) Processor can be implemented on handler trait itself
[
https://issues.apache.org/jira/browse/THRIFT-5559?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17523264#comment-17523264
]
Allen George commented on THRIFT-5559:
--
It's been a very, very long time since I've looked at the code as a while, and
I'm doing this off memory but this was probably a consequence of having to
support the {{extends}} behavior for Thrift services the the simplest, most
robust way for the generator.
A service can extend any number of services from any number of Thrift files.
Extend hierarchies can also be arbitrarily complex. Methods on those services
can have types that themselves have to be resolved in the generated code and
may themselves come from multiple files. By defining traits and generating code
for them on a per-service basis, the generator only ever has to consider the
current file for method declarations. The only place where we have to consider
more than a single file is when doing trait inheritance, and that's fairly
simple, as I only have to consider the names of the traits being extended, not
the methods/types they contain or reference. Finally, the processor itself can
just be defined in terms of the extended trait (and again, the generator does
not have to care about the methods/types in the arbitrarily complex service
extends hierarchy).
> Processor can be implemented on handler trait itself
>
>
> Key: THRIFT-5559
> URL: https://issues.apache.org/jira/browse/THRIFT-5559
> Project: Thrift
> Issue Type: Proposal
> Components: Rust - Compiler
>Reporter: Francisco Ayala
>Priority: Major
>
> Right now the compiler produces code of the following form:
>
> {code:java}
> pub trait ServiceSyncHandler {
> fn handle_test_episode(, arg: types::Type1) ->
> thrift::Result;
> }
> pub struct ServiceSyncProcessor {
> handler: H,
> }
> impl ServiceSyncProcessor {
> pub fn new(handler: H) -> ServiceSyncProcessor {
> ServiceSyncProcessor {
> handler,
> }
> }
> fn process_test_episode(, incoming_sequence_number: i32, i_prot:
> dyn TInputProtocol, o_prot: dyn TOutputProtocol) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(,
> incoming_sequence_number, i_prot, o_prot)
> }
> } {code}
> There is this object called "ServiceSyncProcessor" which wraps the actual
> handler. However it has no other fields, and also only ever utilizes a
> reference to the handler. Thus my question is why aren't the methods
> implemented on the type itself? Like this:
>
>
> {code:java}
> impl dyn ServiceSyncHandler {
> fn process_test_episode(
> ,
> incoming_sequence_number: i32,
> i_prot: dyn TInputProtocol,
> o_prot: dyn TOutputProtocol,
> ) -> thrift::Result<()> {
> TServiceProcessFunctions::process_test_episode(
> self,
> incoming_sequence_number,
> i_prot,
> o_prot,
> )
> }
> }{code}
> In my case this is limiting since I don't want the server to take ownership
> of the processor object, since I'm doing everything in a single thread and
> need this object for other purposes. It was easy enough implementing my own
> server, but this is now blocking me.
>
> My questions are thus: Is there a reason for this design? Would implementing
> the alternative I proposed be a welcome addition?
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (THRIFT-5283) Ability to listen over unix socket in thrift Rust crate
[
https://issues.apache.org/jira/browse/THRIFT-5283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17514565#comment-17514565
]
Allen George commented on THRIFT-5283:
--
[~tokcum] Yes - splitting Windows pipe support into a separate ticket makes
sense. Separately, thank you for your work on the Unix pipe support - I've just
merged it!
> Ability to listen over unix socket in thrift Rust crate
> ---
>
> Key: THRIFT-5283
> URL: https://issues.apache.org/jira/browse/THRIFT-5283
> Project: Thrift
> Issue Type: New Feature
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: Prateek Kumar Nischal
>Priority: Major
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> The rust crate for [thrift|https://crates.io/crates/thrift] right now has the
> ability to create a server [but only over a TCP
> socket|https://github.com/apache/thrift/blob/master/lib/rs/src/server/threaded.rs#L172].
> {code}
> pub fn listen( self, listen_address: A) ->
> thrift::Result<()>
> {code}
> The API requires the trait
> [ToSocketAddrs|https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html] to
> be implemented which is not possible for a Unix Domain Socket.
> Other libraries, for example, python has an option to serve over unix
> sockets. eg
> {code:python}
> TSocket.TServerSocket(unix_socket='/tmp/service.sock')
> {code}
> It would be really nice to be able to get a similar API in rust as well
> unless there is a way to do it already. Please let me know if it already
> exists.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (THRIFT-5283) Ability to listen over unix socket in thrift Rust crate
[
https://issues.apache.org/jira/browse/THRIFT-5283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17508514#comment-17508514
]
Allen George commented on THRIFT-5283:
--
Took a quick look, and in general - looks good! One minor comment about the
test code but otherwise...
As for unit tests, I think you're right: an integration test (whether the
kitchen sink/cross tests) should suffice.
Thank you for your work!
> Ability to listen over unix socket in thrift Rust crate
> ---
>
> Key: THRIFT-5283
> URL: https://issues.apache.org/jira/browse/THRIFT-5283
> Project: Thrift
> Issue Type: New Feature
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: Prateek Kumar Nischal
>Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> The rust crate for [thrift|https://crates.io/crates/thrift] right now has the
> ability to create a server [but only over a TCP
> socket|https://github.com/apache/thrift/blob/master/lib/rs/src/server/threaded.rs#L172].
> {code}
> pub fn listen( self, listen_address: A) ->
> thrift::Result<()>
> {code}
> The API requires the trait
> [ToSocketAddrs|https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html] to
> be implemented which is not possible for a Unix Domain Socket.
> Other libraries, for example, python has an option to serve over unix
> sockets. eg
> {code:python}
> TSocket.TServerSocket(unix_socket='/tmp/service.sock')
> {code}
> It would be really nice to be able to get a similar API in rust as well
> unless there is a way to do it already. Please let me know if it already
> exists.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (THRIFT-5283) Ability to listen over unix socket in thrift Rust crate
[
https://issues.apache.org/jira/browse/THRIFT-5283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17508509#comment-17508509
]
Allen George commented on THRIFT-5283:
--
[~tokcum] Sorry for the delay. I'll take a look over the weekend. At least a
first pass.
> Ability to listen over unix socket in thrift Rust crate
> ---
>
> Key: THRIFT-5283
> URL: https://issues.apache.org/jira/browse/THRIFT-5283
> Project: Thrift
> Issue Type: New Feature
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: Prateek Kumar Nischal
>Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> The rust crate for [thrift|https://crates.io/crates/thrift] right now has the
> ability to create a server [but only over a TCP
> socket|https://github.com/apache/thrift/blob/master/lib/rs/src/server/threaded.rs#L172].
> {code}
> pub fn listen( self, listen_address: A) ->
> thrift::Result<()>
> {code}
> The API requires the trait
> [ToSocketAddrs|https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html] to
> be implemented which is not possible for a Unix Domain Socket.
> Other libraries, for example, python has an option to serve over unix
> sockets. eg
> {code:python}
> TSocket.TServerSocket(unix_socket='/tmp/service.sock')
> {code}
> It would be really nice to be able to get a similar API in rust as well
> unless there is a way to do it already. Please let me know if it already
> exists.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Commented] (THRIFT-5283) Ability to listen over unix socket in thrift Rust crate
[
https://issues.apache.org/jira/browse/THRIFT-5283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17501862#comment-17501862
]
Allen George commented on THRIFT-5283:
--
[~tokcum] Tobias - thank you so much for looking at this issue. Unfortunately
my time is quite limited nowadays, so I'll try and review your PR as
quickly/best I can. That said, I think that to include this feature you should:
# Add some unit tests if possible
# Update the rust <-> rust test harnesses so that you can ensure that the basic
implementation works
(https://github.com/apache/thrift/tree/master/lib/rs/test); NOTE:
unfortunately, actually running the test harness is manual
# Update the cross-test harnesses so that you can ensure that other languages
which support unix sockets can communicate with the rust server/client
(https://github.com/apache/thrift/tree/master/test/rs
https://github.com/apache/thrift/blob/b8920b01cb72af93a716bb203fcd8a1202936b97/test/tests.json#L681)
This would ensure that this change can be properly tested and maintained going
forward.
> Ability to listen over unix socket in thrift Rust crate
> ---
>
> Key: THRIFT-5283
> URL: https://issues.apache.org/jira/browse/THRIFT-5283
> Project: Thrift
> Issue Type: New Feature
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: Prateek Kumar Nischal
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The rust crate for [thrift|https://crates.io/crates/thrift] right now has the
> ability to create a server [but only over a TCP
> socket|https://github.com/apache/thrift/blob/master/lib/rs/src/server/threaded.rs#L172].
> {code}
> pub fn listen( self, listen_address: A) ->
> thrift::Result<()>
> {code}
> The API requires the trait
> [ToSocketAddrs|https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html] to
> be implemented which is not possible for a Unix Domain Socket.
> Other libraries, for example, python has an option to serve over unix
> sockets. eg
> {code:python}
> TSocket.TServerSocket(unix_socket='/tmp/service.sock')
> {code}
> It would be really nice to be able to get a similar API in rust as well
> unless there is a way to do it already. Please let me know if it already
> exists.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
[jira] [Resolved] (THRIFT-5457) Travis fails consistently on a Rust dependency
[
https://issues.apache.org/jira/browse/THRIFT-5457?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5457.
--
Fix Version/s: 0.16.0
Resolution: Fixed
> Travis fails consistently on a Rust dependency
> --
>
> Key: THRIFT-5457
> URL: https://issues.apache.org/jira/browse/THRIFT-5457
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.15.0
>Reporter: Yuxuan Wang
>Assignee: Allen George
>Priority: Major
> Fix For: 0.16.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> This is an [example travis
> build|https://app.travis-ci.com/github/apache/thrift/jobs/532057468] (it seem
> to only fail on the Autotools Ubuntu Bionic task but not the Autotools Ubuntu
> Xenial task):
>
>
> {noformat}
> /root/.cargo/bin/cargo clippy --all -- -D warnings
>Compiling libc v0.2.99
>Compiling autocfg v1.0.1
>Compiling log v0.4.14
> Checking cfg-if v1.0.0
> Checking unicode-width v0.1.8
> Checking bitflags v1.3.2
> Checking vec_map v0.8.2
> Checking integer-encoding v3.0.2
> Checking ansi_term v0.11.0
> Checking byteorder v1.4.3
> Checking strsim v0.8.0
>Compiling num-traits v0.2.14
> Checking textwrap v0.11.0
> Checking num_cpus v1.13.0
> Checking atty v0.2.14
> Checking threadpool v1.8.1
> Checking clap v2.33.3
> Checking ordered-float v1.1.1
> Checking thrift v0.16.0 (/thrift/src/lib/rs)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/app/settings.rs:7:1
>|
> 7 | / bitflags! {
> 8 | | struct Flags: u64 {
> 9 | | const SC_NEGATE_REQS = 1;
> 10 | | const SC_REQUIRED = 1 << 1;
> ... |
> 51 | | }
> 52 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/args/settings.rs:6:1
>|
> 6 | / bitflags! {
> 7 | | struct Flags: u32 {
> 8 | | const REQUIRED = 1;
> 9 | | const MULTIPLE = 1 << 1;
> ... |
> 28 | | }
> 29 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error: aborting due to 2 previous errors
> For more information about this error, try `rustc --explain E0723`.
> error: could not compile `clap`.{noformat}
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Assigned] (THRIFT-5457) Travis fails consistently on a Rust dependency
[
https://issues.apache.org/jira/browse/THRIFT-5457?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George reassigned THRIFT-5457:
Assignee: Allen George
> Travis fails consistently on a Rust dependency
> --
>
> Key: THRIFT-5457
> URL: https://issues.apache.org/jira/browse/THRIFT-5457
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.15.0
>Reporter: Yuxuan Wang
>Assignee: Allen George
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> This is an [example travis
> build|https://app.travis-ci.com/github/apache/thrift/jobs/532057468] (it seem
> to only fail on the Autotools Ubuntu Bionic task but not the Autotools Ubuntu
> Xenial task):
>
>
> {noformat}
> /root/.cargo/bin/cargo clippy --all -- -D warnings
>Compiling libc v0.2.99
>Compiling autocfg v1.0.1
>Compiling log v0.4.14
> Checking cfg-if v1.0.0
> Checking unicode-width v0.1.8
> Checking bitflags v1.3.2
> Checking vec_map v0.8.2
> Checking integer-encoding v3.0.2
> Checking ansi_term v0.11.0
> Checking byteorder v1.4.3
> Checking strsim v0.8.0
>Compiling num-traits v0.2.14
> Checking textwrap v0.11.0
> Checking num_cpus v1.13.0
> Checking atty v0.2.14
> Checking threadpool v1.8.1
> Checking clap v2.33.3
> Checking ordered-float v1.1.1
> Checking thrift v0.16.0 (/thrift/src/lib/rs)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/app/settings.rs:7:1
>|
> 7 | / bitflags! {
> 8 | | struct Flags: u64 {
> 9 | | const SC_NEGATE_REQS = 1;
> 10 | | const SC_REQUIRED = 1 << 1;
> ... |
> 51 | | }
> 52 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/args/settings.rs:6:1
>|
> 6 | / bitflags! {
> 7 | | struct Flags: u32 {
> 8 | | const REQUIRED = 1;
> 9 | | const MULTIPLE = 1 << 1;
> ... |
> 28 | | }
> 29 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error: aborting due to 2 previous errors
> For more information about this error, try `rustc --explain E0723`.
> error: could not compile `clap`.{noformat}
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5457) Travis fails consistently on a Rust dependency
[
https://issues.apache.org/jira/browse/THRIFT-5457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17412308#comment-17412308
]
Allen George commented on THRIFT-5457:
--
>From reading [this|https://issuehunt.io/r/clap-rs/clap/issues/2691], it
>appears the problem was that a transitive dependency {{bitflags}} updated in
>such a way that our MSRV changed underneath us.
> Travis fails consistently on a Rust dependency
> --
>
> Key: THRIFT-5457
> URL: https://issues.apache.org/jira/browse/THRIFT-5457
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.15.0
>Reporter: Yuxuan Wang
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> This is an [example travis
> build|https://app.travis-ci.com/github/apache/thrift/jobs/532057468] (it seem
> to only fail on the Autotools Ubuntu Bionic task but not the Autotools Ubuntu
> Xenial task):
>
>
> {noformat}
> /root/.cargo/bin/cargo clippy --all -- -D warnings
>Compiling libc v0.2.99
>Compiling autocfg v1.0.1
>Compiling log v0.4.14
> Checking cfg-if v1.0.0
> Checking unicode-width v0.1.8
> Checking bitflags v1.3.2
> Checking vec_map v0.8.2
> Checking integer-encoding v3.0.2
> Checking ansi_term v0.11.0
> Checking byteorder v1.4.3
> Checking strsim v0.8.0
>Compiling num-traits v0.2.14
> Checking textwrap v0.11.0
> Checking num_cpus v1.13.0
> Checking atty v0.2.14
> Checking threadpool v1.8.1
> Checking clap v2.33.3
> Checking ordered-float v1.1.1
> Checking thrift v0.16.0 (/thrift/src/lib/rs)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/app/settings.rs:7:1
>|
> 7 | / bitflags! {
> 8 | | struct Flags: u64 {
> 9 | | const SC_NEGATE_REQS = 1;
> 10 | | const SC_REQUIRED = 1 << 1;
> ... |
> 51 | | }
> 52 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/args/settings.rs:6:1
>|
> 6 | / bitflags! {
> 7 | | struct Flags: u32 {
> 8 | | const REQUIRED = 1;
> 9 | | const MULTIPLE = 1 << 1;
> ... |
> 28 | | }
> 29 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error: aborting due to 2 previous errors
> For more information about this error, try `rustc --explain E0723`.
> error: could not compile `clap`.{noformat}
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5457) Travis fails consistently on a Rust dependency
[
https://issues.apache.org/jira/browse/THRIFT-5457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17412301#comment-17412301
]
Allen George commented on THRIFT-5457:
--
Really puzzled what's going on here, and why this would suddenly fail on Bionic
> Travis fails consistently on a Rust dependency
> --
>
> Key: THRIFT-5457
> URL: https://issues.apache.org/jira/browse/THRIFT-5457
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.15.0
>Reporter: Yuxuan Wang
>Priority: Major
>
> This is an [example travis
> build|https://app.travis-ci.com/github/apache/thrift/jobs/532057468] (it seem
> to only fail on the Autotools Ubuntu Bionic task but not the Autotools Ubuntu
> Xenial task):
>
>
> {noformat}
> /root/.cargo/bin/cargo clippy --all -- -D warnings
>Compiling libc v0.2.99
>Compiling autocfg v1.0.1
>Compiling log v0.4.14
> Checking cfg-if v1.0.0
> Checking unicode-width v0.1.8
> Checking bitflags v1.3.2
> Checking vec_map v0.8.2
> Checking integer-encoding v3.0.2
> Checking ansi_term v0.11.0
> Checking byteorder v1.4.3
> Checking strsim v0.8.0
>Compiling num-traits v0.2.14
> Checking textwrap v0.11.0
> Checking num_cpus v1.13.0
> Checking atty v0.2.14
> Checking threadpool v1.8.1
> Checking clap v2.33.3
> Checking ordered-float v1.1.1
> Checking thrift v0.16.0 (/thrift/src/lib/rs)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/app/settings.rs:7:1
>|
> 7 | / bitflags! {
> 8 | | struct Flags: u64 {
> 9 | | const SC_NEGATE_REQS = 1;
> 10 | | const SC_REQUIRED = 1 << 1;
> ... |
> 51 | | }
> 52 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error[E0723]: loops and conditional expressions are not stable in const fn
> -->
> /root/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/args/settings.rs:6:1
>|
> 6 | / bitflags! {
> 7 | | struct Flags: u32 {
> 8 | | const REQUIRED = 1;
> 9 | | const MULTIPLE = 1 << 1;
> ... |
> 28 | | }
> 29 | | }
>| |_^
>|
>= note: for more information, see issue
> https://github.com/rust-lang/rust/issues/57563
>= help: add `#![feature(const_fn)]` to the crate attributes to enable
>= note: this error originates in a macro outside of the current crate (in
> Nightly builds, run with -Z external-macro-backtrace for more info)
> error: aborting due to 2 previous errors
> For more information about this error, try `rustc --explain E0723`.
> error: could not compile `clap`.{noformat}
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Assigned] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George reassigned THRIFT-5392:
Assignee: Allen George
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Assignee: Allen George
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that {{TryFrom}} was implemented and not the
> infallible form {{From}}
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
> backward/forward capabilities I would suggest the introduction of a sentinel
> enum variant (as using {{Result}} does not
> look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
> Unknown, // Maybe the variant name could be configurable
> Ok, Created, Accepted,
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> // As current
> ...
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { Self::Unknown }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self {
> match x {
> HttpStatusCode::Unknown => ::fbthrift::__UNKNOWN_ID,
> HttpStatusCode::Ok => 200,
> HttpStatusCode::Created => 201,
> HttpStatusCode::Accepted => 202,
> }
> }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { Self::from() }
> }
> impl TryFrom for HttpStatusCode {
> type Error =
[jira] [Comment Edited] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17391182#comment-17391182
]
Allen George edited comment on THRIFT-5392 at 8/1/21, 3:51 PM:
---
I don't mind reverting the change and then just using an {{__Unknown(i32)}} to
represent the remaining variants. But, I'll be honest - it's the 3rd time I'm
reworking the enum representation code because of varying community
expectations, and my time/ability to do this is very constrained nowadays. I
absolutely don't mind making changes, but the back and forth isn't great. There
was conversation on the earlier ticket, and I only proceeded after feedback. I
would like some consensus among different users of the library as to what they
want before making any further changes.
was (Author: allengeorge):
I don't mind reverting the change and then just using an {{__Unknown(i32)}} to
represent the remaining variants. But, I'll be honest - it's the 3rd time I'm
reworking the enum representation code because of varying community
expectations, and my time/ability to do this is very constrained. There was
conversation on the earlier ticket, and I only proceeded after feedback. I
would like some consensus among different users of the library as to what they
want before making any further changes.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that {{TryFrom}} was implemented and not the
> infallible form {{From}}
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way
[jira] [Commented] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17391182#comment-17391182
]
Allen George commented on THRIFT-5392:
--
I don't mind reverting the change and then just using an {{__Unknown(i32)}} to
represent the remaining variants. But, I'll be honest - it's the 3rd time I'm
reworking the enum representation code because of varying community
expectations, and my time/ability to do this is very constrained. There was
conversation on the earlier ticket, and I only proceeded after feedback. I
would like some consensus among different users of the library as to what they
want before making any further changes.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that {{TryFrom}} was implemented and not the
> infallible form {{From}}
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
> backward/forward capabilities I would suggest the introduction of a sentinel
> enum variant (as using {{Result}} does not
> look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
> Unknown, // Maybe the variant name could be configurable
> Ok, Created, Accepted,
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> // As current
> ...
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { Self::Unknown }
> }
> impl<'a> From<&'a
[jira] [Comment Edited] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17317112#comment-17317112
]
Allen George edited comment on THRIFT-5392 at 4/8/21, 11:38 AM:
Also note that your concern:
{noformat}
// we allow creating un-existing enums from code
let enum_value = HttpStatusCode(1234); // I would have expected an error
{noformat}
is an **expected** side-effect of the implemented approach: to support
forward-compatibility. You have to have the ability to create enum variants
without a named value and encode them onto the wire. Even with the
{{__Unknown(...)}} variant alternative you would still have to take that value
and encode it onto the wire.
was (Author: allengeorge):
Also note that your concern:
{noformat}
// we allow creating un-existing enums from code
let enum_value = HttpStatusCode(1234); // I would have expected an error
{noformat}
is an **expected** side-effect of the implemented approach: to support
forward-compatibility, you have to have the ability to create enum variants
without a named value and encode them onto the wire. Even with the
{{__Unknown(...)}} variant alternative you would still have to take that value
and encode it onto the wire.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that TryFrom was implemented and not the
> infallible form From
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants
[jira] [Commented] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17317114#comment-17317114
]
Allen George commented on THRIFT-5392:
--
This is a separate concern for a separate ticket:
{noformat]
// we do not allow creating enums from variant names (but we do it from enum
"binary" value)
let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected to
be possible
{noformat}
I avoided doing this because names in Thrift are not stable.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that TryFrom was implemented and not the
> infallible form From
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
> backward/forward capabilities I would suggest the introduction of a sentinel
> enum variant (as using Result does not
> look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
> Unknown, // Maybe the variant name could be configurable
> Ok, Created, Accepted,
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> // As current
> ...
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { Self::Unknown }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self {
> match x {
> HttpStatusCode::Unknown => ::fbthrift::__UNKNOWN_ID,
> HttpStatusCode::Ok => 200,
[jira] [Comment Edited] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17317114#comment-17317114
]
Allen George edited comment on THRIFT-5392 at 4/8/21, 11:37 AM:
This is a separate concern for a separate ticket:
{noformat}
// we do not allow creating enums from variant names (but we do it from enum
"binary" value)
let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected to
be possible
{noformat}
I avoided doing this because names in Thrift are not stable.
was (Author: allengeorge):
This is a separate concern for a separate ticket:
{noformat]
// we do not allow creating enums from variant names (but we do it from enum
"binary" value)
let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected to
be possible
{noformat}
I avoided doing this because names in Thrift are not stable.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that TryFrom was implemented and not the
> infallible form From
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
> backward/forward capabilities I would suggest the introduction of a sentinel
> enum variant (as using Result does not
> look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
> Unknown, // Maybe the variant name could be configurable
> Ok, Created,
[jira] [Commented] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17317112#comment-17317112
]
Allen George commented on THRIFT-5392:
--
Also note that your concern:
{noformat}
// we allow creating un-existing enums from code
let enum_value = HttpStatusCode(1234); // I would have expected an error
{noformat}
is an **expected** side-effect of the implemented approach: to support
forward-compatibility, you have to have the ability to create enum variants
without a named value and encode them onto the wire. Even with the
{{__Unknown(...)}} variant alternative you would still have to take that value
and encode it onto the wire.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that TryFrom was implemented and not the
> infallible form From
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
> backward/forward capabilities I would suggest the introduction of a sentinel
> enum variant (as using Result does not
> look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
> Unknown, // Maybe the variant name could be configurable
> Ok, Created, Accepted,
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> // As current
> ...
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { Self::Unknown }
> }
> impl<'a> From<&'a HttpStatusCode>
[jira] [Commented] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17317108#comment-17317108
]
Allen George commented on THRIFT-5392:
--
Link to the comment where I considered it:
https://issues.apache.org/jira/browse/THRIFT-5314?focusedCommentId=17235560=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17235560
The downsides were that each enum has to have a magic unknown value that
everyone has to check for.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that TryFrom was implemented and not the
> infallible form From
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
> backward/forward capabilities I would suggest the introduction of a sentinel
> enum variant (as using Result does not
> look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
> Unknown, // Maybe the variant name could be configurable
> Ok, Created, Accepted,
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> // As current
> ...
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { Self::Unknown }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self {
> match x {
> HttpStatusCode::Unknown => ::fbthrift::__UNKNOWN_ID,
> HttpStatusCode::Ok => 200,
>
[jira] [Comment Edited] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17317107#comment-17317107
]
Allen George edited comment on THRIFT-5392 at 4/8/21, 11:28 AM:
I considered using a sentinel value, but note that it would have to be
something prefixed specially - for example:
{{Type::__Unknown(actual_unknown_value)}} - to avoid name clashes. In the end,
I took a close look at the Flatbuffers issue linked in the ticket and decided
to use their approach.
As you've noted, given the churn in the enum space (this is the 3rd time I've
had to change the enum generation) I'm incredibly reluctant to change the
output unless I have wider community input.
was (Author: allengeorge):
I considered using a sentinel value, but note that it would have to be
something prefixed specially - for example: {{Type::__Unknown}} - to avoid name
clashes. In the end, I took a close look at the Flatbuffers issue linked in the
ticket and decided to use their approach.
As you've noted, given the churn in the enum space (this is the 3rd time I've
had to change the enum generation) I'm incredibly reluctant to change the
output unless I have wider community input.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that TryFrom was implemented and not the
> infallible form From
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
>
[jira] [Commented] (THRIFT-5392) Thrift Enums should generate forward compatible enum like code
[
https://issues.apache.org/jira/browse/THRIFT-5392?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17317107#comment-17317107
]
Allen George commented on THRIFT-5392:
--
I considered using a sentinel value, but note that it would have to be
something prefixed specially - for example: {{Type::__Unknown}} - to avoid name
clashes. In the end, I took a close look at the Flatbuffers issue linked in the
ticket and decided to use their approach.
As you've noted, given the churn in the enum space (this is the 3rd time I've
had to change the enum generation) I'm incredibly reluctant to change the
output unless I have wider community input.
> Thrift Enums should generate forward compatible enum like code
> --
>
> Key: THRIFT-5392
> URL: https://issues.apache.org/jira/browse/THRIFT-5392
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler, Rust - Library
>Reporter: Samuele Maci
>Priority: Major
> Labels: improvement, rust
>
> I'm starting using thrift interfaces in rust and I've been surprised to
> discover that thrift enums are not generated as rust enums.
> The following thrift enum
> {code:java}
> # Fully made up enum to use as example
> enum HttpStatusCode {
> Ok = 200,
> Created = 201,
> Accepted = 202,
> }
> {code}
> is currently rendered as
> {code:java}
> // The generated code has been shrank a bit for readability
> #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub struct HttpStatusCode(pub i32);
> impl HttpStatusCode {
> pub const Ok: Self = HttpStatusCode(200);
> pub const Created: Self = HttpStatusCode(201);
> pub const Accepted: Self = HttpStatusCode(202);
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> fn enumerate() -> &'static [(HttpStatusCode, &'static str)] {
> &[(Self::Ok, "Ok"), (Self::Created, "Created"), (Self::Accepted,
> "Accepted")]
> }
> fn variants() -> &'static [&'static str] {
> &["Ok", "Created", "Accepted"]
> }
> fn variant_values() -> &'static [HttpStatusCode] {
> &[Self::Ok, Self::Created, Self::Accepted]
> }
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { HttpStatusCode(::fbthrift::__UNKNOWN_ID) }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a HttpStatusCode) -> Self { x.0 }
> }
> impl From for i32 {
> fn from(x: HttpStatusCode) -> Self { x.0 }
> }
> impl From for HttpStatusCode {
> fn from(x: i32) -> Self { Self(x) }
> }
> {code}
> The generated code poses, in my view, some limitations as well as it does not
> use the powerful rust compiler capabilities:
> * having a struct instead of enum removes the capability of the compiler to
> verify for exhaustive matching. The code below would compile
> {code:java}
> let enum_value: HttpStatusCode = foo();
> match enum_value {
> HttpStatusCode::Ok => do_ok(),
> HttpStatusCode::Created => do_ok(),
> // HttpStatusCode::Accepted => ... // This is intentionally left out
> }{code}
> * we allow creating un-existing enums from code
> {code:java}
> let enum_value = HttpStatusCode(1234); // I would have expected an
> error{code}
> I would have expected that TryFrom was implemented and not the
> infallible form From
> * we do not allow creating enums from variant names (but we do it from enum
> "binary" value)
> {code:java}
> let enum_value = HttpStatusCode::try_from("Ok")?; // I would have expected
> to be possible{code}
> I do see that the conversion from rust enum to rust struct was done in
> THRIFT-5314 for forward-compatibility but I'm wondering if that is the best
> way forward to ensure that we can levarage what the rust ecosystem can
> provide us.
> This is mostly meant to lift developers from some tedious and error-prone
> checks which the compiler knows how to do.
> NOTE: Working of a wide code-base and depending on thrift definition of
> thrid part services makes very hard to keep track of all the changes and
> having the compiler reporting the issues is a non-negligible advantage.
> *How could we move forward without impacting way too much on the current
> experience?*
> My idea would be to have back enum variants and in order to have
> backward/forward capabilities I would suggest the introduction of a sentinel
> enum variant (as using Result does not
> look appealing to most)
> {code:java}
> #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
> pub enum HttpStatusCode {
> Unknown, // Maybe the variant name could be configurable
> Ok, Created, Accepted,
> }
> impl ::fbthrift::ThriftEnum for HttpStatusCode {
> // As current
> ...
> }
> impl Default for HttpStatusCode {
> fn default() -> Self { Self::Unknown }
> }
> impl<'a> From<&'a HttpStatusCode> for i32 {
> fn from(x: &'a
[jira] [Resolved] (THRIFT-4098) Support user-defined output namespaces in generated Rust modules
[
https://issues.apache.org/jira/browse/THRIFT-4098?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-4098.
--
Fix Version/s: 0.15.0
Resolution: Fixed
> Support user-defined output namespaces in generated Rust modules
>
>
> Key: THRIFT-4098
> URL: https://issues.apache.org/jira/browse/THRIFT-4098
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
> Fix For: 0.15.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently the Rust compiler assumes that all generated modules are rooted at
> the top-level of your crate (i.e. at {{lib.rs}}). There are many useful cases
> where you want to control exactly where the generated code lives (for example
> - you may want it to be inside another sub-module you control); typically
> this is done via thrift {{namespace}} definitions. The compiler currently
> ignores these declarations. It should be changed to:
> # recognize them if they exist, and generate code with the proper module paths
> # default to the current behavior if no {{namespace}} declarations exist
> # work properly in both cases with service inheritance.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5363) All-caps constant rendered incorrectly
[
https://issues.apache.org/jira/browse/THRIFT-5363?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5363.
--
Fix Version/s: 0.15.0
Resolution: Fixed
> All-caps constant rendered incorrectly
> --
>
> Key: THRIFT-5363
> URL: https://issues.apache.org/jira/browse/THRIFT-5363
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
> Fix For: 0.15.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The all-caps constant {{INT32CONSTANT}} in {{tutorial/tutorial.thrift}} is
> rendered incorrectly as {{I_N_T_32_C_O_N_S_T_A_N_T}}.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5360) Remove deprecated description() methods from Error types
[
https://issues.apache.org/jira/browse/THRIFT-5360?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5360.
--
Resolution: Fixed
> Remove deprecated description() methods from Error types
>
>
> Key: THRIFT-5360
> URL: https://issues.apache.org/jira/browse/THRIFT-5360
> Project: Thrift
> Issue Type: Task
> Components: Rust - Compiler, Rust - Library
>Affects Versions: 0.14.0
>Reporter: Allen George
>Assignee: Allen George
>Priority: Minor
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Rust {{Error}} implementations no longer require you to implement
> {{description}}. They should be removed to allow clippy to run cleanly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (THRIFT-5365) Implement the JSON protocol
Allen George created THRIFT-5365: Summary: Implement the JSON protocol Key: THRIFT-5365 URL: https://issues.apache.org/jira/browse/THRIFT-5365 Project: Thrift Issue Type: New Feature Components: Ruby - Compiler, Rust - Library Reporter: Allen George Implement the JSON protocol. Both using indexes, and full names (for debugging, etc) If possible, make this serde compatible. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Updated] (THRIFT-5364) Remove clippy::vec_box lint override
[
https://issues.apache.org/jira/browse/THRIFT-5364?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-5364:
-
Summary: Remove clippy::vec_box lint override (was: Remove clippy::box_vec
lint override)
> Remove clippy::vec_box lint override
>
>
> Key: THRIFT-5364
> URL: https://issues.apache.org/jira/browse/THRIFT-5364
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Minor
>
> Currently the rust code generator wraps types in a `Box` in two functions:
> # {{to_rust_type}}
> # {{render_type_sync_read}}
> We do this only when a type is a {{typedef}} and
> {{ttypedef->is_forward_typedef() == true}}. This can result in situations
> where the following thrift ({{test/Recursive.thrift}}):
> {noformat}
> struct RecTree {
> 1: list children
> 2: i16 item
> }
> {noformat}
> generates the following rust:
> {noformat}
> #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
> pub struct RecTree {
> pub children: Option>>,
> pub item: Option,
> }
> {noformat}
> Clippy trips this code up with the following error:
> {noformat}
> /root/.cargo/bin/cargo fmt --all -- --check
> /root/.cargo/bin/cargo clippy --all -- -D warnings
> Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
> error: `Vec` is already on the heap, the boxing is unnecessary.
> --> src/recursive.rs:35:24
>|
> 35 | pub children: Option>>,
>|^ help: try:
> `Vec`
>|
>= note: `-D clippy::vec-box` implied by `-D warnings`
>= help: for further information visit
> https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
> error: aborting due to previous error
> error: could not compile `kitchen-sink`.
> {noformat}
> This happens because all container elements
> ({{Vec}},{{BTreeSet}},{{BTreeMap}}) are automatically placed on the heap, so
> a {{Box}} is an additional level of indirection.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-5364) Remove clippy::vec_box lint override
[
https://issues.apache.org/jira/browse/THRIFT-5364?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-5364:
-
Description:
Currently the rust code generator wraps types in a {{Box}} in two functions:
# {{to_rust_type}}
# {{render_type_sync_read}}
We do this only when a type is a {{typedef}} and
{{ttypedef->is_forward_typedef() == true}}. This can result in situations where
the following thrift ({{test/Recursive.thrift}}):
{noformat}
struct RecTree {
1: list children
2: i16 item
}
{noformat}
generates the following rust:
{noformat}
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct RecTree {
pub children: Option>>,
pub item: Option,
}
{noformat}
Clippy trips this code up with the following error:
{noformat}
/root/.cargo/bin/cargo fmt --all -- --check
/root/.cargo/bin/cargo clippy --all -- -D warnings
Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
error: `Vec` is already on the heap, the boxing is unnecessary.
--> src/recursive.rs:35:24
|
35 | pub children: Option>>,
|^ help: try:
`Vec`
|
= note: `-D clippy::vec-box` implied by `-D warnings`
= help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
error: aborting due to previous error
error: could not compile `kitchen-sink`.
{noformat}
This happens because all container elements ({{Vec}},{{BTreeSet}},{{BTreeMap}})
are automatically placed on the heap, so a {{Box}} is an additional level of
indirection.
was:
Currently the rust code generator wraps types in a `Box` in two functions:
# {{to_rust_type}}
# {{render_type_sync_read}}
We do this only when a type is a {{typedef}} and
{{ttypedef->is_forward_typedef() == true}}. This can result in situations where
the following thrift ({{test/Recursive.thrift}}):
{noformat}
struct RecTree {
1: list children
2: i16 item
}
{noformat}
generates the following rust:
{noformat}
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct RecTree {
pub children: Option>>,
pub item: Option,
}
{noformat}
Clippy trips this code up with the following error:
{noformat}
/root/.cargo/bin/cargo fmt --all -- --check
/root/.cargo/bin/cargo clippy --all -- -D warnings
Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
error: `Vec` is already on the heap, the boxing is unnecessary.
--> src/recursive.rs:35:24
|
35 | pub children: Option>>,
|^ help: try:
`Vec`
|
= note: `-D clippy::vec-box` implied by `-D warnings`
= help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
error: aborting due to previous error
error: could not compile `kitchen-sink`.
{noformat}
This happens because all container elements ({{Vec}},{{BTreeSet}},{{BTreeMap}})
are automatically placed on the heap, so a {{Box}} is an additional level of
indirection.
> Remove clippy::vec_box lint override
>
>
> Key: THRIFT-5364
> URL: https://issues.apache.org/jira/browse/THRIFT-5364
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Minor
>
> Currently the rust code generator wraps types in a {{Box}} in two functions:
> # {{to_rust_type}}
> # {{render_type_sync_read}}
> We do this only when a type is a {{typedef}} and
> {{ttypedef->is_forward_typedef() == true}}. This can result in situations
> where the following thrift ({{test/Recursive.thrift}}):
> {noformat}
> struct RecTree {
> 1: list children
> 2: i16 item
> }
> {noformat}
> generates the following rust:
> {noformat}
> #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
> pub struct RecTree {
> pub children: Option>>,
> pub item: Option,
> }
> {noformat}
> Clippy trips this code up with the following error:
> {noformat}
> /root/.cargo/bin/cargo fmt --all -- --check
> /root/.cargo/bin/cargo clippy --all -- -D warnings
> Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
> error: `Vec` is already on the heap, the boxing is unnecessary.
> --> src/recursive.rs:35:24
>|
> 35 | pub children: Option>>,
>|^ help: try:
> `Vec`
>|
>= note: `-D clippy::vec-box` implied by `-D warnings`
>= help: for further information visit
> https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
> error: aborting due to previous error
> error: could not compile `kitchen-sink`.
> {noformat}
> This happens because all container elements
> ({{Vec}},{{BTreeSet}},{{BTreeMap}}) are automatically placed on the heap, so
> a {{Box}} is an additional level of indirection.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Assigned] (THRIFT-4777) Non Blocking Server implementation for Rust
[ https://issues.apache.org/jira/browse/THRIFT-4777?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George reassigned THRIFT-4777: Assignee: (was: Allen George) > Non Blocking Server implementation for Rust > --- > > Key: THRIFT-4777 > URL: https://issues.apache.org/jira/browse/THRIFT-4777 > Project: Thrift > Issue Type: New Feature > Components: Rust - Library >Reporter: Satyender Yadav >Priority: Minor > > Implement Non blocking server in Thrift rust library. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Assigned] (THRIFT-4098) Support user-defined output namespaces in generated Rust modules
[
https://issues.apache.org/jira/browse/THRIFT-4098?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George reassigned THRIFT-4098:
Assignee: Allen George
> Support user-defined output namespaces in generated Rust modules
>
>
> Key: THRIFT-4098
> URL: https://issues.apache.org/jira/browse/THRIFT-4098
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
>
> Currently the Rust compiler assumes that all generated modules are rooted at
> the top-level of your crate (i.e. at {{lib.rs}}). There are many useful cases
> where you want to control exactly where the generated code lives (for example
> - you may want it to be inside another sub-module you control); typically
> this is done via thrift {{namespace}} definitions. The compiler currently
> ignores these declarations. It should be changed to:
> # recognize them if they exist, and generate code with the proper module paths
> # default to the current behavior if no {{namespace}} declarations exist
> # work properly in both cases with service inheritance.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Assigned] (THRIFT-5360) Remove deprecated description() methods from Error types
[
https://issues.apache.org/jira/browse/THRIFT-5360?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George reassigned THRIFT-5360:
Assignee: Allen George
> Remove deprecated description() methods from Error types
>
>
> Key: THRIFT-5360
> URL: https://issues.apache.org/jira/browse/THRIFT-5360
> Project: Thrift
> Issue Type: Task
> Components: Rust - Compiler, Rust - Library
>Affects Versions: 0.14.0
>Reporter: Allen George
>Assignee: Allen George
>Priority: Minor
>
> Rust {{Error}} implementations no longer require you to implement
> {{description}}. They should be removed to allow clippy to run cleanly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Assigned] (THRIFT-5363) All-caps constant rendered incorrectly
[
https://issues.apache.org/jira/browse/THRIFT-5363?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George reassigned THRIFT-5363:
Assignee: Allen George
> All-caps constant rendered incorrectly
> --
>
> Key: THRIFT-5363
> URL: https://issues.apache.org/jira/browse/THRIFT-5363
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
>
> The all-caps constant {{INT32CONSTANT}} in {{tutorial/tutorial.thrift}} is
> rendered incorrectly as {{I_N_T_32_C_O_N_S_T_A_N_T}}.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-5054) Support THeader in Rust
[ https://issues.apache.org/jira/browse/THRIFT-5054?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George updated THRIFT-5054: - Priority: Minor (was: Trivial) > Support THeader in Rust > --- > > Key: THRIFT-5054 > URL: https://issues.apache.org/jira/browse/THRIFT-5054 > Project: Thrift > Issue Type: Improvement > Components: Rust - Library >Reporter: Yu Ding >Priority: Minor > > Hi there, I'm just curious if there's a plan for supporting THeader protocol > in Rust lib. Thanks! > > Best, > Yu -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Updated] (THRIFT-4098) Support user-defined output namespaces in generated Rust modules
[
https://issues.apache.org/jira/browse/THRIFT-4098?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-4098:
-
Priority: Major (was: Minor)
> Support user-defined output namespaces in generated Rust modules
>
>
> Key: THRIFT-4098
> URL: https://issues.apache.org/jira/browse/THRIFT-4098
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Major
>
> Currently the Rust compiler assumes that all generated modules are rooted at
> the top-level of your crate (i.e. at {{lib.rs}}). There are many useful cases
> where you want to control exactly where the generated code lives (for example
> - you may want it to be inside another sub-module you control); typically
> this is done via thrift {{namespace}} definitions. The compiler currently
> ignores these declarations. It should be changed to:
> # recognize them if they exist, and generate code with the proper module paths
> # default to the current behavior if no {{namespace}} declarations exist
> # work properly in both cases with service inheritance.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-5364) Remove clippy::box_vec lint override
[
https://issues.apache.org/jira/browse/THRIFT-5364?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-5364:
-
Priority: Minor (was: Major)
> Remove clippy::box_vec lint override
>
>
> Key: THRIFT-5364
> URL: https://issues.apache.org/jira/browse/THRIFT-5364
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Minor
>
> Currently the rust code generator wraps types in a `Box` in two functions:
> # {{to_rust_type}}
> # {{render_type_sync_read}}
> We do this only when a type is a {{typedef}} and
> {{ttypedef->is_forward_typedef() == true}}. This can result in situations
> where the following thrift ({{test/Recursive.thrift}}):
> {noformat}
> struct RecTree {
> 1: list children
> 2: i16 item
> }
> {noformat}
> generates the following rust:
> {noformat}
> #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
> pub struct RecTree {
> pub children: Option>>,
> pub item: Option,
> }
> {noformat}
> Clippy trips this code up with the following error:
> {noformat}
> /root/.cargo/bin/cargo fmt --all -- --check
> /root/.cargo/bin/cargo clippy --all -- -D warnings
> Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
> error: `Vec` is already on the heap, the boxing is unnecessary.
> --> src/recursive.rs:35:24
>|
> 35 | pub children: Option>>,
>|^ help: try:
> `Vec`
>|
>= note: `-D clippy::vec-box` implied by `-D warnings`
>= help: for further information visit
> https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
> error: aborting due to previous error
> error: could not compile `kitchen-sink`.
> {noformat}
> This happens because all container elements
> ({{Vec}},{{BTreeSet}},{{BTreeMap}}) are automatically placed on the heap, so
> a {{Box}} is an additional level of indirection.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5339) [Rust] Add `TObject` trait to cover sync read & write methods of Struct/Union/Enum
[
https://issues.apache.org/jira/browse/THRIFT-5339?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5339.
--
Resolution: Duplicate
Relevant, but duplicates THRIFT-4100.
> [Rust] Add `TObject` trait to cover sync read & write methods of
> Struct/Union/Enum
> --
>
> Key: THRIFT-5339
> URL: https://issues.apache.org/jira/browse/THRIFT-5339
> Project: Thrift
> Issue Type: New Feature
> Components: Rust - Compiler
>Affects Versions: 0.13.0
>Reporter: Jason Shui
>Priority: Major
> Time Spent: 50m
> Remaining Estimate: 0h
>
> Each Struct/Union/Enum generated by thrift-compiler will get two methods:
> `read_from_in_protocol` and `write_to_out_protocol`.
> Firstly, I think these two methods are representing a certain ability of
> serialization, so they should be put in a trait;
> Moreover, if this trait is provided, we can use trait bounding in our user
> codes, such as
> ```rust
> // pseudo code
> fn serialize_struct(s: T) -> Vec where T: TObject
> { s.write_to_out_protocol(protocol); // do other things.. }
> ```
> It could also be a pretty useful marker trait as you can see.
> The name of `TObject` is only a superficial opinion, but I believe that
> there are enough reasons to make the trait.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5364) Remove clippy::box_vec lint override
[
https://issues.apache.org/jira/browse/THRIFT-5364?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17296551#comment-17296551
]
Allen George commented on THRIFT-5364:
--
Also something to note: {{is_typedef}} and {{is_forward_typedef}} is used in
the Rust code generation to determine if the structs are potentially recursive.
This is the case because these flags are set when the IDL parser is generating
types and hits a type that isn't fully defined. Unfortunately this set of flags
_also_ gets defined for the following non-recursive thrift:
{noformat}
struct Foo {
1: Bar item
}
struct Bar {
1: i16 index
}
{noformat}
In this case we'd end up boxing unnecessarily.
> Remove clippy::box_vec lint override
>
>
> Key: THRIFT-5364
> URL: https://issues.apache.org/jira/browse/THRIFT-5364
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Major
>
> Currently the rust code generator wraps types in a `Box` in two functions:
> # {{to_rust_type}}
> # {{render_type_sync_read}}
> We do this only when a type is a {{typedef}} and
> {{ttypedef->is_forward_typedef() == true}}. This can result in situations
> where the following thrift ({{test/Recursive.thrift}}):
> {noformat}
> struct RecTree {
> 1: list children
> 2: i16 item
> }
> {noformat}
> generates the following rust:
> {noformat}
> #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
> pub struct RecTree {
> pub children: Option>>,
> pub item: Option,
> }
> {noformat}
> Clippy trips this code up with the following error:
> {noformat}
> /root/.cargo/bin/cargo fmt --all -- --check
> /root/.cargo/bin/cargo clippy --all -- -D warnings
> Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
> error: `Vec` is already on the heap, the boxing is unnecessary.
> --> src/recursive.rs:35:24
>|
> 35 | pub children: Option>>,
>|^ help: try:
> `Vec`
>|
>= note: `-D clippy::vec-box` implied by `-D warnings`
>= help: for further information visit
> https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
> error: aborting due to previous error
> error: could not compile `kitchen-sink`.
> {noformat}
> This happens because all container elements
> ({{Vec}},{{BTreeSet}},{{BTreeMap}}) are automatically placed on the heap, so
> a {{Box}} is an additional level of indirection.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5364) Remove clippy::box_vec lint override
[
https://issues.apache.org/jira/browse/THRIFT-5364?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17296550#comment-17296550
]
Allen George commented on THRIFT-5364:
--
I _think_ the best option here is to know what context the code is being
generated in, i.e. whether the type/read is for a top-level container element
or not. If it's a top-level container element we should **never** use boxing,
regardless of whether the type is a typedef or not. But if it's not a top-level
container element then we should use the standard rules.
In other words, add an {{is_top_level_container_element}} as a flag that can
override the default code-generation behavior.
> Remove clippy::box_vec lint override
>
>
> Key: THRIFT-5364
> URL: https://issues.apache.org/jira/browse/THRIFT-5364
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Major
>
> Currently the rust code generator wraps types in a `Box` in two functions:
> # {{to_rust_type}}
> # {{render_type_sync_read}}
> We do this only when a type is a {{typedef}} and
> {{ttypedef->is_forward_typedef() == true}}. This can result in situations
> where the following thrift ({{test/Recursive.thrift}}):
> {noformat}
> struct RecTree {
> 1: list children
> 2: i16 item
> }
> {noformat}
> generates the following rust:
> {noformat}
> #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
> pub struct RecTree {
> pub children: Option>>,
> pub item: Option,
> }
> {noformat}
> Clippy trips this code up with the following error:
> {noformat}
> /root/.cargo/bin/cargo fmt --all -- --check
> /root/.cargo/bin/cargo clippy --all -- -D warnings
> Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
> error: `Vec` is already on the heap, the boxing is unnecessary.
> --> src/recursive.rs:35:24
>|
> 35 | pub children: Option>>,
>|^ help: try:
> `Vec`
>|
>= note: `-D clippy::vec-box` implied by `-D warnings`
>= help: for further information visit
> https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
> error: aborting due to previous error
> error: could not compile `kitchen-sink`.
> {noformat}
> This happens because all container elements
> ({{Vec}},{{BTreeSet}},{{BTreeMap}}) are automatically placed on the heap, so
> a {{Box}} is an additional level of indirection.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (THRIFT-5364) Remove clippy::box_vec lint override
Allen George created THRIFT-5364:
Summary: Remove clippy::box_vec lint override
Key: THRIFT-5364
URL: https://issues.apache.org/jira/browse/THRIFT-5364
Project: Thrift
Issue Type: Improvement
Components: Rust - Compiler
Reporter: Allen George
Currently the rust code generator wraps types in a `Box` in two functions:
# {{to_rust_type}}
# {{render_type_sync_read}}
We do this only when a type is a {{typedef}} and
{{ttypedef->is_forward_typedef() == true}}. This can result in situations where
the following thrift ({{test/Recursive.thrift}}):
{noformat}
struct RecTree {
1: list children
2: i16 item
}
{noformat}
generates the following rust:
{noformat}
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct RecTree {
pub children: Option>>,
pub item: Option,
}
{noformat}
Clippy trips this code up with the following error:
{noformat}
/root/.cargo/bin/cargo fmt --all -- --check
/root/.cargo/bin/cargo clippy --all -- -D warnings
Checking kitchen-sink v0.1.0 (/thrift/src/lib/rs/test)
error: `Vec` is already on the heap, the boxing is unnecessary.
--> src/recursive.rs:35:24
|
35 | pub children: Option>>,
|^ help: try:
`Vec`
|
= note: `-D clippy::vec-box` implied by `-D warnings`
= help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
error: aborting due to previous error
error: could not compile `kitchen-sink`.
{noformat}
This happens because all container elements ({{Vec}},{{BTreeSet}},{{BTreeMap}})
are automatically placed on the heap, so a {{Box}} is an additional level of
indirection.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Reopened] (THRIFT-5054) Support THeader in Rust
[ https://issues.apache.org/jira/browse/THRIFT-5054?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George reopened THRIFT-5054: -- > Support THeader in Rust > --- > > Key: THRIFT-5054 > URL: https://issues.apache.org/jira/browse/THRIFT-5054 > Project: Thrift > Issue Type: Improvement > Components: Rust - Library >Reporter: Yu Ding >Priority: Trivial > > Hi there, I'm just curious if there's a plan for supporting THeader protocol > in Rust lib. Thanks! > > Best, > Yu -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (THRIFT-5054) Support THeader in Rust
[ https://issues.apache.org/jira/browse/THRIFT-5054?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17293804#comment-17293804 ] Allen George commented on THRIFT-5054: -- OK. Reopening. > Support THeader in Rust > --- > > Key: THRIFT-5054 > URL: https://issues.apache.org/jira/browse/THRIFT-5054 > Project: Thrift > Issue Type: Improvement > Components: Rust - Library >Reporter: Yu Ding >Priority: Trivial > > Hi there, I'm just curious if there's a plan for supporting THeader protocol > in Rust lib. Thanks! > > Best, > Yu -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Resolved] (THRIFT-5054) Support THeader in Rust
[ https://issues.apache.org/jira/browse/THRIFT-5054?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George resolved THRIFT-5054. -- Resolution: Won't Fix Since this is Facebook-only, deciding not to support this. > Support THeader in Rust > --- > > Key: THRIFT-5054 > URL: https://issues.apache.org/jira/browse/THRIFT-5054 > Project: Thrift > Issue Type: Improvement > Components: Rust - Library >Reporter: Yu Ding >Priority: Trivial > > Hi there, I'm just curious if there's a plan for supporting THeader protocol > in Rust lib. Thanks! > > Best, > Yu -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (THRIFT-5363) All-caps constant rendered incorrectly
Allen George created THRIFT-5363:
Summary: All-caps constant rendered incorrectly
Key: THRIFT-5363
URL: https://issues.apache.org/jira/browse/THRIFT-5363
Project: Thrift
Issue Type: Bug
Components: Rust - Compiler
Reporter: Allen George
The all-caps constant {{INT32CONSTANT}} in {{tutorial/tutorial.thrift}} is
rendered incorrectly as {{I_N_T_32_C_O_N_S_T_A_N_T}}.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5314) Enum forward compatibility
[ https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George resolved THRIFT-5314. -- Fix Version/s: 0.15.0 Resolution: Fixed Done in https://github.com/apache/thrift/pull/2337 > Enum forward compatibility > -- > > Key: THRIFT-5314 > URL: https://issues.apache.org/jira/browse/THRIFT-5314 > Project: Thrift > Issue Type: Bug > Components: Rust - Compiler, Rust - Library >Affects Versions: 0.13.0, 0.14.0 >Reporter: Remi Dettai >Assignee: Allen George >Priority: Major > Fix For: 0.15.0 > > Time Spent: 0.5h > Remaining Estimate: 0h > > It seems that enums in the Rust implem are not forward compatible. As Thrift > enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case > to an enum, an error will be returned when parsing the message. > Is this intended? Is there a workaround? > (We met this problem in the Rust parquet implem: > https://issues.apache.org/jira/browse/ARROW-10553) -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Resolved] (THRIFT-4101) Make auto-generated Rust enums and unions more user-extensible
[ https://issues.apache.org/jira/browse/THRIFT-4101?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George resolved THRIFT-4101. -- Fix Version/s: 0.15.0 Resolution: Fixed Done in https://github.com/apache/thrift/pull/2337 > Make auto-generated Rust enums and unions more user-extensible > -- > > Key: THRIFT-4101 > URL: https://issues.apache.org/jira/browse/THRIFT-4101 > Project: Thrift > Issue Type: Improvement > Components: Rust - Compiler, Rust - Library >Reporter: Allen George >Assignee: Allen George >Priority: Minor > Fix For: 0.15.0 > > > Since we cannot capture a list of enum variants at compile time certain > user-derived behaviors are hard (or impossible) to express. See previous > discussion here: THRIFT-2945 -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (THRIFT-5283) Ability to listen over unix socket in thrift Rust crate
[
https://issues.apache.org/jira/browse/THRIFT-5283?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17292887#comment-17292887
]
Allen George commented on THRIFT-5283:
--
[~prateeknischal] Is adding this functionality still important to you?
> Ability to listen over unix socket in thrift Rust crate
> ---
>
> Key: THRIFT-5283
> URL: https://issues.apache.org/jira/browse/THRIFT-5283
> Project: Thrift
> Issue Type: New Feature
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: Prateek Kumar Nischal
>Priority: Major
>
> The rust crate for [thrift|https://crates.io/crates/thrift] right now has the
> ability to create a server [but only over a TCP
> socket|https://github.com/apache/thrift/blob/master/lib/rs/src/server/threaded.rs#L172].
> {code}
> pub fn listen( self, listen_address: A) ->
> thrift::Result<()>
> {code}
> The API requires the trait
> [ToSocketAddrs|https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html] to
> be implemented which is not possible for a Unix Domain Socket.
> Other libraries, for example, python has an option to serve over unix
> sockets. eg
> {code:python}
> TSocket.TServerSocket(unix_socket='/tmp/service.sock')
> {code}
> It would be really nice to be able to get a similar API in rust as well
> unless there is a way to do it already. Please let me know if it already
> exists.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-4451) Rust cross test client fails to communicate with multiplexed servers
[
https://issues.apache.org/jira/browse/THRIFT-4451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-4451.
--
Resolution: Fixed
> Rust cross test client fails to communicate with multiplexed servers
>
>
> Key: THRIFT-4451
> URL: https://issues.apache.org/jira/browse/THRIFT-4451
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2018-11-09 at 07.03.39.png, Screen Shot
> 2021-02-27 at 13.31.21.png, rs_client_tests_post_change.txt,
> rs_server_tests_post_change.txt
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> As stated in description. Minimal case is to comment out everything in the
> Rust {{test_client}} leaving only the {{SecondService}} call behind.
> From what I can tell the Rust socket isn't getting *any* bytes at all for the
> response (i.e. it can't even get the first 4 bytes of the message header).
> There is a {{flush()}} call on the remote side - so that's puzzling.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-4451) Rust cross test client fails to communicate with multiplexed servers
[
https://issues.apache.org/jira/browse/THRIFT-4451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-4451:
-
Attachment: rs_client_tests_post_change.txt
> Rust cross test client fails to communicate with multiplexed servers
>
>
> Key: THRIFT-4451
> URL: https://issues.apache.org/jira/browse/THRIFT-4451
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2018-11-09 at 07.03.39.png, Screen Shot
> 2021-02-27 at 13.31.21.png, rs_client_tests_post_change.txt,
> rs_server_tests_post_change.txt
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> As stated in description. Minimal case is to comment out everything in the
> Rust {{test_client}} leaving only the {{SecondService}} call behind.
> From what I can tell the Rust socket isn't getting *any* bytes at all for the
> response (i.e. it can't even get the first 4 bytes of the message header).
> There is a {{flush()}} call on the remote side - so that's puzzling.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-4451) Rust cross test client fails to communicate with multiplexed servers
[
https://issues.apache.org/jira/browse/THRIFT-4451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17292421#comment-17292421
]
Allen George commented on THRIFT-4451:
--
Made change to share the underlying {{TCPStream}} between clients. Post change:
* Ran rs server cross tests. Only failures were {{dotnetstd}}, which for some
reason I can't build on the docker image.
* Ran rs client cross tests. Only failures were {{dotnetstd}}, {{cl}} and
{{py}}. {{dotnetstd}} doesn't build on the docker image. {{py}} can't find a
Thrift lib (unsure why), and {{cl}} seems to be broken on _its_ side.
Looks like the fix works.
> Rust cross test client fails to communicate with multiplexed servers
>
>
> Key: THRIFT-4451
> URL: https://issues.apache.org/jira/browse/THRIFT-4451
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2018-11-09 at 07.03.39.png, Screen Shot
> 2021-02-27 at 13.31.21.png, rs_server_tests_post_change.txt
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> As stated in description. Minimal case is to comment out everything in the
> Rust {{test_client}} leaving only the {{SecondService}} call behind.
> From what I can tell the Rust socket isn't getting *any* bytes at all for the
> response (i.e. it can't even get the first 4 bytes of the message header).
> There is a {{flush()}} call on the remote side - so that's puzzling.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-4451) Rust cross test client fails to communicate with multiplexed servers
[
https://issues.apache.org/jira/browse/THRIFT-4451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-4451:
-
Attachment: rs_server_tests_post_change.txt
> Rust cross test client fails to communicate with multiplexed servers
>
>
> Key: THRIFT-4451
> URL: https://issues.apache.org/jira/browse/THRIFT-4451
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2018-11-09 at 07.03.39.png, Screen Shot
> 2021-02-27 at 13.31.21.png, rs_server_tests_post_change.txt
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> As stated in description. Minimal case is to comment out everything in the
> Rust {{test_client}} leaving only the {{SecondService}} call behind.
> From what I can tell the Rust socket isn't getting *any* bytes at all for the
> response (i.e. it can't even get the first 4 bytes of the message header).
> There is a {{flush()}} call on the remote side - so that's puzzling.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-4451) Rust cross test client fails to communicate with multiplexed servers
[
https://issues.apache.org/jira/browse/THRIFT-4451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17292204#comment-17292204
]
Allen George commented on THRIFT-4451:
--
I made some time recently to work on the Rust Thrift implementation again and
decided to look at this with fresh eyes.
h3. What's failing
I started off by evaluating the state of the world. There have been quite a few
changes/bugfixes to the Rust implementation over the past two years, and I
wanted to see where things were.
The first interesting thing I noticed was that the Rust multi _server_
implementation was solid; every client was able to communicate with it without
issues. The second interesting thing I noticed was that the Rust multi _client_
implementation only failed with some languages, not others. Notably,
communication with Java multi servers was totally fine.
{noformat}
root@3cec70e4dd34:/thrift/src# test/test.py --client rs
Apache Thrift - Integration Test Suite
Mon Feb 22 20:19:10 2021
===
server-client: protocol: transport: result:
c_glib-rs multi-binary buffered-ip success
c_glib-rs multic-compactbuffered-ip success
c_glib-rs multi-binary framed-ipsuccess
c_glib-rs multic-compactframed-ipsuccess
c_glib-rs multi-binary buffered-ip success
c_glib-rs multi-binary framed-ipsuccess
c_glib-rs multi framed-ip
failure(timeout)
c_glib-rs multi buffered-ip
failure(timeout)
c_glib-rs multic-compactbuffered-ip success
c_glib-rs multic-compactframed-ipsuccess
cl-rs multi-binary buffered-ip failure(1)
cl-rs multi-binary framed-ipfailure(1)
cl-rs binarybuffered-ip failure(1)
cl-rs binaryframed-ipfailure(1)
c_glib-rs multicframed-ip
failure(timeout)
c_glib-rs multicbuffered-ip
failure(timeout)
cl-rs multi framed-ip
failure(timeout)
cl-rs multi buffered-ip
failure(timeout)
java-rs multi buffered-ip success
java-rs multi fastframed-framed-ip success
java-rs multi framed-ipsuccess
java-rs multi-binary buffered-ip success
java-rs multi-binary fastframed-framed-ip success
java-rs multi-binary framed-ipsuccess
java-rs multic-compactbuffered-ip success
java-rs multic-compactfastframed-framed-ip success
java-rs multic-compactframed-ipsuccess
java-rs multi-binary buffered-ip success
java-rs multi-binary fastframed-framed-ip success
java-rs multi-binary framed-ipsuccess
java-rs multicfastframed-framed-ip success
java-rs multicbuffered-ip success
java-rs multicframed-ipsuccess
java-rs multic-compactbuffered-ip success
java-rs multic-compactfastframed-framed-ip success
java-rs multic-compactframed-ipsuccess
py-rs multi framed-ipfailure(65)
py-rs multi buffered-ip failure(65)
py-rs multi-binary framed-ipfailure(65)
py-rs multi-binary buffered-ip failure(65)
py-rs binaryframed-ipfailure(65)
py-rs binarybuffered-ip failure(65)
py-rs compact framed-ipfailure(65)
py-rs compact buffered-ip failure(65)
py-rs multicframed-ipfailure(65)
py-rs multicbuffered-ip failure(65)
py-rs multic-compactframed-ipfailure(65)
py-rs multic-compactbuffered-ip failure(65)
py-rs accelc-compactframed-ip
[jira] [Updated] (THRIFT-4451) Rust cross test client fails to communicate with multiplexed servers
[
https://issues.apache.org/jira/browse/THRIFT-4451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-4451:
-
Attachment: Screen Shot 2021-02-27 at 13.31.21.png
> Rust cross test client fails to communicate with multiplexed servers
>
>
> Key: THRIFT-4451
> URL: https://issues.apache.org/jira/browse/THRIFT-4451
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Reporter: Allen George
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2018-11-09 at 07.03.39.png, Screen Shot
> 2021-02-27 at 13.31.21.png
>
>
> As stated in description. Minimal case is to comment out everything in the
> Rust {{test_client}} leaving only the {{SecondService}} call behind.
> From what I can tell the Rust socket isn't getting *any* bytes at all for the
> response (i.e. it can't even get the first 4 bytes of the message header).
> There is a {{flush()}} call on the remote side - so that's puzzling.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5314) Enum forward compatibility
[
https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17288781#comment-17288781
]
Allen George edited comment on THRIFT-5314 at 2/23/21, 1:56 AM:
I've taken a crack at this here: https://github.com/apache/thrift/pull/2337
After looking at the alternatives:
# the one I proposed:
https://issues.apache.org/jira/browse/THRIFT-5314?focusedCommentId=17235560=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17235560)
# the flatbuffer implementation:
https://github.com/google/flatbuffers/pull/6098/files
I decided that comments from others were correct: the flatbuffer one was
cleaner, avoided the use of a magic {{__Unknown}} and still allowed us to use
basic matching.
At the core, we now implement enums using the newtype idiom:
Old way:
{noformat}
pub enum Foo {
VariantOne = 1,
VariantTwo = 2.
...
}
{noformat}
New way:
{noformat}
pub struct Foo(pub i32);
impl Foo {
pub const VARIANT_ONE: Foo = 1;
pub const VARIANT_TWO: Foo = 2;
pub const ENUM_VALUES: &'static [Self] = &[Self::VARIANT_1, Self::VARIANT_2];
...
}
{noformat}
I've also implemented {{std::convert::From}} for some types.
Please note that to conform to the Rust style guide enum variants are now in
uppercase as opposed to camelcase. Whenever possible I prefer to use Rust's
default conventions.
cc [~rdettai]
was (Author: allengeorge):
I've taken a crack at this here: https://github.com/apache/thrift/pull/2337
After looking at the alternatives:
# the one I proposed:
https://issues.apache.org/jira/browse/THRIFT-5314?focusedCommentId=17235560=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17235560)
# the flatbuffer implementation:
https://github.com/google/flatbuffers/pull/6098/files
I decided that comments from others were correct: the flatbuffer one was
cleaner, avoided the use of a magic {{__Unknown}} and still allowed us to use
basic matching.
At the core, we now implement enums using the newtype idiom:
Old way:
{noformat}
pub enum Foo {
VariantOne = 1,
VariantTwo = 2.
...
}
{noformat}
New way:
{noformat}
pub struct Foo(pub i32);
impl Foo {
pub const VARIANT_ONE: Foo = 1;
pub const VARIANT_TWO: Foo = 2;
pub const ENUM_VALUES: &'static [Self] = &[Self::VARIANT_1, Self::VARIANT_2];
...
}
{noformat}
I've also implemented {std::convert::From} for some types.
Please note that to conform to the Rust style guide enum variants are now in
uppercase as opposed to camelcase. Whenever possible I prefer to use Rust's
default conventions.
cc [~rdettai]
> Enum forward compatibility
> --
>
> Key: THRIFT-5314
> URL: https://issues.apache.org/jira/browse/THRIFT-5314
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler, Rust - Library
>Affects Versions: 0.13.0, 0.14.0
>Reporter: Remi Dettai
>Assignee: Allen George
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> It seems that enums in the Rust implem are not forward compatible. As Thrift
> enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case
> to an enum, an error will be returned when parsing the message.
> Is this intended? Is there a workaround?
> (We met this problem in the Rust parquet implem:
> https://issues.apache.org/jira/browse/ARROW-10553)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5314) Enum forward compatibility
[
https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17288781#comment-17288781
]
Allen George commented on THRIFT-5314:
--
I've taken a crack at this here: https://github.com/apache/thrift/pull/2337
After looking at the alternatives:
# the one I proposed:
https://issues.apache.org/jira/browse/THRIFT-5314?focusedCommentId=17235560=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17235560)
# the flatbuffer implementation:
https://github.com/google/flatbuffers/pull/6098/files
I decided that comments from others were correct: the flatbuffer one was
cleaner, avoided the use of a magic {{__Unknown}} and still allowed us to use
basic matching.
At the core, we now implement enums using the newtype idiom:
Old way:
{noformat}
pub enum Foo {
VariantOne = 1,
VariantTwo = 2.
...
}
{noformat}
New way:
{noformat}
pub struct Foo(pub i32);
impl Foo {
pub const VARIANT_ONE: Foo = 1;
pub const VARIANT_TWO: Foo = 2;
pub const ENUM_VALUES: &'static [Self] = &[Self::VARIANT_1, Self::VARIANT_2];
...
}
{noformat}
I've also implemented {std::convert::From} for some types.
Please note that to conform to the Rust style guide enum variants are now in
uppercase as opposed to camelcase. Whenever possible I prefer to use Rust's
default conventions.
cc [~rdettai]
> Enum forward compatibility
> --
>
> Key: THRIFT-5314
> URL: https://issues.apache.org/jira/browse/THRIFT-5314
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler, Rust - Library
>Affects Versions: 0.13.0, 0.14.0
>Reporter: Remi Dettai
>Assignee: Allen George
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> It seems that enums in the Rust implem are not forward compatible. As Thrift
> enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case
> to an enum, an error will be returned when parsing the message.
> Is this intended? Is there a workaround?
> (We met this problem in the Rust parquet implem:
> https://issues.apache.org/jira/browse/ARROW-10553)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (THRIFT-5361) Investigate alternatives to const map/set/list creation
Allen George created THRIFT-5361:
Summary: Investigate alternatives to const map/set/list creation
Key: THRIFT-5361
URL: https://issues.apache.org/jira/browse/THRIFT-5361
Project: Thrift
Issue Type: Task
Components: Rust - Compiler
Affects Versions: 0.14.0
Reporter: Allen George
Investigate the use of crates like {{phf}} or {{lazy_static}} to create const
collections as opposed to the current implementation.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (THRIFT-5360) Remove deprecated description() methods from Error types
Allen George created THRIFT-5360:
Summary: Remove deprecated description() methods from Error types
Key: THRIFT-5360
URL: https://issues.apache.org/jira/browse/THRIFT-5360
Project: Thrift
Issue Type: Task
Components: Rust - Compiler, Rust - Library
Affects Versions: 0.14.0
Reporter: Allen George
Rust {{Error}} implementations no longer require you to implement
{{description}}. They should be removed to allow clippy to run cleanly.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-5314) Enum forward compatibility
[ https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George updated THRIFT-5314: - Affects Version/s: 0.14.0 > Enum forward compatibility > -- > > Key: THRIFT-5314 > URL: https://issues.apache.org/jira/browse/THRIFT-5314 > Project: Thrift > Issue Type: Bug > Components: Rust - Compiler, Rust - Library >Affects Versions: 0.13.0, 0.14.0 >Reporter: Remi Dettai >Assignee: Allen George >Priority: Major > > It seems that enums in the Rust implem are not forward compatible. As Thrift > enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case > to an enum, an error will be returned when parsing the message. > Is this intended? Is there a workaround? > (We met this problem in the Rust parquet implem: > https://issues.apache.org/jira/browse/ARROW-10553) -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Assigned] (THRIFT-5314) Enum forward compatibility
[ https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George reassigned THRIFT-5314: Assignee: Allen George > Enum forward compatibility > -- > > Key: THRIFT-5314 > URL: https://issues.apache.org/jira/browse/THRIFT-5314 > Project: Thrift > Issue Type: Bug > Components: Rust - Compiler, Rust - Library >Affects Versions: 0.13.0 >Reporter: Remi Dettai >Assignee: Allen George >Priority: Major > > It seems that enums in the Rust implem are not forward compatible. As Thrift > enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case > to an enum, an error will be returned when parsing the message. > Is this intended? Is there a workaround? > (We met this problem in the Rust parquet implem: > https://issues.apache.org/jira/browse/ARROW-10553) -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Assigned] (THRIFT-4101) Make auto-generated Rust enums and unions more user-extensible
[ https://issues.apache.org/jira/browse/THRIFT-4101?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George reassigned THRIFT-4101: Assignee: Allen George > Make auto-generated Rust enums and unions more user-extensible > -- > > Key: THRIFT-4101 > URL: https://issues.apache.org/jira/browse/THRIFT-4101 > Project: Thrift > Issue Type: Improvement > Components: Rust - Compiler, Rust - Library >Reporter: Allen George >Assignee: Allen George >Priority: Minor > > Since we cannot capture a list of enum variants at compile time certain > user-derived behaviors are hard (or impossible) to express. See previous > discussion here: THRIFT-2945 -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Resolved] (THRIFT-5299) rs implementation compact protocol seq_id should not use zigzag encoding.
[ https://issues.apache.org/jira/browse/THRIFT-5299?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George resolved THRIFT-5299. -- Fix Version/s: 0.14.1 Resolution: Fixed Fixed in https://github.com/apache/thrift/pull/2336 > rs implementation compact protocol seq_id should not use zigzag encoding. > - > > Key: THRIFT-5299 > URL: https://issues.apache.org/jira/browse/THRIFT-5299 > Project: Thrift > Issue Type: Bug > Components: Rust - Library >Affects Versions: 0.13.0 >Reporter: shuo li >Assignee: Allen George >Priority: Major > Fix For: 0.14.1 > > Time Spent: 40m > Remaining Estimate: 0h > > While reviewing code, I discovered a bug in compact protocol. > The seq_id in message header is decoded with integer-encoding's i32, which > uses zigzag decode implicitly. > > > EDIT: correct the title and desc. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Assigned] (THRIFT-5299) rs implementation compact protocol seq_id should not use zigzag encoding.
[ https://issues.apache.org/jira/browse/THRIFT-5299?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Allen George reassigned THRIFT-5299: Assignee: Allen George > rs implementation compact protocol seq_id should not use zigzag encoding. > - > > Key: THRIFT-5299 > URL: https://issues.apache.org/jira/browse/THRIFT-5299 > Project: Thrift > Issue Type: Bug > Components: Rust - Library >Affects Versions: 0.13.0 >Reporter: shuo li >Assignee: Allen George >Priority: Major > Time Spent: 40m > Remaining Estimate: 0h > > While reviewing code, I discovered a bug in compact protocol. > The seq_id in message header is decoded with integer-encoding's i32, which > uses zigzag decode implicitly. > > > EDIT: correct the title and desc. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Closed] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George closed THRIFT-5300.
Resolution: Invalid
Not an issue in the Rust implementation as per the conversation.
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5299) rs implementation compact protocol seq_id should not use zigzag encoding.
[ https://issues.apache.org/jira/browse/THRIFT-5299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235906#comment-17235906 ] Allen George commented on THRIFT-5299: -- Note: The reason why signed ints are used everywhere for lengths in the Rust lib is (of course) for compatibility between languages. > rs implementation compact protocol seq_id should not use zigzag encoding. > - > > Key: THRIFT-5299 > URL: https://issues.apache.org/jira/browse/THRIFT-5299 > Project: Thrift > Issue Type: Bug > Components: Rust - Library >Affects Versions: 0.13.0 >Reporter: shuo li >Priority: Major > > While reviewing code, I discovered a bug in compact protocol. > The seq_id in message header is decoded with integer-encoding's i32, which > uses zigzag decode implicitly. > > > EDIT: correct the title and desc. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5314) Enum forward compatibility
[
https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235560#comment-17235560
]
Allen George edited comment on THRIFT-5314 at 11/19/20, 4:02 PM:
-
I don't think that's necessary. You could have the same outcome by using
something like:
{noformat}
pub enum RustEnum {
Add = 1,
Subtract = 2,
Multiply = 3,
Divide = 4,
__Unknown(i32),
}
{noformat}
After all, all we need is the ability to parse an unknown enum value on the
wire into a variant that can be processed by the receiving code. If
{{__Unknown}} is generated automatically the reading code can always populate
it with the unrecognized value on the wire.
was (Author: allengeorge):
I don't think that's necessary. You could have the same outcome by using
something like:
{noformat}
pub enum RustEnum {
Add = 1,
Subtract = 2,
Multiply = 3,
Divide = 4,
__Unknown(i32),
}
{noformat}
> Enum forward compatibility
> --
>
> Key: THRIFT-5314
> URL: https://issues.apache.org/jira/browse/THRIFT-5314
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler, Rust - Library
>Affects Versions: 0.13.0
>Reporter: Remi Dettai
>Priority: Major
>
> It seems that enums in the Rust implem are not forward compatible. As Thrift
> enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case
> to an enum, an error will be returned when parsing the message.
> Is this intended? Is there a workaround?
> (We met this problem in the Rust parquet implem:
> https://issues.apache.org/jira/browse/ARROW-10553)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5314) Enum forward compatibility
[
https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235560#comment-17235560
]
Allen George commented on THRIFT-5314:
--
I don't think that's necessary. You could have the same outcome by using
something like:
{noformat}
pub enum RustEnum {
Add = 1,
Subtract = 2,
Multiply = 3,
Divide = 4,
__Unknown(i32),
}
{noformat}
> Enum forward compatibility
> --
>
> Key: THRIFT-5314
> URL: https://issues.apache.org/jira/browse/THRIFT-5314
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler, Rust - Library
>Affects Versions: 0.13.0
>Reporter: Remi Dettai
>Priority: Major
>
> It seems that enums in the Rust implem are not forward compatible. As Thrift
> enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case
> to an enum, an error will be returned when parsing the message.
> Is this intended? Is there a workaround?
> (We met this problem in the Rust parquet implem:
> https://issues.apache.org/jira/browse/ARROW-10553)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5299) rs implementation compact protocol seq_id should not use zigzag encoding.
[
https://issues.apache.org/jira/browse/THRIFT-5299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235545#comment-17235545
]
Allen George edited comment on THRIFT-5299 at 11/19/20, 3:24 PM:
-
II was curious about why this didn't break all the cross tests, so I took a
look at the code again. I noticed that the *only* time the compact protocol
spec uses (varint, non-zigzag) for a signed integer is for:
# collection, message, name sizes
# sequence numbers
It makes no sense for collection sizes and lengths to be negative, and indeed
the spec states that in a number of places. For example:
* {{byte length is the length of the byte array, using var int encoding (must
be >= 0)}} (Binary encoding)
* {{name length is the byte length of the name field, a signed 32 bit integer
encoded as a var int (must be >= 0).}} (Message)
* {{size is the size, a var int (int32), positive values 15 or higher}} (List
and set)
* {{size is the size, a var int (int32), strictly positive values}} (Map)
Indeed, for sizes the rust implementation takes an {{i32}} and immediately
casts it to a {{u32}} before encoding it onto the wire. I also checked
{{integer-rs}}, and as expected unsigned ints are encoded in varint-only form.
So, as far as the spec is concerned, for sizes the rust compact protocol
implementation is doing the right thing (there's a separate question as to why
sizes should *ever* be negative in the library; I'll look into changing all
sizes into {{u32}}, and what the blast radius of that change should be).
This leaves the sequence number, which is signed and unzigzagged as the only
problem. I also checked, and I think there *may* be another issue with writing
out {{i16}} integers. The protobuf varint spec says that {{i16}} should be
written in no more bits than an {{i32}}; I'll have to check and see if that's
the case.
TL;DR:
* Only the sequence number may be a problem here.
* I'll have to check if writing {{i16}} uses more bytes than necessary.
* Field ids and collection/binary/other sizes should be represented in the rust
lib as {{usize}} or {{u##}} types instead of {{i##}} types.
was (Author: allengeorge):
II was curious about why this didn't break all the cross tests, so I took a
look at the code again. I noticed that the *only* time the compact protocol
spec uses (varint, non-zigzag) for a signed integer is for:
# collection, message, name sizes
# sequence numbers
It makes no sense for collection sizes and lengths to be negative, and indeed
the spec states that in a number of places. For example:
* {{byte length is the length of the byte array, using var int encoding (must
be >= 0)}} (Binary encoding)
* {{name length is the byte length of the name field, a signed 32 bit integer
encoded as a var int (must be >= 0).}} (Message)
* {{size is the size, a var int (int32), positive values 15 or higher}} (List
and set)
* {{size is the size, a var int (int32), strictly positive values}} (Map)
Indeed, for sizes the rust implementation takes an {{i32}} and immediately
casts it to a {{u32}} before encoding it onto the wire. I also checked
{{integer-rs}}, and as expected unsigned ints are encoded in varint-only form.
So, as far as the spec is concerned, for sizes the compact protocol is doing
the right thing (there's a separate question as to why sizes should *ever* be
negative in the library; I'll look into changing all sizes into {{u32}}, and
what the blast radius of that change should be).
This leaves the sequence number, which is signed and unzigzagged as the only
problem. I also checked, and I think there *may* be another issue with writing
out {{i16}} integers. The protobuf varint spec says that {{i16}} should be
written in no more bits than an {{i32}}; I'll have to check and see if that's
the case.
TL;DR:
* Only the sequence number may be a problem here.
* I'll have to check if writing {{i16}} uses more bytes than necessary.
* Field ids and collection/binary/other sizes should be represented in the rust
lib as {{usize}} or {{u##}} types instead of {{i##}} types.
> rs implementation compact protocol seq_id should not use zigzag encoding.
> -
>
> Key: THRIFT-5299
> URL: https://issues.apache.org/jira/browse/THRIFT-5299
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Priority: Major
>
> While reviewing code, I discovered a bug in compact protocol.
> The seq_id in message header is decoded with integer-encoding's i32, which
> uses zigzag decode implicitly.
>
>
> EDIT: correct the title and desc.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5299) rs implementation compact protocol seq_id should not use zigzag encoding.
[
https://issues.apache.org/jira/browse/THRIFT-5299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235545#comment-17235545
]
Allen George edited comment on THRIFT-5299 at 11/19/20, 3:23 PM:
-
II was curious about why this didn't break all the cross tests, so I took a
look at the code again. I noticed that the *only* time the compact protocol
spec uses (varint, non-zigzag) for a signed integer is for:
# collection, message, name sizes
# sequence numbers
It makes no sense for collection sizes and lengths to be negative, and indeed
the spec states that in a number of places. For example:
* {{byte length is the length of the byte array, using var int encoding (must
be >= 0)}} (Binary encoding)
* {{name length is the byte length of the name field, a signed 32 bit integer
encoded as a var int (must be >= 0).}} (Message)
* {{size is the size, a var int (int32), positive values 15 or higher}} (List
and set)
* {{size is the size, a var int (int32), strictly positive values}} (Map)
Indeed, for sizes the rust implementation takes an {{i32}} and immediately
casts it to a {{u32}} before encoding it onto the wire. I also checked
{{integer-rs}}, and as expected unsigned ints are encoded in varint-only form.
So, as far as the spec is concerned, for sizes the compact protocol is doing
the right thing (there's a separate question as to why sizes should *ever* be
negative in the library; I'll look into changing all sizes into {{u32}}, and
what the blast radius of that change should be).
This leaves the sequence number, which is signed and unzigzagged as the only
problem. I also checked, and I think there *may* be another issue with writing
out {{i16}} integers. The protobuf varint spec says that {{i16}} should be
written in no more bits than an {{i32}}; I'll have to check and see if that's
the case.
TL;DR:
* Only the sequence number may be a problem here.
* I'll have to check if writing {{i16}} uses more bytes than necessary.
* Field ids and collection/binary/other sizes should be represented in the rust
lib as {{usize}} or {{u##}} types instead of {{i##}} types.
was (Author: allengeorge):
II was curious about why this didn't break all the cross tests, so I took a
look at the code again. I noticed that the *only* time the compact protocol
spec uses (varint, non-zigzag) for a signed integer is for:
# collection, message, name sizes
# sequence numbers
It makes no sense for collection sizes and lengths to be negative, and indeed
the spec states that in a number of places. For example:
* {{byte length is the length of the byte array, using var int encoding (must
be >= 0)}} (Binary encoding)
* {{name length is the byte length of the name field, a signed 32 bit integer
encoded as a var int (must be >= 0).}} (Message)
* {{size is the size, a var int (int32), positive values 15 or higher}} (List
and set)
* {{size is the size, a var int (int32), strictly positive values}} (Map)
Indeed, for sizes the rust implementation takes an {{i32}} and immediately
casts it to a {{u32}} before encoding it onto the wire. I also checked
{{integer-rs}}, and as expected unsigned ints are encoded in varint-only form.
So, as far as the spec is concerned, for sizes the compact protocol is doing
the right thing (there's a separate question as to why sizes should *ever* be
negative in the library; I'll look into changing all sizes into {{u32}}, and
what the blast radius of that change should be).
This leaves the sequence number, which is signed and unzigzagged as the only
problem. I also checked, and I think there *may* be another issue with writing
out {{i16}} integers. The protobuf varint spec says that {{i16}} should be
written in no more bits than an {{i32}}; I'll have to check and see if that's
the case.
> rs implementation compact protocol seq_id should not use zigzag encoding.
> -
>
> Key: THRIFT-5299
> URL: https://issues.apache.org/jira/browse/THRIFT-5299
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Priority: Major
>
> While reviewing code, I discovered a bug in compact protocol.
> The seq_id in message header is decoded with integer-encoding's i32, which
> uses zigzag decode implicitly.
>
>
> EDIT: correct the title and desc.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5299) rs implementation compact protocol seq_id should not use zigzag encoding.
[
https://issues.apache.org/jira/browse/THRIFT-5299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235545#comment-17235545
]
Allen George commented on THRIFT-5299:
--
II was curious about why this didn't break all the cross tests, so I took a
look at the code again. I noticed that the *only* time the compact protocol
spec uses (varint, non-zigzag) for a signed integer is for:
# collection, message, name sizes
# sequence numbers
It makes no sense for collection sizes and lengths to be negative, and indeed
the spec states that in a number of places. For example:
* {{byte length is the length of the byte array, using var int encoding (must
be >= 0)}} (Binary encoding)
* {{name length is the byte length of the name field, a signed 32 bit integer
encoded as a var int (must be >= 0).}} (Message)
* {{size is the size, a var int (int32), positive values 15 or higher}} (List
and set)
* {{size is the size, a var int (int32), strictly positive values}} (Map)
Indeed, for sizes the rust implementation takes an {{i32}} and immediately
casts it to a {{u32}} before encoding it onto the wire. I also checked
{{integer-rs}}, and as expected unsigned ints are encoded in varint-only form.
So, as far as the spec is concerned, for sizes the compact protocol is doing
the right thing (there's a separate question as to why sizes should *ever* be
negative in the library; I'll look into changing all sizes into {{u32}}, and
what the blast radius of that change should be).
This leaves the sequence number, which is signed and unzigzagged as the only
problem. I also checked, and I think there *may* be another issue with writing
out {{i16}} integers. The protobuf varint spec says that {{i16}} should be
written in no more bits than an {{i32}}; I'll have to check and see if that's
the case.
> rs implementation compact protocol seq_id should not use zigzag encoding.
> -
>
> Key: THRIFT-5299
> URL: https://issues.apache.org/jira/browse/THRIFT-5299
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Priority: Major
>
> While reviewing code, I discovered a bug in compact protocol.
> The seq_id in message header is decoded with integer-encoding's i32, which
> uses zigzag decode implicitly.
>
>
> EDIT: correct the title and desc.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-4098) Support user-defined output namespaces in generated Rust modules
[
https://issues.apache.org/jira/browse/THRIFT-4098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235519#comment-17235519
]
Allen George commented on THRIFT-4098:
--
Copying comment from THRIFT-5071:
One can't place thrift files that are dependent on each other in an arbitrary
directory structure, because the generated {{use ...}} declarations assume that
all thrift files are in the same location. For this situation, I *think* the
problem is as follows. Consider:
{noformat}
root
+- common.thrift
+- dir1
+- dependent_1_on_common.thrift
+- ...
+- dir2
+- dependent_2_on_common.thrift
+- ...
{noformat}
The problem here is that the generated {{dependent_1_on_common.rs}} and
{{dependent_2_on_common.rs}} all would have declarations like:
{noformat}
// Autogenerated by Thrift Compiler (0.14.0)
// DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
#![allow(unused_imports)]
#![allow(unused_extern_crates)]
#![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
type_complexity))]
#![cfg_attr(rustfmt, rustfmt_skip)]
use crate::common; < PROBLEMATIC LINE
...
{noformat}
that would obviously break because their location in the rust/filesystem module
hierarchy is not reflected in the thrift-generated code. Assuming the compiler
has the information, I *think* this would be solved by including the Thrift
namespace of the *dependent* file in the generated {{use}} expressions in the
*dependee*.
> Support user-defined output namespaces in generated Rust modules
>
>
> Key: THRIFT-4098
> URL: https://issues.apache.org/jira/browse/THRIFT-4098
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Minor
>
> Currently the Rust compiler assumes that all generated modules are rooted at
> the top-level of your crate (i.e. at {{lib.rs}}). There are many useful cases
> where you want to control exactly where the generated code lives (for example
> - you may want it to be inside another sub-module you control); typically
> this is done via thrift {{namespace}} definitions. The compiler currently
> ignores these declarations. It should be changed to:
> # recognize them if they exist, and generate code with the proper module paths
> # default to the current behavior if no {{namespace}} declarations exist
> # work properly in both cases with service inheritance.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-4098) Support user-defined output namespaces in generated Rust modules
[
https://issues.apache.org/jira/browse/THRIFT-4098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235519#comment-17235519
]
Allen George edited comment on THRIFT-4098 at 11/19/20, 2:39 PM:
-
Copying comment from THRIFT-5071:
One can't place thrift files that are dependent on each other in an arbitrary
directory structure, because the generated {{use ...}} declarations assume that
all thrift files are in the same location. For this situation, I *think* the
problem is as follows. Consider:
{noformat}
root
+- common.thrift
+- dir1
+- dependent_1_on_common.thrift
+- ...
+- dir2
+- dependent_2_on_common.thrift
+- ...
{noformat}
The problem here is that the generated {{dependent_1_on_common.rs}} and
{{dependent_2_on_common.rs}} all would have declarations like:
{noformat}
// Autogenerated by Thrift Compiler (0.14.0)
// DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
#![allow(unused_imports)]
#![allow(unused_extern_crates)]
#![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
type_complexity))]
#![cfg_attr(rustfmt, rustfmt_skip)]
use crate::common; < PROBLEMATIC LINE
...
{noformat}
that would obviously break because their location in the rust/filesystem module
hierarchy is not reflected in the thrift-generated code. Assuming the compiler
has the information, I *think* this would be solved by including the Thrift
namespace of the *dependent* file in the generated {{use}} expressions in the
*dependee*.
was (Author: allengeorge):
Copying comment from THRIFT-5071:
One can't place thrift files that are dependent on each other in an arbitrary
directory structure, because the generated {{use ...}} declarations assume that
all thrift files are in the same location. For this situation, I *think* the
problem is as follows. Consider:
{noformat}
root
+- common.thrift
+- dir1
+- dependent_1_on_common.thrift
+- ...
+- dir2
+- dependent_2_on_common.thrift
+- ...
{noformat}
The problem here is that the generated {{dependent_1_on_common.rs}} and
{{dependent_2_on_common.rs}} all would have declarations like:
{noformat}
// Autogenerated by Thrift Compiler (0.14.0)
// DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
#![allow(unused_imports)]
#![allow(unused_extern_crates)]
#![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
type_complexity))]
#![cfg_attr(rustfmt, rustfmt_skip)]
use crate::common; < PROBLEMATIC LINE
...
{noformat}
that would obviously break because their location in the rust/filesystem module
hierarchy is not reflected in the thrift-generated code. Assuming the compiler
has the information, I *think* this would be solved by including the Thrift
namespace of the *dependent* file in the generated {{use}} expressions in the
*dependee*.
> Support user-defined output namespaces in generated Rust modules
>
>
> Key: THRIFT-4098
> URL: https://issues.apache.org/jira/browse/THRIFT-4098
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
>Reporter: Allen George
>Priority: Minor
>
> Currently the Rust compiler assumes that all generated modules are rooted at
> the top-level of your crate (i.e. at {{lib.rs}}). There are many useful cases
> where you want to control exactly where the generated code lives (for example
> - you may want it to be inside another sub-module you control); typically
> this is done via thrift {{namespace}} definitions. The compiler currently
> ignores these declarations. It should be changed to:
> # recognize them if they exist, and generate code with the proper module paths
> # default to the current behavior if no {{namespace}} declarations exist
> # work properly in both cases with service inheritance.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5071) Rust: rust tutorial can not be compiled with rust edition 2018
[
https://issues.apache.org/jira/browse/THRIFT-5071?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5071.
--
Fix Version/s: 0.14.0
Resolution: Fixed
> Rust: rust tutorial can not be compiled with rust edition 2018
> --
>
> Key: THRIFT-5071
> URL: https://issues.apache.org/jira/browse/THRIFT-5071
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler, Tutorial
>Affects Versions: 0.13.0
> Environment: cargo 1.40.0 (bc8e4c8be 2019-11-22)
> rustc 1.40.0 (73528e339 2019-12-16)
>Reporter: fan liwen
>Assignee: Allen George
>Priority: Major
> Fix For: 0.14.0
>
>
> The rust compiler is broken for rust edition 2018. If we change the tutorial
> cargo (tutorial/rs/Cargo.toml) to edition = "2018", the following error would
> occur:
> {code:java}
> // code placeholder
> error[E0432]: unresolved import `shared`
> --> src/tutorial.rs:30:5
>|
> 30 | use shared;
>| ^^ no `shared` external crateerror: aborting due to previous
> error
> {code}
> The compiler might need to generate *use crate::shared* instead.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5071) Rust: rust tutorial can not be compiled with rust edition 2018
[
https://issues.apache.org/jira/browse/THRIFT-5071?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235517#comment-17235517
]
Allen George commented on THRIFT-5071:
--
(1) was completed this year via https://github.com/apache/thrift/pull/2078
> Rust: rust tutorial can not be compiled with rust edition 2018
> --
>
> Key: THRIFT-5071
> URL: https://issues.apache.org/jira/browse/THRIFT-5071
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Compiler, Tutorial
>Affects Versions: 0.13.0
> Environment: cargo 1.40.0 (bc8e4c8be 2019-11-22)
> rustc 1.40.0 (73528e339 2019-12-16)
>Reporter: fan liwen
>Assignee: Allen George
>Priority: Major
>
> The rust compiler is broken for rust edition 2018. If we change the tutorial
> cargo (tutorial/rs/Cargo.toml) to edition = "2018", the following error would
> occur:
> {code:java}
> // code placeholder
> error[E0432]: unresolved import `shared`
> --> src/tutorial.rs:30:5
>|
> 30 | use shared;
>| ^^ no `shared` external crateerror: aborting due to previous
> error
> {code}
> The compiler might need to generate *use crate::shared* instead.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-5306) Rust library, tutorial, test, cross-test code should not throw any clippy errors
[
https://issues.apache.org/jira/browse/THRIFT-5306?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-5306:
-
Fix Version/s: 0.14.0
> Rust library, tutorial, test, cross-test code should not throw any clippy
> errors
>
>
> Key: THRIFT-5306
> URL: https://issues.apache.org/jira/browse/THRIFT-5306
> Project: Thrift
> Issue Type: Task
> Components: Rust - Library
>Reporter: Allen George
>Assignee: Allen George
>Priority: Minor
> Fix For: 0.14.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> When the rust code was originally written one could run {{cargo clippy}} on
> it without any errors. As Rust has changed in the last 24 versions this is no
> longer true.
> We should run clippy on all the lib, test, tutorial and cross-test code to
> get it into a proper state for further work.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5306) Rust library, tutorial, test, cross-test code should not throw any clippy errors
[
https://issues.apache.org/jira/browse/THRIFT-5306?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5306.
--
Resolution: Fixed
As of linked PR.
> Rust library, tutorial, test, cross-test code should not throw any clippy
> errors
>
>
> Key: THRIFT-5306
> URL: https://issues.apache.org/jira/browse/THRIFT-5306
> Project: Thrift
> Issue Type: Task
> Components: Rust - Library
>Reporter: Allen George
>Assignee: Allen George
>Priority: Minor
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> When the rust code was originally written one could run {{cargo clippy}} on
> it without any errors. As Rust has changed in the last 24 versions this is no
> longer true.
> We should run clippy on all the lib, test, tutorial and cross-test code to
> get it into a proper state for further work.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5314) Enum forward compatibility
[ https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235158#comment-17235158 ] Allen George edited comment on THRIFT-5314 at 11/19/20, 3:55 AM: - What should the right behavior be? If the receiver gets a new enum value that exists in the sender...what should the receiving code generate? EDIT: Note that I'm agreeing there's a problem. I'm not sure what the 'correct' behavior looks like on the receiver's side. For example, does every generated enum have a hidden "__unknown" variant that encodes the unrecognized enum value? was (Author: allengeorge): What should the right behavior be? If the receiver gets a new enum value that exists in the sender...what should the receiving code generate? > Enum forward compatibility > -- > > Key: THRIFT-5314 > URL: https://issues.apache.org/jira/browse/THRIFT-5314 > Project: Thrift > Issue Type: Bug > Components: Rust - Compiler, Rust - Library >Affects Versions: 0.13.0 >Reporter: Remi Dettai >Priority: Major > > It seems that enums in the Rust implem are not forward compatible. As Thrift > enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case > to an enum, an error will be returned when parsing the message. > Is this intended? Is there a workaround? > (We met this problem in the Rust parquet implem: > https://issues.apache.org/jira/browse/ARROW-10553) -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Commented] (THRIFT-5314) Enum forward compatibility
[ https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235158#comment-17235158 ] Allen George commented on THRIFT-5314: -- What should the right behavior be? If the receiver gets a new enum value that exists in the sender...what should the receiving code generate? > Enum forward compatibility > -- > > Key: THRIFT-5314 > URL: https://issues.apache.org/jira/browse/THRIFT-5314 > Project: Thrift > Issue Type: Bug > Components: Rust - Compiler, Rust - Library >Affects Versions: 0.13.0 >Reporter: Remi Dettai >Priority: Major > > It seems that enums in the Rust implem are not forward compatible. As Thrift > enums are mapped 1:1 to Rust enum, if a newer Thrift definition adds a case > to an enum, an error will be returned when parsing the message. > Is this intended? Is there a workaround? > (We met this problem in the Rust parquet implem: > https://issues.apache.org/jira/browse/ARROW-10553) -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-4764) Rust frontend emits deprecated clippy suppression attributes
[
https://issues.apache.org/jira/browse/THRIFT-4764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235155#comment-17235155
]
Allen George edited comment on THRIFT-4764 at 11/19/20, 3:47 AM:
-
Library changes completed as of https://github.com/apache/thrift/pull/2273
Previous work done by others to fix generated code.
was (Author: allengeorge):
Completed as of https://github.com/apache/thrift/pull/2273
> Rust frontend emits deprecated clippy suppression attributes
>
>
> Key: THRIFT-4764
> URL: https://issues.apache.org/jira/browse/THRIFT-4764
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
> Environment: Ubuntu Cosmic x64
> Thrift from master
>Reporter: Mikail Bagishov
>Assignee: Allen George
>Priority: Minor
> Fix For: 0.14.0
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Currently, following lines are inserted:
> {code:none}
> #![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
> type_complexity))]
> {code}
> But this syntax is deprecated now.
> Following should be used instead:
> {code:none}
> #![allow(clippy::too_many_arguments, clippy::type_complexity)]
> {code}
> Note: I fixed this issue in my [fork|http://github.com/MikailBag/thrift], but
> I'm not sure if it will be easier for developers to merge my PR than apply
> changes themselves. Also, I'm student and my Git skills are not very good)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5307) Rust generated code should compile cleanly with clippy
[
https://issues.apache.org/jira/browse/THRIFT-5307?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5307.
--
Fix Version/s: 0.14.0
Resolution: Fixed
Completed as of https://github.com/apache/thrift/pull/2273
> Rust generated code should compile cleanly with clippy
> --
>
> Key: THRIFT-5307
> URL: https://issues.apache.org/jira/browse/THRIFT-5307
> Project: Thrift
> Issue Type: Task
> Components: Rust - Compiler
>Reporter: Allen George
>Assignee: Allen George
>Priority: Minor
> Fix For: 0.14.0
>
>
> When the rust code was originally generated one could run {{cargo clippy}} on
> it without any errors. As Rust has changed in the last 24 versions this is no
> longer true.
> We should update the generated code so that it no longer throws any clippy
> errors.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-4764) Rust frontend emits deprecated clippy suppression attributes
[
https://issues.apache.org/jira/browse/THRIFT-4764?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-4764.
--
Fix Version/s: 0.14.0
Resolution: Fixed
Completed as of https://github.com/apache/thrift/pull/2273
> Rust frontend emits deprecated clippy suppression attributes
>
>
> Key: THRIFT-4764
> URL: https://issues.apache.org/jira/browse/THRIFT-4764
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
> Environment: Ubuntu Cosmic x64
> Thrift from master
>Reporter: Mikail Bagishov
>Assignee: Allen George
>Priority: Minor
> Fix For: 0.14.0
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Currently, following lines are inserted:
> {code:none}
> #![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
> type_complexity))]
> {code}
> But this syntax is deprecated now.
> Following should be used instead:
> {code:none}
> #![allow(clippy::too_many_arguments, clippy::type_complexity)]
> {code}
> Note: I fixed this issue in my [fork|http://github.com/MikailBag/thrift], but
> I'm not sure if it will be easier for developers to merge my PR than apply
> changes themselves. Also, I'm student and my Git skills are not very good)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Issue Comment Deleted] (THRIFT-4764) Rust frontend emits deprecated clippy suppression attributes
[
https://issues.apache.org/jira/browse/THRIFT-4764?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-4764:
-
Comment: was deleted
(was: This has been completed as of 6cd5366b5fe10940d28baff9c7067e3045c3c019)
> Rust frontend emits deprecated clippy suppression attributes
>
>
> Key: THRIFT-4764
> URL: https://issues.apache.org/jira/browse/THRIFT-4764
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
> Environment: Ubuntu Cosmic x64
> Thrift from master
>Reporter: Mikail Bagishov
>Assignee: Allen George
>Priority: Minor
> Fix For: 0.14.0
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Currently, following lines are inserted:
> {code:none}
> #![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
> type_complexity))]
> {code}
> But this syntax is deprecated now.
> Following should be used instead:
> {code:none}
> #![allow(clippy::too_many_arguments, clippy::type_complexity)]
> {code}
> Note: I fixed this issue in my [fork|http://github.com/MikailBag/thrift], but
> I'm not sure if it will be easier for developers to merge my PR than apply
> changes themselves. Also, I'm student and my Git skills are not very good)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Assigned] (THRIFT-4764) Rust frontend emits deprecated clippy suppression attributes
[
https://issues.apache.org/jira/browse/THRIFT-4764?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George reassigned THRIFT-4764:
Assignee: Allen George
> Rust frontend emits deprecated clippy suppression attributes
>
>
> Key: THRIFT-4764
> URL: https://issues.apache.org/jira/browse/THRIFT-4764
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
> Environment: Ubuntu Cosmic x64
> Thrift from master
>Reporter: Mikail Bagishov
>Assignee: Allen George
>Priority: Minor
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Currently, following lines are inserted:
> {code:none}
> #![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
> type_complexity))]
> {code}
> But this syntax is deprecated now.
> Following should be used instead:
> {code:none}
> #![allow(clippy::too_many_arguments, clippy::type_complexity)]
> {code}
> Note: I fixed this issue in my [fork|http://github.com/MikailBag/thrift], but
> I'm not sure if it will be easier for developers to merge my PR than apply
> changes themselves. Also, I'm student and my Git skills are not very good)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-4764) Rust frontend emits deprecated clippy suppression attributes
[
https://issues.apache.org/jira/browse/THRIFT-4764?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17235154#comment-17235154
]
Allen George commented on THRIFT-4764:
--
This has been completed as of 6cd5366b5fe10940d28baff9c7067e3045c3c019
> Rust frontend emits deprecated clippy suppression attributes
>
>
> Key: THRIFT-4764
> URL: https://issues.apache.org/jira/browse/THRIFT-4764
> Project: Thrift
> Issue Type: Improvement
> Components: Rust - Compiler
> Environment: Ubuntu Cosmic x64
> Thrift from master
>Reporter: Mikail Bagishov
>Priority: Minor
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Currently, following lines are inserted:
> {code:none}
> #![cfg_attr(feature = "cargo-clippy", allow(too_many_arguments,
> type_complexity))]
> {code}
> But this syntax is deprecated now.
> Following should be used instead:
> {code:none}
> #![allow(clippy::too_many_arguments, clippy::type_complexity)]
> {code}
> Note: I fixed this issue in my [fork|http://github.com/MikailBag/thrift], but
> I'm not sure if it will be easier for developers to merge my PR than apply
> changes themselves. Also, I'm student and my Git skills are not very good)
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (THRIFT-5307) Rust generated code should compile cleanly with clippy
Allen George created THRIFT-5307:
Summary: Rust generated code should compile cleanly with clippy
Key: THRIFT-5307
URL: https://issues.apache.org/jira/browse/THRIFT-5307
Project: Thrift
Issue Type: Task
Components: Rust - Compiler
Reporter: Allen George
Assignee: Allen George
When the rust code was originally generated one could run {{cargo clippy}} on
it without any errors. As Rust has changed in the last 24 versions this is no
longer true.
We should update the generated code so that it no longer throws any clippy
errors.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (THRIFT-5306) Rust library, tutorial, test, cross-test code should not throw any clippy errors
Allen George created THRIFT-5306:
Summary: Rust library, tutorial, test, cross-test code should not
throw any clippy errors
Key: THRIFT-5306
URL: https://issues.apache.org/jira/browse/THRIFT-5306
Project: Thrift
Issue Type: Task
Components: Rust - Library
Reporter: Allen George
Assignee: Allen George
When the rust code was originally written one could run {{cargo clippy}} on it
without any errors. As Rust has changed in the last 24 versions this is no
longer true.
We should run clippy on all the lib, test, tutorial and cross-test code to get
it into a proper state for further work.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17225130#comment-17225130
]
Allen George edited comment on THRIFT-5300 at 11/3/20, 5:00 AM:
[~shuoli84] This is what I get for trying to squeeze in looking at this ticket
in the middle of other tasks.
I apologize for wasting everyone's time here. I'm stepping back and trying to
state what I've discovered about the spec, and implementations - and hopefully
people can point out where I'm mistaken and missing things.
The issue at hand appears to be a mismatch between the spec, which states that
*in the compact protocol* the type encoded in a struct has a different value
than the type encoded in collections (sets, lists, maps). I've attached
screenshots of the spec and you've referenced it in the description.
I've tried to see what multiple implementations do:
* The rust implementation uses the same type encoding for both compact
structs/collections.
* The golang implementation uses the same type encoding for both compact
structs/collections.
* The Java implementation uses the same type encoding for both compact
structs/collections.
* The Python implementation uses the same type encoding for both compact
structs/collections.
* The Swift implementation uses the same type encoding for both compact
structs/collections. (AFAICT)
* The cpp implementation *appears to follow the spec*.
* The c_glib implementation *appears to follow the spec*.
Before I proceed, can you confirm that the above is your understanding as well?
EDIT:
Rereading the Java implementation closer it appears to follow the spec as well?
If that's the case, I'm starting to understand why this has been so confusing.
was (Author: allengeorge):
[~shuoli84] This is what I get for trying to squeeze in looking at this ticket
in the middle of other tasks.
I apologize for wasting everyone's time here. I'm stepping back and trying to
state what I've discovered about the spec, and implementations - and hopefully
people can point out where I'm mistaken and missing things.
The issue at hand appears to be a mismatch between the spec, which states that
*in the compact protocol* the type encoded in a struct has a different value
than the type encoded in collections (sets, lists, maps). I've attached
screenshots of the spec and you've referenced it in the description.
I've tried to see what multiple implementations do:
* The rust implementation uses the same type encoding for both compact
structs/collections.
* The golang implementation uses the same type encoding for both compact
structs/collections.
* The Java implementation uses the same type encoding for both compact
structs/collections.
* The Python implementation uses the same type encoding for both compact
structs/collections.
* The Swift implementation uses the same type encoding for both compact
structs/collections. (AFAICT)
* The cpp implementation *appears to follow the spec*.
* The c_glib implementation *appears to follow the spec*.
Before I proceed, can you confirm that the above is your understanding as well?
EDIT:
Rereading the Java spec closer it appears to follow the spec as well? If that's
the case, I'm starting to understand why this has been so confusing.
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> *
[jira] [Comment Edited] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17225130#comment-17225130
]
Allen George edited comment on THRIFT-5300 at 11/3/20, 5:00 AM:
[~shuoli84] This is what I get for trying to squeeze in looking at this ticket
in the middle of other tasks.
I apologize for wasting everyone's time here. I'm stepping back and trying to
state what I've discovered about the spec, and implementations - and hopefully
people can point out where I'm mistaken and missing things.
The issue at hand appears to be a mismatch between the spec, which states that
*in the compact protocol* the type encoded in a struct has a different value
than the type encoded in collections (sets, lists, maps). I've attached
screenshots of the spec and you've referenced it in the description.
I've tried to see what multiple implementations do:
* The rust implementation uses the same type encoding for both compact
structs/collections.
* The golang implementation uses the same type encoding for both compact
structs/collections.
* The Java implementation uses the same type encoding for both compact
structs/collections.
* The Python implementation uses the same type encoding for both compact
structs/collections.
* The Swift implementation uses the same type encoding for both compact
structs/collections. (AFAICT)
* The cpp implementation *appears to follow the spec*.
* The c_glib implementation *appears to follow the spec*.
Before I proceed, can you confirm that the above is your understanding as well?
EDIT:
Rereading the Java spec closer it appears to follow the spec as well? If that's
the case, I'm starting to understand why this has been so confusing.
was (Author: allengeorge):
[~shuoli84] This is what I get for trying to squeeze in looking at this ticket
in the middle of other tasks.
I apologize for wasting everyone's time here. I'm stepping back and trying to
state what I've discovered about the spec, and implementations - and hopefully
people can point out where I'm mistaken and missing things.
The issue at hand appears to be a mismatch between the spec, which states that
*in the compact protocol* the type encoded in a struct has a different value
than the type encoded in collections (sets, lists, maps). I've attached
screenshots of the spec and you've referenced it in the description.
I've tried to see what multiple implementations do:
* The rust implementation uses the same type encoding for both compact
structs/collections.
* The golang implementation uses the same type encoding for both compact
structs/collections.
* The Java implementation uses the same type encoding for both compact
structs/collections.
* The Python implementation uses the same type encoding for both compact
structs/collections.
* The Swift implementation uses the same type encoding for both compact
structs/collections. (AFAICT)
* The cpp implementation *appears to follow the spec*.
* The c_glib implementation *appears to follow the spec*.
Before I proceed, can you confirm that the above is your understanding as well?
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>
[jira] [Commented] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17225130#comment-17225130
]
Allen George commented on THRIFT-5300:
--
[~shuoli84] This is what I get for trying to squeeze in looking at this ticket
in the middle of other tasks.
I apologize for wasting everyone's time here. I'm stepping back and trying to
state what I've discovered about the spec, and implementations - and hopefully
people can point out where I'm mistaken and missing things.
The issue at hand appears to be a mismatch between the spec, which states that
*in the compact protocol* the type encoded in a struct has a different value
than the type encoded in collections (sets, lists, maps). I've attached
screenshots of the spec and you've referenced it in the description.
I've tried to see what multiple implementations do:
* The rust implementation uses the same type encoding for both compact
structs/collections.
* The golang implementation uses the same type encoding for both compact
structs/collections.
* The Java implementation uses the same type encoding for both compact
structs/collections.
* The Python implementation uses the same type encoding for both compact
structs/collections.
* The Swift implementation uses the same type encoding for both compact
structs/collections. (AFAICT)
* The cpp implementation *appears to follow the spec*.
* The c_glib implementation *appears to follow the spec*.
Before I proceed, can you confirm that the above is your understanding as well?
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17224972#comment-17224972
]
Allen George edited comment on THRIFT-5300 at 11/2/20, 9:37 PM:
[~shuoli84] and [~ulidtko]: I just checked the spec again (it's been a long
time since I looked at the code and the spec) and I noticed that the spec _is_
correct as well. It clearly points out that the field encoding in structs is
different from the field encoding in lists/sets. I've taken screenshots of the
spec and attached them to the ticket. The values in the rust implementation are
correct.
So:
* There is no bug in the spec.
* There is no bug in the implementation.
I'll also point out that the rust client/server (like most other language
implementations) does cross-language tests. There are definitely bugs (see the
list of explicitly-disabled cross-tests) in the implementation, but this
particular bug should have triggered failures across the board. Separately, I
would have checked other implementations to make sure that the rust one was
consistent (the spec is not exhaustive, and one definitely has to make some
judgement calls in some cases.)
It doesn't seem like there's any action to be taken here at all.
was (Author: allengeorge):
[~shuoli84] and [~ulidtko]: I just checked the spec again (it's been a long
time since I looked at the code and the spec) and I noticed that the spec _is_
correct as well. It clearly points out that the field encoding in structs is
different from the field encoding in lists/sets. I've taken screenshots of the
spec and attached them to the ticket. The values in the rust implementation are
correct.
So:
* There is no bug in the spec.
* There is no bug in the implementation.
I'll also point out that the rust client/server (like most other language
implementations) does cross-language tests. There are definitely bugs (see the
list of explicitly-disabled cross-tests) in the implementation, but this should
have triggered failures across the board.
It doesn't seem like there's any action to be taken here at all.
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17224972#comment-17224972
]
Allen George commented on THRIFT-5300:
--
[~shuoli84][~ulidtko] I just checked the spec again (it's been a long time
since I looked at the code and the spec) and I noticed that the spec _is_
correct as well. It clearly points out that the field encoding in structs is
different from the field encoding in lists/sets. I've taken screenshots of the
spec and attached them to the ticket. The values in the rust implementation are
correct.
So:
* There is no bug in the spec.
* There is no bug in the implementation.
I'll also point out that the rust client/server (like most other language
implementations) does cross-language tests. There are definitely bugs (see the
list of explicitly-disabled cross-tests) in the implementation, but this should
have triggered failures across the board.
It doesn't seem like there's any action to be taken here at all.
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17224972#comment-17224972
]
Allen George edited comment on THRIFT-5300 at 11/2/20, 9:36 PM:
[~shuoli84] and [~ulidtko]: I just checked the spec again (it's been a long
time since I looked at the code and the spec) and I noticed that the spec _is_
correct as well. It clearly points out that the field encoding in structs is
different from the field encoding in lists/sets. I've taken screenshots of the
spec and attached them to the ticket. The values in the rust implementation are
correct.
So:
* There is no bug in the spec.
* There is no bug in the implementation.
I'll also point out that the rust client/server (like most other language
implementations) does cross-language tests. There are definitely bugs (see the
list of explicitly-disabled cross-tests) in the implementation, but this should
have triggered failures across the board.
It doesn't seem like there's any action to be taken here at all.
was (Author: allengeorge):
[~shuoli84][~ulidtko] I just checked the spec again (it's been a long time
since I looked at the code and the spec) and I noticed that the spec _is_
correct as well. It clearly points out that the field encoding in structs is
different from the field encoding in lists/sets. I've taken screenshots of the
spec and attached them to the ticket. The values in the rust implementation are
correct.
So:
* There is no bug in the spec.
* There is no bug in the implementation.
I'll also point out that the rust client/server (like most other language
implementations) does cross-language tests. There are definitely bugs (see the
list of explicitly-disabled cross-tests) in the implementation, but this should
have triggered failures across the board.
It doesn't seem like there's any action to be taken here at all.
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-5300:
-
Attachment: Screen Shot 2020-11-02 at 16.31.31.png
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Updated] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George updated THRIFT-5300:
-
Attachment: Screen Shot 2020-11-02 at 16.31.09.png
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
> Attachments: Screen Shot 2020-11-02 at 16.31.09.png, Screen Shot
> 2020-11-02 at 16.31.31.png
>
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (THRIFT-5299) rs implementation compact protocol seq_id should not use zigzag encoding.
[
https://issues.apache.org/jira/browse/THRIFT-5299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17223685#comment-17223685
]
Allen George commented on THRIFT-5299:
--
[~shuoli84] I'm sorry - I'm a little confused by what's being reported here. Is
this a problem with the rust library, or {{integer_encoding}}'s implementation
of {{read_varint}} and {{write_varint}} or both?
It appears that the desired state is:
* Sequence number: varint
* All other instances of numbers: varint(zigzag)
My understanding of the problem is that currently in the Rust compact impl the
sequence number is **also** in varint(zigzag), which is wrong. Fixing this in
{{integer-encoding-rs}} appears to be a breaking API change. Probably the
simplest thing to do is add two new methods to the compact implementation:
* {{read_i32_seq_num}}
* {{write_i32_seq_num}}
Which works around the problem within the lib itself. Does that seem right?
> rs implementation compact protocol seq_id should not use zigzag encoding.
> -
>
> Key: THRIFT-5299
> URL: https://issues.apache.org/jira/browse/THRIFT-5299
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Priority: Major
>
> While reviewing code, I discovered a bug in compact protocol.
> The seq_id in message header is decoded with integer-encoding's i32, which
> uses zigzag decode implicitly.
>
>
> EDIT: correct the title and desc.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Resolved] (THRIFT-5300) rs compact protocol collection elem type to ttype mapping wrong
[
https://issues.apache.org/jira/browse/THRIFT-5300?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Allen George resolved THRIFT-5300.
--
Assignee: Allen George
Resolution: Invalid
Closing out because this is not a bug in the implementation.
> rs compact protocol collection elem type to ttype mapping wrong
> ---
>
> Key: THRIFT-5300
> URL: https://issues.apache.org/jira/browse/THRIFT-5300
> Project: Thrift
> Issue Type: Bug
> Components: Rust - Library
>Affects Versions: 0.13.0
>Reporter: shuo li
>Assignee: Allen George
>Priority: Major
>
> collection_u8_to_type only overrides bool, but in spec, other types are
> different too.
> In field:
> * {{BOOLEAN_TRUE}}, encoded as {{1}}
> * {{BOOLEAN_FALSE}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {color:#FF}{{I16}}, encoded as {{4}}{color}
> * {color:#FF}{{I32}}, encoded as {{5}}{color}
> * {{I64}}, encoded as {{6}}
> * {{DOUBLE}}, encoded as {{7}}
> * {{BINARY}}, used for binary and string fields, encoded as {{8}}
> * {{LIST}}, encoded as {{9}}
> * {{SET}}, encoded as {{10}}
> * {{MAP}}, encoded as {{11}}
> * {{STRUCT}}, used for both structs and union fields, encoded as {{12}}
> In colleciton:
> * {{BOOL}}, encoded as {{2}}
> * {{BYTE}}, encoded as {{3}}
> * {{DOUBLE}}, encoded as {{4}}
> * {color:#FF}{{I16}}, encoded as {{6}}{color}
> * {color:#FF}{{I32}}, encoded as {{8}}{color}
> * {{I64}}, encoded as {{10}}
> * {{STRING}}, used for binary and string fields, encoded as {{11}}
> * {{STRUCT}}, used for structs and union fields, encoded as {{12}}
> * {{MAP}}, encoded as {{13}}
> * {{SET}}, encoded as {{14}}
> * {{LIST}}, encoded as {{15}}
> {code:java}
> // code placeholder
> fn collection_u8_to_type(b: u8) -> crate::Result {
> match b {
>0x01 => Ok(TType::Bool),
>o => u8_to_type(o),
> }
> }
> fn u8_to_type(b: u8) -> crate::Result {
> match b {
> 0x00 => Ok(TType::Stop),
> 0x03 => Ok(TType::I08), // equivalent to TType::Byte
> 0x04 => Ok(TType::I16),
> 0x05 => Ok(TType::I32),
> 0x06 => Ok(TType::I64),
> 0x07 => Ok(TType::Double),
> 0x08 => Ok(TType::String),
> 0x09 => Ok(TType::List),
> 0x0A => Ok(TType::Set),
> 0x0B => Ok(TType::Map),
> 0x0C => Ok(TType::Struct),
> unkn => Err(crate::Error::Protocol(crate::ProtocolError {
> kind: crate::ProtocolErrorKind::InvalidData,message:
> format!("cannot convert {} into TType", unkn),
>})),
> }}
> {code}
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
