Bug 196351 - [ESNext] Implement support for Numeric Separators
Summary: [ESNext] Implement support for Numeric Separators
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on: 198140
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-28 07:41 PDT by Leo Balter
Modified: 2019-07-21 15:34 PDT (History)
11 users (show)

See Also:


Attachments
Patch (24.49 KB, patch)
2019-05-18 22:47 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (25.14 KB, patch)
2019-05-21 22:59 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (23.43 KB, patch)
2019-05-22 11:26 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Balter 2019-03-28 07:41:26 PDT
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.
Comment 1 Leo Balter 2019-03-28 07:42:59 PDT
Tests are available from Test262 with the tag `numeric-separator-literal`
Comment 2 Ross Kirsling 2019-05-18 22:47:56 PDT
Created attachment 370218 [details]
Patch
Comment 3 Ross Kirsling 2019-05-20 20:07:43 PDT
Self-review comment: Forgot test cases for BigInt and for multiple adjacent separators, will add before landing.
Comment 4 Ross Kirsling 2019-05-21 22:59:01 PDT
Created attachment 370383 [details]
Patch
Comment 5 Keith Miller 2019-05-22 10:51:04 PDT
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?
Comment 6 Ross Kirsling 2019-05-22 11:23:06 PDT
(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.
Comment 7 Ross Kirsling 2019-05-22 11:26:31 PDT
Created attachment 370426 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2019-05-22 12:06:09 PDT
Comment on attachment 370426 [details]
Patch for landing

Clearing flags on attachment: 370426

Committed r245634: <https://trac.webkit.org/changeset/245634>
Comment 9 WebKit Commit Bot 2019-05-22 12:06:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-05-22 12:07:38 PDT
<rdar://problem/51036747>
Comment 11 Tadeu Zagallo 2019-05-22 14:34:31 PDT
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 ''
Comment 12 WebKit Commit Bot 2019-05-22 14:46:29 PDT
Re-opened since this is blocked by bug 198140
Comment 13 Ross Kirsling 2019-05-22 15:29:27 PDT
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.
Comment 14 Ross Kirsling 2019-05-22 15:43:54 PDT
Committed r245655: <https://trac.webkit.org/changeset/245655>