refactor make_instance for const& compat

Summary:
Cython is pretty dumb, so it doesn't understand that it can convert rvalue function returns to const& params. On top of that, it assigns (in generated .cpp) the result of every function call (in .pyx) to a local variable that has to be default constructible. That means we can't do something clevel like const casting to fix the first issue.

This specifically affects `set::insert(const T&)` and `map` key, where we use `map::operator[](const T&)`.

This is a pre-requisite to cleaning up converter logic and removing transitive includes.

V2: # / buildall

Reviewed By: yoney

Differential Revision: D68124625

fbshipit-source-id: 8bc5325d0320a832dc596abe6feb254710ee3619

thrift/compiler/generate/templates/py3/types/make_instance.mustache

@@ -22,6 +22,12 @@ between types.pyx (default mode) and converters.pyx (auto-migrate mode)
 
 }}
     cdef {{> types/cython_cpp_type}} c_inst
+    {{#type:set?}}{{#type:set_elem_type}}
+    cdef {{> types/cython_cpp_type}} c_item
+    {{/type:set_elem_type}}{{/type:set?}}
+    {{#type:map?}}{{#type:key_type}}
+    cdef {{> types/cython_cpp_type}} c_key
+    {{/type:key_type}}{{/type:map?}}
     if items is None:
         return cmove(c_inst)
 {{^type:map?}}
@@ -62,10 +68,13 @@ between types.pyx (default mode) and converters.pyx (auto-migrate mode)
             raise TypeError(f"{item!r} is not of type {{> types/pep484_type}}")
         {{#type:integer?}}
         {{! inject cython int overflow checks }}
-        item = <{{> types/cython_python_type}}> item
+        c_item = <{{> types/cython_python_type}}> item
         {{/type:integer?}}
         {{/type:container?}}
-        c_inst.insert({{> types/cython_python_to_cpp_item}})
+        {{^type:integer?}}
+        c_item = {{> types/cython_python_to_cpp_item}}
+        {{/type:integer?}}
+        c_inst.insert(c_item)
         {{/type:set_elem_type}}
 {{/type:set?}}
 {{#type:map?}}
@@ -82,9 +91,12 @@ between types.pyx (default mode) and converters.pyx (auto-migrate mode)
             raise TypeError(f"{key!r} is not of type {{> types/pep484_type}}")
         {{#type:integer?}}
         {{! inject cython int overflow checks }}
-        key = <{{> types/cython_python_type}}> key
+        c_key = <{{> types/cython_python_type}}> key
         {{/type:integer?}}
         {{/type:container?}}
+        {{^type:integer?}}
+        c_key = {{> types/cython_python_to_cpp_key}}
+        {{/type:integer?}}
         {{/type:key_type}}
         {{#type:value_type}}
         {{#type:container?}}
@@ -103,9 +115,7 @@ between types.pyx (default mode) and converters.pyx (auto-migrate mode)
         {{/type:container?}}
         {{/type:value_type}}
 
-        c_inst[{{#type:key_type}}{{!
-            }}{{> types/cython_python_to_cpp_key}}{{/type:key_type}}{{!
-                }}] = {{#type:value_type}}{{!
+        c_inst[c_key] = {{#type:value_type}}{{!
             }}{{> types/cython_python_to_cpp_item}}{{/type:value_type}}
 {{/type:map?}}
     return cmove(c_inst)

thrift/compiler/test/fixtures/basic-enum/out/py3/gen-py3/test/fixtures/enumstrict/module/types.pyx

@@ -198,15 +198,17 @@ cdef class MyStruct(thrift.py3.types.Struct):
 
 cdef cmap[_test_fixtures_enumstrict_module_cbindings.cMyEnum,string] Map__MyEnum_string__make_instance(object items) except *:
     cdef cmap[_test_fixtures_enumstrict_module_cbindings.cMyEnum,string] c_inst
+    cdef _test_fixtures_enumstrict_module_cbindings.cMyEnum c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, MyEnum):
             raise TypeError(f"{key!r} is not of type MyEnum")
+        c_key = <_test_fixtures_enumstrict_module_cbindings.cMyEnum><int>key
         if not isinstance(item, str):
             raise TypeError(f"{item!r} is not of type str")
 
-        c_inst[<_test_fixtures_enumstrict_module_cbindings.cMyEnum><int>key] = item.encode('UTF-8')
+        c_inst[c_key] = item.encode('UTF-8')
     return cmove(c_inst)
 
 cdef object Map__MyEnum_string__from_cpp(const cmap[_test_fixtures_enumstrict_module_cbindings.cMyEnum,string]& c_map) except *:

thrift/compiler/test/fixtures/basic/out/py3/gen-py3/test/fixtures/basic/module/types.pyx

@@ -1233,12 +1233,14 @@ cdef class UnionToBeRenamed(thrift.py3.types.Union):
 
 cdef cset[float] Set__float__make_instance(object items) except *:
     cdef cset[float] c_inst
+    cdef float c_item
     if items is None:
         return cmove(c_inst)
     for item in items:
         if not isinstance(item, (float, int)):
             raise TypeError(f"{item!r} is not of type float")
-        c_inst.insert(item)
+        c_item = item
+        c_inst.insert(c_item)
     return cmove(c_inst)
 
 cdef object Set__float__from_cpp(const cset[float]& c_set) except *:
@@ -1270,14 +1272,16 @@ cdef object List__i32__from_cpp(const vector[cint32_t]& c_vec) except *:
 
 cdef cset[string] Set__string__make_instance(object items) except *:
     cdef cset[string] c_inst
+    cdef string c_item
     if items is None:
         return cmove(c_inst)
     if isinstance(items, str):
         raise TypeError("If you really want to pass a string into a _typing.AbstractSet[str] field, explicitly convert it first.")
     for item in items:
         if not isinstance(item, str):
             raise TypeError(f"{item!r} is not of type str")
-        c_inst.insert(item.encode('UTF-8'))
+        c_item = item.encode('UTF-8')
+        c_inst.insert(c_item)
     return cmove(c_inst)
 
 cdef object Set__string__from_cpp(const cset[string]& c_set) except *:
@@ -1291,16 +1295,18 @@ cdef object Set__string__from_cpp(const cset[string]& c_set) except *:
 
 cdef cmap[string,cint64_t] Map__string_i64__make_instance(object items) except *:
     cdef cmap[string,cint64_t] c_inst
+    cdef string c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, str):
             raise TypeError(f"{key!r} is not of type str")
+        c_key = key.encode('UTF-8')
         if not isinstance(item, int):
             raise TypeError(f"{item!r} is not of type int")
         item = <cint64_t> item
 
-        c_inst[key.encode('UTF-8')] = item
+        c_inst[c_key] = item
     return cmove(c_inst)
 
 cdef object Map__string_i64__from_cpp(const cmap[string,cint64_t]& c_map) except *:
@@ -1315,17 +1321,19 @@ cdef object Map__string_i64__from_cpp(const cmap[string,cint64_t]& c_map) except
 
 cdef cmap[string,vector[cint32_t]] Map__string_List__i32__make_instance(object items) except *:
     cdef cmap[string,vector[cint32_t]] c_inst
+    cdef string c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, str):
             raise TypeError(f"{key!r} is not of type str")
+        c_key = key.encode('UTF-8')
         if item is None:
             raise TypeError("None is not of type _typing.Sequence[int]")
         if not isinstance(item, List__i32):
             item = List__i32(item)
 
-        c_inst[key.encode('UTF-8')] = List__i32__make_instance(item)
+        c_inst[c_key] = List__i32__make_instance(item)
     return cmove(c_inst)
 
 cdef object Map__string_List__i32__from_cpp(const cmap[string,vector[cint32_t]]& c_map) except *:

thrift/compiler/test/fixtures/complex-union/out/py3/gen-py3/module/types.pyx

@@ -1378,16 +1378,17 @@ cdef object List__string__from_cpp(const vector[string]& c_vec) except *:
 
 cdef cmap[cint16_t,string] Map__i16_string__make_instance(object items) except *:
     cdef cmap[cint16_t,string] c_inst
+    cdef cint16_t c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, int):
             raise TypeError(f"{key!r} is not of type int")
-        key = <cint16_t> key
+        c_key = <cint16_t> key
         if not isinstance(item, str):
             raise TypeError(f"{item!r} is not of type str")
 
-        c_inst[key] = item.encode('UTF-8')
+        c_inst[c_key] = item.encode('UTF-8')
     return cmove(c_inst)
 
 cdef object Map__i16_string__from_cpp(const cmap[cint16_t,string]& c_map) except *:

thrift/compiler/test/fixtures/constants/out/py3/gen-py3/module/types.pyx

@@ -1320,16 +1320,18 @@ cdef object List__i32__from_cpp(const vector[cint32_t]& c_vec) except *:
 
 cdef cmap[string,cint32_t] Map__string_i32__make_instance(object items) except *:
     cdef cmap[string,cint32_t] c_inst
+    cdef string c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, str):
             raise TypeError(f"{key!r} is not of type str")
+        c_key = key.encode('UTF-8')
         if not isinstance(item, int):
             raise TypeError(f"{item!r} is not of type int")
         item = <cint32_t> item
 
-        c_inst[key.encode('UTF-8')] = item
+        c_inst[c_key] = item
     return cmove(c_inst)
 
 cdef object Map__string_i32__from_cpp(const cmap[string,cint32_t]& c_map) except *:
@@ -1363,15 +1365,17 @@ cdef object List__Map__string_i32__from_cpp(const vector[cmap[string,cint32_t]]&
 
 cdef cmap[string,string] Map__string_string__make_instance(object items) except *:
     cdef cmap[string,string] c_inst
+    cdef string c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, str):
             raise TypeError(f"{key!r} is not of type str")
+        c_key = key.encode('UTF-8')
         if not isinstance(item, str):
             raise TypeError(f"{item!r} is not of type str")
 
-        c_inst[key.encode('UTF-8')] = item.encode('UTF-8')
+        c_inst[c_key] = item.encode('UTF-8')
     return cmove(c_inst)
 
 cdef object Map__string_string__from_cpp(const cmap[string,string]& c_map) except *:
@@ -1456,13 +1460,14 @@ cdef object List__string__from_cpp(const vector[string]& c_vec) except *:
 
 cdef cset[cint32_t] Set__i32__make_instance(object items) except *:
     cdef cset[cint32_t] c_inst
+    cdef cint32_t c_item
     if items is None:
         return cmove(c_inst)
     for item in items:
         if not isinstance(item, int):
             raise TypeError(f"{item!r} is not of type int")
-        item = <cint32_t> item
-        c_inst.insert(item)
+        c_item = <cint32_t> item
+        c_inst.insert(c_item)
     return cmove(c_inst)
 
 cdef object Set__i32__from_cpp(const cset[cint32_t]& c_set) except *:
@@ -1476,14 +1481,16 @@ cdef object Set__i32__from_cpp(const cset[cint32_t]& c_set) except *:
 
 cdef cset[string] Set__string__make_instance(object items) except *:
     cdef cset[string] c_inst
+    cdef string c_item
     if items is None:
         return cmove(c_inst)
     if isinstance(items, str):
         raise TypeError("If you really want to pass a string into a _typing.AbstractSet[str] field, explicitly convert it first.")
     for item in items:
         if not isinstance(item, str):
             raise TypeError(f"{item!r} is not of type str")
-        c_inst.insert(item.encode('UTF-8'))
+        c_item = item.encode('UTF-8')
+        c_inst.insert(c_item)
     return cmove(c_inst)
 
 cdef object Set__string__from_cpp(const cset[string]& c_set) except *:
@@ -1497,17 +1504,18 @@ cdef object Set__string__from_cpp(const cset[string]& c_set) except *:
 
 cdef cmap[cint32_t,cint32_t] Map__i32_i32__make_instance(object items) except *:
     cdef cmap[cint32_t,cint32_t] c_inst
+    cdef cint32_t c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, int):
             raise TypeError(f"{key!r} is not of type int")
-        key = <cint32_t> key
+        c_key = <cint32_t> key
         if not isinstance(item, int):
             raise TypeError(f"{item!r} is not of type int")
         item = <cint32_t> item
 
-        c_inst[key] = item
+        c_inst[c_key] = item
     return cmove(c_inst)
 
 cdef object Map__i32_i32__from_cpp(const cmap[cint32_t,cint32_t]& c_map) except *:
@@ -1522,16 +1530,17 @@ cdef object Map__i32_i32__from_cpp(const cmap[cint32_t,cint32_t]& c_map) except
 
 cdef cmap[cint32_t,string] Map__i32_string__make_instance(object items) except *:
     cdef cmap[cint32_t,string] c_inst
+    cdef cint32_t c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, int):
             raise TypeError(f"{key!r} is not of type int")
-        key = <cint32_t> key
+        c_key = <cint32_t> key
         if not isinstance(item, str):
             raise TypeError(f"{item!r} is not of type str")
 
-        c_inst[key] = item.encode('UTF-8')
+        c_inst[c_key] = item.encode('UTF-8')
     return cmove(c_inst)
 
 cdef object Map__i32_string__from_cpp(const cmap[cint32_t,string]& c_map) except *:
@@ -1546,16 +1555,17 @@ cdef object Map__i32_string__from_cpp(const cmap[cint32_t,string]& c_map) except
 
 cdef cmap[cint32_t,cbool] Map__i32_bool__make_instance(object items) except *:
     cdef cmap[cint32_t,cbool] c_inst
+    cdef cint32_t c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, int):
             raise TypeError(f"{key!r} is not of type int")
-        key = <cint32_t> key
+        c_key = <cint32_t> key
         if not isinstance(item, bool):
             raise TypeError(f"{item!r} is not of type bool")
 
-        c_inst[key] = item
+        c_inst[c_key] = item
     return cmove(c_inst)
 
 cdef object Map__i32_bool__from_cpp(const cmap[cint32_t,cbool]& c_map) except *:

thrift/compiler/test/fixtures/enums/out/py3/gen-py3/test/fixtures/enums/module/types.pyx

@@ -368,13 +368,14 @@ cdef class MyStruct(thrift.py3.types.Struct):
 
 cdef cset[cint32_t] Set__i32__make_instance(object items) except *:
     cdef cset[cint32_t] c_inst
+    cdef cint32_t c_item
     if items is None:
         return cmove(c_inst)
     for item in items:
         if not isinstance(item, int):
             raise TypeError(f"{item!r} is not of type int")
-        item = <cint32_t> item
-        c_inst.insert(item)
+        c_item = <cint32_t> item
+        c_inst.insert(c_item)
     return cmove(c_inst)
 
 cdef object Set__i32__from_cpp(const cset[cint32_t]& c_set) except *:

thrift/compiler/test/fixtures/list/out/py3/gen-py3/module/types.pyx

@@ -85,18 +85,19 @@ cdef object List__string__from_cpp(const vector[string]& c_vec) except *:
 
 cdef cmap[cint64_t,vector[string]] Map__i64_List__string__make_instance(object items) except *:
     cdef cmap[cint64_t,vector[string]] c_inst
+    cdef cint64_t c_key
     if items is None:
         return cmove(c_inst)
     for key, item in items.items():
         if not isinstance(key, int):
             raise TypeError(f"{key!r} is not of type int")
-        key = <cint64_t> key
+        c_key = <cint64_t> key
         if item is None:
             raise TypeError("None is not of type _typing.Sequence[str]")
         if not isinstance(item, List__string):
             item = List__string(item)
 
-        c_inst[key] = List__string__make_instance(item)
+        c_inst[c_key] = List__string__make_instance(item)
     return cmove(c_inst)
 
 cdef object Map__i64_List__string__from_cpp(const cmap[cint64_t,vector[string]]& c_map) except *: