Bug 196351 – [ESNext] Implement support for Numeric Separators

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Leo Balter]

https://github.com/tc39/proposal-numeric-separator

The proposal has got re-promoted to Stage 3 in March/2019 and it's ready for implementations.

[Leo Balter]

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

[Ross Kirsling]

Created attachment 370218 [details]
Patch

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 370383 [details]
Patch

[Keith Miller]

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?

[Ross Kirsling]

(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.

[Ross Kirsling]

Created attachment 370426 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370426 [details]
Patch for landing

Clearing flags on attachment: 370426

Committed r245634: <https://trac.webkit.org/changeset/245634>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51036747>

[Tadeu Zagallo]

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 ''

[WebKit Commit Bot]

Re-opened since this is blocked by bug 198140

[Ross Kirsling]

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.

[Ross Kirsling]

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