THRIFT-5823 Fix illegal uses of exceptions as normal struct type
Patch: Jens Geyer
This closes #1928
diff --git a/compiler/cpp/src/thrift/generate/t_generator.cc b/compiler/cpp/src/thrift/generate/t_generator.cc
index 6860237..217cf8a 100644
--- a/compiler/cpp/src/thrift/generate/t_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_generator.cc
@@ -101,35 +101,43 @@
void t_generator::validate(t_function const* f) const {
validate_id(f->get_name());
+ f->validate();
validate(f->get_arglist());
validate(f->get_xceptions());
}
void t_generator::validate(t_service const* s) const {
validate_id(s->get_name());
+ s->validate();
validate(s->get_functions());
}
void t_generator::validate(t_enum const* en) const {
validate_id(en->get_name());
+ en->validate();
validate(en->get_constants());
}
void t_generator::validate(t_struct const* s) const {
validate_id(s->get_name());
+ s->validate();
validate(s->get_members());
}
void t_generator::validate(t_enum_value const* en_val) const {
validate_id(en_val->get_name());
+ en_val->validate();
}
void t_generator::validate(t_typedef const* td) const {
validate_id(td->get_name());
+ td->validate();
}
void t_generator::validate(t_const const* c) const {
validate_id(c->get_name());
+ c->validate();
}
void t_generator::validate(t_field const* f) const {
validate_id(f->get_name());
+ f->validate();
}
void t_generator::validate_id(const string& id) const {
diff --git a/compiler/cpp/src/thrift/parse/t_doc.h b/compiler/cpp/src/thrift/parse/t_doc.h
index 0df893e..4033bc7 100644
--- a/compiler/cpp/src/thrift/parse/t_doc.h
+++ b/compiler/cpp/src/thrift/parse/t_doc.h
@@ -47,6 +47,8 @@
bool has_doc() { return has_doc_; }
+ virtual void validate() const { ; }
+
private:
std::string doc_;
bool has_doc_;
diff --git a/compiler/cpp/src/thrift/parse/t_field.h b/compiler/cpp/src/thrift/parse/t_field.h
index 928fdcf..3346722 100644
--- a/compiler/cpp/src/thrift/parse/t_field.h
+++ b/compiler/cpp/src/thrift/parse/t_field.h
@@ -41,7 +41,7 @@
: type_(type),
name_(name),
key_(0),
- req_(T_OPT_IN_REQ_OUT),
+ req_(T_OPT_IN_REQ_OUT),
value_(nullptr),
xsd_optional_(false),
xsd_nillable_(false),
diff --git a/compiler/cpp/src/thrift/parse/t_function.h b/compiler/cpp/src/thrift/parse/t_function.h
index a3b6f48..57cf5ff 100644
--- a/compiler/cpp/src/thrift/parse/t_function.h
+++ b/compiler/cpp/src/thrift/parse/t_function.h
@@ -83,6 +83,22 @@
std::map<std::string, std::vector<std::string>> annotations_;
+ void validate() const {
+ get_returntype()->validate();
+ if (get_returntype()->get_true_type()->is_xception()) {
+ failure("method %s(): exception type \"%s\" cannot be used as function return", get_name().c_str(), get_returntype()->get_name().c_str());
+ }
+
+ std::vector<t_field*>::const_iterator it;
+ std::vector<t_field*> list = get_arglist()->get_members();
+ for(it=list.begin(); it != list.end(); ++it) {
+ (*it)->get_type()->validate();
+ if( (*it)->get_type()->get_true_type()->is_xception()) {
+ failure("method %s(): exception type \"%s\" cannot be used as function argument %s", get_name().c_str(), (*it)->get_type()->get_name().c_str(), (*it)->get_name().c_str());
+ }
+ }
+ }
+
private:
t_type* returntype_;
std::string name_;
diff --git a/compiler/cpp/src/thrift/parse/t_list.h b/compiler/cpp/src/thrift/parse/t_list.h
index f0b896d..5daa412 100644
--- a/compiler/cpp/src/thrift/parse/t_list.h
+++ b/compiler/cpp/src/thrift/parse/t_list.h
@@ -34,6 +34,12 @@
bool is_list() const override { return true; }
+ void validate() const {
+ if( get_elem_type()->get_true_type()->is_xception()) {
+ failure("exception type \"%s\" cannot be used inside a list", get_elem_type()->get_name().c_str());
+ }
+ }
+
private:
t_type* elem_type_;
};
diff --git a/compiler/cpp/src/thrift/parse/t_map.h b/compiler/cpp/src/thrift/parse/t_map.h
index 9614e68..444fca7 100644
--- a/compiler/cpp/src/thrift/parse/t_map.h
+++ b/compiler/cpp/src/thrift/parse/t_map.h
@@ -37,6 +37,15 @@
bool is_map() const override { return true; }
+ void validate() const {
+ if( get_key_type()->get_true_type()->is_xception()) {
+ failure("exception type \"%s\" cannot be used inside a map", get_key_type()->get_name().c_str());
+ }
+ if( get_val_type()->get_true_type()->is_xception()) {
+ failure("exception type \"%s\" cannot be used inside a map", get_val_type()->get_name().c_str());
+ }
+ }
+
private:
t_type* key_type_;
t_type* val_type_;
diff --git a/compiler/cpp/src/thrift/parse/t_set.h b/compiler/cpp/src/thrift/parse/t_set.h
index c0d4a35..4a02dcc 100644
--- a/compiler/cpp/src/thrift/parse/t_set.h
+++ b/compiler/cpp/src/thrift/parse/t_set.h
@@ -36,6 +36,12 @@
bool is_set() const override { return true; }
+ void validate() const {
+ if( get_elem_type()->get_true_type()->is_xception()) {
+ failure("exception type \"%s\" cannot be used inside a set", get_elem_type()->get_name().c_str());
+ }
+ }
+
private:
t_type* elem_type_;
};
diff --git a/compiler/cpp/src/thrift/parse/t_struct.h b/compiler/cpp/src/thrift/parse/t_struct.h
index d990ead..941712d 100644
--- a/compiler/cpp/src/thrift/parse/t_struct.h
+++ b/compiler/cpp/src/thrift/parse/t_struct.h
@@ -131,6 +131,27 @@
return nullptr;
}
+ void validate() const {
+ std::string what = "struct";
+ if( is_union()) {
+ what = "union";
+ }
+ if( is_xception()) {
+ what = "exception";
+ }
+
+ std::vector<t_field*>::const_iterator it;
+ std::vector<t_field*> list = get_members();
+ for(it=list.begin(); it != list.end(); ++it) {
+ (*it)->get_type()->validate();
+ if (!is_method_xcepts_) { // this is in fact the only legal usage for any exception type
+ if( (*it)->get_type()->get_true_type()->is_xception()) {
+ failure("%s %s: exception type \"%s\" cannot be used as member field type %s", what.c_str(), get_name().c_str(), (*it)->get_type()->get_name().c_str(), (*it)->get_name().c_str());
+ }
+ }
+ }
+ }
+
private:
members_type members_;
members_type members_in_id_order_;
diff --git a/lib/go/test/ClientMiddlewareExceptionTest.thrift b/lib/go/test/ClientMiddlewareExceptionTest.thrift
index 647c433..b48a611 100644
--- a/lib/go/test/ClientMiddlewareExceptionTest.thrift
+++ b/lib/go/test/ClientMiddlewareExceptionTest.thrift
@@ -25,11 +25,11 @@
// This is a special case, we want to make sure that the middleware don't
// accidentally pull result as error.
-exception FooResponse {
+struct/*exception*/ FooResponse { // returning an exception by any means other than "throws" is illegal
}
service ClientMiddlewareExceptionTest {
- FooResponse foo() throws(
+ FooResponse foo() throws( // returning an exception by any means other than "throws" is illegal
1: Exception1 error1,
2: Exception2 error2,
)
diff --git a/lib/go/test/ForwardType.thrift b/lib/go/test/ForwardType.thrift
index 0433c97..9e3670e 100644
--- a/lib/go/test/ForwardType.thrift
+++ b/lib/go/test/ForwardType.thrift
@@ -25,6 +25,7 @@
1: optional Exc foo
}
-exception Exc {
+// FIX: Use of "exception" is illegal. An exception is not a normal struct type and cannot be used as such.
+struct Exc {
1: optional i32 code
}
diff --git a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift5320.exception.thrift b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift5320.exception.thrift
index 37421c0..d61a300 100644
--- a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift5320.exception.thrift
+++ b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift5320.exception.thrift
@@ -24,7 +24,10 @@
exception Task {
- 1: Task left
- 2: Task right
+ 1: ErrorData data // it would be illegal to use exception as struct type
+}
+
+struct ErrorData {
+ 1: string messitsch
}