Incomplete braced quantifiers should be syntax errors in Unicode patterns only

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

Test case:
  /{1/u

Expected:
  SyntaxError thrown

Actual:
  RegExp instance

ECMA262:
  https://tc39.es/ecma262/#prod-annexB-Term (/u flag precludes the use of ExtendedAtom)

  Term[U, N] ::
    [~U] ExtendedAtom[?N]

  ExtendedAtom[N] ::
    InvalidBracedQuantifier

Test262:
  https://test262.report/browse/annexB/built-ins/RegExp/prototype/compile/pattern-string-invalid-u.js
  https://test262.report/browse/built-ins/RegExp/unicode_restricted_incomple_quantifier.js
  https://test262.report/browse/language/literals/regexp/u-invalid-extended-pattern-char.js

[Alexey Shvayka]

Created attachment 388739 [details]
Patch

[Darin Adler]

Comment on attachment 388739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388739&action=review

> Source/JavaScriptCore/yarr/YarrErrorCode.cpp:43
> +        REGEXP_ERROR_PREFIX "incomplete {} quantifier for unicode pattern",   // QuantifierIncomplete

I think Unicode needs a capital "U"

> Source/JavaScriptCore/yarr/YarrParser.h:817
> +                if (m_isUnicode) {
> +                    m_errorCode = ErrorCode::QuantifierIncomplete;
> +                    break;
> +                }

Seems like we need at least one test case that checks that we get the expected error. Maybe that’s not good in WPT tests because the actual error messages will be different across platforms, but if so then we could add a WebKit-specific test. Would you consider adding one?

> Source/JavaScriptCore/yarr/YarrParser.h:-817
> +                // If we did not find a complete quantifer, fall through to the default case.
> +                FALLTHROUGH;
>              }
> -            // if we did not find a complete quantifer, fall through to the default case.
> -            FALLTHROUGH;

This change seems to make the code a tiny bit worse. Having the "FALLTHROUGH" macro inside a block doesn’t quite seem like the best style, and there’s no need to make this change except for stylistic reasons.

[Alexey Shvayka]

Created attachment 389191 [details]
Patch

Capitalize Unicode in error messages and add tests.

[Alexey Shvayka]

(In reply to Darin Adler from comment #2)

Thank you for review(s), Darin. I always appreciate your detailed comments.

> This change seems to make the code a tiny bit worse. Having the
> "FALLTHROUGH" macro inside a block doesn’t quite seem like the best style,
> and there’s no need to make this change except for stylistic reasons.

According to my "research" (grep), "FALLTHROUGH" outside block is relatively popular in Source/WebCore (5 inside, 3 outside), but it is the only occurrence in Source/JavaScriptCore (24 inside, 1 of which is in this file).

[WebKit Commit Bot]

Comment on attachment 389191 [details]
Patch

Rejecting attachment 389191 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 389191, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/13314000

[Alexey Shvayka]

Created attachment 389264 [details]
Patch

Set reviewer.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389264 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389264 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 389264 [details]
Patch

Clearing flags on attachment: 389264

Committed r255452: <https://trac.webkit.org/changeset/255452>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59039623>