Add not function

Summary:
Allows negating conditions more flexibly than `unless` blocks do (especially once `and`/`or` helpers are added). Those blocks will be phased out and replaced with this helper.
Syntax follows handlebars: https://handlebarsjs.com/guide/builtin-helpers.html#sub-expressions

Reviewed By: praihan

Differential Revision: D67060741

fbshipit-source-id: 572680e64deeb335d99edff873951074b55ac36e

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

@@ -17,6 +17,8 @@
 #include <thrift/compiler/whisker/ast.h>
 #include <thrift/compiler/whisker/detail/overload.h>
 
+#include <fmt/core.h>
+
 #include <cassert>
 #include <iterator>
 #include <utility>
@@ -64,9 +66,18 @@ std::string partial_apply::path_string() const {
 
 std::string expression::to_string() const {
   return detail::variant_match(
-      content, [](const variable_lookup& lookup) -> std::string {
-        return lookup.chain_string();
+      content,
+      [](const variable_lookup& v) { return v.chain_string(); },
+      [](const function_call& f) {
+        return detail::variant_match(f.which, [&](function_call::not_tag) {
+          return fmt::format("(not {})", f.args[0].to_string());
+        });
       });
 }
 
+std::string_view expression::function_call::name() const {
+  return detail::variant_match(
+      which, [&](function_call::not_tag) { return "not"; });
+}
+
 } // namespace whisker::ast

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

@@ -114,11 +114,17 @@ struct variable_lookup {
  */
 struct expression {
   source_range loc;
-  std::variant<variable_lookup> content;
+  struct function_call {
+    struct not_tag {};
+    std::variant<not_tag> which;
+    std::vector<expression> args;
+
+    std::string_view name() const;
+  };
+  std::variant<variable_lookup, function_call> content;
 
   std::string to_string() const;
 };
-
 /**
  * A top-level use of an expression within a template body. It is similar to
  * expression except its source_range includes the surrounding "{{ }}".

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

@@ -965,17 +965,52 @@ class parser {
         scan};
   }
 
-  // expression → { variable-lookup }
+  // expression → { variable-lookup | function-call }
   parse_result<ast::expression> parse_expression(parser_scan_window scan) {
     assert(scan.empty());
     auto scan_start = scan.start;
+    using function_call = ast::expression::function_call;
+
     if (parse_result lookup = parse_variable_lookup(scan)) {
       auto expr = std::move(lookup).consume_and_advance(&scan);
       return {
           ast::expression{scan.with_start(scan_start).range(), std::move(expr)},
           scan};
     }
-    return no_parse_result();
+
+    if (!try_consume_token(&scan, tok::l_paren)) {
+      return no_parse_result();
+    }
+
+    function_call func;
+    if (try_consume_token(&scan, tok::kw_not)) {
+      func.which = function_call::not_tag{};
+    } else {
+      report_fatal_error(scan, "unrecognized function {}", scan.peek());
+    }
+
+    while (parse_result cond = parse_expression(scan.make_fresh())) {
+      func.args.push_back(std::move(cond).consume_and_advance(&scan));
+    }
+
+    detail::variant_match(func.which, [&](function_call::not_tag) {
+      if (func.args.size() != 1) {
+        report_fatal_error(
+            scan,
+            "expected 1 argument for helper `not` but got {}",
+            func.args.size());
+      }
+    });
+
+    if (!try_consume_token(&scan, tok::r_paren)) {
+      report_fatal_error(
+          scan,
+          "expected `)` to close helper `{}` but got `{}`",
+          func.name(),
+          scan.peek());
+    }
+
+    return {{scan.with_start(scan_start).range(), std::move(func)}, scan};
   }
 
   // conditional-block →

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

@@ -338,8 +338,16 @@ class render_engine {
 
   object evaluate(const ast::expression& expr) {
     return detail::variant_match(
-        expr.content, [&](const ast::variable_lookup& variable_lookup) {
+        expr.content,
+        [&](const ast::variable_lookup& variable_lookup) {
           return lookup_variable(variable_lookup);
+        },
+        [&](const ast::expression::function_call& func) {
+          return detail::variant_match(
+              func.which, [&](ast::expression::function_call::not_tag) {
+                assert(func.args.size() == 1); // enforced by the parser
+                return object{!evaluate_as_bool(func.args[0])};
+              });
         });
   }
 
@@ -393,25 +401,36 @@ class render_engine {
    * provided value and render_options::strict_boolean_conditional.
    */
   void maybe_report_boolean_coercion(
-      const ast::variable_lookup& lookup, const object& value) {
+      const ast::expression& expr, const object& value) {
     auto diag_level = opts_.strict_boolean_conditional;
-    maybe_report(lookup.loc, diag_level, [&] {
+    maybe_report(expr.loc, diag_level, [&] {
       return fmt::format(
           "Condition '{}' is not a boolean. The encountered value is:\n{}",
-          lookup.chain_string(),
+          expr.to_string(),
           to_string(value));
     });
     if (diag_level == diagnostic_level::error) {
       // Fail rendering in strict mode
       throw abort_rendering();
     }
   }
+  bool evaluate_as_bool(const ast::expression& expr) {
+    const object result = evaluate(expr);
+    return result.visit(
+        [&](boolean value) { return value; },
+        [&](const auto& value) {
+          maybe_report_boolean_coercion(expr, result);
+          return coerce_to_boolean(value);
+        });
+  }
 
   void visit(const ast::section_block& section) {
     const object& section_variable = lookup_variable(section.variable);
 
     const auto maybe_report_coercion = [&] {
-      maybe_report_boolean_coercion(section.variable, section_variable);
+      maybe_report_boolean_coercion(
+          ast::expression{section.variable.loc, section.variable},
+          section_variable);
     };
 
     const auto do_visit = [&](const object& scope) {
@@ -498,36 +517,18 @@ class render_engine {
   }
 
   void visit(const ast::conditional_block& conditional_block) {
-    const auto& variable =
-        std::get<ast::variable_lookup>(conditional_block.condition.content);
-    const object& condition = lookup_variable(variable);
-
-    const auto maybe_report_coercion = [&] {
-      maybe_report_boolean_coercion(variable, condition);
-    };
-
-    const auto do_visit = [&](const object& scope,
-                              const ast::bodies& body_elements) {
-      eval_context_.push_scope(scope);
+    const auto do_visit = [&](const ast::bodies& body_elements) {
+      eval_context_.push_scope(whisker::make::null);
       visit(body_elements);
       eval_context_.pop_scope();
     };
 
-    const auto do_conditional_visit = [&](bool condition) {
-      if (condition ^ conditional_block.unless) {
-        do_visit(whisker::make::null, conditional_block.body_elements);
-      } else if (conditional_block.else_clause.has_value()) {
-        do_visit(
-            whisker::make::null, conditional_block.else_clause->body_elements);
-      }
-    };
-
-    condition.visit(
-        [&](boolean value) { do_conditional_visit(value); },
-        [&](const auto& value) {
-          maybe_report_coercion();
-          do_conditional_visit(coerce_to_boolean(value));
-        });
+    const bool condition = evaluate_as_bool(conditional_block.condition);
+    if (condition ^ conditional_block.unless) {
+      do_visit(conditional_block.body_elements);
+    } else if (conditional_block.else_clause.has_value()) {
+      do_visit(conditional_block.else_clause->body_elements);
+    }
   }
 
   void visit(const ast::partial_apply& partial_apply) {

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

@@ -472,6 +472,36 @@ TEST_F(ParserTest, conditional_block_mismatched_lookup) {
           5)));
 }
 
+TEST_F(ParserTest, conditional_block_with_not) {
+  auto ast = parse_ast(
+      "{{#if (not news.has-update?)}}\n"
+      "  Stuff is happening!\n"
+      "{{/if (not news.has-update?)}}");
+  EXPECT_THAT(diagnostics, testing::IsEmpty());
+  EXPECT_EQ(
+      to_string(*ast),
+      "root [path/to/test-1.whisker]\n"
+      "|- if-block <line:1:1, line:3:31>\n"
+      "| `- expression <line:1:7, col:29> '(not news.has-update?)'\n"
+      "| |- text <line:2:1, col:22> '  Stuff is happening!'\n"
+      "| |- newline <line:2:22, line:3:1> '\\n'\n");
+}
+
+TEST_F(ParserTest, conditional_block_not_wrong_arity) {
+  auto ast = parse_ast(
+      "{{#if (not news.has_updates? news.has_updates?)}}\n"
+      "  Stuff is happening!\n"
+      "{{/if (not news.has_updates? news.has_updates?)}}");
+  EXPECT_FALSE(ast.has_value());
+  EXPECT_THAT(
+      diagnostics,
+      testing::ElementsAre(diagnostic(
+          diagnostic_level::error,
+          "expected 1 argument for helper `not` but got 2",
+          path_to_file(1),
+          1)));
+}
+
 TEST_F(ParserTest, basic_partial_apply) {
   auto ast = parse_ast("{{> path / to / file }}");
   EXPECT_EQ(

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

@@ -573,6 +573,33 @@ TEST_F(RenderTest, unless_else_block) {
   }
 }
 
+TEST_F(RenderTest, if_not_else_block) {
+  {
+    auto result = render(
+        "{{#if (not news.has-update?)}}\n"
+        "Nothing is happening!\n"
+        "{{#else}}\n"
+        "Stuff is {{foo}} happening!\n"
+        "{{/if (not news.has-update?)}}\n",
+        w::map({{"news", w::map({{"has-update?", w::boolean(false)}})}}));
+    EXPECT_THAT(diagnostics(), testing::IsEmpty());
+    EXPECT_EQ(*result, "Nothing is happening!\n");
+  }
+  {
+    auto result = render(
+        "{{#if (not news.has-update?)}}\n"
+        "Nothing is happening!\n"
+        "{{#else}}\n"
+        "Stuff is {{foo}} happening!\n"
+        "{{/if (not news.has-update?)}}\n",
+        w::map(
+            {{"news", w::map({{"has-update?", w::boolean(true)}})},
+             {"foo", w::string("now")}}));
+    EXPECT_THAT(diagnostics(), testing::IsEmpty());
+    EXPECT_EQ(*result, "Stuff is now happening!\n");
+  }
+}
+
 TEST_F(RenderTest, printable_types_strict_failure) {
   {
     auto result = render(

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

@@ -177,7 +177,7 @@ There are three types of `expression`s in Whisker:
   * `{{ (concat (uppercase person.firstName) " " (uppercase person.lastName)) }}`
 
 :::warning
-Function call expressions have not been implemented yet.
+Function call expressions are only partially implemented.
 :::
 
 :::note
@@ -194,7 +194,7 @@ expression → { literal | variable | function-call }
 
 literal             → { string-literal | i64-literal | boolean-literal | null-literal }
 variable            → { "." | (identifier ~ ("." ~ identifier)*) }
-function-call       → { "(" ~ function-lookup ~ expression* ~ ")" }
+function-call       → { "(" ~ ((variable ~ expression*) | builtin-call) ~ ")" }
 
 string-literal  → { <see above> }
 i64-literal     → { <see above> }
@@ -228,7 +228,7 @@ keyword → {
 id_prefix → { alpha |        | '_' | '$' }
 id_suffix → { alpha | digits | '_' | '$' | '-' | '+' | ':' | '?' | '/' }
 
-function-lookup → { variable | "and" | "or" | "not" }
+builtin-call → { ("not" ~ expression) | (("and" | "or") ~ expression ~ expression ~ expression*) }
 ```
 
 </Grammar>