Remove support for unless blocks

Summary: Now that the `not` helper is available we have two ways to express the same construct, and we only want to keep the more flexible one.

Reviewed By: praihan

Differential Revision: D67060739

fbshipit-source-id: 52892f1e3ac36e3d0201e602263702b62f5ff5ce

third-party/thrift/src/thrift/compiler/whisker/ast.h

@@ -152,19 +152,13 @@ struct section_block {
 };
 
 /**
- * A Whisker construct for conditionals. A conditional_block represents one of
- * two (very similar) block types: if-block and unless-block.
+ * A Whisker construct for conditionals, i.e. the if-block.
  * This matches Handlebars:
  *   https://handlebarsjs.com/guide/builtin-helpers.html#if
- *   https://handlebarsjs.com/guide/builtin-helpers.html#unless
  */
 struct conditional_block {
   source_range loc;
-  /**
-   * {{#if     ⇒ unless == false
-   * {{#unless ⇒ unless == true
-   */
-  bool unless;
+
   expression condition;
   bodies body_elements;
 

third-party/thrift/src/thrift/compiler/whisker/parser.cc

@@ -1016,13 +1016,12 @@ class parser {
   // conditional-block →
   //   { cond-block-open ~ body* ~ else-block? ~ cond-block-close }
   // cond-block-open →
-  //   { "{{" ~ "#" ~ ("if" | "unless") ~ expression ~ "}}" }
+  //   { "{{" ~ "#" ~ "if" ~ expression ~ "}}" }
   // else-block → { "{{" ~ "#" ~ "else" ~ "}}" ~ body* }
   // cond-block-close →
-  //   { "{{" ~ "/" ~ ("if" | "unless") ~ expression ~ "}}" }
+  //   { "{{" ~ "/" ~ "if" ~ expression ~ "}}" }
   //
-  // NOTE: the "if" or "unless"  and the expression must match between
-  // open and close
+  // NOTE: the expression must match between open and close
   parse_result<ast::conditional_block> parse_conditional_block(
       parser_scan_window scan) {
     assert(scan.empty());
@@ -1032,27 +1031,18 @@ class parser {
           try_consume_token(&scan, tok::pound))) {
       return no_parse_result();
     }
-    bool inverted;
-    if (try_consume_token(&scan, tok::kw_if)) {
-      inverted = false;
-    } else if (try_consume_token(&scan, tok::kw_unless)) {
-      inverted = true;
-    } else {
+    if (!try_consume_token(&scan, tok::kw_if)) {
       return no_parse_result();
     }
     scan = scan.make_fresh();
 
-    const std::string_view block_name = inverted ? "unless" : "if";
-
     parse_result condition = parse_expression(scan);
     if (!condition.has_value()) {
-      report_expected(
-          scan, fmt::format("expression to open {}-block", block_name));
+      report_expected(scan, fmt::format("expression to open if-block"));
     }
     ast::expression open = std::move(condition).consume_and_advance(&scan);
     if (!try_consume_token(&scan, tok::close)) {
-      report_expected(
-          scan, fmt::format("{} to open {}-block", tok::close, block_name));
+      report_expected(scan, fmt::format("{} to open if-block", tok::close));
     }
     scan = scan.make_fresh();
 
@@ -1080,25 +1070,18 @@ class parser {
       if (!try_consume_token(&scan, kind)) {
         report_expected(
             scan,
-            fmt::format(
-                "{} to close {}-block '{}'",
-                kind,
-                block_name,
-                open.to_string()));
+            fmt::format("{} to close if-block '{}'", kind, open.to_string()));
       }
     };
 
     expect_on_close(tok::open);
     expect_on_close(tok::slash);
-    expect_on_close(inverted ? tok::kw_unless : tok::kw_if);
+    expect_on_close(tok::kw_if);
     condition = parse_expression(scan.make_fresh());
     if (!condition.has_value()) {
       report_expected(
           scan,
-          fmt::format(
-              "expression to close {}-block '{}'",
-              block_name,
-              open.to_string()));
+          fmt::format("expression to close if-block '{}'", open.to_string()));
     }
     ast::expression close = {std::move(condition).consume_and_advance(&scan)};
     if (close.to_string() != open.to_string()) {
@@ -1114,7 +1097,6 @@ class parser {
     return {
         ast::conditional_block{
             scan.with_start(scan_start).range(),
-            inverted,
             std::move(open),
             std::move(bodies),
             std::move(else_block),

third-party/thrift/src/thrift/compiler/whisker/print_ast.cc

@@ -80,10 +80,7 @@ struct ast_visitor {
   void visit(
       const ast::conditional_block& conditional_block,
       tree_printer::scope scope) const {
-    scope.println(
-        " {}-block {}",
-        conditional_block.unless ? "unless" : "if",
-        location(conditional_block.loc));
+    scope.println(" if-block {}", location(conditional_block.loc));
     visit(conditional_block.condition, scope.open_property());
     visit(conditional_block.body_elements, scope.open_node());
 

third-party/thrift/src/thrift/compiler/whisker/render.cc

@@ -524,7 +524,7 @@ class render_engine {
     };
 
     const bool condition = evaluate_as_bool(conditional_block.condition);
-    if (condition ^ conditional_block.unless) {
+    if (condition) {
       do_visit(conditional_block.body_elements);
     } else if (conditional_block.else_clause.has_value()) {
       do_visit(conditional_block.else_clause->body_elements);

third-party/thrift/src/thrift/compiler/whisker/test/parser_test.cc

@@ -313,19 +313,19 @@ TEST_F(ParserTest, basic_if_else) {
 
 TEST_F(ParserTest, unless_block_repeated_else) {
   auto ast = parse_ast(
-      "{{#unless news.has-update?}}\n"
+      "{{#if (not news.has-update?)}}\n"
       "  Stuff is {{foo}} happening!\n"
       "{{#else}}\n"
       "  Nothing is happening!\n"
       "{{#else}}\n"
       "  Nothing is happening!\n"
-      "{{/unless news.has-update?}}");
+      "{{/if (not news.has-update?)}}");
   EXPECT_FALSE(ast.has_value());
   EXPECT_THAT(
       diagnostics,
       testing::ElementsAre(diagnostic(
           diagnostic_level::error,
-          "expected `/` to close unless-block 'news.has-update?' but found `#`",
+          "expected `/` to close if-block '(not news.has-update?)' but found `#`",
           path_to_file(1),
           5)));
 }
@@ -423,17 +423,17 @@ TEST_F(ParserTest, if_close_by_itself) {
 
 TEST_F(ParserTest, conditional_block_mismatched_open_and_close) {
   auto ast = parse_ast(
-      "{{#unless news.has-update?}}\n"
+      "{{#if (not news.has-update?)}}\n"
       "  Stuff is happening!\n"
       "{{#else}}\n"
       "  Nothing is happening!\n"
-      "{{/if}}");
+      "{{/unless}}");
   EXPECT_FALSE(ast.has_value());
   EXPECT_THAT(
       diagnostics,
       testing::ElementsAre(diagnostic(
           diagnostic_level::error,
-          "expected `unless` to close unless-block 'news.has-update?' but found `if`",
+          "expected `if` to close if-block '(not news.has-update?)' but found `unless`",
           path_to_file(1),
           5)));
 }

third-party/thrift/src/thrift/compiler/whisker/test/render_test.cc

@@ -507,17 +507,17 @@ TEST_F(RenderTest, if_block) {
 TEST_F(RenderTest, unless_block) {
   {
     auto result = render(
-        "{{#unless news.has-update?}}\n"
+        "{{#if (not news.has-update?)}}\n"
         "Stuff is {{foo}} happening!\n"
-        "{{/unless news.has-update?}}\n",
+        "{{/if (not news.has-update?)}}\n",
         w::map(
             {{"news", w::map({{"has-update?", w::boolean(false)}})},
              {"foo", w::string("now")}}));
     EXPECT_EQ(*result, "Stuff is now happening!\n");
   }
   {
     auto result = render(
-        "{{#unless news.has-update?}}Stuff is {{foo}} happening!{{/unless news.has-update?}}",
+        "{{#if (not news.has-update?)}}Stuff is {{foo}} happening!{{/if (not news.has-update?)}}",
         w::map({{"news", w::map({{"has-update?", w::boolean(true)}})}}));
     EXPECT_EQ(*result, "");
   }
@@ -551,21 +551,21 @@ TEST_F(RenderTest, if_else_block) {
 TEST_F(RenderTest, unless_else_block) {
   {
     auto result = render(
-        "{{#unless news.has-update?}}\n"
+        "{{#if (not news.has-update?)}}\n"
         "Nothing is happening!\n"
         "{{#else}}\n"
         "Stuff is {{foo}} happening!\n"
-        "{{/unless news.has-update?}}\n",
+        "{{/if (not news.has-update?)}}\n",
         w::map({{"news", w::map({{"has-update?", w::boolean(false)}})}}));
     EXPECT_EQ(*result, "Nothing is happening!\n");
   }
   {
     auto result = render(
-        "{{#unless news.has-update?}}\n"
+        "{{#if (not news.has-update?)}}\n"
         "Nothing is happening!\n"
         "{{#else}}\n"
         "Stuff is {{foo}} happening!\n"
-        "{{/unless news.has-update?}}\n",
+        "{{/if (not news.has-update?)}}\n",
         w::map(
             {{"news", w::map({{"has-update?", w::boolean(true)}})},
              {"foo", w::string("now")}}));

third-party/thrift/src/thrift/doc/contributions/whisker.md

@@ -284,26 +284,6 @@ I don't know who you are.
 
 </Example>
 
-:::note
-`{{#unless}}` blocks are functionally equivalent to `{{#if}}` with a negated condition. However, they are **deprecated** as their behavior can be replicated using `{{#if}}` with the `not` function.
-:::
-
-<Example title={<>Example of <code>unless</code> being redundant</>}>
-
-```handlebars title=example.whisker
-{{! These two blocks have identical behavior }}
-
-{{#unless failed?}}
-Nice!
-{{/unless failed?}}
-
-{{#if (not failed?)}}
-Nice!
-{{/if (not failed?)}}
-```
-
-</Example>
-
 <Grammar>
 
 ```