Skip to content

Commit

Permalink
Remove arraysets if arrays present
Browse files Browse the repository at this point in the history
Summary:
# What?
Remove `arraysets` if `arrays` present.

# Why?
This combination can lead to runtime failures.

# 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.

# 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.

Reviewed By: rmakheja

Differential Revision: D67573143

fbshipit-source-id: cad66b814fdd3f81c2862ce11d41cc0d0d07310a
  • Loading branch information
Satish Kumar authored and facebook-github-bot committed Jan 7, 2025
1 parent 23cc005 commit 82dcc1b
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 1,644 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ class t_hack_generator : public t_concat_generator {
} else if (no_use_hack_collections_ && arrays_) {
throw std::runtime_error(
"Don't use no_use_hack_collections with arrays. Just use arrays");
} else if (arraysets_ && !(legacy_arrays_ || hack_collections_)) {
throw std::runtime_error(
"Don't use arraysets without either legacy_arrays or hack_collections");
} else if (mangled_services_ && has_hack_namespace) {
throw std::runtime_error(
"Don't use mangledsvcs with hack namespaces or package.");
Expand Down Expand Up @@ -1386,7 +1389,7 @@ void t_hack_generator::generate_json_container(
indent(out) << container << " = vec[];\n";
}
} else if (ttype->is_set()) {
if (arraysets_ && (legacy_arrays_ || hack_collections_)) {
if (arraysets_) {
indent(out) << container << " = dict[];\n";
} else if (hack_collections_) {
indent(out) << container << " = Set {};\n";
Expand Down Expand Up @@ -1435,7 +1438,7 @@ void t_hack_generator::generate_json_set_element(
t_field felem(tset->get_elem_type(), elem);
indent(out) << declare_field(&felem, true, true, true).substr(1) << "\n";
generate_json_field(out, namer, &felem, "", "", value);
if (arraysets_ && (legacy_arrays_ || hack_collections_)) {
if (arraysets_) {
indent(out) << prefix_thrift << "[" << elem << "] = true;\n";
} else if (hack_collections_) {
indent(out) << prefix_thrift << "->add(" << elem << ");\n";
Expand Down Expand Up @@ -2304,7 +2307,7 @@ std::string t_hack_generator::render_default_value(const t_type* type) {
dval = "vec[]";
}
} else if (type->is_set()) {
if (arraysets_ && (legacy_arrays_ || hack_collections_)) {
if (arraysets_) {
dval = "dict[]";
} else if (hack_collections_) {
dval = "Set {}";
Expand Down Expand Up @@ -3034,7 +3037,7 @@ void t_hack_generator::generate_php_type_spec(
}
indent(out) << "'etype' => " << type_to_enum(etype) << ",\n";
generate_php_type_spec_shape_elt_helper(out, "elem", etype, depth);
if (arraysets_ && (hack_collections_ || legacy_arrays_)) {
if (arraysets_) {
indent(out) << "'format' => 'array',\n";
} else if (hack_collections_) {
indent(out) << "'format' => 'collection',\n";
Expand Down Expand Up @@ -6684,14 +6687,7 @@ std::string t_hack_generator::type_to_typehint(
} else if (const auto* tset = dynamic_cast<const t_set*>(ttype)) {
std::string prefix;
std::string suffix = ">";
// The order of the ifs matters in the code below.
// Even though there is a small amount of duplicated code below,
// any alternative approaches to achieve the same outcome seem
// make it harder to see the order clearly.
if (arraysets_) {
prefix = "dict";
suffix = ", bool>";
} else if (!legacy_arrays_ && !hack_collections_) {
if (!legacy_arrays_ && !hack_collections_) {
prefix = "keyset";
} else if (arraysets_ || variations[TypeToTypehintVariations::IS_SHAPE]) {
prefix = "dict";
Expand Down Expand Up @@ -7440,7 +7436,7 @@ std::string t_hack_generator::declare_field(
result += " = vec[]";
}
} else if (type->is_set()) {
if (arraysets_ && (legacy_arrays_ || hack_collections_)) {
if (arraysets_) {
result += " = dict[]";
} else if (hack_collections_) {
result += " = Set {}";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
hack: hack:shapes=1,shapes_allow_unknown_fields=1,arrays=1,shapes_use_pipe_structure=1 src/module.thrift
hack_array_sets: hack:shapes=1,shapes_allow_unknown_fields=1,arrays=1,shapes_use_pipe_structure=1,arraysets=1 src/module.thrift
Loading

0 comments on commit 82dcc1b

Please sign in to comment.