From b7b0c97b07cb557e91e033fcf9b696532eba932e Mon Sep 17 00:00:00 2001 From: Satish Kumar Date: Thu, 16 Jan 2025 18:56:13 -0800 Subject: [PATCH] Do not generate abstract property for invariant types. Summary: # What? Do not generate `fbthrift_current_value` if any field of the union is an invariant type. # Why? `fbthrift_current_value` returns a `typing.Union` of the types of the fields in the union plus `None`. If any of the fields have an invariant type, then the type-check will report that the `fbthrift_current_value` in the derived class in `thrift_mutable_types` overrides the attribute defined in the `thrift_abstract_types` base class inconsistently. `thrift_types` doesn't flag this same error due to some unrelated error that has a separate task to fix it. # Build fixtures `buck2 run fbcode//thrift/compiler/test:build_fixtures` # Internal # What was the canary? Instagram pyre check flagged this lone error below (P1711855823). You could say this was a needle-in-the-haystack as well. ``` distillery/igthrift/gen/apache/thrift/protocol/detail/protocol_detail/thrift_mutable_types.pyi:172:4 Inconsistent override [15]: `fbthrift_current_value` overrides attribute defined in `_fbthrift_python_abstract_types.Value` inconsistently. Type `_typing.Union[None, _fbthrift_python_mutable_containers.MutableList[Value], _fbthrift_python_mutable_containers.MutableMap[Value, Value], _fbthrift_python_mutable_containers.MutableSet[Value], Object, _fbthrift_iobuf.IOBuf, bool, bytes, float, int]` is not a subtype of the overridden attribute `_typing.Union[None, _typing.AbstractSet[_fbthrift_python_abstract_types.Value], _typing.Mapping[_fbthrift_python_abstract_types.Value, _fbthrift_python_abstract_types.Value], _typing.Sequence[_fbthrift_python_abstract_types.Value], _fbthrift_iobuf.IOBuf, _fbthrift_python_abstract_types.Object, bool, bytes, float, int]`. ``` # Task to fix `thrift_types.pyi` type checks T212883284 [thrift-python] Fix thrift_types.pyi type-checks Reviewed By: yoney Differential Revision: D68270113 fbshipit-source-id: dde2d437330741239c573ecf64eadccfb44f5b86 --- .../templates/python/thrift_abstract_types.py.mustache | 2 ++ .../gen-python/invariant/thrift_abstract_types.py | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/third-party/thrift/src/thrift/compiler/generate/templates/python/thrift_abstract_types.py.mustache b/third-party/thrift/src/thrift/compiler/generate/templates/python/thrift_abstract_types.py.mustache index 0832ee016e9c1..3461586232abe 100644 --- a/third-party/thrift/src/thrift/compiler/generate/templates/python/thrift_abstract_types.py.mustache +++ b/third-party/thrift/src/thrift/compiler/generate/templates/python/thrift_abstract_types.py.mustache @@ -91,9 +91,11 @@ class {{> structs/unadapted_name}}({{! used in immutable thrift-python, which is the name of the Union (sigh!) }}FbThriftUnionFieldEnum.__name__ = "{{struct:py_name}}" + {{^struct:has_invariant_field?}} @_fbthrift_property {{> fields/maybe_abstract_method_annotation}} def fbthrift_current_value(self) -> {{> types/field_value_pep484_union_type}}: ... + {{/struct:has_invariant_field?}} @_fbthrift_property {{> fields/maybe_abstract_method_annotation}} def fbthrift_current_field(self) -> FbThriftUnionFieldEnum: ... diff --git a/third-party/thrift/src/thrift/compiler/test/fixtures/complex-union/out/python_invariant_types/gen-python/invariant/thrift_abstract_types.py b/third-party/thrift/src/thrift/compiler/test/fixtures/complex-union/out/python_invariant_types/gen-python/invariant/thrift_abstract_types.py index f622cf5bbd30b..a006f6a8dd99f 100644 --- a/third-party/thrift/src/thrift/compiler/test/fixtures/complex-union/out/python_invariant_types/gen-python/invariant/thrift_abstract_types.py +++ b/third-party/thrift/src/thrift/compiler/test/fixtures/complex-union/out/python_invariant_types/gen-python/invariant/thrift_abstract_types.py @@ -71,9 +71,6 @@ class FbThriftUnionFieldEnum(_enum.Enum): FbThriftUnionFieldEnum.__name__ = "InvariantTypes" @_fbthrift_property @_abc.abstractmethod - def fbthrift_current_value(self) -> _typing.Union[None, _typing.Mapping[_fbthrift_StructForInvariantTypes, int], _typing.Mapping[_fbthrift_UnionForInvariantTypes, int]]: ... - @_fbthrift_property - @_abc.abstractmethod def fbthrift_current_field(self) -> FbThriftUnionFieldEnum: ... _fbthrift_InvariantTypes = InvariantTypes