You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@thrift.apache.org by al...@apache.org on 2021/03/01 19:47:16 UTC
[thrift] branch master updated: [THRIFT-5314][THRIFT-4101] Generate
enums that don't error on unexpected values (#2337)
This is an automated email from the ASF dual-hosted git repository.
allengeorge pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/thrift.git
The following commit(s) were added to refs/heads/master by this push:
new 2e90ef5 [THRIFT-5314][THRIFT-4101] Generate enums that don't error on unexpected values (#2337)
2e90ef5 is described below
commit 2e90ef569c1b38f6e0f1279e3f25d2a7f6b5ff99
Author: Allen George <al...@apache.org>
AuthorDate: Mon Mar 1 14:47:04 2021 -0500
[THRIFT-5314][THRIFT-4101] Generate enums that don't error on unexpected values (#2337)
Client: rs
---
compiler/cpp/src/thrift/generate/t_rs_generator.cc | 133 +++++++++++++--------
lib/rs/README.md | 47 ++++++++
lib/rs/test/src/bin/kitchen_sink_server.rs | 6 +-
test/rs/src/bin/test_client.rs | 14 +--
test/rs/src/bin/test_server.rs | 6 +-
tutorial/rs/src/bin/tutorial_client.rs | 4 +-
tutorial/rs/src/bin/tutorial_server.rs | 21 ++--
7 files changed, 159 insertions(+), 72 deletions(-)
diff --git a/compiler/cpp/src/thrift/generate/t_rs_generator.cc b/compiler/cpp/src/thrift/generate/t_rs_generator.cc
index f55b7e0..5f9791a 100644
--- a/compiler/cpp/src/thrift/generate/t_rs_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_rs_generator.cc
@@ -116,7 +116,7 @@ private:
// Write the impl block associated with the rust representation of an enum. This includes methods
// to write the enum to a protocol, read it from a protocol, etc.
- void render_enum_impl(const string& enum_name);
+ void render_enum_impl(t_enum* tenum, const string& enum_name);
// Write a simple rust const value (ie. `pub const FOO: foo...`).
void render_const_value(const string& name, t_type* ttype, t_const_value* tvalue);
@@ -868,49 +868,62 @@ void t_rs_generator::generate_typedef(t_typedef* ttypedef) {
void t_rs_generator::generate_enum(t_enum* tenum) {
string enum_name(rust_camel_case(tenum->get_name()));
render_enum_definition(tenum, enum_name);
- render_enum_impl(enum_name);
+ render_enum_impl(tenum, enum_name);
render_enum_conversion(tenum, enum_name);
}
void t_rs_generator::render_enum_definition(t_enum* tenum, const string& enum_name) {
render_rustdoc((t_doc*) tenum);
f_gen_ << "#[derive(Copy, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]" << endl;
- f_gen_ << "pub enum " << enum_name << " {" << endl;
- indent_up();
-
- vector<t_enum_value*> constants = tenum->get_constants();
- vector<t_enum_value*>::iterator constants_iter;
- for (constants_iter = constants.begin(); constants_iter != constants.end(); ++constants_iter) {
- t_enum_value* val = (*constants_iter);
- render_rustdoc((t_doc*) val);
- f_gen_
- << indent()
- << rust_enum_variant_name(val->get_name())
- << " = "
- << val->get_value()
- << ","
- << endl;
- }
-
- indent_down();
- f_gen_ << "}" << endl;
+ f_gen_ << "pub struct " << enum_name << "(pub i32);" << endl;
f_gen_ << endl;
}
-void t_rs_generator::render_enum_impl(const string& enum_name) {
+void t_rs_generator::render_enum_impl(t_enum* tenum, const string& enum_name) {
f_gen_ << "impl " << enum_name << " {" << endl;
indent_up();
- // taking enum as 'self' here because Thrift enums
- // are represented as Rust enums with integer values
- // it's cheaper to copy the integer as opposed to
- // taking a reference to the enum
+ vector<t_enum_value*> constants = tenum->get_constants();
+
+ // associated constants for each IDL-defined enum variant
+ {
+ vector<t_enum_value*>::iterator constants_iter;
+ for (constants_iter = constants.begin(); constants_iter != constants.end(); ++constants_iter) {
+ t_enum_value* val = (*constants_iter);
+ render_rustdoc((t_doc*) val);
+ f_gen_
+ << indent()
+ << "pub const " << rust_enum_variant_name(val->get_name()) << ": " << enum_name
+ << " = "
+ << enum_name << "(" << val->get_value() << ")"
+ << ";"
+ << endl;
+ }
+ }
+
+ // array containing all IDL-defined enum variants
+ {
+ f_gen_ << indent() << "pub const ENUM_VALUES: &'static [Self] = &[" << endl;
+ indent_up();
+ vector<t_enum_value*>::iterator constants_iter;
+ for (constants_iter = constants.begin(); constants_iter != constants.end(); ++constants_iter) {
+ t_enum_value* val = (*constants_iter);
+ f_gen_
+ << indent()
+ << "Self::" << rust_enum_variant_name(val->get_name())
+ << ","
+ << endl;
+ }
+ indent_down();
+ f_gen_ << indent() << "];" << endl;
+ }
+
f_gen_
<< indent()
- << "pub fn write_to_out_protocol(self, o_prot: &mut dyn TOutputProtocol) -> thrift::Result<()> {"
+ << "pub fn write_to_out_protocol(&self, o_prot: &mut dyn TOutputProtocol) -> thrift::Result<()> {"
<< endl;
indent_up();
- f_gen_ << indent() << "o_prot.write_i32(self as i32)" << endl;
+ f_gen_ << indent() << "o_prot.write_i32(self.0)" << endl;
indent_down();
f_gen_ << indent() << "}" << endl;
@@ -919,10 +932,8 @@ void t_rs_generator::render_enum_impl(const string& enum_name) {
<< "pub fn read_from_in_protocol(i_prot: &mut dyn TInputProtocol) -> thrift::Result<" << enum_name << "> {"
<< endl;
indent_up();
-
f_gen_ << indent() << "let enum_value = i_prot.read_i32()?;" << endl;
- f_gen_ << indent() << enum_name << "::try_from(enum_value)";
-
+ f_gen_ << indent() << "Ok(" << enum_name << "::from(enum_value)" << ")" << endl;
indent_down();
f_gen_ << indent() << "}" << endl;
@@ -932,17 +943,13 @@ void t_rs_generator::render_enum_impl(const string& enum_name) {
}
void t_rs_generator::render_enum_conversion(t_enum* tenum, const string& enum_name) {
- f_gen_ << "impl TryFrom<i32> for " << enum_name << " {" << endl;
+ // From trait: i32 -> ENUM_TYPE
+ f_gen_ << "impl From<i32> for " << enum_name << " {" << endl;
indent_up();
-
- f_gen_ << indent() << "type Error = thrift::Error;";
-
- f_gen_ << indent() << "fn try_from(i: i32) -> Result<Self, Self::Error> {" << endl;
+ f_gen_ << indent() << "fn from(i: i32) -> Self {" << endl;
indent_up();
-
f_gen_ << indent() << "match i {" << endl;
indent_up();
-
vector<t_enum_value*> constants = tenum->get_constants();
vector<t_enum_value*>::iterator constants_iter;
for (constants_iter = constants.begin(); constants_iter != constants.end(); ++constants_iter) {
@@ -950,26 +957,50 @@ void t_rs_generator::render_enum_conversion(t_enum* tenum, const string& enum_na
f_gen_
<< indent()
<< val->get_value()
- << " => Ok(" << enum_name << "::" << rust_enum_variant_name(val->get_name()) << "),"
+ << " => " << enum_name << "::" << rust_enum_variant_name(val->get_name()) << ","
<< endl;
}
- f_gen_ << indent() << "_ => {" << endl;
- indent_up();
- render_thrift_error(
- "Protocol",
- "ProtocolError",
- "ProtocolErrorKind::InvalidData",
- "format!(\"cannot convert enum constant {} to " + enum_name + "\", i)"
- );
+ f_gen_ << indent() << "_ => " << enum_name << "(i)" << endl;
indent_down();
- f_gen_ << indent() << "}," << endl;
+ f_gen_ << indent() << "}" << endl;
+ indent_down();
+ f_gen_ << indent() << "}" << endl;
+ indent_down();
+ f_gen_ << "}" << endl;
+ f_gen_ << endl;
+ // From trait: &i32 -> ENUM_TYPE
+ f_gen_ << "impl From<&i32> for " << enum_name << " {" << endl;
+ indent_up();
+ f_gen_ << indent() << "fn from(i: &i32) -> Self {" << endl;
+ indent_up();
+ f_gen_ << indent() << enum_name << "::from(*i)" << endl;
indent_down();
f_gen_ << indent() << "}" << endl;
+ indent_down();
+ f_gen_ << "}" << endl;
+ f_gen_ << endl;
+ // From trait: ENUM_TYPE -> int
+ f_gen_ << "impl From<" << enum_name << "> for i32 {" << endl;
+ indent_up();
+ f_gen_ << indent() << "fn from(e: " << enum_name << ") -> i32 {" << endl;
+ indent_up();
+ f_gen_ << indent() << "e.0" << endl;
indent_down();
f_gen_ << indent() << "}" << endl;
+ indent_down();
+ f_gen_ << "}" << endl;
+ f_gen_ << endl;
+ // From trait: &ENUM_TYPE -> int
+ f_gen_ << "impl From<&" << enum_name << "> for i32 {" << endl;
+ indent_up();
+ f_gen_ << indent() << "fn from(e: &" << enum_name << ") -> i32 {" << endl;
+ indent_up();
+ f_gen_ << indent() << "e.0" << endl;
+ indent_down();
+ f_gen_ << indent() << "}" << endl;
indent_down();
f_gen_ << "}" << endl;
f_gen_ << endl;
@@ -3295,9 +3326,11 @@ string t_rs_generator::rust_enum_variant_name(const string &name) {
}
if (all_uppercase) {
- return capitalize(camelcase(lowercase(name)));
+ return name;
} else {
- return capitalize(camelcase(name));
+ string modified_name(uppercase(underscore(name)));
+ string_replace(modified_name, "__", "_");
+ return modified_name;
}
}
diff --git a/lib/rs/README.md b/lib/rs/README.md
index 555d219..1e10e35 100644
--- a/lib/rs/README.md
+++ b/lib/rs/README.md
@@ -46,6 +46,51 @@ It does not currently use any Rust 2018 features.
Breaking changes are minimized. When they are made they will be outlined below with transition guidelines.
+##### Thrift 0.15.0
+
+* **[THRIFT-5314]** - Generate enums implementations that are forward compatible (i.e. don't error on unknown values)
+
+ As a result of this change the Rust representation of an enum changes from a standard
+ Rust enum into a newtype struct with associated constants.
+
+ For example:
+
+ ```thrift
+ // THRIFT
+ enum Operation {
+ ADD,
+ SUBTRACT,
+ MULTIPLY,
+ DIVIDE,
+ }
+ ```
+
+ used to generate:
+
+ ```rust
+ // OLD AUTO-GENERATED RUST
+ pub enum Operation {
+ Add,
+ Subtract,
+ Multiply,
+ Divide,
+ }
+ ```
+
+ It *now* generates:
+
+ ```rust
+ // NEW AUTO-GENERATED RUST
+ pub struct Operation(pub i32);
+
+ impl Operation {
+ pub const ADD: Operation = Operation(0);
+ pub const SUBTRACT: Operation = Operation(1);
+ pub const MULTIPLY: Operation = Operation(2);
+ pub const DIVIDE: Operation = Operation(3);
+ }
+ ```
+
##### Thrift 0.14.0
* **[THRIFT-5158]** - Rust library and generator now support Rust 2018 only. Required rust 1.40.0 or higher
@@ -91,7 +136,9 @@ Breaking changes are minimized. When they are made they will be outlined below w
DIVIDE,
}
```
+
It *now* generates:
+
```rust
// NEW AUTO-GENERATED RUST
pub enum Operation {
diff --git a/lib/rs/test/src/bin/kitchen_sink_server.rs b/lib/rs/test/src/bin/kitchen_sink_server.rs
index 6df39e4..8b910b3 100644
--- a/lib/rs/test/src/bin/kitchen_sink_server.rs
+++ b/lib/rs/test/src/bin/kitchen_sink_server.rs
@@ -209,12 +209,12 @@ struct FullHandler;
impl FullMealAndDrinksServiceSyncHandler for FullHandler {
fn handle_full_meal_and_drinks(&self) -> thrift::Result<FullMealAndDrinks> {
println!("full_meal_and_drinks: handling full meal and drinks call");
- Ok(FullMealAndDrinks::new(full_meal(), Drink::CanadianWhisky))
+ Ok(FullMealAndDrinks::new(full_meal(), Drink::CANADIAN_WHISKY))
}
fn handle_best_pie(&self) -> thrift::Result<Pie> {
println!("full_meal_and_drinks: handling pie call");
- Ok(Pie::MississippiMud) // I prefer Pie::Pumpkin, but I have to check that casing works
+ Ok(Pie::MISSISSIPPI_MUD) // I prefer Pie::Pumpkin, but I have to check that casing works
}
}
@@ -259,7 +259,7 @@ fn noodle() -> Noodle {
}
fn ramen() -> Ramen {
- Ramen::new("Mr Ramen".to_owned(), 72, BrothType::Miso)
+ Ramen::new("Mr Ramen".to_owned(), 72, BrothType::MISO)
}
fn napkin() -> Napkin {
diff --git a/test/rs/src/bin/test_client.rs b/test/rs/src/bin/test_client.rs
index 3e20999..476f9eb 100644
--- a/test/rs/src/bin/test_client.rs
+++ b/test/rs/src/bin/test_client.rs
@@ -206,7 +206,7 @@ fn make_thrift_calls(
info!("testEnum");
{
- verify_expected_result(thrift_test_client.test_enum(Numberz::Two), Numberz::Two)?;
+ verify_expected_result(thrift_test_client.test_enum(Numberz::TWO), Numberz::TWO)?;
}
info!("testBinary");
@@ -381,7 +381,7 @@ fn make_thrift_calls(
};
verify_expected_result(
- thrift_test_client.test_multi(1, -123_948, -19_234_123_981, m_snd, Numberz::Eight, 81),
+ thrift_test_client.test_multi(1, -123_948, -19_234_123_981, m_snd, Numberz::EIGHT, 81),
s_cmp,
)?;
}
@@ -395,8 +395,8 @@ fn make_thrift_calls(
// }
{
let mut arg_map_usermap: BTreeMap<Numberz, i64> = BTreeMap::new();
- arg_map_usermap.insert(Numberz::One, 4289);
- arg_map_usermap.insert(Numberz::Eight, 19);
+ arg_map_usermap.insert(Numberz::ONE, 4289);
+ arg_map_usermap.insert(Numberz::EIGHT, 19);
let mut arg_vec_xtructs: Vec<Xtruct> = Vec::new();
arg_vec_xtructs.push(
@@ -429,15 +429,15 @@ fn make_thrift_calls(
user_map: Some(arg_map_usermap),
xtructs: Some(arg_vec_xtructs),
};
- s_cmp_nested_1.insert(Numberz::Two, insanity.clone());
- s_cmp_nested_1.insert(Numberz::Three, insanity.clone());
+ s_cmp_nested_1.insert(Numberz::TWO, insanity.clone());
+ s_cmp_nested_1.insert(Numberz::THREE, insanity.clone());
let mut s_cmp_nested_2: BTreeMap<Numberz, Insanity> = BTreeMap::new();
let empty_insanity = Insanity {
user_map: Some(BTreeMap::new()),
xtructs: Some(Vec::new()),
};
- s_cmp_nested_2.insert(Numberz::Six, empty_insanity);
+ s_cmp_nested_2.insert(Numberz::SIX, empty_insanity);
let mut s_cmp: BTreeMap<UserId, BTreeMap<Numberz, Insanity>> = BTreeMap::new();
s_cmp.insert(1 as UserId, s_cmp_nested_1);
diff --git a/test/rs/src/bin/test_server.rs b/test/rs/src/bin/test_server.rs
index 74be12d..c1f3175 100644
--- a/test/rs/src/bin/test_server.rs
+++ b/test/rs/src/bin/test_server.rs
@@ -268,15 +268,15 @@ impl ThriftTestSyncHandler for ThriftTestSyncHandlerImpl {
) -> thrift::Result<BTreeMap<UserId, BTreeMap<Numberz, Insanity>>> {
info!("testInsanity({:?})", argument);
let mut map_0: BTreeMap<Numberz, Insanity> = BTreeMap::new();
- map_0.insert(Numberz::Two, argument.clone());
- map_0.insert(Numberz::Three, argument);
+ map_0.insert(Numberz::TWO, argument.clone());
+ map_0.insert(Numberz::THREE, argument);
let mut map_1: BTreeMap<Numberz, Insanity> = BTreeMap::new();
let insanity = Insanity {
user_map: None,
xtructs: None,
};
- map_1.insert(Numberz::Six, insanity);
+ map_1.insert(Numberz::SIX, insanity);
let mut ret: BTreeMap<UserId, BTreeMap<Numberz, Insanity>> = BTreeMap::new();
ret.insert(1, map_0);
diff --git a/tutorial/rs/src/bin/tutorial_client.rs b/tutorial/rs/src/bin/tutorial_client.rs
index f7de23f..4bf2ec0 100644
--- a/tutorial/rs/src/bin/tutorial_client.rs
+++ b/tutorial/rs/src/bin/tutorial_client.rs
@@ -67,7 +67,7 @@ fn run() -> thrift::Result<()> {
let logid = 32;
// let's do...a multiply!
- let res = client.calculate(logid, Work::new(7, 8, Operation::Multiply, None))?;
+ let res = client.calculate(logid, Work::new(7, 8, Operation::MULTIPLY, None))?;
println!("multiplied 7 and 8 and got {}", res);
// let's get the log for it
@@ -77,7 +77,7 @@ fn run() -> thrift::Result<()> {
// ok - let's be bad :(
// do a divide by 0
// logid doesn't matter; won't be recorded
- let res = client.calculate(77, Work::new(2, 0, Operation::Divide, "we bad".to_owned()));
+ let res = client.calculate(77, Work::new(2, 0, Operation::DIVIDE, "we bad".to_owned()));
// we should have gotten an exception back
match res {
diff --git a/tutorial/rs/src/bin/tutorial_server.rs b/tutorial/rs/src/bin/tutorial_server.rs
index fbccb69..b6ed7a1 100644
--- a/tutorial/rs/src/bin/tutorial_server.rs
+++ b/tutorial/rs/src/bin/tutorial_server.rs
@@ -123,7 +123,7 @@ impl CalculatorSyncHandler for CalculatorServer {
let res = if let Some(ref op) = w.op {
if w.num1.is_none() || w.num2.is_none() {
Err(InvalidOperation {
- what_op: Some(*op as i32),
+ what_op: Some(op.into()),
why: Some("no operands specified".to_owned()),
})
} else {
@@ -131,19 +131,26 @@ impl CalculatorSyncHandler for CalculatorServer {
let num1 = w.num1.as_ref().expect("operands checked");
let num2 = w.num2.as_ref().expect("operands checked");
- match *op {
- Operation::Add => Ok(num1 + num2),
- Operation::Subtract => Ok(num1 - num2),
- Operation::Multiply => Ok(num1 * num2),
- Operation::Divide => {
+ match op {
+ &Operation::ADD => Ok(num1 + num2),
+ &Operation::SUBTRACT => Ok(num1 - num2),
+ &Operation::MULTIPLY => Ok(num1 * num2),
+ &Operation::DIVIDE => {
if *num2 == 0 {
Err(InvalidOperation {
- what_op: Some(*op as i32),
+ what_op: Some(op.into()),
why: Some("divide by 0".to_owned()),
})
} else {
Ok(num1 / num2)
}
+ },
+ _ => {
+ let op_val: i32 = op.into();
+ Err(InvalidOperation {
+ what_op: Some(op_val),
+ why: Some(format!("unsupported operation type '{}'", op_val)),
+ })
}
}
}