From 0ce65a8937b24cbd909b9a83ad84adfd7d0dfe2e Mon Sep 17 00:00:00 2001 From: Satish Kumar Date: Sat, 11 Jan 2025 06:33:41 -0800 Subject: [PATCH] Make legacy_arrays imply arraysets Summary: # What? Make `legacy_arrays` imply `arraysets`. # Why? `legacy_arrays` require `arraysets`. There is no reason for `arraysets` to appear explicitly. # Fixtures Generated fixtures must not change. # Context `arraysets`, `no_use_hack_collections`, `stricttypes`, `array_migration`, `shape_arraykeys`, `const_collections` are all compiler options that control the generation of container fields. To make code generator simpler and easier to reason about, identify which of these can be removed/merged. The new options based on the ones that are currently in use: `legacy_arrays` replaces `no_use_hack_collections`. `hack_collections`, which was the implicit default in the absence of `arrays` and `no_use_hack_collections`, is now explicit. `arrays` will eventually become the default and cease to exist as an explicit option. `const_collections` imply `hack_collections` and `hack_collections` must not appear explicitly in the presence of `const_collections`. `legacy_arrays` imply `array_sets` and `arraysets` must not appear explicitly in the presence of `legacy_arrays`. # The steps The item in bold corresponds to the current diff. 1. Use new compiler options based on the ones that already exist. 1. Make arrays the default option. 1. Use the legacy arrays + hack collections logical equivalent of arrays. 1. Introduce hack collections wherever necessary. 1. Replace no use hack collections with legacy arrays. 1. Remove arraysets if arrays present. 1. Rename `no_use_hack_collections` compiler option to `legacy_arrays`. 1. Add `hack_collections` compiler option. 1. Remove references to the `arrays` compiler option. 1. Delete the `arrays` compiler option. 1. Remove `const_collections` if not `hack_collections`. 1. Make `const_collections` imply `hack_collections`. 1. **Make `legacy_arrays` imply `arraysets`**. Reviewed By: rmakheja Differential Revision: D67727031 fbshipit-source-id: 8db213c5ef116af96ee5a648ebb0d0f3b8280eaf --- .../src/thrift/compiler/generate/t_hack_generator.cc | 11 ++++++++--- .../thrift/compiler/test/fixtures/php-migration/cmd | 2 +- .../shape_construct_no_use_hack_collections/cmd | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/third-party/thrift/src/thrift/compiler/generate/t_hack_generator.cc b/third-party/thrift/src/thrift/compiler/generate/t_hack_generator.cc index 60b924f0cce216..2bd24abd791c73 100644 --- a/third-party/thrift/src/thrift/compiler/generate/t_hack_generator.cc +++ b/third-party/thrift/src/thrift/compiler/generate/t_hack_generator.cc @@ -116,7 +116,7 @@ class t_hack_generator : public t_concat_generator { json_ = option_is_specified(options, "json"); phps_ = option_is_specified(options, "server"); strict_types_ = option_is_specified(options, "stricttypes"); - arraysets_ = option_is_specified(options, "arraysets"); + auto explicit_arraysets = option_is_specified(options, "arraysets"); no_nullables_ = option_is_specified(options, "nonullables"); from_map_construct_ = option_is_specified(options, "frommap_construct"); shapes_ = option_is_specified(options, "shapes"); @@ -144,16 +144,21 @@ class t_hack_generator : public t_concat_generator { ns_type_ == HackThriftNamespaceType::PACKAGE; has_nested_ns = false; + if (legacy_arrays_ && explicit_arraysets) { + throw std::runtime_error( + "Don't use arraysets with legacy_arrays, because legacy_arrays implies arraysets."); + } + arraysets_ = explicit_arraysets || legacy_arrays_; + if (const_collections_ && explicit_hack_collections) { throw std::runtime_error( "Don't use hack_collections with const_collections, because const_collections implies hack_collections."); } hack_collections_ = explicit_hack_collections || const_collections_; + // legacy_arrays_ is only used to migrate away from php gen if (legacy_arrays_ && strict_types_) { throw std::runtime_error("Don't use legacy_arrays with strict_types"); - } else if (legacy_arrays_ && !arraysets_) { - throw std::runtime_error("Don't use legacy_arrays without arraysets"); } else if (arraysets_ && !(legacy_arrays_ || hack_collections_)) { throw std::runtime_error( "Don't use arraysets without either legacy_arrays or hack_collections"); diff --git a/third-party/thrift/src/thrift/compiler/test/fixtures/php-migration/cmd b/third-party/thrift/src/thrift/compiler/test/fixtures/php-migration/cmd index 0b47735e356e77..b08bc9fd402917 100644 --- a/third-party/thrift/src/thrift/compiler/test/fixtures/php-migration/cmd +++ b/third-party/thrift/src/thrift/compiler/test/fixtures/php-migration/cmd @@ -1 +1 @@ -hack: hack:array_migration=1,arraysets=1,frommap_construct=1,legacy_arrays=1,nullable_everything=1,server=1 src/module.thrift +hack: hack:array_migration=1,frommap_construct=1,legacy_arrays=1,nullable_everything=1,server=1 src/module.thrift diff --git a/third-party/thrift/src/thrift/compiler/test/fixtures/shape_construct_no_use_hack_collections/cmd b/third-party/thrift/src/thrift/compiler/test/fixtures/shape_construct_no_use_hack_collections/cmd index 156801d3f4adba..bce409889cf42a 100644 --- a/third-party/thrift/src/thrift/compiler/test/fixtures/shape_construct_no_use_hack_collections/cmd +++ b/third-party/thrift/src/thrift/compiler/test/fixtures/shape_construct_no_use_hack_collections/cmd @@ -1 +1 @@ -hack: hack:shape_construct=1,arraysets=1,shapes=1,legacy_arrays=1,shapes_use_pipe_structure=1 src/module.thrift +hack: hack:shape_construct=1,shapes=1,legacy_arrays=1,shapes_use_pipe_structure=1 src/module.thrift