From 212fdf8e514f0e1e9295f6d29ec945d4230a3de7 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Tue, 21 Jan 2025 14:12:58 -0800 Subject: [PATCH] Simplify "rollback" on optional or invalid syntax (#48825) Summary: Right now we preserve the state of the CSSSyntaxParser across multiple data type parse attempts, so long that a data type parser consumes an additional component value. This requires data type parsers to be careful to not consume additional forward tokens if it may lead to parse error. We can make this model a lot simpler by instead resetting the parser to original state on data type parse error. We also introduce `peekComponentValue`, and visitor-less `consumeComponentValue` as a convenience, to allow data type parsers to view future component values without advancing, even if the data type parser does return a value, without needing to manually clone the parser. Changelog: [Internal] Reviewed By: lenaic Differential Revision: D68357624 --- .../ReactCommon/react/renderer/css/CSSRatio.h | 10 +-- .../react/renderer/css/CSSSyntaxParser.h | 78 +++++++++++++++-- .../react/renderer/css/CSSValueParser.h | 6 ++ .../css/tests/CSSSyntaxParserTest.cpp | 85 +++++++++++++++++++ 4 files changed, 168 insertions(+), 11 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h b/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h index f6ed8d1ea5d2..567c2d7468b3 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSRatio.h @@ -35,9 +35,7 @@ struct CSSDataTypeParser { if (isValidRatioPart(token.numericValue())) { float numerator = token.numericValue(); - CSSSyntaxParser lookaheadParser{parser}; - - auto hasSolidus = lookaheadParser.consumeComponentValue( + auto hasSolidus = parser.peekComponentValue( CSSComponentValueDelimiter::Whitespace, [&](const CSSPreservedToken& token) { return token.type() == CSSTokenType::Delim && @@ -45,16 +43,16 @@ struct CSSDataTypeParser { }); if (!hasSolidus) { - parser = lookaheadParser; return CSSRatio{numerator, 1.0f}; } + parser.consumeComponentValue(CSSComponentValueDelimiter::Whitespace); + auto denominator = parseNextCSSValue( - lookaheadParser, CSSComponentValueDelimiter::Whitespace); + parser, CSSComponentValueDelimiter::Whitespace); if (std::holds_alternative(denominator) && isValidRatioPart(std::get(denominator).value)) { - parser = lookaheadParser; return CSSRatio{numerator, std::get(denominator).value}; } } diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h index 1dbc7dea4fc4..d3b75265aaa0 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSSyntaxParser.h @@ -9,7 +9,6 @@ #include #include -#include #include @@ -82,7 +81,7 @@ concept CSSComponentValueVisitor = CSSFunctionVisitor || */ template concept CSSUniqueComponentValueVisitors = - (CSSComponentValueVisitor && ...) && + (CSSComponentValueVisitor && ... && true) && ((CSSFunctionVisitor ? 1 : 0) + ... + 0) <= 1 && ((CSSPreservedTokenVisitor ? 1 : 0) + ... + 0) <= 1 && ((CSSSimpleBlockVisitor ? 1 : 0) + ... + 0) <= 1; @@ -140,17 +139,47 @@ class CSSSyntaxParser { * @returns the visitor returned value, or a default constructed value if no * visitor was matched, or a syntax error occurred. */ - template + template constexpr ReturnT consumeComponentValue( CSSComponentValueDelimiter delimiter, const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors); - template + template constexpr ReturnT consumeComponentValue( const CSSComponentValueVisitor auto&... visitors) requires(CSSUniqueComponentValueVisitors); + /** + * 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 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 + constexpr ReturnT peekComponentValue( + CSSComponentValueDelimiter delimiter, + const CSSComponentValueVisitor auto&... visitors) + requires(CSSUniqueComponentValueVisitors); + + template + constexpr ReturnT peekComponentValue( + const CSSComponentValueVisitor auto&... visitors) + requires(CSSUniqueComponentValueVisitors); + /** * The parser is considered finished when there are no more remaining tokens * to be processed @@ -262,6 +291,15 @@ struct CSSComponentValueVisitorDispatcher { return ReturnT{}; } + constexpr ReturnT peekComponentValue( + CSSComponentValueDelimiter delimiter, + const VisitorsT&... visitors) { + auto originalParser = parser; + auto ret = consumeComponentValue(delimiter, visitors...); + parser = originalParser; + return ret; + } + constexpr std::optional visitFunction( const CSSComponentValueVisitor auto& visitor, const CSSComponentValueVisitor auto&... rest) { @@ -291,6 +329,11 @@ struct CSSComponentValueVisitorDispatcher { } constexpr std::optional visitFunction() { + while (parser.peek().type() != CSSTokenType::CloseParen) { + parser.consumeToken(); + } + parser.consumeToken(); + return {}; } @@ -315,7 +358,11 @@ struct CSSComponentValueVisitorDispatcher { return visitSimpleBlock(endToken, rest...); } - constexpr std::optional visitSimpleBlock(CSSTokenType /*endToken*/) { + constexpr std::optional visitSimpleBlock(CSSTokenType endToken) { + while (parser.peek().type() != endToken) { + parser.consumeToken(); + } + parser.consumeToken(); return {}; } @@ -329,6 +376,7 @@ struct CSSComponentValueVisitorDispatcher { } constexpr std::optional visitPreservedToken() { + parser.consumeToken(); return {}; } }; @@ -353,4 +401,24 @@ constexpr ReturnT CSSSyntaxParser::consumeComponentValue( CSSComponentValueDelimiter::None, visitors...); } +template +constexpr ReturnT CSSSyntaxParser::peekComponentValue( + CSSComponentValueDelimiter delimiter, + const CSSComponentValueVisitor auto&... visitors) + requires(CSSUniqueComponentValueVisitors) +{ + return CSSComponentValueVisitorDispatcher{ + *this} + .peekComponentValue(delimiter, visitors...); +} + +template +constexpr ReturnT CSSSyntaxParser::peekComponentValue( + const CSSComponentValueVisitor auto&... visitors) + requires(CSSUniqueComponentValueVisitors) +{ + return peekComponentValue( + CSSComponentValueDelimiter::None, visitors...); +} + } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h b/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h index 7acdbc72a364..00868c4745e0 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h +++ b/packages/react-native/ReactCommon/react/renderer/css/CSSValueParser.h @@ -75,9 +75,11 @@ class CSSValueParser { CSSValidDataTypeParser... RestParserT> constexpr ReturnT tryConsumePreservedToken(const CSSPreservedToken& token) { if constexpr (CSSPreservedTokenSink) { + auto currentParser = parser_; if (auto ret = ParserT::consumePreservedToken(token, parser_)) { return *ret; } + parser_ = currentParser; } if constexpr (CSSSimplePreservedTokenSink) { @@ -104,9 +106,11 @@ class CSSValueParser { const CSSSimpleBlock& block, CSSSyntaxParser& blockParser) { if constexpr (CSSSimpleBlockSink) { + auto currentParser = blockParser; if (auto ret = ParserT::consumeSimpleBlock(block, blockParser)) { return *ret; } + blockParser = currentParser; } return tryConsumeSimpleBlock(block, blockParser); @@ -127,9 +131,11 @@ class CSSValueParser { const CSSFunctionBlock& func, CSSSyntaxParser& blockParser) { if constexpr (CSSFunctionBlockSink) { + auto currentParser = blockParser; if (auto ret = ParserT::consumeFunctionBlock(func, blockParser)) { return *ret; } + blockParser = currentParser; } return tryConsumeFunctionBlock(func, blockParser); diff --git a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp index 2848b0f2b7ab..4061daa3f190 100644 --- a/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/css/tests/CSSSyntaxParserTest.cpp @@ -392,4 +392,89 @@ TEST(CSSSyntaxParser, unconsumed_simple_block_args) { EXPECT_EQ(funcValue, std::nullopt); } +TEST(CSSSyntaxParser, peek_does_not_consume) { + CSSSyntaxParser parser{"foo()"}; + auto funcValue = parser.peekComponentValue>( + [&](const CSSFunctionBlock& function, CSSSyntaxParser& /*blockParser*/) { + EXPECT_EQ(function.name, "foo"); + return function.name; + }); + + EXPECT_EQ(funcValue, "foo"); + + auto funcValue2 = parser.peekComponentValue>( + [&](const CSSFunctionBlock& function, CSSSyntaxParser& /*blockParser*/) { + EXPECT_EQ(function.name, "foo"); + return function.name; + }); + + EXPECT_EQ(funcValue2, "foo"); + + auto funcValue3 = + parser.consumeComponentValue>( + [&](const CSSFunctionBlock& function, + CSSSyntaxParser& /*blockParser*/) { + EXPECT_EQ(function.name, "foo"); + return function.name; + }); + + EXPECT_EQ(funcValue3, "foo"); + + auto funcValue4 = parser.peekComponentValue>( + [&](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( + CSSComponentValueDelimiter::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( + CSSComponentValueDelimiter::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( + CSSComponentValueDelimiter::Whitespace, + [](const CSSPreservedToken& token) { + EXPECT_EQ(token.type(), CSSTokenType::Ident); + EXPECT_EQ(token.stringValue(), "bar"); + return token.stringValue(); + }); + + EXPECT_EQ(identValue, "bar"); +} + } // namespace facebook::react