Bug 196351

Tests are available from Test262 with the tag `numeric-separator-literal`

Created attachment 370218 [details] Patch

Self-review comment: Forgot test cases for BigInt and for multiple adjacent separators, will add before landing.

Created attachment 370383 [details] Patch

Comment on attachment 370383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370383&action=review r=me with some nits. > Source/JavaScriptCore/parser/Lexer.cpp:1519 > // Optimization: most hexadecimal values fit into 4 bytes. Nit: Add ASSERT(isASCIIHexDigit(m_current)) it makes it clear that the caller is expected to do this check. > Source/JavaScriptCore/parser/Lexer.cpp:1578 > Ditto on ASSERT but for binary. > Source/JavaScriptCore/parser/Lexer.cpp:1630 > Ditto on ASSERT but for Octal. > Source/JavaScriptCore/parser/Lexer.cpp:1686 > Ditto on ASSERT but for Decimal. > JSTests/stress/numeric-literal-separators.js:13 > +function shouldThrow(script, errorType) { > + let error; > + try { > + eval(script); > + } catch (e) { > + error = e; > + } Can you have these tests also run through the Number constructor. Also can you test appending a bunch of digits (maybe even all 0s) so we hit the slow path in the lexer?

(In reply to Keith Miller from comment #5) > Comment on attachment 370383 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370383&action=review > > r=me with some nits. > > > Source/JavaScriptCore/parser/Lexer.cpp:1519 > > // Optimization: most hexadecimal values fit into 4 bytes. > > Nit: Add ASSERT(isASCIIHexDigit(m_current)) it makes it clear that the > caller is expected to do this check. > > > Source/JavaScriptCore/parser/Lexer.cpp:1578 > > > > Ditto on ASSERT but for binary. > > > Source/JavaScriptCore/parser/Lexer.cpp:1630 > > > > Ditto on ASSERT but for Octal. > > > Source/JavaScriptCore/parser/Lexer.cpp:1686 > > > > Ditto on ASSERT but for Decimal. > > > JSTests/stress/numeric-literal-separators.js:13 > > +function shouldThrow(script, errorType) { > > + let error; > > + try { > > + eval(script); > > + } catch (e) { > > + error = e; > > + } > > Can you have these tests also run through the Number constructor. Also can > you test appending a bunch of digits (maybe even all 0s) so we hit the slow > path in the lexer? Will do! Just to be clear, this is only for literals, so it doesn't apply to the Number constructor or to parseInt/parseFloat.

Created attachment 370426 [details] Patch for landing

Comment on attachment 370426 [details] Patch for landing Clearing flags on attachment: 370426 Committed r245634: <https://trac.webkit.org/changeset/245634>

All reviewed patches have been landed. Closing bug.

<rdar://problem/51036747>

It seems like this completely broke the lexer. After this patch, running `jsc -e ''` gives me following crash: ASSERTION FAILED: isASCIIDigit(m_current) ./parser/Lexer.cpp(1681) : Optional<JSC::Lexer::NumberParseResult> JSC::Lexer<unsigned char>::parseDecimal() [T = unsigned char] 1 0x105e5afa9 WTFCrash 2 0x105e5dd6b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x10707dbee JSC::Lexer<unsigned char>::parseDecimal() 4 0x10707b922 JSC::Lexer<unsigned char>::lexWithoutClearingLineTerminator(JSC::JSToken*, unsigned int, bool) 5 0x107079e81 JSC::Lexer<unsigned char>::lex(JSC::JSToken*, unsigned int, bool) 6 0x107095883 JSC::Parser<JSC::Lexer<unsigned char> >::next(unsigned int) 7 0x1070f88d3 JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseVariableDeclarationList<JSC::SyntaxChecker>(JSC::SyntaxChecker&, int&, JSC::SyntaxChecker::DestructuringPattern&, JSC::SyntaxChecker::Expression&, JSC::JSTextPosition&, JSC::JSTextPosition&, JSC::JSTextPosition&, JSC::Parser<JSC::Lexer<unsigned char> >::VarDeclarationListConte xt, JSC::DeclarationType, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, bool&) 8 0x1070f5ba6 JSC::SyntaxChecker::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseVariableDeclaration<JSC::SyntaxChecker>(JSC::SyntaxChecker&, JSC::DeclarationType, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType) 9 0x1070f5399 JSC::SyntaxChecker::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatementListItem<JSC::SyntaxChecker>(JSC::SyntaxChecker&, JSC::Identifier const*&, unsigned int*) 10 0x1070f4aa8 JSC::SyntaxChecker::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::SyntaxChecker>(JSC::SyntaxChecker&, JSC::SourceElementsMode) 11 0x107123d01 JSC::ASTBuilder::FunctionBody JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionBody<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SyntaxChecker&, JSC::JSTokenLocation const&, int, int, int, int, JSC::ConstructorKind, JSC::SuperBinding, JSC::FunctionBodyType, unsigned int, JSC::SourceParseMode) 12 0x1070e4c8a bool JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionInfo<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::FunctionNameRequirements, JSC::SourceParseMode, bool, JSC::ConstructorKind, JSC::SuperBinding, int, JSC::ParserFunctionInfo<JSC::ASTBuilder>&, JSC::Parser<JSC::Lexer<unsigned char> >::FunctionDefinitionType, WTF::Optional<int>)::'lambda0'()::operator ()() const 13 0x1070e0f07 bool JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionInfo<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::FunctionNameRequirements, JSC::SourceParseMode, bool, JSC::ConstructorKind, JSC::SuperBinding, int, JSC::ParserFunctionInfo<JSC::ASTBuilder>&, JSC::Parser<JSC::Lexer<unsigned char> >::FunctionDefinitionType, WTF::Optional<int>) 14 0x1070dcd68 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseFunctionExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 15 0x1070d9232 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parsePrimaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 16 0x1070d6e95 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 17 0x10712eaa0 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 18 0x10712e146 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 19 0x10712d33c JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 20 0x10712bea8 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Parser<JSC::Lexer<unsigned char> >::ExpressionErrorClassifier&) 21 0x1070d3ef2 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 22 0x1070da0e3 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 23 0x1070d92df JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parsePrimaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 24 0x1070d6e95 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 25 0x10712eaa0 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseUnaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 26 0x10712e146 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseBinaryExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 27 0x10712d33c JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseConditionalExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 28 0x10712bea8 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Parser<JSC::Lexer<unsigned char> >::ExpressionErrorClassifier&) 29 0x1070d3ef2 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 30 0x1070da0e3 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) 31 0x10714e992 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionStatement<JSC::ASTBuilder>(JSC::ASTBuilder&) [1] 82893 segmentation fault DYLD_FRAMEWORK_PATH=$VM $VM/jsc -e ''

Re-opened since this is blocked by bug 198140

So it looks like we can't ASSERT isASCIIDecimal in parseDecimal for existing reasons. This is worth refactoring to fix, but I don't want to add complication here, so I'll handle it in a follow-up patch.

Committed r245655: <https://trac.webkit.org/changeset/245655>