Skip to content

Commit 036534c

Browse files
Mizuchifacebook-github-bot
authored andcommitted
Disallow python patch of unsafe patch
Summary: In ICSP, unsafe patch is still used in IDL: https://fburl.com/code/29nc4mqx Currently in order to use python patch, we will need to enable python patch for those unsafe patch thrift files as well. .e.g, ``` include "path/to/gen_patch_foo.thrift" struct Bar { 1: gen_patch_foo fooPatch; 2: string msg; } ``` In order to enable python patch on Bar, we need to enable it on "path/to/gen_patch_foo.thrift" as well. ``` thrift_patch_library( gen_patch_python_api_experimental = True, ... ) ``` This has two problems 1. Users are able to create patch of unsafe patch, which is a confusion concept. 2. It makes python patch harder to enable, since we need to enable it for all dependents unsafe patch as well. To fix the two issues above, this diff will skip generating python field patch for fields that contain unsafe patch. e.g., after this diff, we will skip `fooPatch` field (as if it doesn't exist) when generating `BarPatch`. Differential Revision: D67880408 fbshipit-source-id: de047e18d4caa164ae3a4e24e1b3baa25fbc3284
1 parent 41db10d commit 036534c

File tree

4 files changed

+44
-6
lines changed

4 files changed

+44
-6
lines changed

third-party/thrift/src/thrift/compiler/generate/python/util.cc

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,32 @@ bool is_type_iobuf(const t_type* type) {
3131
return is_type_iobuf(cpp2::get_type(type));
3232
}
3333

34+
bool is_unsafe_patch_program(const t_program* prog) {
35+
return prog ? boost::algorithm::starts_with(prog->name(), "gen_patch_")
36+
: false;
37+
}
38+
39+
bool type_contains_unsafe_patch(const t_type* type) {
40+
if (auto typedf = dynamic_cast<t_typedef const*>(type)) {
41+
return type_contains_unsafe_patch(typedf->get_type());
42+
}
43+
44+
if (auto map = dynamic_cast<t_map const*>(type)) {
45+
return type_contains_unsafe_patch(map->get_key_type()) ||
46+
type_contains_unsafe_patch(map->get_val_type());
47+
}
48+
49+
if (auto set = dynamic_cast<t_set const*>(type)) {
50+
return type_contains_unsafe_patch(set->get_elem_type());
51+
}
52+
53+
if (auto list = dynamic_cast<t_list const*>(type)) {
54+
return type_contains_unsafe_patch(list->get_elem_type());
55+
}
56+
57+
return is_unsafe_patch_program(type->program());
58+
}
59+
3460
std::vector<std::string> get_py3_namespace(const t_program* prog) {
3561
t_program::namespace_config conf;
3662
conf.no_top_level_domain = true;

third-party/thrift/src/thrift/compiler/generate/python/util.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ bool is_type_iobuf(std::string_view name);
3737

3838
bool is_type_iobuf(const t_type* type);
3939

40+
bool is_unsafe_patch_program(const t_program* prog);
41+
42+
bool type_contains_unsafe_patch(const t_type* type);
43+
4044
std::vector<std::string> get_py3_namespace(const t_program* prog);
4145

4246
std::string get_py3_namespace_with_name_and_prefix(

third-party/thrift/src/thrift/compiler/generate/t_mstch_python_generator.cc

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,8 @@ class python_mstch_program : public mstch_program {
219219
{"included_module_path", it->ns},
220220
{"included_module_mangle", it->ns_mangle},
221221
{"has_services?", it->has_services},
222-
{"has_types?", it->has_types}});
222+
{"has_types?", it->has_types},
223+
{"is_unsafe_patch?", it->is_unsafe_patch}});
223224
}
224225
return a;
225226
}
@@ -300,6 +301,7 @@ class python_mstch_program : public mstch_program {
300301
std::string ns_mangle;
301302
bool has_services;
302303
bool has_types;
304+
bool is_unsafe_patch;
303305
};
304306

305307
void gather_included_program_namespaces() {
@@ -317,7 +319,7 @@ class python_mstch_program : public mstch_program {
317319
included_program, get_option("root_module_prefix")),
318320
!included_program->services().empty(),
319321
has_types,
320-
};
322+
is_unsafe_patch_program(included_program)};
321323
}
322324
}
323325

@@ -670,6 +672,8 @@ class python_mstch_type : public mstch_type {
670672
{"type:external_program?", &python_mstch_type::is_external_program},
671673
{"type:integer?", &python_mstch_type::is_integer},
672674
{"type:iobuf?", &python_mstch_type::is_iobuf},
675+
{"type:contains_unsafe_patch?",
676+
&python_mstch_type::contains_unsafe_patch},
673677
{"type:has_adapter?", &python_mstch_type::adapter},
674678
});
675679
register_volatile_methods(
@@ -774,6 +778,10 @@ class python_mstch_type : public mstch_type {
774778

775779
mstch::node is_iobuf() { return is_type_iobuf(type_); }
776780

781+
mstch::node contains_unsafe_patch() {
782+
return type_contains_unsafe_patch(type_);
783+
}
784+
777785
mstch::node adapter() {
778786
return adapter_node(
779787
adapter_annotation_, transitive_adapter_annotation_, context_, pos_);

third-party/thrift/src/thrift/compiler/generate/templates/patch/thrift_patch.py.mustache

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,11 @@ import {{program:safe_patch_module_path}}.thrift_types as _fbthrift_safe_patch_t
5555
{{/if (not program:safe_patch?)}}
5656

5757
{{#program:include_namespaces}}
58-
{{#has_types?}}
58+
{{#if has_types?}}{{#if (not is_unsafe_patch?)}}
5959

6060
import {{included_module_path}}.{{> ../python/types/types_import_path}} as {{included_module_mangle}}__{{> ../python/types/types_import_path}}
6161
import {{included_module_path}}.thrift_patch
62-
{{/has_types?}}
62+
{{/if (not is_unsafe_patch?)}}{{/if has_types?}}
6363
{{/program:include_namespaces}}
6464

6565
{{#program:structs}}
@@ -68,7 +68,7 @@ class {{struct:py_name}}Patch(
6868
Base{{#if struct:union?}}Union{{#else}}Struct{{/if struct:union?}}Patch[{{program:module_mangle}}__thrift_types.{{struct:py_name}}]
6969
):
7070
pass
71-
{{#struct:fields_ordered_by_id}}{{#field:type}}
71+
{{#struct:fields_ordered_by_id}}{{#field:type}}{{#if (not type:contains_unsafe_patch?)}}
7272
@property
7373
def {{field:py_name}}(self) -> {{> types/field_patch_type}}[
7474
{{> ../python/types/unadapted_pep484_type}},
@@ -99,5 +99,5 @@ class {{struct:py_name}}Patch(
9999
{{/if (not program:safe_patch?)}}
100100

101101

102-
{{/field:type}}{{/struct:fields_ordered_by_id}}
102+
{{/if (not type:contains_unsafe_patch?)}}{{/field:type}}{{/struct:fields_ordered_by_id}}
103103
{{/program:structs}}

0 commit comments

Comments
 (0)