Skip to content

Commit

Permalink
Disallow python patch of unsafe patch
Browse files Browse the repository at this point in the history
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
  • Loading branch information
TJ Yin authored and facebook-github-bot committed Jan 9, 2025
1 parent f318d46 commit f3e92c9
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 6 deletions.
26 changes: 26 additions & 0 deletions thrift/compiler/generate/python/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,32 @@ bool is_type_iobuf(const t_type* type) {
return is_type_iobuf(cpp2::get_type(type));
}

bool is_unsafe_patch_program(const t_program* prog) {
return prog ? boost::algorithm::starts_with(prog->name(), "gen_patch_")
: false;
}

bool type_contains_unsafe_patch(const t_type* type) {
if (auto typedf = dynamic_cast<t_typedef const*>(type)) {
return type_contains_unsafe_patch(typedf->get_type());
}

if (auto map = dynamic_cast<t_map const*>(type)) {
return type_contains_unsafe_patch(map->get_key_type()) ||
type_contains_unsafe_patch(map->get_val_type());
}

if (auto set = dynamic_cast<t_set const*>(type)) {
return type_contains_unsafe_patch(set->get_elem_type());
}

if (auto list = dynamic_cast<t_list const*>(type)) {
return type_contains_unsafe_patch(list->get_elem_type());
}

return is_unsafe_patch_program(type->program());
}

std::vector<std::string> get_py3_namespace(const t_program* prog) {
t_program::namespace_config conf;
conf.no_top_level_domain = true;
Expand Down
4 changes: 4 additions & 0 deletions thrift/compiler/generate/python/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ bool is_type_iobuf(std::string_view name);

bool is_type_iobuf(const t_type* type);

bool is_unsafe_patch_program(const t_program* prog);

bool type_contains_unsafe_patch(const t_type* type);

std::vector<std::string> get_py3_namespace(const t_program* prog);

std::string get_py3_namespace_with_name_and_prefix(
Expand Down
12 changes: 10 additions & 2 deletions thrift/compiler/generate/t_mstch_python_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ class python_mstch_program : public mstch_program {
{"included_module_path", it->ns},
{"included_module_mangle", it->ns_mangle},
{"has_services?", it->has_services},
{"has_types?", it->has_types}});
{"has_types?", it->has_types},
{"is_unsafe_patch?", it->is_unsafe_patch}});
}
return a;
}
Expand Down Expand Up @@ -300,6 +301,7 @@ class python_mstch_program : public mstch_program {
std::string ns_mangle;
bool has_services;
bool has_types;
bool is_unsafe_patch;
};

void gather_included_program_namespaces() {
Expand All @@ -317,7 +319,7 @@ class python_mstch_program : public mstch_program {
included_program, get_option("root_module_prefix")),
!included_program->services().empty(),
has_types,
};
is_unsafe_patch_program(included_program)};
}
}

Expand Down Expand Up @@ -670,6 +672,8 @@ class python_mstch_type : public mstch_type {
{"type:external_program?", &python_mstch_type::is_external_program},
{"type:integer?", &python_mstch_type::is_integer},
{"type:iobuf?", &python_mstch_type::is_iobuf},
{"type:contains_unsafe_patch?",
&python_mstch_type::contains_unsafe_patch},
{"type:has_adapter?", &python_mstch_type::adapter},
});
register_volatile_methods(
Expand Down Expand Up @@ -774,6 +778,10 @@ class python_mstch_type : public mstch_type {

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

mstch::node contains_unsafe_patch() {
return type_contains_unsafe_patch(type_);
}

mstch::node adapter() {
return adapter_node(
adapter_annotation_, transitive_adapter_annotation_, context_, pos_);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ import {{program:safe_patch_module_path}}.thrift_types as _fbthrift_safe_patch_t
{{/if (not program:safe_patch?)}}

{{#program:include_namespaces}}
{{#has_types?}}
{{#if has_types?}}{{#if (not is_unsafe_patch?)}}

import {{included_module_path}}.{{> ../python/types/types_import_path}} as {{included_module_mangle}}__{{> ../python/types/types_import_path}}
import {{included_module_path}}.thrift_patch
{{/has_types?}}
{{/if (not is_unsafe_patch?)}}{{/if has_types?}}
{{/program:include_namespaces}}

{{#program:structs}}
Expand All @@ -68,7 +68,7 @@ class {{struct:py_name}}Patch(
Base{{#if struct:union?}}Union{{#else}}Struct{{/if struct:union?}}Patch[{{program:module_mangle}}__thrift_types.{{struct:py_name}}]
):
pass
{{#struct:fields_ordered_by_id}}{{#field:type}}
{{#struct:fields_ordered_by_id}}{{#field:type}}{{#if (not type:contains_unsafe_patch?)}}
@property
def {{field:py_name}}(self) -> {{> types/field_patch_type}}[
{{> ../python/types/unadapted_pep484_type}},
Expand Down Expand Up @@ -99,5 +99,5 @@ class {{struct:py_name}}Patch(
{{/if (not program:safe_patch?)}}


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

0 comments on commit f3e92c9

Please sign in to comment.