[skip ci] Do not consume component value on visitor failure (#48985)

Summary:

This reverts some of the behavior I added in D68357624, since peeking a component value is non-obviously more expensive than manually copying the parser, and needing to peek will be a pain for flat lists of values (like for box-shadow).

Changelog: [internal]

Reviewed By: lenaic

Differential Revision: D68733518

Author: NickGerleman

packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h

@@ -35,14 +35,21 @@ struct CSSSimpleBlock {
   CSSTokenType openBracketType{};
 };
 
+/**
+ * Describes a valid return type for a CSSSyntaxParser visitor
+ */
+template <typename T>
+concept CSSSyntaxVisitorReturn =
+    std::is_default_constructible_v<T> && std::equality_comparable<T>;
+
 /**
  * A CSSFunctionVisitor is called to start parsing a function component value.
  * At this point, the Parser is positioned at the start of the function
  * component value list. It is expected that the visitor finishes before the end
  * of the function block.
  */
 template <typename T, typename ReturnT>
-concept CSSFunctionVisitor =
+concept CSSFunctionVisitor = CSSSyntaxVisitorReturn<ReturnT> &&
     requires(T visitor, CSSFunctionBlock func, CSSSyntaxParser& blockParser) {
       { visitor(func, blockParser) } -> std::convertible_to<ReturnT>;
     };
@@ -52,7 +59,7 @@ concept CSSFunctionVisitor =
  * component value.
  */
 template <typename T, typename ReturnT>
-concept CSSPreservedTokenVisitor =
+concept CSSPreservedTokenVisitor = CSSSyntaxVisitorReturn<ReturnT> &&
     requires(T visitor, CSSPreservedToken token) {
       { visitor(token) } -> std::convertible_to<ReturnT>;
     };
@@ -63,7 +70,7 @@ concept CSSPreservedTokenVisitor =
  * of the block.
  */
 template <typename T, typename ReturnT>
-concept CSSSimpleBlockVisitor =
+concept CSSSimpleBlockVisitor = CSSSyntaxVisitorReturn<ReturnT> &&
     requires(T visitor, CSSSimpleBlock block, CSSSyntaxParser& blockParser) {
       { visitor(block, blockParser) } -> std::convertible_to<ReturnT>;
     };
@@ -72,16 +79,17 @@ concept CSSSimpleBlockVisitor =
  * Any visitor for a component value.
  */
 template <typename T, typename ReturnT>
-concept CSSComponentValueVisitor = CSSFunctionVisitor<T, ReturnT> ||
-    CSSPreservedTokenVisitor<T, ReturnT> || CSSSimpleBlockVisitor<T, ReturnT>;
+concept CSSComponentValueVisitor = CSSSyntaxVisitorReturn<ReturnT> &&
+    (CSSFunctionVisitor<T, ReturnT> || CSSPreservedTokenVisitor<T, ReturnT> ||
+     CSSSimpleBlockVisitor<T, ReturnT>);
 
 /**
  * Represents a variadic set of CSSComponentValueVisitor with no more than one
  * of a specific type of visitor.
  */
 template <typename ReturnT, typename... VisitorsT>
-concept CSSUniqueComponentValueVisitors =
-    (CSSComponentValueVisitor<VisitorsT, ReturnT> && ... && true) &&
+concept CSSUniqueComponentValueVisitors = CSSSyntaxVisitorReturn<ReturnT> &&
+    (CSSComponentValueVisitor<VisitorsT, ReturnT> && ...) &&
     ((CSSFunctionVisitor<VisitorsT, ReturnT> ? 1 : 0) + ... + 0) <= 1 &&
     ((CSSPreservedTokenVisitor<VisitorsT, ReturnT> ? 1 : 0) + ... + 0) <= 1 &&
     ((CSSSimpleBlockVisitor<VisitorsT, ReturnT> ? 1 : 0) + ... + 0) <= 1;
@@ -106,7 +114,9 @@ enum class CSSDelimiter {
  * https://www.w3.org/TR/css-syntax-3/#component-value
  */
 class CSSSyntaxParser {
-  template <typename ReturnT, CSSComponentValueVisitor<ReturnT>... VisitorsT>
+  template <
+      CSSSyntaxVisitorReturn ReturnT,
+      CSSComponentValueVisitor<ReturnT>... VisitorsT>
   friend struct CSSComponentValueVisitorDispatcher;
 
  public:
@@ -130,6 +140,11 @@ class CSSSyntaxParser {
    * higher-level data structure, and continue parsing within its scope using
    * the same underlying CSSSyntaxParser.
    *
+   * The state of the parser is reset if a visitor returns a default-constructed
+   * value for the given return type, even if it previously advanced the parser.
+   * If no visitor returns a non-default-constructed value, the component value
+   * will not be consumed.
+   *
    * https://www.w3.org/TR/css-syntax-3/#consume-component-value
    *
    * @param <ReturnT> caller-specified return type of visitors. This type will
@@ -142,47 +157,16 @@ class CSSSyntaxParser {
    * @returns the visitor returned value, or a default constructed value if no
    * visitor was matched, or a syntax error occurred.
    */
-  template <typename ReturnT = std::nullptr_t>
+  template <CSSSyntaxVisitorReturn ReturnT>
   constexpr ReturnT consumeComponentValue(
       CSSDelimiter delimiter,
       const CSSComponentValueVisitor<ReturnT> auto&... visitors)
     requires(CSSUniqueComponentValueVisitors<ReturnT, decltype(visitors)...>);
 
-  template <typename ReturnT = std::nullptr_t>
+  template <CSSSyntaxVisitorReturn ReturnT>
   constexpr ReturnT consumeComponentValue(
       const CSSComponentValueVisitor<ReturnT> auto&... visitors)
     requires(CSSUniqueComponentValueVisitors<ReturnT, decltype(visitors)...>);
-
-  /**
-   * Peek at the next component value without consuming it. The component value
-   * is provided to a passed in "visitor", typically a lambda which accepts the
-   * component value in a new scope. The visitor may read this component
-   * parameter into a higher-level data structure, and continue parsing within
-   * its scope using the same underlying CSSSyntaxParser.
-   *
-   * https://www.w3.org/TR/css-syntax-3/#consume-component-value
-   *
-   * @param <ReturnT> caller-specified return type of visitors. This type will
-   * be set to its default constructed state if consuming a component value with
-   * no matching visitors, or syntax error
-   * @param visitors A unique list of CSSComponentValueVisitor to be called on a
-   * match
-   * @param delimiter The expected delimeter to occur before the next component
-   * value
-   * @returns the visitor returned value, or a default constructed value if no
-   * visitor was matched, or a syntax error occurred.
-   */
-  template <typename ReturnT = std::nullptr_t>
-  constexpr ReturnT peekComponentValue(
-      CSSDelimiter delimiter,
-      const CSSComponentValueVisitor<ReturnT> auto&... visitors)
-    requires(CSSUniqueComponentValueVisitors<ReturnT, decltype(visitors)...>);
-
-  template <typename ReturnT = std::nullptr_t>
-  constexpr ReturnT peekComponentValue(
-      const CSSComponentValueVisitor<ReturnT> auto&... visitors)
-    requires(CSSUniqueComponentValueVisitors<ReturnT, decltype(visitors)...>);
-
   /**
    * The parser is considered finished when there are no more remaining tokens
    * to be processed
@@ -226,7 +210,9 @@ class CSSSyntaxParser {
   CSSTokenType terminator_{CSSTokenType::EndOfFile};
 };
 
-template <typename ReturnT, CSSComponentValueVisitor<ReturnT>... VisitorsT>
+template <
+    CSSSyntaxVisitorReturn ReturnT,
+    CSSComponentValueVisitor<ReturnT>... VisitorsT>
 struct CSSComponentValueVisitorDispatcher {
   CSSSyntaxParser& parser;
 
@@ -275,6 +261,7 @@ struct CSSComponentValueVisitorDispatcher {
         break;
     }
 
+    parser = originalParser;
     return ReturnT{};
   }
 
@@ -331,15 +318,6 @@ struct CSSComponentValueVisitorDispatcher {
     return false;
   }
 
-  constexpr ReturnT peekComponentValue(
-      CSSDelimiter delimiter,
-      const VisitorsT&... visitors) {
-    auto originalParser = parser;
-    auto ret = consumeComponentValue(delimiter, visitors...);
-    parser = originalParser;
-    return ret;
-  }
-
   constexpr std::optional<ReturnT> visitFunction(
       const CSSComponentValueVisitor<ReturnT> auto& visitor,
       const CSSComponentValueVisitor<ReturnT> auto&... rest) {
@@ -357,7 +335,8 @@ struct CSSComponentValueVisitorDispatcher {
       auto functionValue = visitor({name}, blockParser);
       parser.advanceToBlockParser(blockParser);
       parser.consumeWhitespace();
-      if (parser.peek().type() == CSSTokenType::CloseParen) {
+      if (parser.peek().type() == CSSTokenType::CloseParen &&
+          functionValue != ReturnT{}) {
         parser.consumeToken();
         return functionValue;
       }
@@ -369,11 +348,6 @@ struct CSSComponentValueVisitorDispatcher {
   }
 
   constexpr std::optional<ReturnT> visitFunction() {
-    while (parser.peek().type() != CSSTokenType::CloseParen) {
-      parser.consumeToken();
-    }
-    parser.consumeToken();
-
     return {};
   }
 
@@ -388,7 +362,7 @@ struct CSSComponentValueVisitorDispatcher {
       auto blockValue = visitor({openBracketType}, blockParser);
       parser.advanceToBlockParser(blockParser);
       parser.consumeWhitespace();
-      if (parser.peek().type() == endToken) {
+      if (parser.peek().type() == endToken && blockValue != ReturnT{}) {
         parser.consumeToken();
         return blockValue;
       }
@@ -399,29 +373,27 @@ struct CSSComponentValueVisitorDispatcher {
   }
 
   constexpr std::optional<ReturnT> visitSimpleBlock(CSSTokenType endToken) {
-    while (parser.peek().type() != endToken) {
-      parser.consumeToken();
-    }
-    parser.consumeToken();
     return {};
   }
 
   constexpr std::optional<ReturnT> visitPreservedToken(
       const CSSComponentValueVisitor<ReturnT> auto& visitor,
       const CSSComponentValueVisitor<ReturnT> auto&... rest) {
     if constexpr (CSSPreservedTokenVisitor<decltype(visitor), ReturnT>) {
-      return visitor(parser.consumeToken());
+      auto ret = visitor(parser.consumeToken());
+      if (ret != ReturnT{}) {
+        return ret;
+      }
     }
     return visitPreservedToken(rest...);
   }
 
   constexpr std::optional<ReturnT> visitPreservedToken() {
-    parser.consumeToken();
     return {};
   }
 };
 
-template <typename ReturnT>
+template <CSSSyntaxVisitorReturn ReturnT>
 constexpr ReturnT CSSSyntaxParser::consumeComponentValue(
     CSSDelimiter delimiter,
     const CSSComponentValueVisitor<ReturnT> auto&... visitors)
@@ -432,31 +404,12 @@ constexpr ReturnT CSSSyntaxParser::consumeComponentValue(
       .consumeComponentValue(delimiter, visitors...);
 }
 
-template <typename ReturnT>
+template <CSSSyntaxVisitorReturn ReturnT>
 constexpr ReturnT CSSSyntaxParser::consumeComponentValue(
     const CSSComponentValueVisitor<ReturnT> auto&... visitors)
   requires(CSSUniqueComponentValueVisitors<ReturnT, decltype(visitors)...>)
 {
   return consumeComponentValue<ReturnT>(CSSDelimiter::None, visitors...);
 }
 
-template <typename ReturnT>
-constexpr ReturnT CSSSyntaxParser::peekComponentValue(
-    CSSDelimiter delimiter,
-    const CSSComponentValueVisitor<ReturnT> auto&... visitors)
-  requires(CSSUniqueComponentValueVisitors<ReturnT, decltype(visitors)...>)
-{
-  return CSSComponentValueVisitorDispatcher<ReturnT, decltype(visitors)...>{
-      *this}
-      .peekComponentValue(delimiter, visitors...);
-}
-
-template <typename ReturnT>
-constexpr ReturnT CSSSyntaxParser::peekComponentValue(
-    const CSSComponentValueVisitor<ReturnT> auto&... visitors)
-  requires(CSSUniqueComponentValueVisitors<ReturnT, decltype(visitors)...>)
-{
-  return peekComponentValue<ReturnT>(CSSDelimiter::None, visitors...);
-}
-
 } // namespace facebook::react

packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp

@@ -382,88 +382,6 @@ TEST(CSSSyntaxParser, unconsumed_simple_block_args) {
   EXPECT_EQ(funcValue, std::nullopt);
 }
 
-TEST(CSSSyntaxParser, peek_does_not_consume) {
-  CSSSyntaxParser parser{"foo()"};
-  auto funcValue = parser.peekComponentValue<std::optional<std::string_view>>(
-      [&](const CSSFunctionBlock& function, CSSSyntaxParser& /*blockParser*/) {
-        EXPECT_EQ(function.name, "foo");
-        return function.name;
-      });
-
-  EXPECT_EQ(funcValue, "foo");
-
-  auto funcValue2 = parser.peekComponentValue<std::optional<std::string_view>>(
-      [&](const CSSFunctionBlock& function, CSSSyntaxParser& /*blockParser*/) {
-        EXPECT_EQ(function.name, "foo");
-        return function.name;
-      });
-
-  EXPECT_EQ(funcValue2, "foo");
-
-  auto funcValue3 =
-      parser.consumeComponentValue<std::optional<std::string_view>>(
-          [&](const CSSFunctionBlock& function,
-              CSSSyntaxParser& /*blockParser*/) {
-            EXPECT_EQ(function.name, "foo");
-            return function.name;
-          });
-
-  EXPECT_EQ(funcValue3, "foo");
-
-  auto funcValue4 = parser.peekComponentValue<std::optional<std::string_view>>(
-      [&](const CSSFunctionBlock& function, CSSSyntaxParser& /*blockParser*/) {
-        EXPECT_EQ(function.name, "foo");
-        return function.name;
-      });
-
-  EXPECT_EQ(funcValue4, std::nullopt);
-}
-
-TEST(CSSSyntaxParser, preserved_token_without_visitor_consumed) {
-  CSSSyntaxParser parser{"foo bar"};
-
-  parser.consumeComponentValue();
-
-  auto identValue = parser.consumeComponentValue<std::string_view>(
-      CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) {
-        EXPECT_EQ(token.type(), CSSTokenType::Ident);
-        EXPECT_EQ(token.stringValue(), "bar");
-        return token.stringValue();
-      });
-
-  EXPECT_EQ(identValue, "bar");
-}
-
-TEST(CSSSyntaxParser, function_without_visitor_consumed) {
-  CSSSyntaxParser parser{"foo(a, b, c) bar"};
-
-  parser.consumeComponentValue();
-
-  auto identValue = parser.consumeComponentValue<std::string_view>(
-      CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) {
-        EXPECT_EQ(token.type(), CSSTokenType::Ident);
-        EXPECT_EQ(token.stringValue(), "bar");
-        return token.stringValue();
-      });
-
-  EXPECT_EQ(identValue, "bar");
-}
-
-TEST(CSSSyntaxParser, simple_block_without_visitor_consumed) {
-  CSSSyntaxParser parser{"{a foo(abc)} bar"};
-
-  parser.consumeComponentValue();
-
-  auto identValue = parser.consumeComponentValue<std::string_view>(
-      CSSDelimiter::Whitespace, [](const CSSPreservedToken& token) {
-        EXPECT_EQ(token.type(), CSSTokenType::Ident);
-        EXPECT_EQ(token.stringValue(), "bar");
-        return token.stringValue();
-      });
-
-  EXPECT_EQ(identValue, "bar");
-}
-
 TEST(CSSSyntaxParser, solidus_delimiter) {
   CSSSyntaxParser parser{"foo / bar"};
 
@@ -606,4 +524,39 @@ TEST(CSSSyntaxParser, delimeter_not_consumed_when_no_component_value) {
   EXPECT_TRUE(hasComma);
 }
 
+TEST(CSSSyntaxParser, component_value_not_consumed_on_visitor_failure) {
+  CSSSyntaxParser parser{"foo"};
+
+  bool visitor1Attempted = false;
+  bool visitor1Ret =
+      parser.consumeComponentValue<bool>([&](const CSSPreservedToken& token) {
+        EXPECT_EQ(token.stringValue(), "foo");
+        visitor1Attempted = true;
+        return false;
+      });
+
+  EXPECT_TRUE(visitor1Attempted);
+  EXPECT_FALSE(visitor1Ret);
+
+  bool visitor2Attempted = false;
+  parser.consumeComponentValue<bool>([&](const CSSPreservedToken& token) {
+    EXPECT_EQ(token.stringValue(), "foo");
+    visitor2Attempted = true;
+    return true;
+  });
+
+  EXPECT_TRUE(visitor2Attempted);
+  EXPECT_TRUE(visitor2Attempted);
+
+  bool visitor3Attempted = false;
+  bool visitor3Ret =
+      parser.consumeComponentValue<bool>([&](const CSSPreservedToken& token) {
+        visitor3Attempted = true;
+        return true;
+      });
+
+  EXPECT_FALSE(visitor3Attempted);
+  EXPECT_FALSE(visitor3Ret);
+}
+
 } // namespace facebook::react